search

mpv-player / mpv

🎥 Command line video player
https://mpv.io
Other
26.76k stars 2.84k forks source link

sub: add --(sub/osd)-border-style & refactoring option naming to match ASS spec #14195

Open ruihe774 opened 1 month ago

ruihe774 commented 1 month ago

that draw background that tightly wraps each line of text.

Fixes #14194

github-actions[bot] commented 1 month ago

Download the artifacts for this pull request:

Windows * [mpv-i686-w64-mingw32](https://nightly.link/mpv-player/mpv/actions/artifacts/1678859557.zip) * [mpv-x86_64-w64-mingw32](https://nightly.link/mpv-player/mpv/actions/artifacts/1678867633.zip) * [mpv-x86_64-windows-msvc](https://nightly.link/mpv-player/mpv/actions/artifacts/1678879776.zip)
macOS * [mpv-macos-12-intel](https://nightly.link/mpv-player/mpv/actions/artifacts/1678860125.zip) * [mpv-macos-13-intel](https://nightly.link/mpv-player/mpv/actions/artifacts/1678852813.zip) * [mpv-macos-14-arm](https://nightly.link/mpv-player/mpv/actions/artifacts/1678848124.zip)
ruihe774 commented 1 month ago

@KonoVitoDa plz test it.

KonoVitoDa commented 1 month ago

Works perfectly, thank you very much!

hooke007 commented 1 month ago

close https://github.com/mpv-player/mpv/issues/10464 instead

llyyr commented 1 month ago

Can we also switch to tight by default?

ruihe774 commented 1 month ago

Can we also switch to tight by default?

What's the point to make a breaking change? IMO this option is only related to personal preference. Users can easily set this option by themselves.

KonoVitoDa commented 1 month ago

How do I get the updated version with the new feature implemented?

ruihe774 commented 1 month ago

How do I get the updated version with the new feature implemented?

Before this is merged, you can get the build artifacts from https://github.com/mpv-player/mpv/actions/workflows/build.yml. Keep in mind you need to find build artifacts of correct PR.

After this is merged, you can get it from unofficial nightly builds https://mpv.io/installation/

llyyr commented 1 month ago

What's the point to make a breaking change? IMO this option is only related to personal preference. Users can easily set this option by themselves.

Users can change many options by themselves, but there's still value in the default value being something that doesn't need to be changed for most users. I'd argue the current bounding box is not wanted by anyone.

na-na-hi commented 1 month ago

I'd argue the current bounding box is not wanted by anyone.

Patches welcome. Please contain default changes and associated bikeshedding to a separate PR.

ruihe774 commented 1 month ago

What's the point to make a breaking change? IMO this option is only related to personal preference. Users can easily set this option by themselves.

Users can change many options by themselves, but there's still value in the default value being something that doesn't need to be changed for most users. I'd argue the current bounding box is not wanted by anyone.

Worth noting that BorderStyle=3 has suboptimal behavior for non-opaque colors. e.g.: BorderStyle=3

I guess it was the reason mpv chose 4 as the default.

KonoVitoDa commented 1 month ago

Will this update also change the way --sub-back-color is selected? Because on my mpv.config it's set to #000000 (black), but in this specific part it seems to be using the sub's original border color: Erai-raws Sousou no Frieren - 01 1080p 81106F0B mkv_snapshot_01 31 425 【MPV】【LINES】 (Portuguese(Brazil))

Erai-raws Sousou no Frieren - 01 1080p 81106F0B mkv_snapshot_01 31 466 【MPV】【LINES】 (Portuguese(Brazil))

ruihe774 commented 1 month ago

Will this update also change the way --sub-back-color is selected? Because on my mpv.config it's set to #000000 (black), but in this specific part it seems to be using the sub's original border color: Erai-raws Sousou no Frieren - 01 1080p 81106F0B mkv_snapshot_01 31 425 【MPV】【LINES】 (Portuguese(Brazil))

Erai-raws Sousou no Frieren - 01 1080p 81106F0B mkv_snapshot_01 31 466 【MPV】【LINES】 (Portuguese(Brazil))

The behavior did not change. It only applies to SRT subtitles. If you want to override ASS subtitles that select a BackColor itself , you may use --sub-ass-style-overrides.

KonoVitoDa commented 1 month ago

ASS subtitles that select a BackColor itself

But this same subtitle is shown with the black back color in the old version, so it seems to be something related to the new feature: Erai-raws Sousou no Frieren - 01 1080p 81106F0B mkv_snapshot_01 31 508 【MPV】【LINES】 (Portuguese(Brazil))

ruihe774 commented 1 month ago

ASS subtitles that select a BackColor itself

But this same subtitle is shown with the black back color in the old version, so it seems to be something related to the new feature: Erai-raws Sousou no Frieren - 01 1080p 81106F0B mkv_snapshot_01 31 508 【MPV】【LINES】 (Portuguese(Brazil))

Please attach your log file and subtitle files (you can extract it using ffmpeg) so that I can look into it.

KonoVitoDa commented 1 month ago

Please attach your log file and subtitle files (you can extract it using ffmpeg) so that I can look into it.

output-new.txt output-old.txt [Erai-raws] Sousou no Frieren - 01 [1080p][81106F0B]_Subtitles02.POR.zip

The video file is the 01 from here: https:// nyaa . si/view/1752250

ruihe774 commented 1 month ago

I cannot reproduce it. With the same commit of mpv and same set of subtitle related options in your output-old.txt, the BackColor and BorderStyle are also selected by ASS subtitle file itself.

Screenshot: Screenshot from 2024-05-22 16-43-43

My log: mpv.log

My options:

[   0.001][v][cplayer] Setting option 'config' = 'no' (flags = 8)
[   0.001][v][cplayer] Setting option 'sub-files-append' = 'Erai-raws.Sousou.no.Frieren.-.01.1080p.81106F0B._Subtitles02.POR/[Erai-raws] Sousou no Frieren - 01 [1080p][81106F0B]_Subtitles02.POR.ass' (flags = 8)
[   0.001][v][cplayer] Setting option 'sub-back-color' = '#000000' (flags = 8)
[   0.001][v][cplayer] Setting option 'sub-border-color' = '#000000' (flags = 8)
[   0.001][v][cplayer] Setting option 'sub-ass-vsfilter-aspect-compat' = 'yes' (flags = 8)
[   0.001][v][cplayer] Setting option 'sub-scale-with-window' = 'no' (flags = 8)
[   0.001][v][cplayer] Setting option 'sub-ass-scale-with-window' = 'yes' (flags = 8)
[   0.001][v][cplayer] Setting option 'sub-font-size' = '53' (flags = 8)
[   0.001][v][cplayer] Setting option 'sub-pos' = '100' (flags = 8)
[   0.001][v][cplayer] Setting option 'sub-margin-x' = '25' (flags = 8)
[   0.001][v][cplayer] Setting option 'sub-margin-y' = '22' (flags = 8)
[   0.001][v][cplayer] Setting option 'sub-use-margins' = '' (flags = 8)
[   0.001][v][cplayer] Setting option 'osd-font-size' = '25' (flags = 8)
[   0.001][v][cplayer] Setting option 'osd-color' = '#FFFFFFFF' (flags = 8)
[   0.001][v][cplayer] Setting option 'osd-back-color' = '#000000' (flags = 8)
[   0.001][v][cplayer] Setting option 'log-file' = '/var/data/mpv.log' (flags = 8)
KonoVitoDa commented 1 month ago

Really weird. Is not it related to the used FFmpeg version? Btw, I don't know why the FFmpeg version on my output-old.txt is shown as N-115357-g0c1304ae1 while I have ffmpeg version 2024-05-20-git-127ded5078-full_build-www.gyan.dev on my PATH. mpv's updater also says FFmpeg doesn't exist (I downloaded my build from here).

ruihe774 commented 1 month ago

Btw, I don't know why the FFmpeg version on my output-old.txt is shown as N-115357-g0c1304ae1 while I have ffmpeg version 2024-05-20-git-127ded5078-full_build-www.gyan.dev on my PATH.

mpv's CI statically links with ffmpeg. It's not overridable. Many unofficial builds also do that.

Really weird. Is not it related to the used FFmpeg version?

I don't think it is related to ffmpeg version. I think it is related to your scripts. You may try --no-config to disable them and have a check.

Anyway, imo --sub-back-color is intended to work with only SRT (and other plain text) subtitles that does not define styles. @na-na-hi I wonder whether I have a correct understanding.

na-na-hi commented 1 month ago

It's documented that most subtitle options don't affect ASS subtitles. --sub-ass-* options exist for that purpose.

KonoVitoDa commented 1 month ago

Oh, I was sleepy and forgot to mention that I use this keybind to switch between sub's original style and my own style: d cycle-values sub-ass-override "force" "no"

Both of the screenshots I shared before were with sub-ass-override set to force, but I don't understand why on _mpv-x8664-20240522-git-0dd6321 it made the back color black while on the artifact version _mpv-x86_64-w64-mingw32_ it used original sub's border color.

ruihe774 commented 1 month ago

Oh, I was sleepy and forgot to mention that I use this keybind to switch between sub's original style and my own style: d cycle-values sub-ass-override "force" "no"

Both of the screenshots I shared before were with sub-ass-override set to force, but I don't understand why on _mpv-x8664-20240522-git-0dd6321 it made the back color black while on the artifact version _mpv-x86_64-w64-mingw32_ it used original sub's border color.

Reproduced with --sub-ass-override=force.

Screenshot from 2024-05-23 01-35-51

KonoVitoDa commented 1 month ago

Well, if this only occurs with specific subtitle lines (this one is a typesetting), so I think it's not a big problem. Actually it makes sense to have a totally grey sub-back-color instead of a black sub-back-color with a grey sub-border over it.

KonoVitoDa commented 1 month ago

Actually it makes sense to have a totally grey sub-back-color than a black sub-back-color with a grey sub-border over it.

But it would be great if this behavior could also be customized by user.

ruihe774 commented 1 month ago

It is related to the selection of different colors in ASS. BorderStyle=3 will use OutlineColor and BorderStyle=4 will use BackColor. You can use --sub-border-color to customize OutlineColor. I'm still refactoring related options to make it more consistent (e.g. renaming --sub-border-color to --sub-outline-color to match the upstream ASS spec.)

na-na-hi commented 1 month ago

The problem is actually with how BorderStyle = 4 is implemented in libass. Normally, subtitle rendering works like this:

  1. The text shadow is drawn with BackColour if the shadow is drawn (has non-zero offset). It respects the BorderStyle setting, so if it's set to 3, the rectangles are drawn.
  2. The text border is drawn with OutlineColour if the border is drawn (has non-zero size). It respects the BorderStyle setting, so if it's set to 3, the rectangles are drawn.
  3. The text glyph is drawn.

When BorderStyle = 4 was bolted on libass in https://github.com/libass/libass/commit/4648a02281ff0860cb06e3f56df9095b8a114294, step 1 is replaced as the following if BorderStyle = 4:

  1. The background rectangle is drawn with BackColour.

Note that it disrespects the shadow offset, so if the offset is zero, the background is still drawn.

For the sample file, when the text border is drawn, it respects the BorderStyle setting and draws the background with the OutlineColour. Even if the text has a non-zero shadow offset, its color is overwritten by the later drawn border color. AFAIK this is expected ASS behavior, while the nonstandard BorderStyle = 4 disregards that. See https://github.com/libass/libass/pull/105#issuecomment-44789530.

BorderStyle=3 will use OutlineColor and BorderStyle=4 will use BackColor.

This is not true, see above. BorderStyle = 3 uses both colors. It's just the shadow color isn't visible when the shadow offset is zero and thus not drawn.

ruihe774 commented 1 month ago

For the sample file, when the text border is drawn, it respects the BorderStyle setting and draws the background with the OutlineColour. Even if the text has a non-zero shadow offset, its color is overwritten by the later drawn border color. AFAIK this is expected ASS behavior, while the nonstandard BorderStyle = 4 disregards that. See https://github.com/libass/libass/pull/105#issuecomment-44789530.

BorderStyle=3 will use OutlineColor and BorderStyle=4 will use BackColor.

This is not true, see above. BorderStyle = 3 uses both colors. It's just the shadow color isn't visible when the shadow offset is zero and thus not drawn.

Yeah. OutlineColor, BackColor, and BorderStyle are orthogonal options.

ruihe774 commented 1 month ago

I've made some breaking changes to option naming to match ASS spec. Some option "dispatching" logic has been removed. IMO now it's more simple and intuitive.

na-na-hi commented 1 month ago

There is no benefit of the border -> outline renaming change. Just add some clarification in the documentation instead. Please revert.

ruihe774 commented 1 month ago

There is no benefit of the border -> outline renaming change. Just add some clarification in the documentation instead. Please revert.

I do not agree.

na-na-hi commented 1 month ago
  • It matches the naming in specs.

border is a better name for 99% of the cases.

  • BorderStyle=4 does not use --sub-border-color, which leads to confusion.

There is no confusion: it's libass-specific and not part of the ASS spec. It's not supported by xy-VSFilter.

  • --(sub/osd)-shadow-color is removed. It is already a breaking change. No meaning to continue using the old names for compatibility reasons.

This breaking is not needed when you can alias it to --sub-back-color.

ruihe774 commented 1 month ago
  • It matches the naming in specs.

border is a better name for 99% of the cases.

  • BorderStyle=4 does not use --sub-border-color, which leads to confusion.

There is no confusion: it's libass-specific and not part of the ASS spec. It's not supported by xy-VSFilter.

  • --(sub/osd)-shadow-color is removed. It is already a breaking change. No meaning to continue using the old names for compatibility reasons.

This breaking is not needed when you can alias it to --sub-back-color.

OK. I keep them as aliases. No interface breaking change now.

KonoVitoDa commented 1 month ago

Hi. Any news?

ruihe774 commented 1 month ago

Hi. Any news?

My work is finished. Please be patient. People need time to review and test the PR.

guidocella commented 1 month ago

I think the 2 commits can be squashed, and

local has_shadow = mp.get_property('osd-back-color'):sub(2, 3) == '00'

in console.lua should become

local has_shadow = mp.get_property('osd-back-color'):find('box$') == nil
ruihe774 commented 3 weeks ago

Rebased. I think it's time to merge it. @na-na-hi @kasper93

kasper93 commented 2 weeks ago

Rebased. I think it's time to merge it. @na-na-hi @kasper93

I don't follow all the sub/osd styles, ask someone else to review those.

na-na-hi commented 2 weeks ago

Also --(sub/osd)-border-style=none doesn't work. It behaves the same as outline-and-shadow.

na-na-hi commented 2 weeks ago

@Dudemanguy These new option names stick to whatever names the ASS format uses, but as pointed out, the meanings of these names can be completely different depending on the border style. Is it a good idea, for example, to require the user to set --sub-shadow-offset to control the margin of the background box?

I just want to make sure that these names aren't confusing so that they won't be unnecessarily renamed/removed after the fact.

Dudemanguy commented 2 weeks ago

These new option names stick to whatever names the ASS format uses, but as pointed out, the meanings of these names can be completely different depending on the border style.

If what happens changes completely depending on the border style, I think we're just doomed, no? I don't have any good name ideas anyway.

na-na-hi commented 2 weeks ago

If what happens changes completely depending on the border style, I think we're just doomed, no?

The alternative is that we add separate options that make sense, like one for each background color, shadow color, border color, margin size etc., and apply the appropriate ones based on the border style selected. I think it makes no sense that if I want to change the border style from opaque box to background box I need to change the values of both border size and shadow offset just to keep the margin size, for example. Instead it can be a single margin size option which depending on the border style set, sets the corresponding underlying ASS properties.

ruihe774 commented 1 week ago

IMO 1:1 mappings from CLI options to upstream's options are intuitive. Despite historical reasons, ASS spec and libass are designed to be in that shape, and users can consult the spec of ASS and the doc of libass by themselves if in doubt. Using a complex option handling and mapping logic will make things complicated and hard to understand, unless we thoroughly describe our logic in the documentation, which is a maintain burden and a reinvention of existing spec.

na-na-hi commented 1 week ago

What should be done with --(sub/osd)-border-style=none? It still doesn't work as documented. This needs to be fixed or dropped as a choice.

ruihe774 commented 1 week ago

What should be done with --(sub/osd)-border-style=none? It still doesn't work as documented. This needs to be fixed or dropped as a choice.

Sorry, I forgot it.

It turns out 0 is not a valid value for BorderStyle in ASS. Now I just remove it.