options: add --secondary-sub-pos

[karelrooted]

The default value is 0 (on the top of the screen)

related issue: #3022

[github-actions]

Download the artifacts for this pull request:

Windows

• mpv-i686-w64-mingw32
• mpv-x86_64-w64-mingw32

[guidocella]

The option works. The references to SD_CTRL_SET_TOP can be removed since it's no longer used.

[karelrooted]

replace newline with space when display two subtitle at the bottom will have a better user experience, is adding another option --sub-newline-to-space acceptable ?

[karelrooted]

The option works. The references to SD_CTRL_SET_TOP can be removed since it's no longer used.

the unused SD_CTRL_SET_TOP is removed

[guidocella]

We should decide now whether to implement all secondary sub options in a separate variable like --secondary-sub-visibility or in an array like --secondary-sub-delay.

replace newline with space when display two subtitle at the bottom will have a better user experience, is adding another option --sub-newline-to-space acceptable ?

That's up to maintainers but it should be in a later PR and newline and space should probably be pluralized.

[dyphire]

This has not completely closed #3022 , it still forces the subtitle style to be stripped for secondary subtitles.
I tried to add a new option of --secondary-sub-override to control the subtitle style override for secondary subtitles, and will submit it after this PR is merge.

[karelrooted]

This has not completely closed #3022 , it still forces the subtitle style to be stripped for secondary subtitles. I tried to add a new option of --secondary-sub-override to control the subtitle style override for secondary subtitles, and will submit it after this PR is merge.

PR is updated to not close #3022
I mainly use this to play bilingual subtitle, only tested it on converted SRT subtitle, I think there is a reason secondary subtitle's ass style is being striped by default in the original design, so that behavior is maintained ( maybe when there is two ass subtitle ,usually there position style will cause them to overlap? )

[dyphire]

This has not completely closed #3022 , it still forces the subtitle style to be stripped for secondary subtitles. I tried to add a new option of --secondary-sub-override to control the subtitle style override for secondary subtitles, and will submit it after this PR is merge.

PR is updated to not close #3022 I mainly use this to play bilingual subtitle, only tested it on converted SRT subtitle, I think there is a reason secondary subtitle's ass style is being striped by default in the original design, so that behavior is maintained ( maybe when there is two ass subtitle ,usually there position style will cause them to overlap? )

The subtitle style that override secondary subtitles is meaningful, so the new option maintains its default behavior.

In certain scenarios, it can also be useful to preserve the subtitle style of ass subtitles for secondary subtitles. For example, some people prefer to preserve the subtitle style when combining bilingual ass subtitles, while others may obtain Danmaku within ass subtitle format when playing videos from websites such as niconico and bilibili. They also need to preserve the subtitle style to obtain the correct rendering when displaying as secondary subtitles.

[Lypheo]

Btw, it’s actually quite trivial to make secondary subs fully configurable, but I’m not sure how to do it without all that ugly code duplication.

[Dudemanguy]

You can just do this to duplicate it. Of course you would need to adjust sec_sub_visibility and sub_delay[2] as well. The only weird thing is that this actually hits an assert that was introduced in 4cb264a. I'm not really sure if that's needed.

diff --git a/options/m_config_core.c b/options/m_config_core.c
index 08a76eb76d..27534b817c 100644
--- a/options/m_config_core.c
+++ b/options/m_config_core.c
@@ -466,8 +466,8 @@ static void add_sub_group(struct m_config_shadow *shadow, const char *name_prefi
                           const struct m_sub_options *subopts)
 {
     // Can't be used multiple times.
-    for (int n = 0; n < shadow->num_groups; n++)
-        assert(shadow->groups[n].group != subopts);
+    //for (int n = 0; n < shadow->num_groups; n++)
+    //    assert(shadow->groups[n].group != subopts);
 
     if (!name_prefix)
         name_prefix = "";
diff --git a/options/options.c b/options/options.c
index 41cfce7fc0..ce8096e186 100644
--- a/options/options.c
+++ b/options/options.c
@@ -284,11 +284,9 @@ const struct m_sub_options mp_sub_filter_opts = {
 const struct m_sub_options mp_subtitle_sub_opts = {
     .opts = (const struct m_option[]){
         {"sub-delay", OPT_FLOAT(sub_delay[0])},
-        {"secondary-sub-delay", OPT_FLOAT(sub_delay[1])},
         {"sub-fps", OPT_FLOAT(sub_fps)},
         {"sub-speed", OPT_FLOAT(sub_speed)},
         {"sub-visibility", OPT_BOOL(sub_visibility)},
-        {"secondary-sub-visibility", OPT_BOOL(sec_sub_visibility)},
         {"sub-forced-events-only", OPT_BOOL(sub_forced_events_only)},
         {"stretch-dvd-subs", OPT_BOOL(stretch_dvd_subs)},
         {"stretch-image-subs-to-screen", OPT_BOOL(stretch_image_subs)},
@@ -667,6 +665,7 @@ static const m_option_t mp_opts[] = {
     {"cover-art-whitelist", OPT_BOOL(coverart_whitelist)},
 
     {"", OPT_SUBSTRUCT(subs_rend, mp_subtitle_sub_opts)},
+    {"secondary", OPT_SUBSTRUCT(subs2_rend, mp_subtitle_sub_opts)},
     {"", OPT_SUBSTRUCT(subs_filt, mp_sub_filter_opts)},
     {"", OPT_SUBSTRUCT(osd_rend, mp_osd_render_sub_opts)},
 
diff --git a/options/options.h b/options/options.h
index f1940266ca..d2c66d4d37 100644
--- a/options/options.h
+++ b/options/options.h
@@ -198,6 +198,7 @@ typedef struct MPOpts {
     bool cursor_autohide_fs;
 
     struct mp_subtitle_opts *subs_rend;
+    struct mp_subtitle_opts *subs2_rend;
     struct mp_sub_filter_opts *subs_filt;
     struct mp_osd_render_opts *osd_rend;

[karelrooted]

Btw, it’s actually quite trivial to make secondary subs fully configurable, but I’m not sure how to do it without all that ugly code duplication.

fully configurable sounds good, I will close this PR in favor of @Lypheo 's fully configurable version

[dyphire]

Is it really necessary to implement all sub-* options for secondary subtitles? I am skeptical about the practicality of this.

[Dudemanguy]

Yeah I'm not really sure all of them are needed either. We do keep getting people wanting to customize things for secondary subtitles however so it probably makes sense to generalize somewhat instead of constantly adding yet another --secondary-* option.

[dyphire]

Yeah I'm not really sure all of them are needed either. We do keep getting people wanting to customize things for secondary subtitles however so it probably makes sense to generalize somewhat instead of constantly adding yet another --secondary-* option.

This also sounds reasonable. If we want to add fully configurable --secondary-* options for secondary subtitles, please also note that setting their default values separately for --secondary-sub-pos and --secondary-sub-ass-override options to maintain the current default behavior. The above example changes missed this point.

Although I still can't see the benefits of applying different behaviors for many --sub-* options, it means confusion in subtitle behavior for me, so I still don't like this kind of change.

[Lypheo]

fully configurable sounds good, I will close this PR in favor of @Lypheo 's fully configurable version

Don’t, I didn’t intend to make a PR. You can incorporate it into yours if you want.

[Dudemanguy]

Given that secondary-sub-pos is a bit special since it also removes a sd_ctrl. I think we can get this PR sorted out first and then refactor it later.

[Dudemanguy]

Works fine

[Dudemanguy]

I will follow up with a small refactor shortly.

[oldk1331]

Hi, this --secondary-sub-pos option does not take effect on bluray pgs subtitles.

It works after changing opts->sub_pos to (order == 1 ? opts->sec_sub_pos : opts->sub_pos) in sub/sd_lavc.c.

[karelrooted]

Hi, this --secondary-sub-pos option does not take effect on bluray pgs subtitles.

It works after changing opts->sub_pos to (order == 1 ? opts->sec_sub_pos : opts->sub_pos) in sub/sd_lavc.c.

I only tested it on ass subtitle, forgot the image subtitle, would you mind submit a PR

[oldk1331]

Could you do the fix please? I'm a bit lost after the recent --secondary-sub-ass-override option and refactor.