gpu-next: subtitles rendered incorrectly when use "--video-sync=display- modes" on hdr and dolbyvision video

dyphire commented 2 years ago

Important Information

Provide following Information:

mpv version

mpv 0.34.0-205-g57f42cee84 Copyright © 2000-2022 mpv/MPlayer/mplayer2 projects
built on Thu Feb 24 12:36:19 UTC 2022
FFmpeg library versions:
libavutil       57.21.100
libavcodec      59.21.100
libavformat     59.17.102
libswscale      6.5.100
libavfilter     8.27.100
libswresample   4.4.100
FFmpeg version: git-2022-02-24-8ef03c2f

Windows Version win11 beta
Source of the mpv binary built based on shinchiro
If known which version of mpv introduced the problem
Possible screenshot or video of visual glitches

Reproduction steps

open hdr or dolbyvision video with external subtitles and mpv use --vo=gpu-next --video-sync=display-resample

Expected behavior

The subs should render normally like use --vo=gpu-next --video-sync=audio

Actual behavior

--vo=gpu-next --video-sync=audio

--vo=gpu-next --video-sync=display-resample

In fact, this also happens in other options for --video-sync=display- modes

For reference, the following is the test content of --vo=gpu. --vo=gpu --video-sync=display-resample --blend-subtitles=no

--vo=gpu --video-sync=display-resample --blend-subtitles=yes

Log file

--vo=gpu-next --video-sync=display-resample test.log

Sample files

N/A

dyphire commented 2 years ago

When I use this option of --video-sync=audio, I also found other operations that will destroy subtitle rendering.

Reproduction steps

open hdr or dolbyvision video with external subtitles and mpv use --vo=gpu-next --video-sync=audio
pause and open OSD or OSC
the subs also rendered incorrectly

haasn commented 2 years ago

@dyphire Does this patch fix it?

diff --git a/video/out/vo_gpu_next.c b/video/out/vo_gpu_next.c
index 572093b65b..9888374545 100644
--- a/video/out/vo_gpu_next.c
+++ b/video/out/vo_gpu_next.c
@@ -275,10 +275,16 @@ static void update_overlays(struct vo *vo, struct mp_osd_res res, double pts,
             .tex = entry->tex,
             .parts = entry->parts,
             .num_parts = entry->num_parts,
-            .color = frame->color,
             .coords = PL_OVERLAY_COORDS_DST_FRAME,
         };

+        // Use the frame's native color space by default, except for HDR
+        if (pl_color_space_is_hdr(&ol->color)) {
+            ol->color = pl_color_space_unknown;
+        } else {
+            ol->color = frame->color;
+        }
+
         switch (item->format) {
         case SUBBITMAP_BGRA:
             ol->mode = PL_OVERLAY_NORMAL;

dyphire commented 2 years ago

@dyphire Does this patch fix it?

I used the patch to built test, but unfortunately it wasn't fixed. the test build files: mpv-x86_64-20220224-git-6d5a389.7z --vo=gpu-next --video-sync=display-resample The new subtitle rendering looks unchanged log: test.log

haasn commented 2 years ago

Sorry, my patch was wrong. Correct was just:

diff --git a/video/out/vo_gpu_next.c b/video/out/vo_gpu_next.c
index 572093b65b..9ced95336b 100644
--- a/video/out/vo_gpu_next.c
+++ b/video/out/vo_gpu_next.c
@@ -279,6 +279,9 @@ static void update_overlays(struct vo *vo, struct mp_osd_res res, double pts,
             .coords = PL_OVERLAY_COORDS_DST_FRAME,
         };

+        if (pl_color_space_is_hdr(&ol->color))
+            ol->color = pl_color_space_unknown;
+
         switch (item->format) {
         case SUBBITMAP_BGRA:
             ol->mode = PL_OVERLAY_NORMAL;

With that it appears fixed on my end. Though note that there's some fundamental ambiguity here - what color space should subtitles on HDR videos be considered at? Because the answer clearly isn't "HDR".

Maybe something like this patch makes even more sense:

diff --git a/video/out/vo_gpu_next.c b/video/out/vo_gpu_next.c
index 572093b65b..b517d44b67 100644
--- a/video/out/vo_gpu_next.c
+++ b/video/out/vo_gpu_next.c
@@ -275,10 +275,18 @@ static void update_overlays(struct vo *vo, struct mp_osd_res res, double pts,
             .tex = entry->tex,
             .parts = entry->parts,
             .num_parts = entry->num_parts,
-            .color = frame->color,
+            .color.primaries = frame->color.primaries,
+            .color.transfer = frame->color.transfer,
             .coords = PL_OVERLAY_COORDS_DST_FRAME,
         };

+        // Reject HDR/wide gamut subtitles out of the box, since these are
+        // probably not intended to match the video color space.
+        if (pl_color_primaries_is_wide_gamut(ol->color.primaries))
+            ol->color.primaries = PL_COLOR_PRIM_UNKNOWN;
+        if (pl_color_transfer_is_hdr(ol->color.transfer))
+            ol->color.transfer = PL_COLOR_TRC_UNKNOWN;
+
         switch (item->format) {
         case SUBBITMAP_BGRA:
             ol->mode = PL_OVERLAY_NORMAL;

haasn commented 2 years ago

Note that there still is a minor difference caused by a difference in blending in the output space vs blending in linear light (or even the image source colorspace). But this is arguably a feature, not a bug, because blending in linear light is usually the better way to blend, anyway. There is also a difference caused by the non-blended subtitle rendering path currently ignoring e.g. output ICC profiles, but this is a bug which I hope to fix with a rewrite of the color management system in libplacebo.

dyphire commented 2 years ago

With that it appears fixed on my end.

Yes, either of these two patches can fix it. I prefer the second patch.

There is also a difference caused by the non-blended subtitle rendering path currently ignoring e.g. output ICC profiles, but this is a bug which I hope to fix with a rewrite of the color management system in libplacebo.

I'm glad to hear this message and look forward to its realization.

mpv-player / mpv