Small color refactoring by Dudemanguy · Pull Request #17395 · mpv-player/mpv

Dudemanguy · 2026-02-10T22:57:27Z

cc anybody that has hardware to make sure this actually works. In theory, there should be no functional change and hdr should still work on drm and wayland.

The main goals here are to use target_csp for drm, introduce set_color, and use that for wayland and drm.

github-actions · 2026-02-10T23:49:40Z

Download the artifacts for this pull request:

Windows

macOS

video/out/gpu/context.h

kasper93 · 2026-02-11T00:54:30Z

video/out/gpu/context.h

    pl_color_space_t (*preferred_csp)(struct ra_ctx *ctx, const struct pl_color_space source);

+    // See ra_swapchain_fns.set_color. Optional.
+    void (*set_color)(struct ra_ctx *ctx);


This mostly overlap my work in #17345 where I intended to add VOCTRL_COLOR_SPACE_HINT to handle colorspace switching. I'm happy to let you do the changes here.

However, set_color shouldn't depend on target_parameters. Set color should negotiate parameters, not the other way around.

We have it backward currently for DRM, but instead of tangling it more we should untangle it.

How it should work:

target_csp gives us some target display parameters or in case of Wayland compositor preferred colorspace

We do some mangling of this metadata (not important)

We call colorspace_hint
This in case of libplacebo impl is pl_swapchain_colorspace_hint, in our case it can be voctrl or set_color function

We get swapchain configured colorspace. In case of libplacebo this is given in target frame by pl_swapchain_start_frame. We could mimic that, but frankly it's probably enough if set_color from 3. returns configured (or will be configured) colorspace based on the request

We use information from 4. to render the image and set target_params. Target params are informational mostly, should not be used in color selection process. But they could be in which case, they should be managed by whatever is managing the color. For example not in gpu-next which is only updating them, because we set color inside of libplacebo. But we have wiggle room here, it's not important how exactly this works, so I guess whatever is easier.

Key point is that we need to have an api to set color and get the feedback what color we should render to. And we should not have circular dependency of target_params. They were only added to show parameters in stats.lua in fact ;p

That's mostly the logic flow I had in mind. Currently my problem is that the equivalent to set_color that's currently being used is being set after the page flip. It should really be before that. I think that's why DRM is off by one frame. For wayland, it does not matter so much since we currently offload to mesa for vulkan and then vo_dmabuf_wayland is its own thing. But if we ever want to pass through, it should be fixed.

With regards to target_params, we're already using them everywhere downstream to backends as the source of truth for whatever colorspace to use. We could change that to something else if you want, but there would still need to be some params defined somewhere that are known to be good to use for this purpose.

In terms of wayland, all the various color APIs basically do at the end of the day is tell the compositor what we're rendering as. They don't actually negotiate anything beyond that so imo it makes the most sense for vo_gpu_next to have all the logic on what to pick. I guess you could theoretically want some kind of fallback logic if the compositor doesn't support the desired colorspace so you pick the "next best one" or something, but that doesn't really seem worth it.

That's mostly the logic flow I had in mind. Currently my problem is that the equivalent to set_color that's currently being used is being set after the page flip.

That's why 3. happens before start_frame (any draw or flip). It doesn't have to apply the color immediately, but it should remember the request and apply at best convenience, from api point of view it's not important as long as it does happen before the flip as you said.

With regards to target_params, we're already using them everywhere downstream to backends as the source of truth for whatever colorspace to use. We could change that to something else if you want, but there would still need to be some params defined somewhere that are known to be good to use for this purpose.

I know, but it was kinda a shortcut to avoid adding api, which now we are adding. The dependency should be reversed. First we "hint" colorspace, ask if it's applied and render to this colorspace. Instead of render to this colorspace and set it based on whatever was rendered. This gives us more flexibility.

Remember that most of those target parameters can be override by user, so while target_csp colorspace would be supported after user options it might not be anymore and we shouldn't just render garbage in this case.

In terms of wayland, all the various color APIs basically do at the end of the day is tell the compositor what we're rendering as. They don't actually negotiate anything beyond that so imo it makes the most sense for vo_gpu_next to have all the logic on what to pick. I guess you could theoretically want some kind of fallback logic if the compositor doesn't support the desired colorspace so you pick the "next best one" or something, but that doesn't really seem worth it.

Sure, it can be as simple as, "this is not supported, render sRGB". In practice what happens is that we can ask to render A, but if A is not supported we need to render B as a fallback.

video/out/drm_common.c

Dudemanguy · 2026-02-11T19:42:55Z

video/out/wayland_common.c

+    if (color_space_changed)
+        set_color_management(wl, hint);
+    if (color_repr_changed)
+        set_color_representation(wl);


Also I forgot this existed which kind of messes up this API. This one needs repr so passing down the colorspace hint to it is meaningless. So it still uses target_params. Not sure how we want to handle it.

in practice from all VOs except dmabuf we would output RGB. But to make it complete we can add repr parameter in set_color or add separate set_repr api.

Maybe just image_params with hint and repr in set_color I guess? I don't want to overcomplicate it for just one real user.

I ended up passing the entire params with hint as the color since set_color_representation requires stuff like imgfmt and so on.

video/out/opengl/context_drm_egl.c

video/out/drm_common.c

video/out/vo_gpu_next.c

kasper93 · 2026-02-11T21:25:03Z

video/out/vo_gpu_next.c

-            pl_swapchain_colorspace_hint(p->sw, &hint);
+        if (sw->fns->set_color)
+            sw->fns->set_color(sw, &hint);
+        pl_swapchain_colorspace_hint(p->sw, &hint);


We should really have proper context definition for gpu-next by now, and just implement set_color that calls pl_swapchain_colorspace_hint.

Either way, this has to be else.

if (sw->fns->set_color) sw->fns->set_color(sw, &hint); else pl_swapchain_colorspace_hint

Oh so this is exclusive? We don't call pl_swapchain_colorspace_hint if the backend does stuff? Would this mess with wayland in some way since that currently relies on mesa or does that still work. Sorry for not being privy to the code but I assumed this was what set the swapchain in the underlying vulkan.

It's bit messy, because pl_swapchain_colorspace_hint not only sets color space, but also select backbuffer format. Also for Vulkan on Wayland we have to set VK_COLOR_SPACE_PASS_THROUGH_EXT. But in this case, it would have to be

if (sw->fns->set_color) sw->fns->set_color(sw, &hint); pl_swapchain_colorspace_hint(sw, PASS_THROUGH) else pl_swapchain_colorspace_hint(sw, hint)

which would be possible after https://code.videolan.org/videolan/libplacebo/-/merge_requests/796 (I think I will add sw param to always force passthrough)

We are fighting with libplacebo here, sadly we don't have it in our repository to control the code, so it is what it is.

In most cases it's exclusive, because we don't support hinting d3d11 or Vulkan. Only Wayland directly, in which case we shouldn't hint Vulkan.

kasper93 · 2026-02-11T21:27:10Z

video/out/vo_gpu_next.c

    struct pl_frame target;
    pl_frame_from_swapchain(&target, &swframe);
-    bool strict_sw_params = target_hint && !pass_colorspace && p->next_opts->target_hint_strict;
+    bool strict_sw_params = target_hint && p->next_opts->target_hint_strict;


Here if we don't use pl function to set color, we have to do

if (sw->fns->set_color) target.color = hint;

There is strong requirement that set_color updates hint according to own capabilities.

I guess it needs external_colorspace bool, like I did in my PR to make sure we actually called the set_color or maybe sw->fns->set_color && target_hint is enough.

video/out/wayland_common.c

kasper93 · 2026-02-11T21:31:41Z

video/out/wayland_common.c

+    if (color_space_changed)
+        set_color_management(wl, hint);
+    if (color_repr_changed)
+        set_color_representation(wl);


in practice from all VOs except dmabuf we would output RGB. But to make it complete we can add repr parameter in set_color or add separate set_repr api.

video/out/drm_common.c

video/out/opengl/context_drm_egl.c

video/out/drm_common.c

video/out/vo_gpu_next.c

This was originally added way back when specifically for drm so it can play nice with libplacebo for HDR. This is pretty ugly though and a bit hacky. There's a better way to integrate this with target_csp. So drop the pass_colorspace stuff in this commit and we'll change the implementation in the next commit.

For the drm context, we can use the target_csp so vo_gpu_next knows what colorspace to output to. We just need to be sure the monitor is able to display bt2020. For now, all we detect on the EDID side is either pq or hlg so ensure at least one of those works before we send back something to vo_gpu_next to use for target_csp.

video/out/vo_gpu_next.c

Anything related color management gets complex and there is a lot of platform-specific code that gets called in relation to this. Currently, this stuff gets called in various places in the render loop which mostly works but it's prone to "off by 1 frame" errors and also isn't clean. Introduce this helper to hopefully unify all this kind of stuff under one umbrella. For integration with vo_gpu_next, this needs to be called before flip_page and we should stop abusing target_params for this purpose. The next couple of commits will implement this in two places.

With this ordering, we ensure that the colorspace is set before the first atomic commit.

Move this out of swap_buffers. This makes it more consistent with what vo_dmabuf_wayland does.

Depending on the state being true before advancing through frames works but it's error prone and leads to various edge cases you have to account for (e.g. like 339ac9b). It's better invert it so that setting color management is the special case that sets the boolean to true to stall the playloop. The logic can then be simplified, and the vo_wayland_handle_color call is moved up to be directly after we create the buffer.

kasper93

LGTM, thanks!

We can iterate on top of that.

Dudemanguy force-pushed the colorspace-stuff branch from bdc2212 to 0720c19 Compare February 10, 2026 23:08

kasper93 reviewed Feb 11, 2026

View reviewed changes

Dudemanguy force-pushed the colorspace-stuff branch from 0720c19 to a537d37 Compare February 11, 2026 19:38

Dudemanguy commented Feb 11, 2026

View reviewed changes

video/out/drm_common.c Outdated Show resolved Hide resolved

Dudemanguy commented Feb 11, 2026

View reviewed changes

kasper93 mentioned this pull request Feb 11, 2026

vo_gpu_next: set color directly using Wayland protocol #17345

Open

kasper93 reviewed Feb 11, 2026

View reviewed changes

Dudemanguy force-pushed the colorspace-stuff branch 4 times, most recently from 52ccf3b to 6b42cbb Compare February 12, 2026 05:00