--target-colorspace-hint and --idle=yes won't work correctly together

[mitzsch]

Important Information

Provide following Information:

• mpv version: mpv 0.34.0-292-g3458651010
• Windows Version: Windows 10 21H2
• Source of the mpv binary: Windows builds by shinchiro
• GPU: RTX3050

Reproduction steps

Run mpv.com HDR.mkv --vo=gpu-next --target-colorspace-hint=yes --gpu-api=vulkan --idle=yes. Your TV will correctly switch to HDR but after the file ends it will stay in HDR, whereas the same line with --idle=no will set the TV back to SDR after the file is finished. When opening an SDR file after that (with --idle=yes) the TV goes back to SDR and stays there after the SDR file is over.

Expected behavior

Switch back to SDR, when the HDR file ends with --idle=yes

Actual behavior

TV stays in HDR after the HDR file ends with --idle=yes

Log file

I´m sorry. I´ve not pulled the logs from testing. No idea if they are really needed as the issue is easily reproducible with the mentioned command line.

Sample files

Any HDR file will trigger this. (I tried it with the "The World in HDR" mkv file from the Kodi samples)

[haasn]

Does this patch fix it?

diff --git a/video/out/vo_gpu_next.c b/video/out/vo_gpu_next.c
index 628417218b..b4b59b53ef 100644
--- a/video/out/vo_gpu_next.c
+++ b/video/out/vo_gpu_next.c
@@ -876,7 +876,7 @@ static void draw_frame(struct vo *vo, struct vo_frame *frame)
         if (opts->target_trc)
             hint.transfer = mp_prim_to_pl(opts->target_trc);
         pl_swapchain_colorspace_hint(p->sw, &hint);
-    } else if (!p->target_hint) {
+    } else {
         pl_swapchain_colorspace_hint(p->sw, NULL);
     }
 

[mitzsch]

Thanks for your quick reply! Unfortunately, I´m having some compiling issues with the mpv-winbuild-cmake repo currently and can´t test your patch! :/ Maybe someone else can test this?

[mitzsch]

I've managed to get it built with the patch applied. Unfortunately, I´m not at home right now but I will be next week and will test it with the HDR TV. I will report back! :)

If someone else wants to test it in the meantime => here you can download it: mpv-x86_64-20220522-git-9022b1b-idle-hdr-patch

[mitzsch]

Just tried it and it did not work, unfortunately. Same behaviour as before, the TV still stays in HDR after the file ends. :/

Do you have another patch I could try?

Edit: i should have mentioned, when mpv with --idle=yes is stuck in HDR, hitting STRG + C and therefore force quitting mpv, changes the TV back to SDR.

[gbooker]

@haasn Given what the OP is describing, wouldn't this be something that should be handled in uninit and not draw_frame?

Some context which may be of help: The OP's real use case is doing this inside Plex HTPC as described in this forum thread (https://forums.plex.tv/t/hdr-metadata-passthrough-plex-htpc-for-windows/794358). Here MPV is passed an HWND via --wid and it plays there. After playback finishes, the playback stack uninitializes itself until the next playback but the window itself is not destroyed. In this context, the uninit is called (at least I would presume by the log line vo/gpu-next/win32: uninit). This circumstance is decently simulated by using --idle=yes which doesn't destroy the window after playback ends. I suspect that in normal playback MPV is relying on the driver to set the colorspace back to the original value on window destruction. In some ways this is similar to #10191 where MPV was relying on the driver to cleanup a VRAM leak on window destruction.

[haasn]

@gbooker Oh, I see now. I was confusing --idle with --keep-open, which would have kept the vout initialized, but drawing blank frames.

Actually, this is somewhat confusing - since the vout gets uninitialized, the swapchain (and indeed, entire vulkan context), gets uninitialized as well. The fact that this does not reset the HDR mode smells to me like a serious driver bug, actually. However, we could definitely work around it in pl_swapchain_destroy.

[haasn]

diff --git a/src/vulkan/swapchain.c b/src/vulkan/swapchain.c
index 3b66ee63..de117d93 100644
--- a/src/vulkan/swapchain.c
+++ b/src/vulkan/swapchain.c
@@ -409,6 +409,7 @@ static void vk_sw_destroy(pl_swapchain sw)
         vk->DestroySemaphore(vk->dev, p->sems.elem[i].out, PL_VK_ALLOC);
     }
 
+    set_hdr_metadata(p, &pl_hdr_metadata_empty); // remove leftover metadata
     vk->DestroySwapchainKHR(vk->dev, p->swapchain, PL_VK_ALLOC);
     pl_mutex_destroy(&p->lock);
     pl_free((void *) sw);

Does this diff fix it, then? (To libplacebo)

[gbooker]

I'll have to defer to @mitzsch for this testing as I don't have the setup (yet).

[mitzsch]

I will try building it with this patch to libplacebo and report back! Thanks! :)

[mitzsch]

Build it with the patch applied. If you want to test it => https://we.tl/t-wNl6UMsBPi I can only try it in about two weeks. :/

[gbooker]

Actually, this is somewhat confusing - since the vout gets uninitialized, the swapchain (and indeed, entire vulkan context), gets uninitialized as well. The fact that this does not reset the HDR mode smells to me like a serious driver bug, actually.

This may be possible but it could be worse: I did test this on a machine with an Intel GPU (the OP is using Nvidia) without the patch. While it toggles into HDR it doesn't toggle back. In fact, even without idle set it still doesn't toggle back, not even when the app quits. I have to go into the windows color settings and toggle HDR on and then off again to get it off. Same results with the above build, though this could be a different issue than the OP.

As a side not, when using --target-colorspace-hint=yes the playback has a bunch of green lines but I would attribute that to something else.

[mitzsch]

Just tried the build with the patched libplacebo and unfortunately, it still behaves like before the patch. HDR stays on after the file ends with --idle=yes and only after quitting mpv, the TV switched back to SDR ... :/

[gbooker]

I've been starting to think more and more that this could be driver design flaw in relation to the lifetime of the HDR metadata. While the documentation does state The metadata will persist until a subsequent vkSetHdrMetadataEXT changes it., the call is on a swapchain which would imply it shouldn't persist to the display when there isn't a swapchain setting HDR metadata. With Nvidia it seems that it is persisting past the (child) window destruction and up until the app quits (perhaps the destruction of an application or parent window context?). With Intel, it seems to persist without end (it's still set even after the app exits) until something else changes it.

That said, I'm not sure that set_hdr_metadata (and by extension vkSetHdrMetadataEXT) would ever toggle the display out of HDR. It is still setting HDR metadata on the swapchain and for the driver to toggle the display to SDR would require the driver to notice that the metadata is not actually HDR anymore. Given the above lifetimes, it seems the driver keeps this info not just at the swapchain but in other layers too and changing the display to SDR would require those layers to know that no other swapchains exist which have HDR metadata set. I expect given this complexity and the observed behavior, the transfer of the metadata through the layers to the actual display output only turns on HDR if it was off and nothing else.

[gbooker]

@mitzsch I was looking at your comment in the Plex forums and I noticed one piece that caught my attention.

Switching the TV into HDR works but not back to SDR …. Opening an SDR file or quitting Plex HTPC will switch it back to SDR.

If playing an SDR file does properly switch the display out of HDR, then there is something in the setup of playback of an SDR file that does this. So I looked at the setup and I saw this piece of code: https://github.com/mpv-player/mpv/blob/2c46ae8ea3d9ae32b52eae9092f0517d69dcedbc/video/out/vo_gpu_next.c#L872-L881 The first part is setting up the metadata passthrough. The latter case calls https://github.com/haasn/libplacebo/blob/619969b2a77436892392a05c3dbea5a710b9c0c4/src/swapchain.c#L54 which calls just this when the colorspace is NULL:

    struct pl_swapchain_colors fix = {0};
    sw->impl->colorspace_hint(sw, &fix);

This calls https://github.com/haasn/libplacebo/blob/619969b2a77436892392a05c3dbea5a710b9c0c4/src/vulkan/swapchain.c#L849 (vk_sw_colorspace_hint) which essentially does two things:

    bool ok = pick_surf_format(sw, csp);
    set_hdr_metadata(p, &csp->hdr);

(csp is all zeros in this case) The second call is what you tested last time around but I wonder if the first one is what knocks the display out of HDR and into SDR.

So maybe this patch to MPV may do the trick (untested so it may not work or even crash):

diff --git a/video/out/vo_gpu_next.c b/video/out/vo_gpu_next.c
index 8b2d783727..4413d6bba1 100644
--- a/video/out/vo_gpu_next.c
+++ b/video/out/vo_gpu_next.c
@@ -1302,6 +1302,10 @@ static char *get_cache_file(struct priv *p)
 static void uninit(struct vo *vo)
 {
     struct priv *p = vo->priv;
+
+    // Switch the colorspace back
+    pl_swapchain_colorspace_hint(p->sw, NULL);
+
     pl_queue_destroy(&p->queue); // destroy this first
     for (int i = 0; i < MP_ARRAY_SIZE(p->osd_state.entries); i++)
         pl_tex_destroy(p->gpu, &p->osd_state.entries[i].tex);

I may test this setup (HDR Display on an Nvidia GPU) myself but it may take a few weeks before I get around to it. So you may beat me to testing this out.

[haasn]

So maybe this patch to MPV may do the trick (untested so it may not work or even crash):

I would be surprised if this does anything because I already posted an effectively equivalent patch above that did not help.

[gbooker]

I would be surprised if this does anything because I already posted an effectively equivalent patch above that did not help.

Not quite the same; there is one difference. I mentioned this:

[This] calls (vk_sw_colorspace_hint) which essentially does two things:

    bool ok = pick_surf_format(sw, csp);
    set_hdr_metadata(p, &csp->hdr);

(csp is all zeros in this case) The second call is what you tested last time around but I wonder if the first one is what knocks the display out of HDR and into SDR.

Though, if this doesn't help, then I wonder what in the process of playing SDR content is doing the trick.

[mitzsch]

@gbooker I tried your patch but unfortunately, it did not work - same behavior as before... Build => https://we.tl/t-o8cSAVu05H

[haasn]

Not quite the same; there is one difference. I mentioned this:

Oh, yes, I see your point. Unfortunately, I know that this function (pick_surf_format) does not do anything on its own - it doesn't touch the swapchain or display.

[gbooker]

Drat. I was hoping it was something simple as this. So I wonder what it is with a subsequent playback of SDR content that causes the drivers to kick out of HDR output.