GNOME Bugzilla – Bug 788288
Brightness slider is very laggy when dragging it with the mouse
Last modified: 2021-07-05 14:23:04 UTC
STEPS TO REPRODUCE: 1. Click on the brightness slider in the top panel and start dragging it around quickly 2. Release the mouse button after having moved the slider a few times to the left and right EXPECTED OUTCOME: Slider should keep up with the mouse movements and don't feel laggy ACTUAL OUTCOME: The slider doesn't keep up with the mouse movements, since the value reflected there mirrors the actual brightness level, which can't be changed that fast
I don't think there's a clear way to fix this, since it depends on how fast the brightness levels can actually be changed, so I have a proposal: Shen dragging the slider with the mouse, avoid updating the brightness value until the drag-end event has been reached, so that there's only one event instead of multiple ones, which relies a better experience (IMHO) to the user. Note that this doesn't impact other ways this slider could be changed (e.g. keyboard based), so the only noticeable difference for the user would be that the feature of seeing how brightness changes dynamically while dragging the slider is gone, which means being inconsistent with how other sliders (e.g. volume) behave But considering how laggy that experience can be for this one, I think an exception here wouldn't be a bad thing.
Created attachment 360595 [details] [review] brightness: When dragging the slider, only update the value on drag-end events This patch implements the suggested approach. Let me know what you think
(In reply to Mario Sánchez Prada from comment #2) > Created attachment 360595 [details] [review] [review] > brightness: When dragging the slider, only update the value on drag-end > events > > This patch implements the suggested approach. Let me know what you think What if we do something like: _sliderChanged: function(slider, value) { this._sliderValue = value; if (!this._sliderIsDragging) { this._updateBrightness(); return; } if (this._updateBrightnessTimeout != 0) { Mainloop.source_remove(this._updateBrightnessTimeout); this._updateBrightnessTimeout = 0; } this._updateBrightnessTimeout = Mainloop.timeout_add(UPDATE_BRIGHTNESS_TIMEOUT, Lang.bind(this, function() { this._titleEntryTimeout = 0; this._updateBrightness(); return false; })); }, Doing so, the user doesn't have to release the slider to see the brightness updated.
I thought of using a timeout too, but the problem I see with that approach is that the TIMEOUT would be a completely arbitrary number, and what works well in one machine doesn't necessarily have to work well in different (e.g. slower) machines. For instance, I've reproduced the issue in both a laptop with a i74600U CPU and another one with an old Celeron-like CPU and the sluggishness is, unsurprisingly, much worse in the old one. That's why I think doing this on drag-end is a cleaner and simpler solution, but if more people agrees on the timeout-based thing, I'd be ok too as long as the timeout was not "too low" (which, again, would be a guessing/random game).
Created attachment 360773 [details] [review] brightness: When dragging the slider, only update the value on drag-end events New patch after fixing a mistake in _slideDragEnd(), pointed out by Joaquim Rocha.
Ping?
Talking to Cosimo, I realized this patch is broken in that it fixes the situation when dragging only, but not in other cases such as: * Moving the slider with the mouse wheel (as no drag events are generated) * Moving the slider with the keyboard by keeping the arrow keys pressed down for a while Also, per Cosimo's suggestion [1], it might be interesting to try to fix this at the g-s-d level, so that we don't need to care about being dragging, timeouts and whatnot, and simply don't commit changes to brightness faster than what's actually possible. The bug is still valid, though, so let's keep it open [1] https://github.com/endlessm/gnome-shell/pull/149#issuecomment-335937116
Comment on attachment 360773 [details] [review] brightness: When dragging the slider, only update the value on drag-end events Rejecting my own proposal. No hard feelings with myself, though
(In reply to Mario Sánchez Prada from comment #7) ... > > Also, per Cosimo's suggestion [1], it might be interesting to try to fix > this at the g-s-d level, so that we don't need to care about being dragging, > timeouts and whatnot, and simply don't commit changes to brightness faster > than what's actually possible. We should probably have gsd cache "future brightness" from dbus, then wait until the helper process finished before trying to spawn a new one, or we turn the brightness helper into a transient 'brightness-helperd' (that dies after a couple of seconds timeout or something) and have gsd shove the events to the helper without respawning it each time.
Created attachment 361546 [details] [review] brightness: Only update slider value on map So here's another idea for a fix on the gnome-shell side: Currently we update the proxy's Brightness property when the slider value changes and vice-versa. However as applying a brightness change is usually far from immediate, the two values get out of sync and we end up with a sluggish or jiggling slider. As a workaround, only set the slider value on map and let it represent the requested brightness value otherwise.
(In reply to Florian Müllner from comment #10) > Created attachment 361546 [details] [review] [review] > brightness: Only update slider value on map > > So here's another idea for a fix on the gnome-shell side: > Currently we update the proxy's Brightness property when the slider > value changes and vice-versa. However as applying a brightness change > is usually far from immediate, the two values get out of sync and we > end up with a sluggish or jiggling slider. As a workaround, only set > the slider value on map and let it represent the requested brightness > value otherwise. How would this work if you open the popup and then press the brightness keys?
I tested this patch and, while it fixes the problem of the slider being dragged, I'm not sure the effect is better, or less confusing, than before: If I move the slider quickly back and forth and then stop moving, I can see how the brightness changes are still happening for a while after that point, trying to catch up with all the different values the slider has been through. Also, as Jonas suggested, it doesn't seem to update the slider now when updating the brightness with the brightness keys.
Created attachment 363729 [details] [review] power: Spawn the backlight helper asynchronously when setting a value When setting, we don't need to wait for the command to finish as there's nothing useful to report back to the caller. This should make users of the backlight DBus API, like gnome-shell, more responsive, since we won't block processing DBus calls. -- Here's a simple (read "could be quickly pushed to stable") attempt at fixing this in g-s-d. In my testing it seem to work much better.
Tested this patch too on my laptop with the latest g-s-d and gnome-shell from master and the effect I'm getting is that brightness doesn't change at all when moving the slider in the top panel. Maybe I'm missing something else?
Review of attachment 363729 [details] [review]: Couldn't this potentially race? I.e. if we spawn two processes almost at the same time "set-brightness 55" then "set-brightness 60", isn't it theoretically possible that the kernel decides to give the second process higher priority, meaning we'll end up with an incorrect brightess.
If we want to improve the helper, we should probably mirror the helper xf86-video-intel is using for the xrandr brightness properties, that helper gets spawned only once and then it gets new brightness values written to its stdin (as 32 bit ints I believe) whenever the brightness changes, this way g-s-d can simply write values to the pipe as fast as it wants. This will still cause a lag though, some backlight interfaces may have 10000 levels, and the slider fires of events like crazy, while actually setting the brightness is slow. I guess if we use the write to stdin method, we could keep reading new values from stdin until that would block, that way we use the pipe to stdin as a buffer and empty it each time before setting the brightness.
Gnome 3.28 has the same problem on Arch Linux.
Any progress on this bug? Still valid for Gnome 3.29.91.
(In reply to Strangiato from comment #18) > Any progress on this bug? No. If there is progress on a bug you can find out by reading the previous comments on that bug.
*** Bug 797096 has been marked as a duplicate of this bug. ***
just a note, the brightness slider in settings seems to work way better (but still with some glitches): Settings -> Power -> Power Savings -> Screen brightness
Also filed as https://gitlab.gnome.org/GNOME/gnome-shell/issues/656
Created attachment 373922 [details] [review] brightness: Count and ignore brightness value updates so slider doesn't jump, and snap to 5% increments to reduce number of events. So here's a slightly hacky proposal: as we move the slider, count the N times we update the value, then ignore the first N g-properties-changed events received, under the assumption that these are just the same values mirrored back. This completely eliminates the jumpiness, except if you do something like quickly mashing the media brightness keys while dragging the slider at the same time (but even then, it's not as bad as before). Additionally, I've restricted the brightness values to 5% increments, which on my system (Thinkpad T430s) eliminated most of the delay in updating the brightness.
(In reply to Nicholas Parkanyi from comment #23) > Created attachment 373922 [details] [review] [review] > brightness: Count and ignore brightness value updates so slider doesn't > jump, and snap to 5% increments to reduce number of events. Thank you for the patch, but this should already be fixed by this commit (which got merged after the 3.30 release: https://gitlab.gnome.org/GNOME/gnome-settings-daemon/commit/8152a20bae04c84f028b1eae04f42f4c27215da8 Please give that commit a try and let us know if it fixes things.
GNOME is going to shut down bugzilla.gnome.org in favor of gitlab.gnome.org. As part of that, we are mass-closing older open tickets in bugzilla.gnome.org which have not seen updates for a longer time (resources are unfortunately quite limited so not every ticket can get handled). If you can still reproduce the situation described in this ticket in a recent and supported software version, then please follow https://wiki.gnome.org/GettingInTouch/BugReportingGuidelines and create a new ticket at https://gitlab.gnome.org/GNOME/gnome-shell/-/issues/ Thank you for your understanding and your help.