Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

vo_gpu_next: use proper color for subtitles #12405

Merged
merged 1 commit into from
Sep 21, 2023
Merged

Conversation

kasper93
Copy link
Contributor

Target frame parameters were used instead of source frame.

Also for HDR videos it seems that subtitles target SDR white, so set that too.

@kasper93
Copy link
Contributor Author

/cc @haasn

video/out/vo_gpu_next.c Show resolved Hide resolved
Copy link
Member

@haasn haasn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, thinking about it more, I'm not convinced this is correct.

Consider two pitfalls:

  1. OSD is now interpreted according to the colorspace of the video, which makes no sense. Why would the video colorspace affect the appearance of the mpv UI?
  2. HDR movie with normal, non-bitmapped libass-rendered soft subtitles now get interpreted as encoded in BT.2020/PQ. But this is nonsensical.

There are three conflicting desires here:

  1. Desire for softsubbed signs to match video (requires interpreting subtitles in video colorspace)
  2. Desire for OSD/OSC/UI to appear consistent (independent of video and display device, requires interpreting in constant reference space e.g. bt.709 sRGB)
  3. Desire for PGS subtitles in video-encoded color space to appear correctly (presumably this is what motivated this commit?)

Whether subtitles should be rendered in the video colorspace or not was actually previously implicitly controlled by blend-subtitles=yes, which is behavior that was preserved in vo_gpu_next.

So the status quo is more correct and consistent than this PR. Can you clarify what bug you are fixing?

@kasper93
Copy link
Contributor Author

kasper93 commented Sep 17, 2023

OSD is now interpreted according to the colorspace of the video, which makes no sense. Why would the video colorspace affect the appearance of the mpv UI?

Doesn't change the appearance of OSD for PL_OVERLAY_MONOCHROME.

HDR movie with normal, non-bitmapped libass-rendered soft subtitles now get interpreted as encoded in BT.2020/PQ. But this is nonsensical.

And how nonsensical is interpreting them as if they were in target colorspace, which is not related to soft subtitles.

Desire for softsubbed signs to match video (requires interpreting subtitles in video colorspace)
Desire for PGS subtitles in video-encoded color space to appear correctly (presumably this is what motivated this commit?)

This is the same, doesn't it?

Whether subtitles should be rendered in the video colorspace or not was actually previously implicitly controlled by blend-subtitles=yes, which is behavior that was preserved in vo_gpu_next.

I don't want blend-subtitles.

So the status quo is more correct and consistent than this PR. Can you clarify what bug you are fixing?

Point 3 that you pointed. PGS subtitles are not rendered correctly.

@kasper93 kasper93 marked this pull request as draft September 17, 2023 01:22
@haasn
Copy link
Member

haasn commented Sep 18, 2023

OSD is now interpreted according to the colorspace of the video, which makes no sense. Why would the video colorspace affect the appearance of the mpv UI?

Doesn't change the appearance of OSD for PL_OVERLAY_MONOCHROME.

Are you sure? In monochrome mode, the color is set in pl_overlay_part which is still interpreted according to pl_overlay.repr/color.

I think that the best resolution here would be to actually modify render_object() to forward obj->is_sub to res, and then set the colorspace based on that field - if subtitles, use image color; if OSD, use target.color.

We could even go more fine-grained than that:

  • BGRA subs -> image color
  • libass subs -> target color
  • OSD -> target color, or even sRGB

One case to think about is HDR output case. Do we want to blind people with 4000 nits UI, just because output is is set to PQ mode? So maybe we should render OSD as sRGB always, and render subtitles as source csp / sRGB based on whether they're BGRA or libass?

@haasn
Copy link
Member

haasn commented Sep 18, 2023

cc @Nevcairiel what do you do in JRVR?

@kasper93
Copy link
Contributor Author

kasper93 commented Sep 18, 2023

One case to think about is HDR output case. Do we want to blind people with 4000 nits UI, just because output is is set to PQ mode? So maybe we should render OSD as sRGB always, and render subtitles as source csp / sRGB based on whether they're BGRA or libass?

Yes, that's what I'm thinking. Render OSD at 203 nits. Apparently the same should be done for PGS.

I need to test that, but I also don't think blend-subtitles is correct rn.

EDIT:

To be honest, I think I know what to do for PGS also for OSD I would figure it out, but I never know what should be ASS target color space? Are they tagged? Should they target sRGB? Should they target actually video too, with exception of HDR?

I think ASS are made to look good with current renders, so we need to preserve whatever we are doing now really... even if it doesn't make much sense in the grand scheme of things.

@Nevcairiel
Copy link

Nevcairiel commented Sep 18, 2023

cc @Nevcairiel what do you do in JRVR?

I generally always let the source of the overlays or subtitles specify the color.

In practice this means:
Overlays -> sRGB or PQ HDR (only UHD BDs provide PQ HDR overlays in menus)
Text Subtitles -> The subtitle renderer provides color hints, if none are set, generic sRGB
Bitmap Subtitles -> As specified by the source, can be SDR 709, 601, or PQ

All of these are then of course rendered to the target color.

If the target output is HDR, I render the SDR sources to a brightness of 180 nits, which is a value arrived at via just testing. It might actually need re-visiting now that libplacebo uses other algorithms for overlay gamut mapping, since 203 nits default seemed to desaturate SDR overlays quite a bit back then.

HDR subtitles on SDR output, I copy the images HDR metadata (assuming its HDR), but its a relatively uncommon case (only UHD BDs really have HDR subtitles, never seen anything else), so this approach is not well supported by feedback, and slightly too dark subtitles might not be that immediately obvious.

@haasn
Copy link
Member

haasn commented Sep 19, 2023

cc @Nevcairiel what do you do in JRVR?

I generally always let the source of the overlays or subtitles specify the color.

In practice this means: Overlays -> sRGB or PQ HDR (only UHD BDs provide PQ HDR overlays in menus) Text Subtitles -> The subtitle renderer provides color hints, if none are set, generic sRGB Bitmap Subtitles -> As specified by the source, can be SDR 709, 601, or PQ

All of these are then of course rendered to the target color.

If the target output is HDR, I render the SDR sources to a brightness of 180 nits, which is a value arrived at via just testing. It might actually need re-visiting now that libplacebo uses other algorithms for overlay gamut mapping, since 203 nits default seemed to desaturate SDR overlays quite a bit back then.

HDR subtitles on SDR output, I copy the images HDR metadata (assuming its HDR), but its a relatively uncommon case (only UHD BDs really have HDR subtitles, never seen anything else), so this approach is not well supported by feedback, and slightly too dark subtitles might not be that immediately obvious.

Whence do you get all this metadata? Can we get the same in mpv somehow? Currently all of our OSD stuff is pretty much a black box.

@Nevcairiel
Copy link

Whence do you get all this metadata? Can we get the same in mpv somehow? Currently all of our OSD stuff is pretty much a black box.

Most of it is .. guessed, or assumed based on the format/source.
Like, DVD/VOBSUB is assumed to be 601 (not that anyone cares about these low-quality subs), PGS is 709, unless its from UHD BD and the video is HDR, then it might be PQ.

ASS has color headers in the scripts .. if those are present. If not, probably sRGB. Other text subtitles have no color information, so they are just sRGB by definition.

@kasper93
Copy link
Contributor Author

@haasn: Actually it is quite easy, libass subs are converted already to sRGB following some compatibility logic for vsfilter... the rest should follow video colorspace in practice.

video/out/vo_gpu_next.c Outdated Show resolved Hide resolved
All SUBBITMAP_LIBASS are converted to sRGB beforehand, for the rest we
need to use video color space.
@haasn haasn merged commit 704afb8 into mpv-player:master Sep 21, 2023
13 checks passed
@kasper93 kasper93 deleted the subs_o branch October 7, 2023 15:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants