Bug 323979 - Zoom: Mouse Tracking Lags
Summary: Zoom: Mouse Tracking Lags
Status: RESOLVED FIXED
Alias: None
Product: kwin
Classification: Unclassified
Component: effects-various (show other bugs)
Version: 4.11.0
Platform: Compiled Sources Linux
: NOR normal (vote)
Target Milestone: ---
Assignee: KWin default assignee
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-08-24 18:04 UTC by Michael Chang
Modified: 2013-09-04 12:38 UTC (History)
0 users

See Also:
Latest Commit:
Version Fixed In: 4.11.2


Attachments
Revert effects->cursorPos() to QCursor::pos() (1.08 KB, patch)
2013-08-24 18:06 UTC, Michael Chang
Details
Keep track of previous position and mask (1.85 KB, patch)
2013-08-30 18:13 UTC, Michael Chang
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Michael Chang 2013-08-24 18:04:30 UTC
After upgrading from 4.10.5 to 4.11.0, mouse tracking in the Zoom effect started to lag.

The problem is most obvious when moving the mouse very quickly to the edge of the screen. Sometimes the zoomed part of the desktop will not update until you move the mouse again. It also seems to be more significant while windows are opening or closing; after a window has been open awhile, things mostly go back to normal.

My Zoom settings are set to Scale and Centered. I use a rather high zoom (~10x).

After some experimenting, I think I've tracked down the problem to the change from using "QCursor::pos()" to "effects->cursorPos()". Changing back to QCursor::pos() in zoom.cpp seems to fix the issue.

Reproducible: Always
Comment 1 Michael Chang 2013-08-24 18:06:37 UTC
Created attachment 81895 [details]
Revert effects->cursorPos() to QCursor::pos()

Applying this patch seems ot fix the problem.
Comment 2 Thomas Lübking 2013-08-24 19:35:42 UTC
> Applying this patch seems ot fix the problem.

There's a simple test:

qDebug() << effects->cursorPos() << QCursor::pos(); // do they differ at all?

You can spare the changes to zoomIn/Out for they don't affect your settings anyway.

You should test the above since a revert to QCursor::pos() is not possible (becomes problematic with Qt5)
Comment 3 Michael Chang 2013-08-24 20:29:10 UTC
(In reply to comment #2)
> qDebug() << effects->cursorPos() << QCursor::pos(); // do they differ at all?

Thanks for the quick reply.

It appears that the two values occasionally differ slightly, but that's not the main problem. Even when using QCursor::pos(), simply adding the above line causes the lag.

I also tested with just the line:
const QPoint p1 = effects->cursorPos();

directly above
QPoint p = QCursor::pos();

(not using p1 at all) and the lag occurs. So it seems like calling effects->cursorPos() is the problem.
Comment 4 Michael Chang 2013-08-24 20:38:58 UTC
A bit more info:

I was able to reliably reproduce the slowdown by opening System Settings and moving the mouse around inside that dialog. It seemed like when the mouse was outside the dialog, it was okay, but inside it was very unresponsive.

This makes me think it might have something to do with the tooltips or hover animations, but that's just a guess.
Comment 5 Martin Flöser 2013-08-24 20:50:14 UTC
reverting to QCursor::pos() is no matter what the wrong solution. If at all 
it's fixing the symptoms and not the main issue.

cursorPos() and QCursor::pos() should work in a similar way, except that 
QCursor::pos() does a roundtrip to the XServer every time. This "might" 
explain the lag when QCursor::pos() is not used, but clearly not if both are 
used.

Apart from the added roundtrip QCursor::pos() is no solution for the future as 
in Wayland KWin and not Qt controls the cursor position.
Comment 6 Thomas Lübking 2013-08-24 21:01:42 UTC
(In reply to comment #3)
> (In reply to comment #2)
> > qDebug() << effects->cursorPos() << QCursor::pos(); // do they differ at all?

> the main problem. Even when using QCursor::pos(), simply adding the above
> line causes the lag.

Ok, that's kinda odd.
can you measure calling times for QCursor::pos() and effects->cursorPos()?

QElapsedTimer t;
t.start();
QPoint p1 = effects->cursorPos();
qint64 t1 =t.nsecsElapsed();
t.start();
QPoint p1 = QCursor::pos();
qint64 t2 =t.nsecsElapsed();
qDebug() << "Effects::cursorPos(): " << t1 << "QCursor::pos()" << t2;

(beware the typos ;-)
Comment 7 Michael Chang 2013-08-25 13:56:11 UTC
(In reply to comment #6)
> Ok, that's kinda odd.
> can you measure calling times for QCursor::pos() and effects->cursorPos()?

Hmm, it doesn't ook like it's the calling time. In fact, I think effects->cursorPos() is slightly faster on average.

I also checked to see how long drawScreen() took overall, and the time between calls. It seems like the QCursor version of drawScreen to take longer (12-15ms vs 6-10), even though it's much smoother. But I'm not sure how meaningful that is.
Comment 8 Thomas Lübking 2013-08-25 14:30:03 UTC
(In reply to comment #7)

> I also checked to see how long drawScreen() took overall, and the time
> between calls. It seems like the QCursor version of drawScreen to take
> longer (12-15ms vs 6-10), even though it's much smoother. But I'm not sure
> how meaningful that is.

Ok?

What we have is that

- the simple call of effects->cursor() makes the zoom effect "lag" for you (please ensure it's the simple invocation by just adding sth. like
   QPoint pt = effects->cursorPos();
   pt += QPoint(1,1);
ie. no printout but ensure gcc does not just optimize the call away.

- It's however not that the call itself would somehow delay operations.

Do you have a printout of the cursor position comparism?

Next guess:
In kwin/cursor.cpp:217 comment

void X11Cursor::doGetPos()
{
// this line    if (m_timeStamp != XCB_TIME_CURRENT_TIME &&
// this line            m_timeStamp == QX11Info::appTime()) {
        // time stamps did not change, no need to query again
// this line       return;
// this line    }
// this line    m_timeStamp = QX11Info::appTime();
    ScopedCPointer<xcb_query_pointer_reply_t> pointer(xcb_query_pointer_reply(connection(),
        xcb_query_pointer_unchecked(connection(), rootWindow()), NULL));
    if (!pointer) {
        return;
    }
    m_buttonMask = pointer->mask;
    updatePos(pointer->root_x, pointer->root_y);
// this line    m_resetTimeStampTimer->start(0);
}

And see what happens.
Comment 9 Michael Chang 2013-08-27 13:58:57 UTC
(In reply to comment #8)
> - the simple call of effects->cursor() makes the zoom effect "lag" for you
> (please ensure it's the simple invocation by just adding sth. like

Yes, this causes the lag.

> Next guess:
> In kwin/cursor.cpp:217 comment
Still lags with this change.

I've done a bit more testing, and here's what I've found. In Cursor::pos() (cursor.cpp:53), if I comment out the two lines that were there and add:

-- s_self->m_pos = QCursor::pos();
-- return QCursor::pos();
It lags

-- s_self->m_pos = QCursor::pos();
-- return s_self->m_pos;
It lags

-- s_self->m_pos = QPoint(1,2);
-- s_self->m_pos += QPoint(1,1);
-- return QCursor::pos();
It works fine

I have no idea why this would be... I feel like something else is happening that I'm not accounting for, but I'm rather confident in these results. I've tried each multiple times and in different orders, for what that's worth.

Also, to be a bit more precise about the observed behavior: it seems like the "lag" isn't a matter of time. The mouse still moves fine and all the tooltips/animations activate. The problem is that the zoomed part does not recenter around the mouse. If I keep moving the mouse ,it will eventually recenter. I've also had the same behavior in Proportional and Push modes.

Thanks for your help. Happy to trying anything else you can think of.
Comment 10 Thomas Lübking 2013-08-27 16:48:41 UTC
(In reply to comment #9)

> -- s_self->m_pos = QPoint(1,2);
> -- s_self->m_pos += QPoint(1,1);
> -- return QCursor::pos();
> It works fine

iow, you keep m_pos unsynced to the actual position.

Lines 91-98

void Cursor::updatePos(const QPoint &pos)
{
//    if (m_pos == pos) {
//        return;
//    }
    m_pos = pos;
    emit posChanged(m_pos);
}
 
> Also, to be a bit more precise about the observed behavior: it seems like
> the "lag" isn't a matter of time. The mouse still moves fine
For the unscaled cursor that would be little surprise (done in framebuffer and HW, pretty much unrelated to everything else)
For the scaled cursor, that'd indeed be  interesting.
Comment 11 Thomas Lübking 2013-08-27 16:56:38 UTC
PS:
what focus policy do you use?
Click to focus, focus follows mouse,...?
Comment 12 Michael Chang 2013-08-27 17:59:33 UTC
(In reply to comment #10)
> iow, you keep m_pos unsynced to the actual position.
Yeah, although I figured that ::pos() will always return an accurate position.

> Lines 91-98
Commenting the lines out didn't have an effect.

> For the unscaled cursor that would be little surprise (done in framebuffer
> and HW, pretty much unrelated to everything else)
> For the scaled cursor, that'd indeed be  interesting.
It's happening for both unscaled and scaled cursor.

> PS:
> what focus policy do you use?
> Click to focus, focus follows mouse,...?
I'm using Click to Focus.
Comment 13 Michael Chang 2013-08-30 15:31:26 UTC
I think I found something.

In cursor.cpp line 228:
void X11Cursor::mousePolled()
{
    const QPoint last = currentPos();
    const uint16_t lastMask = m_buttonMask;
    doGetPos(); // Update if needed
    //if (last != currentPos() || lastMask != m_buttonMask) {
        emit mouseChanged(currentPos(), last,
            x11ToQtMouseButtons(m_buttonMask), x11ToQtMouseButtons(lastMask),
            x11ToQtKeyboardModifiers(m_buttonMask), x11ToQtKeyboardModifiers(lastMask));
    //}
}

Commenting out the if statement fixes the problem.

My understanding of what's going on is that Cursor::pos() calls doGetPos(), which updates the cursor position. So when this function checks if the possition has changed since the last doGetPos(), it has not, even though the position has changed since the last time we emitted mouseChanged().

Not sure what the correct fix is. Maybe emit mouseChanged whenever we emit posChanged? Not clear on the difference between the two.
Comment 14 Michael Chang 2013-08-30 18:13:27 UTC
Created attachment 82039 [details]
Keep track of previous position and mask

Something like this seems to work well. Would this be reasonable?
Comment 15 Thomas Lübking 2013-08-30 21:25:26 UTC
Patch and problem description are in gerenal correct. Calling Cursor::pos() from one location breaks polling for all signal listeners. It also explains why the problem is more prominent on "spontanous" repaints (when a window opens) and not by those caused by the zoom itself.
Very good catch.

Do you have a KDE account / commit rights?

I suggest to rename
-    QPoint m_lastPos;
-    uint16_t m_lastMask;
+    QPoint m_lastPollingPos;
+    uint16_t m_lastPollingMask;

Please wait for a comment from Martin or open a review request, but as far as i'm concerned, this can and should go in.
Comment 16 Michael Chang 2013-08-30 22:01:51 UTC
(In reply to comment #15)
> Do you have a KDE account / commit rights?

Thanks, Thomas.

I have a KDE account, but not commit rights. What's the best way to proceed? Should I open a reveiw request?

I think a potential alternative solution would be to modify Cursor::pos() to not call doGetPos(), and only update cursor position in mousePolled. Is there a preference for which is better? (I'm not sure whether mouse polling can/should be relied upon.)
Comment 17 Michael Chang 2013-08-30 23:34:46 UTC
I created a review request: https://git.reviewboard.kde.org/r/112392/
Comment 18 Thomas Lübking 2013-08-30 23:42:44 UTC
(In reply to comment #16)

> I have a KDE account, but not commit rights. What's the best way to proceed?
Wait on the RR to be dealt with, then either git and identity.kde.org setup to be able to push it [1] or upload the commit as "git format-patch" and one of as as commit monkey =)

> I think a potential alternative solution would be to modify Cursor::pos() to
> not call doGetPos(), and only update cursor position in mousePolled.
No. MousePolling is not done by default/all the time (actually it's ugly and we'll hopefully get rid of it altogether), so the cursor pos does have to be updated in Cursor::pos() - it just must not prevent updates being sent from polling to poll listeners.
Comment 19 Michael Chang 2013-09-01 01:25:31 UTC
(In reply to comment #18)
> (In reply to comment #16)
> 
> > I have a KDE account, but not commit rights. What's the best way to proceed?
> Wait on the RR to be dealt with, then either git and identity.kde.org setup
> to be able to push it [1] or upload the commit as "git format-patch" and one
> of as as commit monkey =)

Was I supposed to add people to the request?
Comment 20 Martin Flöser 2013-09-02 12:16:00 UTC
> Was I supposed to add people to the request?
No, you added group kwin, that is just fine. It lands in my inbox and the only 
reason for no reaction yet is called "weekend" and 106 unread mails in my bug 
folder ;-)
Comment 21 Martin Flöser 2013-09-04 12:38:31 UTC
Git commit 9dc8bfafbddab411dd1c895c3679a566e0768eae by Martin Gräßlin, on behalf of Michael Chang.
Committed on 02/09/2013 at 15:00.
Pushed by graesslin into branch 'KDE/4.11'.

Correctly detect mouseChanged between polls.

REVIEW: 112392
FIXED-IN: 4.11.2

M  +6    -4    kwin/cursor.cpp

http://commits.kde.org/kde-workspace/9dc8bfafbddab411dd1c895c3679a566e0768eae