wayland: add support for xx-color-management-v4

[Dudemanguy]

This is #14936 but with only the xx-color-management-v4 part since that is used by compositors in the wild nowadays (mutter 47 and kwin 6.2) and users can actually benefit.

@denkdran: If you could test out this branch, that would be good so any necessary fixes/tweaks can be caught.

[github-actions]

Download the artifacts for this pull request:

Windows

• mpv-i686-w64-mingw32
• mpv-x86_64-w64-mingw32
• mpv-x86_64-windows-msvc

macOS

• mpv-macos-13-intel
• mpv-macos-14-arm
• mpv-macos-15-arm

[denkdran]

I gave this a round of testing, I'm observing the same behavior as with #14936

Meaning:

• Works well with vo=gpu-next and vulkan if target-prim=bt.2020 and target-trc=pq are set
• target-colorspace-hint does not seem to have an effect (works with vk_hdr_layer + env var)
• vo=dmabuf-wayland has washed out colors
• vo=gpu flickers between washed out and correct colors with vulkan, no flicker and washed out colors without vulkan

[Dudemanguy]

I added a workaround for ENABLE_HDR_WSI=1 so hopefully it doesn't crash now. Also I made a small change to use a different surface for color management for vo=dmabuf-wayland. Does it make any difference on your end?

[kasper93]

Works well with vo=gpu-next and vulkan if target-prim=bt.2020 and target-trc=pq are set

It doesn't. The metadata is not passed through, it only sends default 0/10000 luminance, but it is not correct and your video will not look correct, even though it might trigger your display HDR mode.

Without VK_hdr_layer it is likely that Vulkan is not exposing HDR colorspaces, for example.

Available surface configurations:
    0: VK_FORMAT_A2R10G10B10_UNORM_PACK32       VK_COLOR_SPACE_SRGB_NONLINEAR_KHR
    1: VK_FORMAT_B8G8R8A8_SRGB                  VK_COLOR_SPACE_SRGB_NONLINEAR_KHR
    2: VK_FORMAT_B8G8R8A8_UNORM                 VK_COLOR_SPACE_SRGB_NONLINEAR_KHR
    3: VK_FORMAT_R8G8B8A8_SRGB                  VK_COLOR_SPACE_SRGB_NONLINEAR_KHR
    4: VK_FORMAT_R8G8B8A8_UNORM                 VK_COLOR_SPACE_SRGB_NONLINEAR_KHR
    5: VK_FORMAT_R16G16B16A16_UNORM             VK_COLOR_SPACE_SRGB_NONLINEAR_KHR
    6: VK_FORMAT_R16G16B16A16_SFLOAT            VK_COLOR_SPACE_SRGB_NONLINEAR_KHR
    7: VK_FORMAT_R5G6B5_UNORM_PACK16            VK_COLOR_SPACE_SRGB_NONLINEAR_KHR

We create swapchain selecting one of those colorspaces. And as direct consequence target_params is set to sRGB (bt.709 + bt.1886 actually). Any HDR metadata is stripped.

Of course setting target-prim=bt.2020 and target-trc=pq forces output to be PQ, but it doesn't pass original video metadata, because it is override by default ones.

The exercise of using color management while the Vulkan colorspace doesn't support HDR is not correct way to do anything here. And while we could hack it to pass around the metadata, until proper HDR surfaces are provided, it doesn't make sense.

Correct way in this case is to use VK_hdr_layer and wait for native support for Vulkan HDR surfaces.

target-colorspace-hint does not seem to have an effect (works with vk_hdr_layer + env var)

Yes, it doesn't. If only VK_COLOR_SPACE_SRGB_NONLINEAR_KHR is provided, no HDR will be used. Same reason as above.

vo=dmabuf-wayland has washed out colors

This is the same as flicker that can be observed with vo=gpu and sometimes with vo=gpu-next. Just timing is different. Setting metadata repeatedly is clearing it initially and in case of vo=dmabuf-wayland it is always "unset". Changing it to signal it once make it work correctly.

@Dudemanguy: Could we send metadata only if it has changed from last successful send?

vo=gpu flickers between washed out and correct colors with vulkan, no flicker and washed out colors without vulkan

The same issue as above.

Note that vo=gpu doesn't have any ability to pass-through metadata, it doesn't have --target-colorspace-hint, so in fact it won't set target colorspace correctly. And this likely will not change, as we don't add new features to vo=gpu, especially in HDR area.

EDIT:

After few adjustments it seems to work correctly now, at least for dmabuf-wayland where all metadata is correct. For gpu/gpu-next it would need adjustment to make them aware that downstream can accept PQ colorspace, but like I said before, it is little bit in conflict with Vulkan surfaces at least. I think it is out of the scope of this PR.

[Dudemanguy]

This fixes #12543 as long as the compositor supports the protocol + right formats.

[denkdran]

Thanks a lot @kasper93 for all the explanations, I'm learning a lot here.

Right, I was mostly focused on getting accurate colors, i.e. seeing correct-ish BT.2020/PQ already made me quite happy. I did not check for any other metadata.
Indeed, I see that the available vulkan surfaces are limited to sRGB without vk_hdr_layer, so mpv's behavior makes sense.

Is there a good way to check that the metadata gets passed through fine without relying on how the image actually looks?

Visually, I was not able to make out a difference (might just be my untrained eyes) between vk_hdr_layer and the implementation here + gpu-next. Since you mentioned it - at least on my side I did not observe any flickering with gpu-next in my tests.

@Dudemanguy Retested this PR and results on my side are:

• dmabuf-wayland: colors look correct, target-colorspace-hint works -- I do observe a slight green tint in the bottom OSD, but nothing broken otherwise as far as I can see
• gpu-next + vk_hdr_layer works as expected as well with this change (no error anymore)
• One observation, not sure if a valid case: setting dmabuf-wayland and the env var ENABLE_HDR_WSI=1 leads to washed out colors - I guess it's expected with the current logic. Not sure why someone would do this, I ran into it by accident.

[kasper93]

Indeed, I see that the available vulkan surfaces are limited to sRGB without vk_hdr_layer, so mpv's behavior makes sense.

I was thinking, we might do something to support this case better. But we would need to know that we can signal HDR medatada to compositor, even though surface is sRGB. Could be possible, but little bit hacky. Though might be needed for opengl to make this case work too. Not sure yet, what to do here.

EDIT: Here is upstream work for supporting this in Vulkan, https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/28762 once this is merged, mpv would work without any more changes.

Visually, I was not able to make out a difference (might just be my untrained eyes) between vk_hdr_layer and the implementation here + gpu-next.

The same here. Actually not sure why this happens, maybe max luminance is not used fully, but anyway, we know of gpu/gpu-next limitation now.

Since you mentioned it - at least on my side I did not observe any flickering with gpu-next in my tests.

It was quite rare, but I could see that, but it is fixed now, with the changed Dudemanguy did yesterday. Was the same issue as for vo=gpu just lest frequent in playback.

dmabuf-wayland: colors look correct, target-colorspace-hint works

Just for the record, target-colorspace-hint option is only for gpu-next, nothing else uses it.

I do observe a slight green tint in the bottom OSD, but nothing broken otherwise as far as I can see

Green is chroma zeroed, probably we have dirty line after conversion. Not related to this PR though. If you can report it with more information how to reproduce it, we can see. But likely it is driver bug when uploading the data.

One observation, not sure if a valid case: setting dmabuf-wayland and the env var ENABLE_HDR_WSI=1 leads to washed out colors - I guess it's expected with the current logic. Not sure why someone would do this, I ran into it by accident.

Indeed, whole thing is currently disabled when ENABLE_HDR_WSI is defined. @Dudemanguy could we make it more smart to disable itself only in some cases? Or at least not in dmabuf case?

[Dudemanguy]

Indeed, whole thing is currently disabled when ENABLE_HDR_WSI is defined. @Dudemanguy could we make it more smart to disable itself only in some cases? Or at least not in dmabuf case?

It should not do anything for --vo=dmabuf-wayland now.

Edit: Although I kind of don't like this. It should really only apply on vulkan. I'll redo it in a second.

[Dudemanguy]

OK should only apply on vulkan now.

[kasper93]

No need to check that, the layer will not be available if not set. Avoids some internal knowledge of layer configuration.

Some thoughts about Vulkan suport as a whole. I didn't think much about it, but like mentioned in previous comments, gpu/gpu-next is kind of not compatible with this protocol. vo_gpu generally don't know how to pass hdr metadata, gpu-next relies on hdr vulkan surfaces to be available. Now I'm thinking. To make it work correctly we need to teach --target-colorspace-hint to communicate with wayland output code to request metadata. I'm not sure how to do it nicely, didn't think really about it. My point here is to make it work there need to be some more logic in gpu-next to know that there is possible to notify colorspace.

That being said, we can have current status of this PR, but frankly except for dmabuf_wayland it needs more to work correctly and likely this part of code will have to be removed.

Very high level, I'm thinking.

1. vo.c provides "colorspace hint" available flag and possibly what colorspaces are available
2. gpu-next.c checks this and depending on this and internal (vulkan) state and target-colorspace-hint decides if it should be used. setting some flag in vo context.
3. gpu-next.c is now aware and sets target_params correctly for hdr output
4. this flag checked if color management should be used

I think it should work well.

P.S. Note that while current state will enable hdr mode in output display, the metadata wouldn't be correct, so likely we will get reports about it. Currently only dmabuf_wayland works fully correct.

EDIT: I don't know if you want to look into it. I think we can ignore vo_gpu at this point. For vo_gpu_next it should be little bit plumbing to connect things, but should be ok. Probably not a blocker for this PR, but something to keep in mind.

[Dudemanguy]

No need to check that, the layer will not be available if not set.

The layer shows up for me in the output as long as I have it installed.

I was under the impression it only did actually did something if you set the right parameters for vo_gpu_next (target-colorspace-hint and so on) and that the wayland code didn't need to be aware of this. e.g. if vo_gpu_next doesn't find an actual usable surface, the target params would just be bt.1886 and then this protocol does nothing. I'm aware that you can force a target-prim and get an incorrect result due to driver limitations but I don't think there's really anything we can do about that?

Edit: Basically as long as vo_gpu_next sets the right target params, everything should work fine no? I didn't think we need anything more than that.

[kasper93]

e.g. if vo_gpu_next doesn't find an actual usable surface, the target params would just be bt.1886 and then this protocol does nothing. I'm aware that you can force a target-prim and get an incorrect result due to driver limitations but I don't think there's really anything we can do about that?

Yes, that's correct. But if we want to depend on the driver providing correct Vulkan surface and interface to set HDR parameters. We don't need to set them manually at all. In which case this protocol should be disabled for anything else than dmabuf_waylad.

What I'm saying is that we can still output PQ from gpu-next to "sRGB" sutface, nothing stops us to do that, except that, after we need to use this color management to signal colorspace, because Vulkan driver won't do that for us.

Basically as long as vo_gpu_next sets the right target params, everything should work fine no?

Well, yes... but currently vo_gpu_next will set target params to HDR only if HDR is supported and target-colorspace-hint is enabled and working. Making use of this wayland protocol redundant and even not wanted, because it would overwrite already signaled values by Vulkan.

Basically we can internally do what currently is not supported by Vulkan and won't be ever supported by OpenGL. But this needs some negotiation between vo_gpu_next and wayland_common.c. vo_gpu_next needs to be aware that it can set target_params to some value.

In summary, I think ctx->vo->wl->hdr_wsi should be more generic "enabled" flag and likely disabled by default and enabled only by dmabuf_wayland currently.

[Dudemanguy]

Oh it's pretty simple to just not use the protocol for vo_gpu_next and vo_gpu (?) for now if that's all that ultimately needs to be done. vo_gpu_next doesn't need to be aware of this as it can be completely handled in wayland code.

[kasper93]

Yes, if that's ok with you we can disable it. And note that Vulkan interface is preferred in this case, native or through hdr layer.

[kasper93]

I think vo_wlshm.c should also set target_params from input frame, no?

[Dudemanguy]

Yeah I think this just got mixed around from rebasing from previous changes. Should this just copy what vo_dmabuf_wayland does?

[kasper93]

Yes, I think so.

[Dudemanguy]

I looked at vo_wlshm again and actually it needs a ton of work before it can even be usable. It's hardcoded to use only bgr0 or 0rgb depending on the endian so there's really zero use in exposing any of this color stuff until that gets smarter. I will just drop this to only vo_dmabuf_wayland. We can always add more VOs later when it makes sense.

[Dudemanguy]

Based on all the discussion, now only vo_dmabuf_wayland actually uses this protocol since it is the only VO that's currently in a state where it will benefit from this protocol. vo_wlshm can be reasonably fixed to handle more formats but I will save that for a future change. Not sure yet how we would handle opengl or if we even want to since vulkan can already work without any other changes from us.