Created attachment 177551 [details]
Windows Fast Brightness Change

Hmm, it's instant for me on my hardware.

It takes about the same amount of time on my hardware as in the "slow change" video, although I had assumed that was intentional to avoid potentially disorienting rapid swings in brightness occurring with imprecise mouse input / scrolling.

(In reply to Nate Graham from comment #2)
> Hmm, it's instant for me on my hardware.

Interesting. Because as you can see in the Windows video it's instant for me whereas it's slow in KDE on my same 2 monitors. So it's clearly possible to be instant on my hardware, it just isn't.

Btw it takes the same amount of time when I do `ddcutil -d 1 setvcp 10 50` for example. Since it's basically doing the same thing as powerdevil. I'd be curious if this is instant for you too, Nate? And if you're doing this on a laptop or an external monitor?

(In reply to Saul Fautley from comment #4)
> Btw it takes the same amount of time when I do `ddcutil -d 1 setvcp 10 50`
> for example. Since it's basically doing the same thing as powerdevil. I'd be
> curious if this is instant for you too, Nate? And if you're doing this on a
> laptop or an external monitor?

This would seem to be something in how ddcutil and/or your distribution (which wasn't specified on the bug report - which one are you using?) have that configured? Do the ideas on that page and in this thread - https://github.com/rockowitz/ddcutil/issues/240 - around changing the --sleep-multiplier setting change the response time?

John's comment #3 spells out the gist of it, let me repeat and confirm this.

For external monitors, Plasma will apply an intentional, currently hardcoded 1 second delay before writing brightness values. This is to minimize writes to the monitor's built-in storage, which for some monitors can wear out within a few years if writing to it several times per day. Ideally we want to avoid writing a barrage of brightness values when the user drags the brightness slider or presses brightness keys several times in short succession, that's what the delay achieves.

We adopted this behavior and timeout value from Dell's tools for Windows. It's a little annoying because the monitor could react much faster, but still better than getting user reports in a few years saying "Plasma killed my monitor early".

Now, one could make an argument for making this configurable. I think the current Display Configuration settings page is already way overloaded, if we decide to add such a setting then we should probably wait until that page gets reworked into several sub-category tabs or such. I'm not 100% convinced it's necessary, and might not work on that kind of checkbox myself, but interested in hearing other opinions.

(In reply to John Kizer from comment #5)
> Do the ideas on that page and in this thread - https://github.com/rockowitz/ddcutil/issues/240 - around changing the --sleep-multiplier setting change the response time?

I've tried a bunch of different values for `--sleep-multiplier` and other args but it makes no difference. Using `--bus` makes ddcutil faster, as mentioned in that issue and in this issue's description, but it can't be applied to powerdevil or system-wide like other args. I'm on Arch which I've added to the bug report now.

(In reply to Jakob Petsovits from comment #6)

Thanks very much for the feedback on this Jakob. However I'm not convinced this is the cause of the issue at hand, or at least not the full picture.

First off, brightness updates using the slider were much quicker right before the update you pushed for https://bugs.kde.org/show_bug.cgi?id=481793. Was it your patch that also added the 1 second delay?

Secondly, the delay isn't only seen when using the slider. It's also present when changing brightness via dbus, e.g. `qdbus6 org.kde.ScreenBrightness /org/kde/ScreenBrightness/display0 SetBrightness 50 0`. Nate added the "with slider" to the title of this bug, which I've removed now because I don't think it's accurate.

I understand not wanting to spam monitors with ddc messages when moving the slider. But surely the timeout should only apply to the brightness slider widget then and not to any brightness change made with powerdevil?

(In reply to Saul Fautley from comment #7)
> Thanks very much for the feedback on this Jakob. However I'm not convinced
> this is the cause of the issue at hand, or at least not the full picture.

Thanks, yeah, I seem to have judged too quickly, especially in light of you having already tracked down the commit that caused this for you.

> First off, brightness updates using the slider were much quicker right
> before the update you pushed for
> https://bugs.kde.org/show_bug.cgi?id=481793. Was it your patch that also
> added the 1 second delay?

No, the timeout was added by this commit, initially for Plasma 6.1: https://invent.kde.org/plasma/powerdevil/-/commit/fd277d84bc0f5ddf55075a23a479f2944eaabee7

The commit that fixed Bug 481793 was merged to the master branch a few weeks after the timeout was introduced, but it was backported to 6.0.4 whereas the timeout was not. So users would have had access to the fix before the timeout appeared. Not sure how you tracked down the exact culprit, just note that both happened within a month from each other.

> Secondly, the delay isn't only seen when using the slider. It's also present
> when changing brightness via dbus, e.g. `qdbus6 org.kde.ScreenBrightness
> /org/kde/ScreenBrightness/display0 SetBrightness 50 0`. Nate added the "with
> slider" to the title of this bug, which I've removed now because I don't
> think it's accurate.

Yep, the slider is only one way to interface with the PowerDevil's brightness service and does not add any extra delay. Thanks for the correction.

> I understand not wanting to spam monitors with ddc messages when moving the
> slider. But surely the timeout should only apply to the brightness slider
> widget then and not to any brightness change made with powerdevil?

I was pondering that as well at the time and you do have a point. Part of why we picked the current approach was certainly because it was easier to implement, and still possible to improve on later if necessary. Part of it comes down to, is the applet the only spot we worry about? What about dimming (mainly handled by KWin), or multiple brightness keys in quick succession (handled by the daemon), or another app entirely? Are we expecting all of them to think about these issues from the start?

Back at the time, I was prototyping/suggesting an extra parameter for the setBrightness() operation that would specify the type of brightness change requested - one-shot like with power state profile changes, or continuous like with the slider. It got a little abstract (among other things, laptop backlight displays need different behavior than external monitors) and I didn't move forward with it in the end as I felt there were more important things to tackle.

Also, Qt's slider component doesn't have an API to let me know when the mouse button / touchpad finger is pressed down and raised back up again, and also there's scroll wheel events which obviously don't have an explicit beginning or end. So in the end it's murky anyway, and questionable whether I'd even get the API right to make it actually good. But I'd be happy to review patches from someone else who wants to take a new shot at limiting the delay only to slider and slider-like movement.

So, to backtrack to the original bug report: At which point would we consider this bug closed?

- Once the 1-second delay is configurable via powerdevilrc?
- Once the 1-second delay is configurable via UI?
- Once `--disable-cross-instance-locks` is configurable via UI? (Note that powerdevil will already use any existing ddcutilrc.)
- All of the above?

> As a bonus, if it's possible to cache and use the `--bus` arg when opening monitor handles with libddcutil, it may speed it up as mentioned in the docs below. Using this with ddcutil cli reduces update time by over 50%.

I'm not sure about that, because we only initialize libddcutil once at session startup and then it just keeps running. The ddcutil CLI needs to re-initialize it every time, so caching would make sense there, but if it's never closed then we don't need to remember any `--bus` value? Or am I missing something?

Thank you very much for the insight on this Jakob.

You are 100% correct that this is due to the delay timeout, and in fact not due to your fix for bug 481793. I made an assumption based on updating shortly after your patch and didn't consider it might be due to a different change. I've confirmed this by rebuilding powerdevil with a shorter delay and brightness updates are virtually instant now.

Personally I'd consider this fixed if the delay could be configured in any way (powerdevilrc would work great imo). I don't think a UI is necessary at all, since there's likely not too many people that care enough, and for the ones that do and are willing to take the risk of a reduced delay, editing a config file is more than easy enough. I don't think any other updates or APIs are warranted to be honest.