Skip to content

options: add secondary-sub-scale#18166

Open
idontwannagoo wants to merge 1 commit into
mpv-player:masterfrom
idontwannagoo:feat/secondary-sub-scale
Open

options: add secondary-sub-scale#18166
idontwannagoo wants to merge 1 commit into
mpv-player:masterfrom
idontwannagoo:feat/secondary-sub-scale

Conversation

@idontwannagoo

@idontwannagoo idontwannagoo commented Jun 21, 2026

Copy link
Copy Markdown

Adds --secondary-sub-scale to allow scaling secondary subtitles independently from primary subtitles.

By default the option is default, which preserves the existing behavior by inheriting --sub-scale. Setting a numeric value overrides the scale for secondary subtitles only.

Fixes #14989.

Tested with:

  • uvx meson test -C build --print-errorlogs

AI disclosure:
This contribution was developed with AI assistance. I have reviewed the changes, understand what was changed and why, and take responsibility for the code and review responses.

@verygoodlee

verygoodlee commented Jun 21, 2026

Copy link
Copy Markdown
Contributor

By default the option is default, which preserves the existing behavior by inheriting --sub-scale.

I don't think this is necessary, as the --secondary-sub-delay/--secondary-sub-pos option never inherits --sub-delay/--sub-pos.

does this default value support the add/multiply command? If it does, run add secondary-sub-scale 1 first and then run add secondary-sub-scale -1, what value (default or numeric value) will it restore? this is unclear to users.

@idontwannagoo

idontwannagoo commented Jun 21, 2026

Copy link
Copy Markdown
Author

By default the option is default, which preserves the existing behavior by inheriting --sub-scale.

I don't think this is necessary, as the --secondary-sub-delay/--secondary-sub-pos option never inherits --sub-delay/--sub-pos.

does this default value support the add/multiply command? If it does, run add secondary-sub-scale 1 first and then run add secondary-sub-scale -1, what value (default or numeric value) will it restore? this is unclear to users.

To clarify before I change this: the current patch uses default to preserve the existing behavior where --sub-scale affects both primary and secondary subtitles.

However, this does make the command behavior less clear because default is represented as NaN, so the generic add/multiply commands do not meaningfully operate on it while the option is still at default: add secondary-sub-scale 1 keeps it at default, and adding -1 afterwards also keeps it at default. The commands only start behaving numerically after the option has first been set to an explicit value, for example with set secondary-sub-scale 1.

If we instead make --secondary-sub-scale default to numeric 1, the option would be consistent with --secondary-sub-delay/--secondary-sub-pos and command behavior would be clearer, but users who currently rely on --sub-scale affecting both subtitle tracks would need to set both options.

Another compatibility concern is that frontends, scripts, or existing user configurations may currently expose or set only --sub-scale. If --secondary-sub-scale defaults to 1, those setups would no longer scale secondary subtitles unless they are updated to set the new option as well.

Which behavior would be preferred for mpv? I can adjust the patch accordingly.

@idontwannagoo

Copy link
Copy Markdown
Author

Would another option be to keep default for compatibility, but make add/multiply operate on the inherited effective value?

For example, if secondary-sub-scale is still default, add secondary-sub-scale 0.1 would first use the current --sub-scale value as the base, then store the result as an explicit secondary-sub-scale value.

This would preserve the existing behavior for users/frontends that only set --sub-scale, while still making the command behavior clearer.

@idontwannagoo

idontwannagoo commented Jun 21, 2026

Copy link
Copy Markdown
Author

I pushed an implementation of this approach: default still preserves the existing --sub-scale behavior, while add/multiply now operate on the inherited effective value and store the result as an explicit value. I also added tests for both secondary-sub-scale and options/secondary-sub-scale.

With the follow-up commit, add/multiply on default use the inherited effective value as the starting point and then store the result as an explicit numeric value.

So add secondary-sub-scale 1 followed by add secondary-sub-scale -1 restores it to numeric 1, not default. To return to inherited behavior, the user would still set secondary-sub-scale back to default.

I think this is still less surprising than having add/multiply leave the option unchanged while it is at default.

@Dudemanguy

Copy link
Copy Markdown
Member

Sorry for the late response after you did all this extra work but I agree with verygoodlee. I don't think a default value is needed. Several other secondary-sub-* options were added after the fact and they all control things totally separately. As long as it's marked as an interface change, I think it is much cleaner just to handle it like the other options.

@idontwannagoo idontwannagoo force-pushed the feat/secondary-sub-scale branch from 50ac839 to 690730f Compare July 4, 2026 12:05
@idontwannagoo

Copy link
Copy Markdown
Author

Thanks for the review. I updated the patch so --secondary-sub-scale is now an independent option with a default value of 1, matching the behavior of the other secondary-sub-* options instead of inheriting from --sub-scale.

The option is applied to both ASS and bitmap subtitle rendering paths, documented in the manpage, and included in watch-later options.

Comment thread options/options.c Outdated
@idontwannagoo idontwannagoo force-pushed the feat/secondary-sub-scale branch from 690730f to 79761c7 Compare July 4, 2026 14:02
@idontwannagoo

Copy link
Copy Markdown
Author

Updated, thanks. I moved sub-scale into mp_subtitle_shared_opts as sub_scale[2], so sub-scale and secondary-sub-scale are now handled the same way as the other primary/secondary subtitle options.

@Dudemanguy Dudemanguy left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Just some style stuff mainly.

Comment thread DOCS/interface-changes/secondary-sub-scale.txt
Comment thread sub/sd_ass.c Outdated
Comment thread sub/sd_lavc.c Outdated
Comment thread sub/sd_lavc.c Outdated
Comment thread DOCS/man/options.rst Outdated
This allows scaling secondary subtitles independently from primary subtitles. The option defaults to 1 like --sub-scale and follows the other secondary-sub-* options by not inheriting from the primary subtitle setting.

Fixes mpv-player#14989.
@idontwannagoo idontwannagoo force-pushed the feat/secondary-sub-scale branch from 79761c7 to 21fd20a Compare July 4, 2026 15:53
@idontwannagoo

Copy link
Copy Markdown
Author

Updated, thanks.

@idontwannagoo idontwannagoo requested a review from Dudemanguy July 4, 2026 16:00

@Dudemanguy Dudemanguy left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks LGTM now. Will actually test later.

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.

secondary-sub-scale option?

3 participants