Bug 350676

Same here with Macbook Pro 2014 (some intel card).

Max Screen Brightness for me says 100.

*** Bug 352442 has been marked as a duplicate of this bug. ***

The kded output might show something interesting. Output is grouped by pressing the brightness down key.

powerdevil: Screen brightness value:  703
powerdevil: Screen brightness value max:  937

powerdevil: Screen brightness value:  703
powerdevil: Screen brightness value max:  937
powerdevil: set screen brightness value:  656
powerdevil: Screen brightness value:  703
powerdevil: Screen brightness value max:  937

powerdevil: Screen brightness value:  656
powerdevil: Screen brightness value max:  937

powerdevil: Screen brightness value:  656
powerdevil: Screen brightness value max:  937
powerdevil: set screen brightness value:  609
powerdevil: Screen brightness value:  656
powerdevil: Screen brightness value max:  937

I did some investigation on this problem by adding few debug lines, and I nailed it. What is happening here is following:

1. User presses DecreaseBrightness button first time.
- UPower backend starts an QPropertyAnimation (PowerDevilUPowerBackend::setBrightness)
- Animation runs several setBrightness calls 
- Each call produces an XRandR event saying that brightness is changed; but since animation is still running, PowerDevilUPowerBackend::slotScreenBrightnessChanged just ignores it. 
NB: it does not update m_cachedBrightnessMap!!!
2. Uses presses DecreaseBrightness button second time
- UPower backend gets PowerDevilUPowerBackend::brightnessKeyPressed slot called
- since current brightness (which is correct and new value) differs from one cached (cache was not updated; it points to old value), it just updates cache and ignores the call!

3. In case of brightness increase, animation stops BEFORE sending last valueChanged() signal. Therefore it passes the PowerDevilUPowerBackend::slotScreenBrightnessChanged check (animation is no longer running) and updates the cache. 

---
It seems like QPropertyAnimation is somehow buggy. It emits valueChanged signal even without animation actually running, just when startValue and endValue are set. I believe it shouldn't happen; in our case this leads to following behaviour:

powerdevil: set screen brightness value:  422
 --- set start value 469
 --- setting value  422
 --- set end value 422
 --- setting value  469
 --- start

Here I added a debug print right after each line in PowerDevilUPowerBackend::setBrightness; while "setting value" message is being print in animationValueChanged. This actually might lead to  screen brightness flickering, which might be another bug.

Created attachment 94505 [details]
patch that removes QPropertyAnimation

I would prefer removing animation routine at all. I mean, why do we need it? It just makes response to brightness keys slower (due to implemented delay there; and I've seen people complaining about it), not really making it much smoother and more eye-candier. With that patch in my case that bug disappears, removing also all the delays.

Well, imho, such animation is more or less the standard behavior in real world (Windows, Android, ...).

Though from reading our implementation there are definitely some issue here.

stop will not reset the progress, only when start() is called. Adjust the start and end value will change the value since it will be recalculated. E.g when stop is called the progress is 50%, the value will be adjusted after the range is changed, so Qt implementation makes some sense here. But that's not we need.

disconnect the signal after stop() and reconnect before start() may work.

https://git.reviewboard.kde.org/r/125156/

Funny, although after fixing this brightness gets updated correctly, OSD doesn't. It still gets updated each second keypress, but seems like it is another issue somewhere in OSD/QML code. Somehow ProgressBar value (which is its property) changes, but progressbar itself doesn't get repainted. Both in Breeze and Oxygen styles. Weird.

Could it be that you're using EGL? Does the highlight in eg. Kickoff also only update every second item? If so, that's a driver issue.

(In reply to Kai Uwe Broulik from comment #10)
> Could it be that you're using EGL? Does the highlight in eg. Kickoff also
> only update every second item? If so, that's a driver issue.

Yeah, same in kickoff. Thanks for clarifying, will wait for driver updates then :)

BTW, another issue related to animation routine: all keypresses that are happening during animation are ignored (due to the very same check it didn't update the cache). So if I want to considerably reduce my brightness and I press "down" key 5 times in a row, it just reduces my brightness by single step (5%). That's frustrating; instead I guess it should launch new "animation" with endValue being previous endValue-step and startValue being current brightness. Or just update cache so this check doesn't fail.

(In reply to Kai Uwe Broulik from comment #10)
> Could it be that you're using EGL? Does the highlight in eg. Kickoff also
> only update every second item? If so, that's a driver issue.

So glad to learn that here accidentally! I suffered from the same issue for my plasmoid and I had tried to so many hackish way to workaround it but never succeed.

(In reply to Igor Poboiko from comment #11)
> (In reply to Kai Uwe Broulik from comment #10)
> > Could it be that you're using EGL? Does the highlight in eg. Kickoff also
> > only update every second item? If so, that's a driver issue.
> 
> Yeah, same in kickoff. Thanks for clarifying, will wait for driver updates
> then :)
> 
Disable vsync helps me to workaround the issue, you may try that to see if it works for you..

Git commit 1005dd9a25c5fafea1f781c6d2a49fe99e219a34 by Weng Xuetian.
Committed on 12/09/2015 at 01:28.
Pushed by xuetianweng into branch 'Plasma/5.4'.

Fix the brightness update

1. Update to m_cachedBrightnessMap is ignored during the animation. This
change queries the real brightness after animation is finished.
2. setting start/endvalue will also trigger the valueChanged in
QPropertyAnimation, disconnect the slot on valueChanged after stop() and
reconnect it before start().
Related: bug 346456
REVIEW: 125156
FIXED-IN: 5.4.2

M  +3    -0    daemon/backends/upower/powerdevilupowerbackend.cpp

http://commits.kde.org/powerdevil/1005dd9a25c5fafea1f781c6d2a49fe99e219a34

(In reply to Igor Poboiko from comment #11)
> BTW, another issue related to animation routine: all keypresses that are
> happening during animation are ignored (due to the very same check it didn't
> update the cache). So if I want to considerably reduce my brightness and I
> press "down" key 5 times in a row, it just reduces my brightness by single
> step (5%). That's frustrating; instead I guess it should launch new
> "animation" with endValue being previous endValue-step and startValue being
> current brightness. Or just update cache so this check doesn't fail.

FYI, https://git.reviewboard.kde.org/r/125182/

(In reply to Weng Xuetian from comment #12)
> Disable vsync helps me to workaround the issue, you may try that to see if
> it works for you..
No, unfortunately it didn't.

(In reply to Weng Xuetian from comment #14)
> FYI, https://git.reviewboard.kde.org/r/125182/
What's the status of that patch? 
That check was there from very beginning, as far as I can see from git log. 
But I guess it shouldn't be there. Seems like the logic there is following: if cache is somehow broken, we should update a cache, but ignore a keypress. I believe it's not a correct behavior. We should just update a cache and keep going!
And removing that check would have also solved that issue as well.

(In reply to Igor Poboiko from comment #15)
> (In reply to Weng Xuetian from comment #14)
> > FYI, https://git.reviewboard.kde.org/r/125182/
> What's the status of that patch? 
> That check was there from very beginning, as far as I can see from git log. 
> But I guess it shouldn't be there. Seems like the logic there is following:
> if cache is somehow broken, we should update a cache, but ignore a keypress.
> I believe it's not a correct behavior. We should just update a cache and
> keep going!
> And removing that check would have also solved that issue as well.
You may want to comment on the review instead of discuss at the bug report here.
The reason that I don't want to update the cachedBrightness is, other usage of it shows that it always keeps real hardware brightness value with in it is used to whether to send brightness changed signal or not. From existing code, it seems that the code tries to avoid querying hardware value frequently and send a lot of brightness changed during animation.

The reason that the animation value is fake is because something like this may happen: you set 35 to hardware, but hardware probably gives you 30 in the end.