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.

Sign up for GitHub

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

Jump to bottom

Sub ass style overrides always applied #4927

Open
1 task done
low-batt opened this issue May 11, 2024 · 5 comments · May be fixed by #4928
Open
1 task done

Sub ass style overrides always applied #4927

low-batt opened this issue May 11, 2024 · 5 comments · May be fixed by #4928
Assignees
@low-batt
Labels
bug feature: preferences progress: pr ready

Comments

@low-batt
Copy link
Contributor

System and IINA version:

  • macOS 13.6.3
  • IINA 1.3.4

Expected behavior:
When the setting Ignore ASS styles is not enabled style overrides are not applied.

Actual behavior:
When the setting Ignore ASS styles is not enabled IINA sets the mpv sub-ass-override option to yes causing style overrides to be applied. From the mpv manual entry for the sub-ass-override option:

--sub-ass-override=<yes|no|force|scale|strip>
Control whether user style overrides should be applied. Note that all of these overrides try to be somewhat smart about figuring out whether or not a subtitle is considered a "sign".

  • no: Render subtitles as specified by the subtitle scripts, without overrides.
  • yes: Apply all the --sub-ass-* style override options. Changing the default for any of these options can lead to incorrect subtitle rendering (default).
  • force: Like yes, but also force all --sub-* options. Can break rendering easily.
  • scale: Like yes, but also apply --sub-scale.
  • strip: Radically strip all ASS tags and styles from the subtitle. This is equivalent to the old --no-ass / --no-sub-ass options.

This also controls some bitmap subtitle overrides, as well as HTML tags in formats like SRT, despite the name of the option.

When the setting Ignore ASS styles is not enabled IINA should be setting sub-ass-override to no.

Steps to reproduce:

  • Start IINA
  • Click on Settings… under the IINA menu
  • The settings panel appears
  • On the left side of the panel click on Subtitle
  • In the ASS Subtitles section uncheck the Ignore ASS styles setting
  • Move the thumb of the Override level slider all the way to the left
  • On the left side of the panel click on Advanced
  • Slide the Enable advanced settings toggle button to be on (blue)
  • In the Additional mpv options section click on +
  • A new entry appears in the table
  • Double click on name and replace it with sub-ass-style-overrides
  • Double click on value and replace it with FontName=Wingdings
  • Confirm the panel looks like the screenshots below
  • Restart IINA to activate the new settings
  • Start a video playing with the attached subtitle file
  • Even though Ignore ASS styles is not enabled the style override will be applied and the subtitles will look like the screenshot below

subtitle-settings
advanced-settings

With Ignore ASS styles disabled the styles in the subtitle file were still overridden:
result

The subtitle file used for testing (with .txt added to make GitHub happy):
big-buck-bunny.ass.txt

  • MPV does not have this problem.

How often does this happen?
Every time.
@low-batt low-batt self-assigned this May 11, 2024
low-batt added a commit that referenced this issue May 11, 2024
@low-batt
Fix sub ass style overrides always applied, #4927
92c7e30 
This commit will:
- Change the subOverrideHandler Transformer in MPVController.mpvInit to
  return "no" when "Ignore ASS styles" is disabled
- Change PrefSubViewController.xib to only enable the "Override level"
  slider when "Ignore ASS styles" is enabled
- Add 20 to the horizontal location of the "Override level" label to
  indent this setting under the "Ignore ASS styles" setting

This changes IINA to set the mpv option "sub-ass-override" to "no" when
the setting for overriding styles in Advanced Substation Alpha subtitles
is not enabled. This commit also indents the "Override level" setting to
indicate it is a subordinate option and disables the slider if
"Ignore ASS styles" is not enabled.
@low-batt low-batt linked a pull request May 11, 2024 that will close this issue
Draft
2 tasks
@low-batt low-batt linked a pull request May 11, 2024 that will close this issue
Fix sub ass style overrides always applied, #4927 #4928
Draft
2 tasks
@uiryuu
Copy link
Member

uiryuu commented May 24, 2024

Does this mean when ignoreAssStyles is false (which is the default), all ASS styles are override by mpv? That's a huge problem, but why no one raised an issue for that?

@low-batt
Copy link
Contributor Author

Likely because It is being set to yes:

  • yes: Apply all the --sub-ass-* style override options.

And IINA does not provide access to the --sub-ass-* options. They give you a way to set styles specifically for the ASS subtitles and not change text subtitles.

However when working on this it seemed like mpv was incorrectly applying sub-scale with sub-ass-override set to yes. I noticed this when working on another problem with this setting, namely it does not provide the ability to set sub-ass-override to scale. I started working on that and have some changes, but as that would be for the feature release I put that work on the side and went back to RTL issues. I still need to enter an issue for the missing scale support and possibly a mpv issue as well. I will try and get back to that soon.

We need to check in with @lhc70000 as he may want this fix to be held until the feature release beta as if mpv is applying sub-scale then this fix could cause a change in behavior.

I noticed this when checking Hebrew translations and noticed the text showing the setting was not localized. That caused me to take a close look at this feature.

This issue shows why it is desirable to merge PR #4926. When setting mpv properties and executing mpv commands are logged it is possible to notice such problems.

@lhc70000
Copy link
Member

It was set to yes at the beginning, probably because yes was the default value. If the user opens mpv with no options, its value will be yes. However, if we merge #4928 and the user opens IINA without changing any settings, its value will be no, causing a discrepancy between IINA's behavior and mpv's.

I think when implementing this I was trying to minimize such discrepancies. When disabled, it's the default mpv behavior; when enabled, it's set to strip so all ass styles are controllable by using the options in the preference panel (so the user won't be confused by the different override levels).

I agree that it's an issue to set it to yes when it explicitly says "disabled". If we want to be consistent, we should set the default to ignoreAssStyles: true and subOverrideLevel: yes. However, in this case, the user may not immediately know what to do if they want to override ASS styles (they need to drag the slider to the right vs. clicking the checkbox in previous versions).

With the default yes value, it's also handy to scale the subtitle using the slider in the sidebar (although this may be considered as incorrect mpv behavior).

So, I'm not sure whether it's a bug because the help text says

If enabled, all ASS subtitles will be drawn using the styles below.

which should be true (right?)

So we have two choices

  • Apply the fix in Fix sub ass style overrides always applied, #4927 #4928, but I strongly recommend changing the default to ignoreAssStyles: true and subOverrideLevel: yes, otherwise the scale slider in the sidebar still stop working, which may surprise some users. However, users may find it difficult to enable ASS style overriding.
  • Leave it as-is.

@low-batt
Copy link
Contributor Author

If enabled, all ASS subtitles will be drawn using the styles below.

This is incorrect. There are multiple problems. IINA is not aligned with the mpv functionality, if I'm understanding the mpv behavior.

mpv provides options such as sub-ass-scale-with-window

--sub-ass-scale-with-window=<yes|no>
Like --sub-scale-with-window, but affects subtitles in ASS format only.

This allows a user to specify styles specifically for ASS subtitles. This matches up with --sub-ass-override=yes:

  • yes: Apply all the --sub-ass-* style override options.

For the settings IINA provides --sub-ass-override must be set at least to force:

  • force: Like yes, but also force all --sub-* options

I'm pretty sure this:

With the default yes value, it's also handy to scale the subtitle using the slider in the sidebar (although this may be considered as incorrect mpv behavior).

Is a result of a mpv defect. Caught me preparing to file an issue about it. If --sub-scale is applied with --sub-ass-override set to yes then what does setting it to scale do?:

  • scale: Like yes, but also apply --sub-scale.

I started on changes to add scale which IINA is missing, but wanted to check with mpv as to why this feature doesn't seem to be working as documented.

Because sub-scale is being applied with --sub-ass-override set to yes it is a problem that IINA users are unable to set --sub-ass-override set to no if they want scaling to only apply to text subtitles.

Using a checkbox and a slider is all fine. But currently styles are being overriden with the checkbox unchecked. That is not how this mpv feature should work.

The slider should be:

  • yes
  • scale
  • force
  • strip

At least I think that is how this mpv feature should work. I will try and get an mpv issue posted and we can see if I am confused about how this feature works.

For sure lets not merge this for 1.3.5. Maybe we end up with ignoreAssSytles: true and subOverrideLevel: scale as defaults.

@low-batt
Copy link
Contributor Author

I've entered mpv issue mpv-player/mpv#14229.

I've removed this issue from the 1.3.5 release and changed the PR to be a draft.

Seems like a correct UI representation of IINA's current behavior would not have a toggle and would just be a slider with scale, force and strip.

I'm thinking wait until we hear from mpv before doing anything more. Could be I'm confused about how this mpv feature works.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@low-batt low-batt

Labels
bug feature: preferences progress: pr ready
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Fix sub ass style overrides always applied, #4927 iina/iina
3 participants
@lhc70000 @uiryuu @low-batt