colors change as subtitles display, or when osd is drawn when using dmabuf-wayland

[stereomato]

mpv Information

mpv v0.38.0-dirty Copyright © 2000-2024 mpv/MPlayer/mplayer2 projects
 built on Jul  3 2024 05:59:22
libplacebo version: v7.349.0
FFmpeg version: n7.0.1
FFmpeg library versions:
   libavutil       59.8.100
   libavcodec      61.3.100
   libavformat     61.1.100
   libswscale      8.1.100
   libavfilter     10.1.100
   libswresample   5.1.100

Other Information

• Linux version: Arch Linux
• Kernel Version: Linux hostname 6.10.0-arch1-2 #\1 SMP PREEMPT_DYNAMIC Mon, 22 Jul 2024 17:28:23 +0000 x86_64 GNU/Linux
• GPU Model: Alder Lake-P GT2 [Iris Xe Graphics] [8086:46A6]
• Mesa/GPU Driver Version: Mesa 24.1.4-arch1.2
• Window Manager and Version: mutter 46.3.1
• Source mpv: arch linux package
• Introduced in version: no idea

Reproduction Steps

1. open video using mpv --no-config --dmabuf-wayland $videoFile
2. set mpv to fullscreen mode
3. notice how colors change/flicker as subtitles display, or when the osd is drawn

Expected Behavior

Colors not changing/flickering like that, regardless of fullscreen or windowed mode.

Actual Behavior

Colors change/flicker strangely, when in fullscreen mode, and subtitles display or the osd is drawn.

Log File

output.txt

Sample Files

No response

I carefully read all instruction and confirm that I did the following:

• [X] I tested with the latest mpv version to validate that the issue is not already fixed.
• [X] I provided all required information including system and mpv version.
• [X] I produced the log file with the exact same set of files, parameters, and conditions used in "Reproduction Steps", with the addition of --log-file=output.txt.
• [X] I produced the log file while the behaviors described in "Actual Behavior" were actively observed.
• [X] I attached the full, untruncated log file.
• [X] I attached the backtrace in the case of a crash.

[Dudemanguy]

I'm not able to reproduce on git master or 0.38 on mutter 46.3.1. Flickering when subtitles show sounds like a compositor and/or driver bug to me. We just draw shm buffers for the subtitles/osd and attach it to a different surface and hand that off to the compositor. Could you try a different compositor (weston, kwin, whatever) and see if the same thing happens?

[mahkoh]

GNOME is likely switching between direct scanout and compositing whenever subtitles appear disappear. Alternatively GNOME might be using an overlay plane for the subtitles and that plane might be getting activated/deactivated.

[ghost]

I could reproduce it with weston. Exactly how it happens with Gnome. Also, this only happens with dmabuf-wayland, not with gpu-next. Maybe Intel specific, then?

[llyyr]

can reproduce on wlroots with amdgpu as well. only happens when direct scanout is active

[Dudemanguy]

Well I'm not a compositor developer but I assumed this was supposed to work with direct scanout without any weirdness? In fact, rmader himself was the one that submitted the patch in #12237 to make it so direct scanout was possible on mutter when the the osd was not being shown. I would assume that, at least at the time, nothing was flickering when the osd would hide/appear. @rmader: any insight here?

[rmader]

GNOME is likely switching between direct scanout and compositing whenever subtitles appear disappear.

This is most likely the case and the slight color mismatch is unfortunately expected for the time being until we have the color-management and color-representation protocols. Until then compositors have to guess and unfortunately Mutter and Weston currently default to e.g. bt601 color curves - which is slightly different from bt709.

While we could tweak that - e.g. defaulting to bt709 as much more content uses that - most work in that area just focuses on getting the proper protocols landed/implemented. On the side of Gnome we'll probably be too late for 47, however 48 will likely work out.

[rmader]

P.S.: as an intermediate solution it might make sense to always keep the overlay visible in the given situation. That will increase power consumption accordingly of course.

[Dudemanguy]

Hmm I see. So in that case we can just pretend the osd surface always has contents if the colorspace isn't bt601. Does this work for you guys?

diff --git a/video/out/vo_dmabuf_wayland.c b/video/out/vo_dmabuf_wayland.c
index e2dafdaa6f..6322a02cee 100644
--- a/video/out/vo_dmabuf_wayland.c
+++ b/video/out/vo_dmabuf_wayland.c
@@ -541,7 +541,7 @@ static void resize(struct vo *vo)
     set_viewport_source(vo, src);
 }
 
-static bool draw_osd(struct vo *vo, struct mp_image *cur, double pts)
+static bool draw_osd(struct vo *vo, struct mp_image *cur, enum pl_color_system sys, double pts)
 {
     struct priv *p = vo->priv;
     struct mp_osd_res *res = &p->screen_osd_res;
@@ -562,7 +562,9 @@ static bool draw_osd(struct vo *vo, struct mp_image *cur, double pts)
                                                MP_ARRAY_SIZE(act_rc), &num_act_rc,
                                                mod_rc, MP_ARRAY_SIZE(mod_rc), &num_mod_rc);
 
-    p->osd_surface_has_contents = num_act_rc > 0;
+    // Unfortunately since there is no way to signal the actual colorspace to
+    // compositors, we have to pretend this is always true if it is not bt601.
+    p->osd_surface_has_contents = sys == PL_COLOR_SYSTEM_BT_601 ? num_act_rc > 0 : true;
 
     if (!osd || !num_mod_rc)
         goto done;
@@ -591,6 +593,7 @@ static void draw_frame(struct vo *vo, struct vo_frame *frame)
     struct vo_wayland_state *wl = vo->wl;
     struct buffer *buf;
     struct osd_buffer *osd_buf;
+    enum pl_color_system sys = PL_COLOR_SYSTEM_UNKNOWN;
     double pts;
 
     if (!vo_wayland_check_visible(vo)) {
@@ -614,16 +617,16 @@ static void draw_frame(struct vo *vo, struct vo_frame *frame)
 
         if (buf && buf->frame) {
             struct mp_image *image = buf->frame->current;
+            sys = image->params.repr.sys;
             wl_surface_attach(wl->video_surface, buf->buffer, 0, 0);
             wl_surface_damage_buffer(wl->video_surface, 0, 0, image->w,
                                      image->h);
-
         }
     }
 
     osd_buf = osd_buffer_get(vo);
     if (osd_buf && osd_buf->buffer) {
-        if (draw_osd(vo, &osd_buf->image, pts) && p->osd_surface_has_contents) {
+        if (draw_osd(vo, &osd_buf->image, sys, pts) && p->osd_surface_has_contents) {
             wl_surface_attach(wl->osd_surface, osd_buf->buffer, 0, 0);
             wl_surface_damage_buffer(wl->osd_surface, 0, 0, osd_buf->image.w,
                                      osd_buf->image.h);

It might be worth vendoring in the color management protocol on our side and using it instead of waiting for it to be official.

[rmader]

So in that case we can just pretend the osd surface always has contents if the colorspace isn't bt601.

Yeah, should somewhat work for the time being.

It might be worth vendoring in the color management protocol on our side and using it instead of waiting for it to be official.

That's kinda happening already, however the issue is rather the compositor implementations, not the protocol. At least for Weston and Mutter I can attest though that we're much closer now to have thing working.

If you're interested, here's a small Gstreamer prototype that I'll be further working on next month: https://gitlab.freedesktop.org/gstreamer/gstreamer/-/merge_requests/6830

[Dudemanguy]

For this specific problem, it seems like color-representation would suffice. I would have no problem vendoring that for mpv usage since it's pretty important for our use case and if it would be helpful for compositors.

Of course, color-management is of interest to us as well but that seems quite a bit more complicated.

[Dudemanguy]

Can anyone confirm if this still occurs after #15048? I think it should work on mutter 47 now unless color-representation is also needed.

[yejingchen]

Can anyone confirm if this still occurs after #15048? I think it should work on mutter 47 now unless color-representation is also needed.

I can reproduce this on KWin 6.2.0 and mpv commit 187fffd, but only when:

1. HDR and ICC profile is disabled or set to None;
2. Video pixel format is nv12

mpv Information

mpv v0.39.0-127-g187fffd0f5 Copyright © 2000-2024 mpv/MPlayer/mplayer2 projects
 built on Oct 15 2024 05:20:32
libplacebo version: v7.349.0
FFmpeg version: n7.0.2
FFmpeg library versions:
   libavcodec      61.3.100
   libavdevice     61.1.100
   libavfilter     10.1.100
   libavformat     61.1.100
   libavutil       59.8.100
   libswresample   5.1.100
   libswscale      8.1.100

Other Information

• Linux version: Arch Linux
• Kernel Version: Linux yjc-hpeb 6.11.3-arch1-1 # 1 SMP PREEMPT_DYNAMIC Thu, 10 Oct 2024 20:11:06 +0000 x86_64 GNU/Linux
• GPU Model: Advanced Micro Devices, Inc. [AMD/ATI] Rembrandt [Radeon 680M]
• Mesa/GPU Driver Version: Mesa 24.2.4-arch1.1
• Window Manager and Version: kwin 6.2.0
• Source mpv: https://github.com/archlinuxcn/repo/blob/47faecfd5b15ad2cce8416e3dae5bbd81532f2bd/archlinuxcn/mpv-git/PKGBUILD

[Dudemanguy]

Can you post of a log of that happening? If kwin just happens to not be using its color management stuff at all then that's expected. But I think there's probably a gap item on our end too when it comes to bt. 1886.

[yejingchen]

Can you post of a log of that happening? If kwin just happens to not be using its color management stuff at all then that's expected. But I think there's probably a gap item on our end too when it comes to bt. 1886.

mpv.log

I played the video for a few seconds, triggered the color change, then quit.

[Dudemanguy]

Compositor does not support transfer function: ITU-R Rec. BT.1886 (CRT emulation + OOTF)

Yeah that's the problem. It would be worth writing this transfer function wayland side but kwin doesn't support custom transfer functions yet so it wouldn't solve it. What happens if you try to force a trc? e.g. with --target-trc=gamma2.2?

[yejingchen]

What happens if you try to force a trc? e.g. with --target-trc=gamma2.2?

mpv.2.log

There is still color change, but I noticed that the osd info (press i) the Display transfer function is still bt.1886.

[Dudemanguy]

Oh sorry I forgot that option only does anything for gpu-next. It actually makes sense for vo_dmabuf_wayland to support it too. That's something I should do. If you don't mind, could you hack this patch in real quick? Obviously don't daily drive it.

diff --git a/video/out/wayland_common.c b/video/out/wayland_common.c
index 998fc9ea26..28e359ef51 100644
--- a/video/out/wayland_common.c
+++ b/video/out/wayland_common.c
@@ -2421,6 +2421,7 @@ static int set_colorspace(struct vo_wayland_state *wl)
     struct pl_color_space color = wl->vo->target_params->color;
     int xx_primaries = wl->primaries_map[color.primaries];
     int xx_transfer = wl->transfer_map[color.transfer];
+    xx_transfer = XX_COLOR_MANAGER_V4_TRANSFER_FUNCTION_GAMMA22;
 
     if (xx_primaries == -1)
         MP_VERBOSE(wl, "Compositor does not support color primary: %s\n", pl_color_primaries_name(color.primaries));

[yejingchen]

If you don't mind, could you hack this patch in real quick

Yeah, the color didn't change with osd now, although the i info window still says bt.1886 display transfer.

mpv.3.log

[Dudemanguy]

Thanks. So that means we should just always set something in that case. I wonder what the sanest enum to pick would be. Guess I'll look at what compositors do.

[Dudemanguy]

Mutter on master should work out of the box now with pretty much all real world files since https://gitlab.gnome.org/GNOME/mutter/-/merge_requests/4062. For everything else, you may have to trick the compositor by specifying some other primary and/or transfer function. e.g. using --vf=format:gamma=srgb should work for kwin.

If your compositor doesn't support xx-color-management you are still toast of course but nothing we can do about that.

[yejingchen]

Mutter on master should work out of the box now with pretty much all real world files since https://gitlab.gnome.org/GNOME/mutter/-/merge_requests/4062. For everything else, you may have to trick the compositor by specifying some other primary and/or transfer function. e.g. using --vf=format:gamma=srgb should work for kwin.

If your compositor doesn't support xx-color-management you are still toast of course but nothing we can do about that.

May I ask what it actually means by "map BT.1886 to BT.709"? Does mpv convert the buffer internally to a state suitable for compositor to apply BT.709 TF on it, or mpv just pass the buffer unchanged, letting the compositor apply BT.709 TF when it really should be applying BT.1886 TF?

[Dudemanguy]

We don't do any transformations. mpv just signals the transfer function of the surface to the compositor. For libplacebo, PL_COLOR_TRC_BT_1886 is what is selected for SDR. The wayland protocol doesn't technically have that but instead it has XX_COLOR_MANAGER_V4_TRANSFER_FUNCTION_BT709 as an enum. Someone better at color than me can probably explain the details some more since BT.1886 isn't technically the same as BT.709, but for our purposes here they should be essentially equivalent.

A compositor that gets XX_COLOR_MANAGER_V4_TRANSFER_FUNCTION_BT709 I'd expect should know the surface is SDR and along with the corresponding primary, it should do the right thing.

[yejingchen]

Well I seem to understand now. And it is kinda unfortunate that BT.1886 is not included in H.273 CICP when it is out for over a decade.

[rmader]

While we could tweak that - e.g. defaulting to bt709 as much more content uses that - most work in that area just focuses on getting the proper protocols landed/implemented. On the side of Gnome we'll probably be too late for 47, however 48 will likely work out.

The next versions of Mutter (46.8, 47.4 and 48) will now default to bt709 matrix coefficients and ensure the KMS drivers use the same (instead of relying on their default values). I.e. this issue is now fixed and there shouldn't be any visible difference between compositing and offloading any more, see https://gitlab.gnome.org/GNOME/mutter/-/merge_requests/4204

Work on the color protocols also continues, but might not land fully for 48, in which case e.g. HDR10 content will continue to look wrong - but at least consistent.