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

gpu-next: abnormal display of ass subtitles in HDR videos #13673

Closed
dyphire opened this issue Mar 9, 2024 · 11 comments · Fixed by #13710
Closed

gpu-next: abnormal display of ass subtitles in HDR videos #13673

dyphire opened this issue Mar 9, 2024 · 11 comments · Fixed by #13710
Labels

Comments

@dyphire
Copy link
Contributor

dyphire commented Mar 9, 2024

Important Information

Provide following Information:

Reproduction steps

Play HDR videos with ass subtitles in vo=gpu-next.

Expected behavior

The color and brightness of the ass subtitles are displayed normally.
image

Actual behavior

Since the 9325ebe commit, the color displayed in the ass subtitles has been abnormal, and the brightness is also too bright.
image

This commit attempts to solve an existing issue of #13381, but it introduces a more serious issue.

In fact, there are very few ass subtitles produced in the HDR color space, and it should not be assumed that the color space of ass subtitles is consistent with the color space of HDR videos and rendered in the color space of HDR videos by default, which will lead to incorrect rendering of most ass subtitles. The color error is may be a minor issue in comparison, and abnormal brightness is even more fatal as it makes the subtitles no longer readable.

I suggest adding an option to control the new subtitle rendering behavior and disabling it by default.
The ideas mentioned there seem more reasonable to me: #13381 (comment)

Log file

mpv.log

Sample file

@dyphire dyphire added the os:win label Mar 9, 2024
@kasper93
Copy link
Contributor

kasper93 commented Mar 9, 2024

This commit attempts to solve an existing issue of #13381, but it introduces a more serious issue.

By design. Like I said in the other thread #13381 (comment)

The main issue with "just use video colorspace duh" is that it works now, because like said above everything is bt.709, but I assure you no one wants to create HDR subtitles. But since there is no HDR anime, the problem didn't arise yet. Frankly it is shortsighted from libass devs that the subtitle colorspace is still not defined. I don't want to make this thread about what should be and fixing the world. I will just patch mpv to whatever you guys want and leave others with the unsolved problem.

You can see the rest of discussion. The real issue, reported by madshi 6 years ago, is here libass/libass#297 go complain there. I want to raise awareness of this issue instead of adding workarounds in mpv.

It is trivial to fallback to sRGB (or anything) for HDR content, like before, but why mpv has to make this decision if upstream explicitly says to use video colorspace? Because this is what VSFilter did in 00s.

@dyphire
Copy link
Contributor Author

dyphire commented Mar 9, 2024

This commit attempts to solve an existing issue of #13381, but it introduces a more serious issue.

By design. Like I said in the other thread #13381 (comment)

The main issue with "just use video colorspace duh" is that it works now, because like said above everything is bt.709, but I assure you no one wants to create HDR subtitles. But since there is no HDR anime, the problem didn't arise yet. Frankly it is shortsighted from libass devs that the subtitle colorspace is still not defined. I don't want to make this thread about what should be and fixing the world. I will just patch mpv to whatever you guys want and leave others with the unsolved problem.

You can see the rest of discussion. The real issue, reported by madshi 6 years ago, is here libass/libass#297 go complain there. I want to raise awareness of this issue instead of adding workarounds in mpv.

It is trivial to fallback to sRGB (or anything) for HDR content, like before, but why mpv has to make this decision if upstream explicitly says to use video colorspace? Because this is what VSFilter did in 00s.

libass/libass#297 (comment)

Yes. Draw the SDR subtitle in the correct SDR color space rather than project it to HDR color space to avoid over-brightness and over-saturation

This is what we have now: over-brightness and over-saturation, and the new behavior of MPV has brought more problems. I do not object to the practice of rendering subtitles into the video color space, but an option should be provided to control it to avoid any issues that may arise.

@kasper93
Copy link
Contributor

kasper93 commented Mar 9, 2024

libass/libass#297 (comment)

This comment is not from libass dev and just a discussion point. I know how to handle it correctly, but why is it on mpv to handle issues of others?

@dyphire
Copy link
Contributor Author

dyphire commented Mar 9, 2024

libass/libass#297 (comment)

This comment is not from libass dev and just a discussion point. I know how to handle it correctly, but why is it on mpv to handle issues of others?

The current situation is that libass cannot determine whether the subtitles are SDR or HDR subtitles, as the ass standard does not provide this metadata. The discussion attempts to establish a new standard and rendering method, but please remember that even if it is ultimately implemented, it will still disrupt the rendering of existing subtitles. The new behavior of mpv always renders subtitles in the video color space, which is equivalent to telling libass to always treat them as HDR subtitle processing.

I can't see that this is a libass issue rather than an mpv issue. Shouldn't there be an option to control whether to treat it as HDR subtitle rendering? Just like the existing options of sub-ass-vsfilter-color-compat.

@kasper93
Copy link
Contributor

kasper93 commented Mar 9, 2024

which is equivalent to telling libass to always treat them as HDR subtitle processing.
I can't see that this is a libass issue rather than an mpv issue.

We are not telling libass anything, it is libass telling us how to draw subtitles. Or not telling. It is libass issue, because currently their position is to "render subtitles in video colorspace always". I want to highlight how bad this behavior is, because apparently 6 years of discussion in libass/libass#297 is not enough.

Don't worry, I will add an option for this eventually, but this is not my or mpv's place to decide how ass spec or libass should behave. It is not my responsibility to define how subtitles should be rendered. And upstream is firm about video colorspace rendering.

Also if it is not clear, if subtitles are not rendered correctly in HDR video, they are invalid and were not mastered for HDR video colorspace. Which violate current libass/ass/vsfilter policy of rendering subtitles onto raw frame before color conversions.

@dyphire
Copy link
Contributor Author

dyphire commented Mar 9, 2024

which is equivalent to telling libass to always treat them as HDR subtitle processing.
I can't see that this is a libass issue rather than an mpv issue.

We are not telling libass anything, it is libass telling us how to draw subtitles. Or not telling. It is libass issue, because currently their position is to "render subtitles in video colorspace always". I want to highlight how bad this behavior is, because apparently 6 years of discussion in libass/libass#297 is not enough.

Don't worry, I will add an option for this eventually, but this is not my or mpv's place to decide how ass spec or libass should behave. It is not my responsibility to define how subtitles should be rendered. And upstream is firm about video colorspace rendering.

Now I understand your point of view. Yes, the behavior of always rendering subtitles in the video color space is terrible. The only correct solution is to introduce a new header for ass to distinguish HDR subtitles. Subtitles without HDR headers should be considered as rendering SDR subtitles in the SDR color space.

Obviously, this dispute has not ended yet. As a temporary solution, I will temporarily revert the commit it for myself.

@kasper93
Copy link
Contributor

kasper93 commented Mar 9, 2024

Also, I just notice it has been recently documented. In libass/libass#735

https://github.com/libass/libass/blob/649a7c2e1fc6f4188ea1a89968560715800b883d/libass/ass_types.h#L233-L237

 * Further note all of the above only concerns the RGB values.
 * Color primaries and transfer charateristics of ASS subtitles
 * must always match their associated video.
 * (This indeed has some undesirable effects on HDR videos,
 * but no mechanism avoiding this is yet standardized.)

kasper93 added a commit to kasper93/mpv that referenced this issue Mar 9, 2024
Option to control what colorspace subtitle should be converted to before
drawing them. Upstream ASS specification says that all subtitles should
be rendered with color primaries and transfer matching their associated
video.

This has certain limitations in the case of HDR/WCG content. See the
discussion in libass/libass#297.

To mitigate current situation add an option for users to control the
render target of subtitles.

This could be also accomplished with --blend-subtitles to some degree,
but I don't want to mux options that are not really clear to affect this
certain case.

See-Also: https://github.com/libass/libass/blob/649a7c2e1fc6f4188ea1a89968560715800b883d/libass/ass_types.h#L233-L237
See-Also: libass/libass#297
See-Also: mpv-player#13381
Fixes: mpv-player#13673
@kasper93
Copy link
Contributor

kasper93 commented Mar 9, 2024

@dyphire you can try #13674, if this fixes your issue. I don't like that users will have to manually manage subtitle target colorspace, but I agree that it is not users fault that we don't have better way to handle it currently, so let give them option.

kasper93 added a commit to kasper93/mpv that referenced this issue Mar 9, 2024
Option to control what colorspace subtitle should be converted to before
drawing them. Upstream ASS specification says that all subtitles should
be rendered with color primaries and transfer matching their associated
video.

This has certain limitations in the case of HDR/WCG content. See the
discussion in libass/libass#297.

To mitigate current situation add an option for users to control the
render target of subtitles.

This could be also accomplished with --blend-subtitles to some degree,
but I don't want to mux options that are not really clear to affect this
certain case.

See-Also: https://github.com/libass/libass/blob/649a7c2e1fc6f4188ea1a89968560715800b883d/libass/ass_types.h#L233-L237
See-Also: libass/libass#297
See-Also: mpv-player#13381
Fixes: mpv-player#13673
kasper93 added a commit to kasper93/mpv that referenced this issue Mar 9, 2024
Option to control what colorspace subtitle should be converted to before
drawing them. Upstream ASS specification says that all subtitles should
be rendered with color primaries and transfer matching their associated
video.

This has certain limitations in the case of HDR/WCG content. See the
discussion in libass/libass#297.

To mitigate current situation add an option for users to control the
render target of subtitles.

This could be also accomplished with --blend-subtitles to some degree,
but I don't want to mux options that are not really clear to affect this
certain case.

See-Also: https://github.com/libass/libass/blob/649a7c2e1fc6f4188ea1a89968560715800b883d/libass/ass_types.h#L233-L237
See-Also: libass/libass#297
See-Also: mpv-player#13381
Fixes: mpv-player#13673
kasper93 added a commit to kasper93/mpv that referenced this issue Mar 9, 2024
Option to control what colorspace subtitle should be converted to before
drawing them. Upstream ASS specification says that all subtitles should
be rendered with color primaries and transfer matching their associated
video.

This has certain limitations in the case of HDR/WCG content. See the
discussion in libass/libass#297.

To mitigate current situation add an option for users to control the
render target of subtitles.

This could be also accomplished with --blend-subtitles to some degree,
but I don't want to mux options that are not really clear to affect this
certain case.

See-Also: https://github.com/libass/libass/blob/649a7c2e1fc6f4188ea1a89968560715800b883d/libass/ass_types.h#L233-L237
See-Also: libass/libass#297
See-Also: mpv-player#13381
Fixes: mpv-player#13673
kasper93 added a commit to kasper93/mpv that referenced this issue Mar 9, 2024
Option to control what colorspace subtitle should be converted to before
drawing them. Upstream ASS specification says that all subtitles should
be rendered with color primaries and transfer matching their associated
video.

This has certain limitations in the case of HDR/WCG content. See the
discussion in libass/libass#297.

To mitigate current situation add an option for users to control the
render target of subtitles.

This could be also accomplished with --blend-subtitles to some degree,
but I don't want to mux options that are not really clear to affect this
certain case.

See-Also: https://github.com/libass/libass/blob/649a7c2e1fc6f4188ea1a89968560715800b883d/libass/ass_types.h#L233-L237
See-Also: libass/libass#297
See-Also: mpv-player#13381
Fixes: mpv-player#13673
@dyphire
Copy link
Contributor Author

dyphire commented Mar 10, 2024

@dyphire you can try #13674, if this fixes your issue. I don't like that users will have to manually manage subtitle target colorspace, but I agree that it is not users fault that we don't have better way to handle it currently, so let give them option.

That PR is indeed the solution currently needed, but it did not work properly during testing and crashed after running the --vo=gpu-next --sub-ass-colorspace=sdr command.
This is the log file: mpv.log

kasper93 added a commit to kasper93/mpv that referenced this issue Mar 10, 2024
Option to control what colorspace subtitle should be converted to before
drawing them. Upstream ASS specification says that all subtitles should
be rendered with color primaries and transfer matching their associated
video.

This has certain limitations in the case of HDR/WCG content. See the
discussion in libass/libass#297.

To mitigate current situation add an option for users to control the
render target of subtitles.

This could be also accomplished with --blend-subtitles to some degree,
but I don't want to mux options that are not really clear to affect this
certain case.

See-Also: https://github.com/libass/libass/blob/649a7c2e1fc6f4188ea1a89968560715800b883d/libass/ass_types.h#L233-L237
See-Also: libass/libass#297
See-Also: mpv-player#13381
Fixes: mpv-player#13673
@kasper93
Copy link
Contributor

@dyphire should be fixed now.

@dyphire
Copy link
Contributor Author

dyphire commented Mar 10, 2024

@dyphire should be fixed now.

Yes, it's working now.

kasper93 added a commit to kasper93/mpv that referenced this issue Mar 16, 2024
Upstream ASS specification says that all subtitles should be rendered
with color primaries and transfer matching their associated video. But
as expected after further discussion the decision has been made to
fallback to SDR mode in case of HDR video.

See-Also: https://github.com/libass/libass/blob/649a7c2e1fc6f4188ea1a89968560715800b883d/libass/ass_types.h#L233-L237
See-Also: libass/libass#297
See-Also: mpv-player#13381
Fixes: mpv-player#13673
kasper93 added a commit that referenced this issue Mar 18, 2024
Upstream ASS specification says that all subtitles should be rendered
with color primaries and transfer matching their associated video. But
as expected after further discussion the decision has been made to
fallback to SDR mode in case of HDR video.

See-Also: https://github.com/libass/libass/blob/649a7c2e1fc6f4188ea1a89968560715800b883d/libass/ass_types.h#L233-L237
See-Also: libass/libass#297
See-Also: #13381
Fixes: #13673
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
2 participants