wayland: add support for frog-color-management-v1 #14917

Dudemanguy · 2024-09-25T03:31:32Z

Completely untested. I don't even own any HDR displays. In theory, this should do something on Kwin which supports this protocol (didn't check if that is in a release or not yet). Basically this uses frog-color-management to implement a --wayland-colorspace-hint option which is a graphics API-independent way to pass HDR metadata to the compositor.

Some obvious questions (not an exhaustive list) that need to be answered.

Does this even work?
What happens if you set --target-colorspace-hint and --wayland-colorpace-hint at the same time
Is it sufficient to assume we only care about setting primaries/etc. only when it's HDR?
frog_color_managed_surface_set_hdr_metadata doesn't seem to have an obvious way to "unset" itself; is simply setting the primaries and transfer function back to unknown sufficient?
Should we be always setting primaries and transfer?
Currently this uses wl->surface for the frog color surface, but does that even work for dmabuf-wayland (which uses multiple subsurface)?

Any feedback and testing much appreciated. Probably should split the primaries/transfer part from the hdr metadata part now that I look at it again.

llyyr · 2024-09-25T03:34:27Z

Should we be always setting primaries and transfer?

I think so, we have to in order to resolve issues like #14592. There's no reason for --wayland-colorpace-hint to exist, we should always set whatever metadata we have.

Dudemanguy · 2024-09-25T03:51:42Z

I've separated the general colorspace and hdr metadata parts now.

github-actions · 2024-09-25T04:48:31Z

Download the artifacts for this pull request:

Windows

macOS

Dudemanguy · 2024-09-25T15:20:05Z

Tested a bit on KWin 6.1.5. Some observations:

KWin does indeed recognize the protocol and use it (yay)
Setting the transfer function for gpu and gpu-next results in oversaturated colors since KWin does some color-related stuff on its own when you do that. Perhaps that should only be set with the --wayland-colorspace-hint flag and not always like colorspace? (Error on my part: fixed now).
Wasn't able to test hdr on dmabuf-wayland because kwin doesn't support p010 and it gets converted to rgb30 which just isn't workable. But in the colorspace is set along with the hdr metadata so in theory it should work if compositor support is there.
frog-color-management could use more primary enums (easy fix)

kasper93 · 2024-09-26T02:53:55Z

meson.build

@@ -1025,6 +1025,8 @@ endforeach
 features += {'wayland': wayland_deps and wayland['header'] and wayland['scanner'].found()}

 if features['wayland']
+    frog_protocols = dependency('frog-protocols', required: false)


Shouldn't we add meson wrap for those? I think this is recommended way of importing protocols. We could do the same as https://gitlab.freedesktop.org/mesa/mesa/-/blob/e328df0c575a5c193001d06c5659a70d7c071f62/subprojects/frog-protocols.wrap

Well distros are already packaging it. I don't know if we have a stance on including wraps yet. Last time we tried a wrap didn't go so well but maybe we just did it wrong.

options/options.c

video/out/wayland_common.c

Quackdoc · 2024-09-26T19:51:39Z

I am curious, is there any reason to use frog color management and not xx_color_management?

IIRC xx color management is a copy of the PR for upstream and both are supported by kwin.

Dudemanguy · 2024-09-26T20:11:17Z

I am curious, is there any reason to use frog color management and not xx_color_management?

I'm actually literally working on an xx-color-management-v4 branch right now.

rhjdvsgsgks · 2024-09-28T18:43:42Z

video/out/vo_dmabuf_wayland.c

@@ -610,6 +610,7 @@ static void draw_frame(struct vo *vo, struct vo_frame *frame)

    pts = frame->current ? frame->current->pts : 0;
    if (frame->current) {
+        vo_wayland_handle_hdr_metadata(wl);


dmabuf-wayland dont have wl->vo->target_params, which would cause a access to null pointer in vo_wayland_handle_hdr_metadata. thats why im using frame->current->params in #14865 . can we workaround it like this?

Suggested change

vo_wayland_handle_hdr_metadata(wl);

wl->vo->target_params = &frame->current->params;

vo_wayland_handle_hdr_metadata(wl);

rhjdvsgsgks · 2024-09-28T18:50:21Z

Completely untested. I don't even own any HDR displays.

https://invent.kde.org/plasma/kwin/-/merge_requests/6138 can be used for testing hdr on sdr display

Dudemanguy · 2024-09-28T18:59:36Z

I'm probably not going to merge this since #14936 does the same and more.

The big color-management MR in wayland-protocols has been in development for a very long time and honestly that protocol is super complex and painful to parse. The frog protocol stuff is relatively new but it turns out that kwin actually has support for this. Additionally, Arch and Fedora even package these protocols. Given that mpv is certainly a client that benefits from color stuff, it's not too much work to plug in this simple protocol so users can actually start using it. This adds an --wayland-colorspace-hint option which passes hdr metadata to the compositor. Unlike the existing solution (using vulkan), this is completely graphics API agnostic and will work with any wayland backend.

Dudemanguy · 2024-10-10T20:19:54Z

Not sure this has any real utility when compared to xx-color-management, so will close for now. We can revisit later if for some reason it turns out this is useful.

Dudemanguy force-pushed the frog-protocols branch from 2d0e00a to 066c18e Compare September 25, 2024 03:49

Dudemanguy force-pushed the frog-protocols branch 3 times, most recently from f89dc70 to 3fcbe75 Compare September 25, 2024 04:08