GNOME Bugzilla – Bug 746328
evdev: Keep track of the pointer coordinate ourself
Last modified: 2015-03-18 15:15:02 UTC
Where the clutter evdev backend is used most commonly (GNOME Shell), there may be various things that stall main thread such as Javascript, drawing things etc. When moving the pointer because of relative motion events, we check the current pointer coordinate on the stage and adds the relative motion, but when there is something stalling the main thread, we might receive many libinput events, queueing the resulting clutter events, without the clutter events being processed before the next one is queued. This causes the pointer position to lag behind because the queued pointer events are effectively lost as they all add to the last current stage pointer position. To improve this situation slightly, keep track of the pointer position ourself in the evdev backend, and add to that. This will at least not drop the pointer motions when the main thread is stalling, but the pointer cursor will still stutter and instead jump a bit. This has the side effect of making the internal function _clutter_input_device_set_coords not effect the internal coordinate state of the evdev stage, but AFAICS there is nothing depending on that so that should be fine. We may still drop motion events when the stalls are long enough for the evdev queue to be filled, but that is nothing that can be dealt with in clutter.
Created attachment 299568 [details] [review] evdev: Keep track of the pointer coordinate ourself
Review of attachment 299568 [details] [review]: I think your commit message is a bit deceptive because it makes it sound like this is some sort of partial workaround that makes things a bit better if an app is slow - but it looks like it actually fixes a fundamental correctness issue. There's no reason to expect that we'll only get *one* motion event from evdev for each clutter frame, even if we are running at 60fps. And the input device coordinates are only updated once per clutter frame. I'm surprised this isn't more noticeable than it is. I think all the referenes to stalling the main thread, etc, can be dropped from the commit message, and you can simply say something like: When multiple relative motion events are received and queued, we can't base the relative => absolute motion conversion off of the stage pointer position, since that is only updated when the queue is processed at the beginning of each frame. The effect of trying to use the stage pointer position was that subsequent motion events were effectively dropped. To improve things, switch to keeping track of the pointer position ourselves in the evdev backend and adding to that. This has the side effect of making the internal function _clutter_input_device_set_coords not effect the internal coordinate state of the evdev stage, but AFAICS there is nothing depending on that so that should be fine. If the evdev queue filling up was an actual problem, it's not all that hard to use a thread to read events - but it's not worth the work if we don't have an issue. The content of the patch looks good to me.
I agree with Owen's assessment. The patch looks good, but the commit message should be updated.
Nice catch, this indeed may account for the "gnome shell pointer is slow when hovering $FOO" seen intermitently on #gnome-shell lately. I guess it would be positive to conciliate this with _clutter_input_device_set_coords(), or just hardcode something for pointer warping purposes, it's probably not a priority anyway.
(In reply to Owen Taylor from comment #2) > Review of attachment 299568 [details] [review] [review]: > > I think your commit message is a bit deceptive because it makes it sound > like this is some sort of partial workaround that makes things a bit better > if an app is slow - but it looks like it actually fixes a fundamental > correctness issue. There's no reason to expect that we'll only get *one* > motion event from evdev for each clutter frame, even if we are running at > 60fps. And the input device coordinates are only updated once per clutter > frame. I'm surprised this isn't more noticeable than it is. True. It was all the stalling that caused me to notice and correct it which was what I was considering when writing the commit message, but later realized that obviously not only stalls cause us to get more than one libinput event in the queue. I'll upload a new patch.
(In reply to Owen Taylor from comment #2) > Review of attachment 299568 [details] [review] [review]: > > ... <snip> ... > > If the evdev queue filling up was an actual problem, it's not all that hard > to use a thread to read events - but it's not worth the work if we don't > have an issue. It's libinput that reads from evdev and libinput is not thread safe. We'd need to put all of libinput in another thread which I'd very much think is a bad idea, or make libinput thread safe which also seems quite unnecessary. And it wouldn't really help that much since the situations when the evdev queue gets filled up we stalled so long (according to my observation) that I don't think making a big pointer jump is any better than not moving.
Created attachment 299726 [details] [review] evdev: Keep track of the pointer coordinate ourself When multiple relative motion events are received and queued, we can't base the relative => absolute motion conversion off of the stage pointer position, since that is only updated when the queue is processed at the beginning of each frame. The effect of trying to use the stage pointer position was that subsequent motion events were effectively dropped. To improve things, switch to keeping track of the pointer position ourselves in the evdev backend and adding to that. This has the side effect of making the internal function _clutter_input_device_set_coords not effect the internal coordinate state of the evdev stage, but AFAICS there is nothing depending on that so that should be fine.
Review of attachment 299726 [details] [review]: Looks good. It should be good to have this in 1.22, and still need to do a fix for the GDK master clock implementation, so I'll do a release anyway.
(In reply to Carlos Garnacho from comment #4) > Nice catch, this indeed may account for the "gnome shell pointer is slow > when hovering $FOO" seen intermitently on #gnome-shell lately. > > I guess it would be positive to conciliate this with > _clutter_input_device_set_coords(), or just hardcode something for pointer > warping purposes, it's probably not a priority anyway. We already expose clutter_evdev_warp_pointer () via clutter-evdev.h, but that function will still work even with the attached patch applied.
Attachment 299726 [details] pushed as 615b0f4 - evdev: Keep track of the pointer coordinate ourself
(In reply to Jonas Ådahl from comment #9) > (In reply to Carlos Garnacho from comment #4) > We already expose clutter_evdev_warp_pointer () via clutter-evdev.h, but > that function will still work even with the attached patch applied. Oh right, I missed the notify_absolute_motion() change in the diff view.