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

ruihe774 · 2024-05-21T03:16:51Z

that draw background that tightly wraps each line of text.

Fixes #14194

github-actions · 2024-05-21T03:29:03Z

Download the artifacts for this pull request:

Windows

macOS

ruihe774 · 2024-05-21T03:30:02Z

@KonoVitoDa plz test it.

KonoVitoDa · 2024-05-21T03:40:01Z

Works perfectly, thank you very much!

DOCS/man/options.rst

hooke007 · 2024-05-21T13:25:34Z

close #10464 instead

llyyr · 2024-05-21T16:40:01Z

Can we also switch to tight by default?

ruihe774 · 2024-05-21T17:20:47Z

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 · 2024-05-21T18:57:00Z

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

ruihe774 · 2024-05-21T19:00:54Z

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 · 2024-05-21T19:33:28Z

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 · 2024-05-21T20:17:27Z

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 · 2024-05-22T04:21:54Z

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.:

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

KonoVitoDa · 2024-05-22T05:38:07Z

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:

ruihe774 · 2024-05-22T06:36:53Z

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:

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 · 2024-05-22T07:07:15Z

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:

ruihe774 · 2024-05-22T07:25:02Z

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:

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

KonoVitoDa · 2024-05-22T08:01:40Z

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 · 2024-05-22T08:50:08Z

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:

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 · 2024-05-22T09:17:28Z

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 · 2024-05-22T14:40:47Z

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 · 2024-05-22T14:56:08Z

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

KonoVitoDa · 2024-05-22T17:15:20Z

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-x86_64-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 · 2024-05-22T17:36:33Z

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-x86_64-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.

KonoVitoDa · 2024-05-22T19:48:10Z

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 · 2024-05-22T19:51:20Z

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 · 2024-05-22T19:51:52Z

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.)

ruihe774 · 2024-05-23T10:57:29Z

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.

DOCS/man/options.rst

DOCS/interface-changes/sub-border-style.txt

sub/osd.c

DOCS/man/options.rst

KonoVitoDa · 2024-05-31T18:38:37Z

Hi. Any news?

ruihe774 · 2024-06-01T16:16:45Z

Hi. Any news?

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

guidocella · 2024-06-01T20:10:13Z

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 · 2024-06-17T06:07:06Z

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

kasper93 · 2024-06-24T14:10:15Z

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.

DOCS/man/options.rst

na-na-hi · 2024-06-24T17:24:47Z

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

na-na-hi · 2024-06-24T17:35:11Z

sub/ass_mp.c

-    if (opts->back_color.a) {
-        style->BackColour = MP_ASS_COLOR(opts->back_color);
-        style->BorderStyle = 4; // opaque box
-    } else {
-        style->BackColour = MP_ASS_COLOR(opts->shadow_color);
-        style->BorderStyle = 1; // outline
-    }


Maybe keep this compatibility if --*-border-style isn't explicitly set. This means keep these two colors as separate options and avoid changing the default for back-color to a non-transparent value.

IMO it's too smart. I consider these options orthogonal.

na-na-hi · 2024-06-24T22:18:06Z

@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.

sub/osd.c

Dudemanguy · 2024-06-25T02:47:08Z

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 · 2024-06-25T04:59:58Z

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 · 2024-06-26T09:41:03Z

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 · 2024-06-27T04:54:25Z

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 · 2024-06-27T05:28:04Z

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.

na-na-hi reviewed May 21, 2024

View reviewed changes

DOCS/man/options.rst Outdated Show resolved Hide resolved

ruihe774 force-pushed the back-color-tight branch 2 times, most recently from 00ecac4 to e306696 Compare May 21, 2024 04:27

ruihe774 changed the title ~~sub: add option sub-back-color-tight~~ sub: add --sub-back-style and --osd-back-style options May 21, 2024

kasper93 approved these changes May 21, 2024

View reviewed changes

na-na-hi suggested changes May 23, 2024

View reviewed changes

DOCS/man/options.rst Outdated Show resolved Hide resolved

DOCS/man/options.rst Outdated Show resolved Hide resolved

DOCS/interface-changes/sub-border-style.txt Outdated Show resolved Hide resolved

sub/osd.c Outdated Show resolved Hide resolved

ruihe774 force-pushed the back-color-tight branch from 4f581c7 to 894f577 Compare May 23, 2024 12:42

na-na-hi reviewed May 23, 2024

View reviewed changes

DOCS/man/options.rst Outdated Show resolved Hide resolved

ruihe774 force-pushed the back-color-tight branch from 77e663d to c68f873 Compare May 23, 2024 14:48

ruihe774 force-pushed the back-color-tight branch 2 times, most recently from c52a886 to cb5b9aa Compare June 2, 2024 05:59

ruihe774 force-pushed the back-color-tight branch from cb5b9aa to ddb5388 Compare June 17, 2024 06:05

Dudemanguy mentioned this pull request Jun 23, 2024

m_option: add a way for aliases to use sub option prefix #14421

Merged

ruihe774 force-pushed the back-color-tight branch from ddb5388 to 5737107 Compare June 24, 2024 14:07

ruihe774 requested a review from Dudemanguy June 24, 2024 14:08

na-na-hi reviewed Jun 24, 2024

View reviewed changes

DOCS/man/options.rst Outdated Show resolved Hide resolved

na-na-hi reviewed Jun 24, 2024

View reviewed changes

Dudemanguy reviewed Jun 25, 2024

View reviewed changes

sub/osd.c Outdated Show resolved Hide resolved

ruihe774 force-pushed the back-color-tight branch from 5737107 to 6e37fed Compare June 26, 2024 09:32

ruihe774 added 2 commits June 27, 2024 13:26

sub: add (sub/osd)-border-style; renaming sub options

d18869b

console: check osd-border-style for has_shadow

51508c1

ruihe774 force-pushed the back-color-tight branch from 6e37fed to 51508c1 Compare June 27, 2024 05:26

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

Are you sure you want to change the base?

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

Conversation

ruihe774 commented May 21, 2024

github-actions bot commented May 21, 2024 • edited Loading

ruihe774 commented May 21, 2024

KonoVitoDa commented May 21, 2024

hooke007 commented May 21, 2024

llyyr commented May 21, 2024

ruihe774 commented May 21, 2024

KonoVitoDa commented May 21, 2024

ruihe774 commented May 21, 2024

llyyr commented May 21, 2024

na-na-hi commented May 21, 2024

ruihe774 commented May 22, 2024

KonoVitoDa commented May 22, 2024

ruihe774 commented May 22, 2024

KonoVitoDa commented May 22, 2024 • edited Loading

ruihe774 commented May 22, 2024 • edited Loading

KonoVitoDa commented May 22, 2024

ruihe774 commented May 22, 2024 • edited Loading

KonoVitoDa commented May 22, 2024

ruihe774 commented May 22, 2024 • edited Loading

na-na-hi commented May 22, 2024 • edited Loading

KonoVitoDa commented May 22, 2024 • edited Loading

ruihe774 commented May 22, 2024

KonoVitoDa commented May 22, 2024 • edited Loading

KonoVitoDa commented May 22, 2024

ruihe774 commented May 22, 2024 • edited Loading

ruihe774 commented May 23, 2024 • edited Loading

KonoVitoDa commented May 31, 2024

ruihe774 commented Jun 1, 2024

guidocella commented Jun 1, 2024

ruihe774 commented Jun 17, 2024

kasper93 commented Jun 24, 2024

na-na-hi commented Jun 24, 2024

na-na-hi Jun 24, 2024 • edited Loading

Choose a reason for hiding this comment

ruihe774 Jun 26, 2024

Choose a reason for hiding this comment

na-na-hi commented Jun 24, 2024

Dudemanguy commented Jun 25, 2024

na-na-hi commented Jun 25, 2024 • edited Loading

ruihe774 commented Jun 26, 2024 • edited Loading

na-na-hi commented Jun 27, 2024

ruihe774 commented Jun 27, 2024

github-actions bot commented May 21, 2024 •

edited

Loading

KonoVitoDa commented May 22, 2024 •

edited

Loading

ruihe774 commented May 22, 2024 •

edited

Loading

ruihe774 commented May 22, 2024 •

edited

Loading

ruihe774 commented May 22, 2024 •

edited

Loading

na-na-hi commented May 22, 2024 •

edited

Loading

KonoVitoDa commented May 22, 2024 •

edited

Loading

KonoVitoDa commented May 22, 2024 •

edited

Loading

ruihe774 commented May 22, 2024 •

edited

Loading

ruihe774 commented May 23, 2024 •

edited

Loading

na-na-hi Jun 24, 2024 •

edited

Loading

na-na-hi commented Jun 25, 2024 •

edited

Loading

ruihe774 commented Jun 26, 2024 •

edited

Loading