Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support color management protocols #8715

Merged
merged 15 commits into from
Jan 7, 2025
Merged

Support color management protocols #8715

merged 15 commits into from
Jan 7, 2025

Conversation

UjinT34
Copy link
Contributor

@UjinT34 UjinT34 commented Dec 14, 2024

Describe your PR, what does it fix/add?

Add support for color management protocols xx-color-management-v4 and frog-color-management-v1. Will only store and pass color management properties. The actual CM should be implemented by someone else with means to verify that it works as expected. Passing hdr metadata without processing should be enough for fullscreen gaming.

experimental:wide_color_gamut sets the colorspace to BT2020_RGB. Might not be needed.
experimental:xx_color_management_v4 enables both protocols. Requires restart.
experimental:hdr forces hdr in desktop mode. Will look weird without proper CM.

Works only with correctly parsed EDID CTA-861 block. Will not work with hdr data from DisplayID blocks.

Is there anything you want to mention? (unchecked code, possible bugs, found problems, breaking compatibility, etc.)

Mostly copied code from other protocols. Might mess with pointers, have some redundant stuff and miss some bits.
Tested only the frog protocol. Most of the clients support both or frog only and will choose frog by default.

Is it ready for merging, or does it need work?

Ready. Requires hyprwm/aquamarine#112

  • xx-color-management-v4
  • frog-color-management-v1
  • send hdr metadata

@vaxerski
Copy link
Member

ref #4933

@vaxerski
Copy link
Member

I'd also mark this as draft

@UjinT34
Copy link
Contributor Author

UjinT34 commented Dec 15, 2024

Frog CM can be tested. Most of the gaming stuff https://wiki.archlinux.org/title/KDE#HDR should work the same. I use https://github.com/Zamundaaa/VK_hdr_layer

Will only work with successfully parsed EDID and fullscreen applications.

@vaxerski
Copy link
Member

idek how to test it or how to spot the difference. Only piece of equipment I have with hdr is my laptop

@UjinT34
Copy link
Contributor Author

UjinT34 commented Dec 15, 2024

AQ_TRACE=1 HYPRLAND_TRACE=1 and believe what logs say %)
Without the support hdr setting in games is not available. With incorrect support enabling hdr will result in obviously wrong colors (undersaturated/oversaturated/too dim/too bright). The tricky part is to spot slightly wrong colors. I guess some colorimetry equipment might be needed.
Laptop might not work hyprwm/aquamarine#116. Mine doesn't.

@vaxerski
Copy link
Member

well then I can't test. My monitors are old, garbage TN panels. No HDR here.

@abihf
Copy link

abihf commented Dec 16, 2024

Nice work @UjinT34

I tried mpv and Silent Hill 2, they seem work. But I need to manually set experimental:hdr=true . Also my second monitor isn't back to sdr after I disable the option.

I will try to compare the color with KDE later.

EDIT: I was wrong, I don't need to enable desktop hdr.

@TKasperczyk
Copy link

TKasperczyk commented Dec 18, 2024

I confirmed it's working in MPV using the following demo: https://4kmedia.org/lg-new-york-hdr-uhd-4k-demo/

Log samples:

[TRACE] ColorManagement supportsBT2020 true, supportsPQ true
[TRACE] ColorManagement primaries 0.677734375,0.3134765625 0.283203125,0.6484375 0.1484375,0.068359375 0.3125,0.3291015625
[TRACE] ColorManagement max avg 417.7095, min 0.12436535, max 417.7095, brightness 0.5
[LOG] [AQ] drm: Modesetting DP-2 with [email protected]
[TRACE] [AQ] drm: Committed a buffer, updating state
[TRACE] [AQ] drm: CDRMFB: buffer has drmfb attachment with fb 5c85274f8e20
[TRACE] [AQ] Connector blob id 115: clock 592000, 2560x1440, vrefresh 144, name: 2560x1440
[ERR] [AQ] atomic drm: setting hdr min 622, max 209, avg 209, content 209, primaries 33887,15674 14160,32422 7422,3418 33887,15625

I tested the video simultaneously in Hyprland and i3, switching between them to compare on the same monitor (AG322QC4). With hyprshade disabled, there was a subtle but noticeable difference - the colors were more vivid on Hyprland. To verify, I relaunched it from the main branch and performed the same comparison. This time, I observed no difference compared to i3, confirming that the changes made to aquamarine and hyprland were successful. Well done @UjinT34 !
Enabling experimental:hdr caused color distortion, as expected, so I kept it disabled.
GPU: GTX1080 with nvidia-dkms 565.77-2.

Regarding the last log entry in the sample, was AQ_LOG_ERROR used for debugging purposes? Relevant code part

@UjinT34 UjinT34 changed the title WIP Support color management protocols Support color management protocols Dec 18, 2024
@UjinT34
Copy link
Contributor Author

UjinT34 commented Dec 18, 2024

Regarding the last log entry in the sample, was AQ_LOG_ERROR used for debugging purposes? Relevant code part

Fixed it

@hanatos
Copy link
Contributor

hanatos commented Dec 21, 2024

I guess some colorimetry equipment might be needed.

i have access to an (ex-) xRite i1 DisplayPro colorimeter. is there anything in particular you'd want me to test? displaycal offers profile verification using a firefox window. would ff be colour managed here? i think not.. otherwise i could verify.

@nnra6864
Copy link
Contributor

well the thing is, at least for my monitor, you'd need to fullscreen the HDR content, and it will also black out my monitor for around 1s which imo is kinda annoying

Exact same for me.
I thought this was expected behavior tho, am I wrong?

@littleblack111
Copy link
Contributor

littleblack111 commented Dec 31, 2024

well the thing is, at least for my monitor, you'd need to fullscreen the HDR content, and it will also black out my monitor for around 1s which imo is kinda annoying

Exact same for me. I thought this was expected behavior tho, am I wrong?

ya its expected behavior, but still wanna mitigate it somehow though

also

https://drmdb.emersion.fr/properties/3435973836/DEGAMMA_LUT (not supported by AQ)

so that would be another thing that we need to work on AQ?

@UjinT34
Copy link
Contributor Author

UjinT34 commented Jan 3, 2025

so that would be another thing that we need to work on AQ?

Added support output->state->setDeGammaLut

@vaxerski should be ready for merging. Nothing to do here from the protocol perspective. At least until https://gitlab.freedesktop.org/wayland/wayland-protocols/-/merge_requests/14 is merged and some CM is ready.

@Trimutex
Copy link
Contributor

Trimutex commented Jan 3, 2025

Great work getting this set up! I like using it already.

Something I want to mention though and want to double check: experimental:hdr = true results in a stacking cpu usage increase to Hyprland. The other options as well as in-game options don't affect this and turning of HDR doesn't clear it out and it requires a Hyprland restart. Stacking is around 30% every 15-20m of playing a game.

Is this something other's experience with it? Running an Intel Arc using Xe so want to check if its a driver level issue.

@abihf
Copy link

abihf commented Jan 4, 2025

@Trimutex I have 2 HDR monitors. After playing HDR content (mpv or games), one can't go back to sdr, the other can but the color is over-saturated. To workaround it, I cycled wide_gammut config between true and false without restarting Hyprland.

I personally patch aquamarine to enable wide_gammut only when hdr metadata is exist

@UjinT34
Copy link
Contributor Author

UjinT34 commented Jan 4, 2025

Great work getting this set up! I like using it already.

Something I want to mention though and want to double check: experimental:hdr = true results in a stacking cpu usage increase to Hyprland. The other options as well as in-game options don't affect this and turning of HDR doesn't clear it out and it requires a Hyprland restart. Stacking is around 30% every 15-20m of playing a game.

Is this something other's experience with it? Running an Intel Arc using Xe so want to check if its a driver level issue.

Does this happen in kwin? This code creates hdr metadata and submits it for each frame. Same as kwin. It is the only thing I suspect to cause any cpu issues. Doesn't happen to me on nvidia. Note that some kwin settings might persist after kwin exits, better to disable hdr before exiting.

If HDR mode is not cleared than it is likely a driver or a monitor bug. Cycling wide_color_gamut probably doesn't do anything special. Looks like something needs to trigger modesetting.

@abihf
Copy link

abihf commented Jan 4, 2025

@Trimutex
Copy link
Contributor

Trimutex commented Jan 4, 2025

Great work getting this set up! I like using it already.
Something I want to mention though and want to double check: experimental:hdr = true results in a stacking cpu usage increase to Hyprland. The other options as well as in-game options don't affect this and turning of HDR doesn't clear it out and it requires a Hyprland restart. Stacking is around 30% every 15-20m of playing a game.
Is this something other's experience with it? Running an Intel Arc using Xe so want to check if its a driver level issue.

Does this happen in kwin? This code creates hdr metadata and submits it for each frame. Same as kwin. It is the only thing I suspect to cause any cpu issues. Doesn't happen to me on nvidia. Note that some kwin settings might persist after kwin exits, better to disable hdr before exiting.

If HDR mode is not cleared than it is likely a driver or a monitor bug. Cycling wide_color_gamut probably doesn't do anything special. Looks like something needs to trigger modesetting.

Tested on KDE Plasma 6, the CPU usage increase doesn't occur there.

Hyprland usage increases to 100% and stays there requiring a restart, affecting stuff like cursor movement as if the desktop is struggling. Maybe this is a driver bug that Plasma accounts for? Not too sure where to check deeper on what's keeping Hyprland busy.

Side note to clarify: HDR toggle works and it clearly goes back to SDR, Hyprland CPU usage just stays at the 100%.

@vaxerski
Copy link
Member

vaxerski commented Jan 5, 2025

needs aq dep bump, passing ci (possibly pull aq git) and resolving of the cpu issue. I'll test the MR later today or tomorrow and review this

@Trimutex
Copy link
Contributor

Trimutex commented Jan 6, 2025

Some additional notes:

  • CPU usage increase with HDR doesn't need to be playing any game
  • Having applications open (i.e. Discord) and closing them as time continues cleans up some of the CPU usage increase
  • Render time increases with CPU usage

@vaxerski
Copy link
Member

vaxerski commented Jan 6, 2025

I have tested the MR and it... seems to work more or less. Although I don't really use HDR and stuff, I can clearly see the colorspace difference when enabling HDR and target colorspace vs not doing so. No-HDR one is overexposed and too bright.

src/desktop/WLSurface.hpp Outdated Show resolved Hide resolved
src/protocols/ColorManagement.cpp Outdated Show resolved Hide resolved
src/Compositor.hpp Outdated Show resolved Hide resolved
src/protocols/ColorManagement.cpp Outdated Show resolved Hide resolved
src/protocols/ColorManagement.cpp Outdated Show resolved Hide resolved
src/protocols/ColorManagement.cpp Outdated Show resolved Hide resolved
src/protocols/ColorManagement.hpp Outdated Show resolved Hide resolved
src/protocols/ColorManagement.hpp Outdated Show resolved Hide resolved
src/protocols/ColorManagement.cpp Show resolved Hide resolved
src/protocols/FrogColorManagement.cpp Outdated Show resolved Hide resolved
Copy link
Member

@vaxerski vaxerski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm, a nice round MR to merge.

image

Just needs a wiki mr.

@UjinT34
Copy link
Contributor Author

UjinT34 commented Jan 7, 2025

hyprwm/hyprland-wiki#932

@vaxerski
Copy link
Member

vaxerski commented Jan 7, 2025

we ball

@vaxerski vaxerski merged commit 830350a into hyprwm:main Jan 7, 2025
11 checks passed
@izmyname
Copy link
Contributor

izmyname commented Jan 7, 2025

That's impressive. AFAIK, only kwin supported HDR, until now

@iddm
Copy link
Contributor

iddm commented Jan 8, 2025

I am curious about this: do we need support from Firefox/Chromium/etc to enable us to view YouTube (or any other streaming content) in HDR?

@knoxode
Copy link

knoxode commented Jan 8, 2025

I am curious about this: do we need support from Firefox/Chromium/etc to enable us to view YouTube (or any other streaming content) in HDR?

As far as I understand the wayland client needs to support HDR, so in the case of linux, yes Firefox needs to support the underlying protocol, before you can "just use" HDR on youtube in Firefox, atleast (I don't use chrome).

@ninetailedtori
Copy link

So dynamic HDR should be supported by Hyprland as of this merge? Or is this just the color management protocols that got fixed up? Static HDR is still crispy fried oversaturation for me, just tryna get on the same page with regards to this PR as I'm a little new here to hyprland :)

@knoxode
Copy link

knoxode commented Jan 9, 2025

So dynamic HDR should be supported by Hyprland as of this merge? Or is this just the color management protocols that got fixed up? Static HDR is still crispy fried oversaturation for me, just tryna get on the same page with regards to this PR as I'm a little new here to hyprland :)

I'm not really a 'developer' so take my words with a grain of salt but to reiterate - if the client supports it then it will work.

Just based on the naming scheme alone, I would hazard a guess that this supports the CM protocols, nothing more, nothing less.

Further implementations beyond CM protocol support surely require more code to be written - either in hyprland, or in the wayland client.

@UjinT34
Copy link
Contributor Author

UjinT34 commented Jan 9, 2025

It supports CM protocols and passes hdr metadata as is. Should be enough for HDR10 which is static. Afaik HDR10+ (dynamic) isn't supported by the linux kernel.
If by dynamic you mean auto switch on and off than it will work for hdr mode unless you force in on with experimental:hdr. experimental:wide_color_gamut doesn't have any automation.

@caffeine01
Copy link

caffeine01 commented Jan 9, 2025

YO i was literally just thinking "i wonder if there is any progress on hyprland hdr", i see that this has been merged, click, DAMN

@caffeine01
Copy link

yo wtf why these colours look goofy asf

@caffeine01
Copy link

caffeine01 commented Jan 9, 2025

everything is incredibly oversaturated with experimental:hdr set to true, ebdbb2 looks almost neon highlighter yellow, tbf probably a monitor issue as its a fucking LG Ultragear scam hdr monitor
EDIT:
nvm, some of it was down to wlsunset, but still the contrast is fucked, the outside of text is like pixelated and shit

@ninetailedtori
Copy link

It supports CM protocols and passes hdr metadata as is. Should be enough for HDR10 which is static. Afaik HDR10+ (dynamic) isn't supported by the linux kernel.

If by dynamic you mean auto switch on and off than it will work for hdr mode unless you force in on with experimental:hdr. experimental:wide_color_gamut doesn't have any automation.

Forcing it on is still quite crispy, it seems. In terms of passing metadata onto other applications, I'll test with some games and video apps in a bit 🫡

@UjinT34
Copy link
Contributor Author

UjinT34 commented Jan 10, 2025

Forcing it on is still quite crispy, it seems. In terms of passing metadata onto other applications, I'll test with some games and video apps in a bit 🫡

There is no tone mapping if you force it on. It's for testing for now. Will be usable with some CM.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.