Bug 493844

Summary: Brightness slider for connected TV is duplicated
Product: [Plasma] Powerdevil Reporter: Bart Ribbers <bribbers>
Component: generalAssignee: Plasma Bugs List <plasma-bugs>
Status: RESOLVED FIXED    
Severity: normal CC: jpetso, natalie_clarius, nate, slartibart70, username85312
Priority: NOR    
Version: 6.1.90   
Target Milestone: ---   
Platform: Other   
OS: Linux   
Latest Commit: Version Fixed In: 6.2.1
Sentry Crash Report:
Attachments: brightness and color

Description Bart Ribbers 2024-09-29 18:28:00 UTC
Running the Plasma 6.2 beta, I noticed that for my TV Plasma shows 2 brightness sliders. One is currently set to 60% (the actual brightness of the TV), the other to 100%. If I drag the top one around the brightness changes and the bottom one stays untouched. If I drag the bottom one around it also drags the top one with it.
Comment 1 Nate Graham 2024-09-30 19:39:08 UTC
I'm assuming you don't actually have more than one screen connected, right? If you do, the presence of one slider per screen is intentional, and the bug is that one of then is broken.
Comment 2 Bart Ribbers 2024-09-30 19:43:06 UTC
I think you misunderstood. There are 2 sliders _for the same screen_. I do have 2 other screens plugged in and they have their own perfectly functioning sliders. But the 3rd screen gets 2 sliders which has the broken behaviour I mentioned so the applet ends up with 4 sliders in total for 3 screens.

Interestingly enough that's what happened yesterday, right now I just see the 3 sliders as expected. I'm not sure what the difference here is.
Comment 3 Nate Graham 2024-10-03 21:41:20 UTC
I see. How strange!

Could you paste the output of `kscreen-doctor -o`?
Comment 4 Jakob Petsovits 2024-10-03 23:01:40 UTC
I can reproduce something similar, except for "Built-in Screen". Reproduction steps (tested on a Wayland session):

1. Laptop with one monitor plugged in, both screens enabled.
2. In Power Management settings, set lid action to "Sleep" and check "Even when an external monitor is connected".
3. Suspend the laptop by closing the lid.
4. Disconnect monitor cable.
5. Wake up by raising the lid.

OBSERVED:
Built-in Screen exists twice now. (Also, the external monitor brightness slider is gone, but that's expected.) Like Bart says, moving the bottom slider also moves the top one with it, but moving the top slider does not move the bottom slider. Both will affect brightness. Lists by kscreen-doctor and org.kde.ScreenBrightness display names are correct.

EXPECTED:
Only one slider, for Built-in Screen, remains.

Let's do some debugging to see if this is a bug in the applet or in sending D-Bus DisplayAdded/DisplayRemoved signals.
Comment 5 slartibart70 2024-10-06 10:09:31 UTC
same here:
After initial login, i have to sliders for internal screen (laptop) and external (usb-c)
Let the machine sleep, and afterwards we have 2 sliders for usb-c connected screen.

And, the display brightness is way off:
On the monitor-controls, the setting is: brightness: 30
Plasma shows initially: built-in: 20%, HP/usb-c: 50%

Overall brightness is as i want it.
After sleep, the external monitor is way too dark, i even need to put it to over 70% to get a similar brightness as before.

This is just awful... how can i stop plasma from handling the brightness? (any off-switch somewhere?)

===

kscreen-doctor -o
Output: 1 eDP-1
        enabled
        connected
        priority 1
        Panel
        Modes:  1:2880x1800@90*!  2:2880x1800@60  3:1920x1200@90  4:1920x1080@90  5:1600x1200@90  6:1680x1050@90  7:1280x1024@90  8:1440x900@90  9:1280x800@90  10:1280x720@90  11:1024x768@90  12:800x600@90  13:640x480@90  14:1600x1200@60  15:1280x1024@60  16:1024x768@60  17:2560x1600@60  18:1920x1200@60  19:1280x800@60  20:2880x1620@60  21:2560x1440@60  22:1920x1080@60  23:1600x900@60  24:1368x768@60  25:1280x720@60 
        Geometry: 0,0 2304x1440
        Scale: 1.25
        Rotation: 1
        Overscan: 0
        Vrr: incapable
        RgbRange: unknown
        HDR: disabled
        Wide Color Gamut: disabled
        ICC profile: none
        Color profile source: sRGB
        Brightness control: supported, set to 20%
Output: 2 DP-5
        enabled
        connected
        priority 2
        DisplayPort
        Modes:  26:3840x2160@60*!  27:3840x2160@30  28:1920x1200@60  29:1920x1080@60  30:1920x1080@60  31:1920x1080@60  32:1920x1080@50  33:1600x1200@60  34:1680x1050@60  35:1600x900@60  36:1280x1024@60  37:1440x900@60  38:1280x800@60  39:1280x720@60  40:1280x720@60  41:1280x720@60  42:1280x720@50  43:1024x768@60  44:800x600@60  45:720x576@50  46:720x576@50  47:720x480@60  48:720x480@60  49:720x480@60  50:720x480@60  51:640x480@60  52:640x480@60  53:640x480@60  54:720x400@70  55:1600x1200@60  56:1280x1024@60  57:1024x768@60  58:2560x1600@60  59:3200x1800@60  60:2880x1620@60  61:2560x1440@60  62:1920x1080@60  63:1600x900@60  64:1368x768@60  65:1280x720@60 
        Geometry: 2304,0 3840x2160
        Scale: 1
        Rotation: 1
        Overscan: 0
        Vrr: incapable
        RgbRange: unknown
        HDR: incapable
        Wide Color Gamut: incapable
        ICC profile: none
        Color profile source: sRGB
        Brightness control: supported, set to 78%

Operating System: Fedora Linux 40
KDE Plasma Version: 6.2.0
KDE Frameworks Version: 6.7.0
Qt Version: 6.7.2
Kernel Version: 6.10.13-200.fc40.x86_64 (64-bit)
Graphics Platform: Wayland
Processors: 16 × AMD Ryzen 7 PRO 7840U w/ Radeon 780M Graphics
Memory: 58,5 GiB of RAM
Graphics Processor: AMD Radeon 780M
Manufacturer: LENOVO
Product Name: 21K5000JGE
System Version: ThinkPad P14s Gen 4
Comment 6 slartibart70 2024-10-06 10:11:15 UTC
Created attachment 174478 [details]
brightness and color
Comment 7 slartibart70 2024-10-06 10:25:47 UTC
even with the increased brightness (here: 78% or more), the external screen does not return to its initial brightness.
Only logoff/login helps (this is annoying to say the least!!!)

I have already disabled power-management > dim automatically : never
Let's see if this helps

A 'restart' of the brightness of plasma could be very helpful.
Brightnessctl and ddcutil can't oversteer what plasma is doing at the moment... so better to have the plasma 'feature' switchable in the future until testing is thoroughly done and this thing works properly???
Comment 8 Jakob Petsovits 2024-10-07 18:05:03 UTC
(In reply to slartibart70 from comment #7)
> even with the increased brightness (here: 78% or more), the external screen
> does not return to its initial brightness.

That's a different bug. Un-dimming back to a each monitor's original brightness was Bug 493111 which is fixed for 6.2.1, at least as good (or bad) as Plasma ever was at reverting brightness after wake-up. Still needs improvements as mentioned there. There's also Bug 482713 for more robust handling of DDC/CI errors in PowerDevil. There's also https://invent.kde.org/plasma/kwin/-/issues/238 to avoid confusing KWin software brightness fallbacks if PowerDevil does end up losing access to DDC/CI anyway.

Thank you for posting details, and please try to stay on topic within this bug. This one is for duplicated controls only, whereas unexpected brightness differences between applet and monitor hardware should be discussed in a different bug report.
Comment 9 slartibart70 2024-10-07 18:11:24 UTC
yes, thank you for the reminder... things gotten off hand, i was just angry about having the necessary logoff/login to restore initial brightness.
Anyway, the power-options  'dim automatically : never' do help!
I'm hoping for the best with 6.2.1 :-)
Comment 10 Jakob Petsovits 2024-10-07 19:21:35 UTC
(In reply to Jakob Petsovits from comment #4)
> I can reproduce something similar, except for "Built-in Screen".
>
> (...)
>
> Let's do some debugging to see if this is a bug in the applet or in sending
> D-Bus DisplayAdded/DisplayRemoved signals.

QDBusViewer confirms that PowerDevil emits the correct DisplayRemoved signals, meaning no issues in the daemon internally related to this. So the next question was whether the applet will correctly receive these signals and update the display model.

It turns out that the async nature of checkDisplayNames()/updateDisplayNames() in ScreenBrightnessControl is what causes this issue. In a nutshell, the change signals come in fairly quickly, and the use of QCoro means that another call to queryAndInsertDisplay() may be fired before the previous one had finished.

I've got a fix for this, which bluntly rips out the use of async functions where display model changes are involved. Which is most of them, really. I'm sure there's a more efficient fix, but I'll stick to just making it correct for now.
Comment 11 Bug Janitor Service 2024-10-07 22:48:32 UTC
A possibly relevant merge request was started @ https://invent.kde.org/plasma/powerdevil/-/merge_requests/435
Comment 12 Jakob Petsovits 2024-10-07 22:53:11 UTC
(In reply to Jakob Petsovits from comment #10)
> I've got a fix for this, which bluntly rips out the use of async functions
> where display model changes are involved. Which is most of them, really. I'm
> sure there's a more efficient fix, but I'll stick to just making it correct
> for now.

I decided to go for the slightly more efficient fix and submitted the MR with QCoro/async left in place, but still making sure the update isn't executed twice at the same time.
Comment 13 Jakob Petsovits 2024-10-08 04:30:54 UTC
Git commit 7e69b58422bfb6fd99dbc422a00cbe87579b1116 by Jakob Petsovits.
Committed on 07/10/2024 at 22:51.
Pushed by jpetso into branch 'master'.

applets/brightness: Avoid producing duplicate display sliders

Sometimes a display configuration change results in more than one
change signal. These can happen in quick succession of each other.

The async code in ScreenBrightnessControl had a problem with this,
because it would sometimes interleave parts of its model update
function with another call of the same function. This would cause
duplicate brightness sliders to appear for the same display.

The solution is to make sure we don't try to update the model
while another update is already running.

One approach would be to simply take QCoro and async calls out
in favor of synchronous calls which are easier to reason about.
However, this would mean we block plasmashell for a longer time.

Instead, this commit introduces a call guard to the update function
which prevents another asynchronous call from entering its main body
of code while an existing update is already in the works.
After the update has finished, we will repeat the update process
in case another update request had been issued in the meantime.

M  +32   -19   applets/brightness/plugin/screenbrightnesscontrol.cpp
M  +3    -2    applets/brightness/plugin/screenbrightnesscontrol.h

https://invent.kde.org/plasma/powerdevil/-/commit/7e69b58422bfb6fd99dbc422a00cbe87579b1116
Comment 14 Jakob Petsovits 2024-10-08 04:34:49 UTC
Git commit ea91fa1ba728ba387c81c5cce2fde1ca34c0c83f by Jakob Petsovits.
Committed on 08/10/2024 at 04:31.
Pushed by jpetso into branch 'Plasma/6.2'.

applets/brightness: Avoid producing duplicate display sliders

Sometimes a display configuration change results in more than one
change signal. These can happen in quick succession of each other.

The async code in ScreenBrightnessControl had a problem with this,
because it would sometimes interleave parts of its model update
function with another call of the same function. This would cause
duplicate brightness sliders to appear for the same display.

The solution is to make sure we don't try to update the model
while another update is already running.

One approach would be to simply take QCoro and async calls out
in favor of synchronous calls which are easier to reason about.
However, this would mean we block plasmashell for a longer time.

Instead, this commit introduces a call guard to the update function
which prevents another asynchronous call from entering its main body
of code while an existing update is already in the works.
After the update has finished, we will repeat the update process
in case another update request had been issued in the meantime.


(cherry picked from commit 7e69b58422bfb6fd99dbc422a00cbe87579b1116)

Co-authored-by: Jakob Petsovits <jpetso@petsovits.com>

M  +32   -19   applets/brightness/plugin/screenbrightnesscontrol.cpp
M  +3    -2    applets/brightness/plugin/screenbrightnesscontrol.h

https://invent.kde.org/plasma/powerdevil/-/commit/ea91fa1ba728ba387c81c5cce2fde1ca34c0c83f
Comment 15 Nate Graham 2024-10-11 18:15:56 UTC
*** Bug 494552 has been marked as a duplicate of this bug. ***