Created attachment 81895 [details]
Revert effects->cursorPos() to QCursor::pos()

Applying this patch seems ot fix the problem.

> 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)

(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.

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.

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.

(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 ;-)

(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.

(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.

(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.

(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.

PS:
what focus policy do you use?
Click to focus, focus follows mouse,...?

(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.

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.

Created attachment 82039 [details]
Keep track of previous position and mask

Something like this seems to work well. Would this be reasonable?

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.

(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.)

I created a review request: https://git.reviewboard.kde.org/r/112392/

(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.

(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?

> 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 ;-)

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