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.

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

player: add --subs-match-os-language option #12100

Merged
merged 4 commits into from
Aug 28, 2023

Conversation

Dudemanguy
Copy link
Member

@Dudemanguy Dudemanguy commented Aug 7, 2023

Unfortunately, the auto slang behavior still proved to be problematic in some instances. Essentially, it would be as if you set slang to the value of your OS language (if unset) which is not necessarily desirable and results in subs getting selected when they shouldn't. For example, with --no-config on en_US.UTF-8 would basically behave like --slang=en and cause english subtitles to be selected even if it is not the default track. Flipping --subs-with-matching-audio to no again would simply cause the opposite problem where people that do set slang have subtitles not actually picked.

So instead, just divorce this concept from --slang all together because it doesn't really fit there (additionally, it removes it from --alang and --vlang as well but auto has no real use there). Instead, use a --subs-match-os-language option that handles this, turn it on by default, but always prefer --slang over it if it is set. In the case where --slang is not set and we are using the OS langs, then we can proceed to ignore the --subs-with-matching-audio complexity all together and skip picking the subtitles if the audio language matches.

The idea is to have the best of both worlds. I think the only remaining change in behavior was with the sub language == os language case, but not 100% sure. Testing welcome.

@sfan5
Copy link
Member

sfan5 commented Aug 8, 2023

additionally, it removes it from --alang and --vlang as well but auto has no real use there

While uncommon I think you could use alang=auto (optionally alongside slang=auto) fine, so there is some potential for use.
I'd rather having smart handling added to auto than a new option that again only modifies subtitle behaviour.

Alternatively, what about not doing anything? If users don't set slang or subs-with-matching-audio that means they don't care to influence subtitle selection. When mpv then selects a track matching the OS language, is it that bad?
Unlike the option changes that were reverted in #12015 I think this is quite unobstructive.

@aufkrawall
Copy link

I'm hesitant to ask, but may this finally fix behavior with forced subtitle selection, i.e. #8396 ?

Issue case:
Config:

alang=eng,ger
slang=eng,ger
sub-forced-only=yes

File contains 1. [ger] and 2. [eng] as audio tracks. Contained subtitle tracks are 1. [ger] Forced, 2. [ger] and 3. [eng] . With current git-master and the config above, it selects 2. [eng] audio (good, as expected) and as subtitle track 3. [eng] (terrible, it should select 1. [ger] Forced instead).

I'm sorry if this is perceived as hijacking this PR, but I'm pretty sure mpv's current behavior is seen as outright sadistic by a fair number of users. :(

@Dudemanguy
Copy link
Member Author

Dudemanguy commented Aug 8, 2023

While uncommon I think you could use alang=auto (optionally alongside slang=auto) fine, so there is some potential for use.

Is there a real usecase for someone wanting the audio language to match their OS language? I don't think this is something we would ever assume and someone wanting particular languages for audio would just configure it anyway. The only real point of auto is to make it a default; it's not like there's a practical benefit to me specifying auto in a config file instead of just en (or whatever language applicable).

If users don't set slang or subs-with-matching-audio that means they don't care to influence subtitle selection. When mpv then selects a track matching the OS language, is it that bad?

Yes, it can be. The current behavior can result in a track being selected even if the media file doesn't tag it as default or forced.

With current git-master and the config above, it selects 2. [eng] audio (good, as expected) and as subtitle track 3. [eng] (terrible, it should select 1. [ger] Forced instead).

With this PR it might not select the eng subtitle track (depends on how it is tagged). It still wouldn't select the german subs because forced tracks are only ever autoselected if they match the language of the audio.

@aufkrawall
Copy link

Thanks, I think that's at least an improvement (basically restores some previous behavior?).

If you are interested in this, I'd be highly grateful if you could expand or change behavior of --sub-forced-only=yes in e.g. a subsequent PR. LAV Filters has the option "Only forced subtitles" for Subtitle Selection Mode (like imho the name of the aforementioned mpv option suggests) and in the example described above, it leads to [ger] Forced being selected also with eng,ger load order configured.

@Dudemanguy
Copy link
Member Author

Adding an always choice to that sounds reasonable and would be pretty simple I think.

@hooke007
Copy link
Contributor

hooke007 commented Aug 9, 2023

I have another related question here.
Snipaste_2023-08-09_23-56-14
The new element seems to be added & designed so casually. Is there any important reason that it have to show? Honestly, I think adding a button instead to switch video tracks or mkv's editions would be more useful.

@Dudemanguy
Copy link
Member Author

Not really related to this. No opinion personally. I barely even have any files with forced subtitles though.

@hooke007
Copy link
Contributor

hooke007 commented Aug 10, 2023

Well, I missed the PR #12015 . IMO, UI/UX should also be a part of 'behavior' and I should post my comments at that time.
I believe the option --sub-forced-only could be better only stay in mpv.conf rather than keep being highlight in UI. Unless someone really think changing its value frequently at runtime is necessary.
Anyway, just some personal views.

BTW, even though the button hides, it will affect sub's behavior. Try to click on that area and sub will hide or show.

@sfan5
Copy link
Member

sfan5 commented Aug 14, 2023

Yes, it can be. The current behavior can result in a track being selected even if the media file doesn't tag it as default or forced.

And mpv did not select such tracks before?

@Dudemanguy
Copy link
Member Author

And mpv did not select such tracks before?

Correct. Consider this example. If you play it with --no-config an english system (or with LANG=en_US.UTF-8 for instance), it will select the subtitle track. It shouldn't but this happens because --slang=auto is effectively the same as --slang=en. Going back to before the subtitle changes (like 78285e9), it didn't select that track which is correct (not a default/forced track). Now with those changes, such a track wasn't selected because --subs-with-matching-audio was set to no by default but that had all the other issues as well. When I flipped it back to yes later, then we had this problem.

Considering that #11913 also happens. I'm considering just re-doing the default track selection again since it just seems wrong.

@Dudemanguy Dudemanguy force-pushed the sub-os-lang branch 3 times, most recently from 383696c to 3f4b7f0 Compare August 26, 2023 05:21
@Dudemanguy
Copy link
Member Author

I'm considering just re-doing the default track selection again since it just seems wrong.

I've done this so this PR is now 4 parts.

  1. Remove the auto option handling from slang/alang/vlang.
  2. Simplify select_default_track so it's actually readable (hopefully) and fix some incorrect selection with regards to forced tracks.
  3. Add --subs-match-os-language as a replacement for auto.
  4. Add always to --subs-fallback-forced.

Testing welcome to make sure I didn't break something somewhere. For a sample file, here's this if it's handy (has a lot of subtitle tracks in it). I fictitiously changed the the arabic track to Japanese (sue me) since my machine has english and japanese locales readily available (i.e. testing with LANG=ja_JP.UTF-8). I don't know if anyone out there actually tags multiple default tracks but LANG=ja_JP.UTF-8 mpv --no-config will select the fake Japanese sub track. Without the environment variable it selects english. With --no-subs-match-os-language, it just always picks the first one (english).

@aufkrawall
Copy link

Thanks a lot!
It now behaves as I would think makes the most sense. My config contains only alang=eng,ger and it only selects forced subtitles by default, which is what I would expect to happen. Hope it works great for others too.

Though I don't think I have any sample that contains multiple forced subtitle tracks. I suppose with such a file, mpv would select a forced subtitle track according to the alang list order? At least this would seem logical to me.

@Dudemanguy
Copy link
Member Author

Dudemanguy commented Aug 27, 2023

Well no, it picks based on slang order in the hypothetical case where you are passing always to --subs-fallback-forced. alang is not actually directly relevant per se. What matters is that the audio language of the currently selected audio track matches the language of the forced subtitles. If this is the same scenario that you described earlier, I'd not expect the german subs to be selected if your config really is only alang=eng,ger.

@sfan5
Copy link
Member

sfan5 commented Aug 27, 2023

Correct. Consider this example. If you play it with --no-config an english system (or with LANG=en_US.UTF-8 for instance), it will select the subtitle track. It shouldn't but this happens because --slang=auto is effectively the same as --slang=en.

Feel free to go ahead with the changes then.

@Dudemanguy
Copy link
Member Author

@aufkrawall: Could you clarify a bit and/or possibly provide a sample? The behavior you're describing honestly sounds like a bug with this PR. It shouldn't be selecting german forced subs if the audio is english unless you are also passing always to --subs-fallback-forced. I did some tests myself with some dual audio releases and such and couldn't reproduce anything like what you described.

@aufkrawall
Copy link

Sure, I'll take another look at it in a few hours.

@aufkrawall
Copy link

It selects [ger] Forced with alang=eng,ger as sole mpv config entry when playing this sample:
https://0x0.st/H9UJ.mkv

@Dudemanguy
Copy link
Member Author

Dudemanguy commented Aug 27, 2023

Oh okay it happens with LANG=de_DE.UTF-8 but not my normal locale (english) so I assume your locale must be German. That subtitle track is tagged as both default and forced so that's why it gets picked which is indeed correct (the default part matters not the forced here). Now the question is whether to consider it a feature or a bug that the German track doesn't get picked for me by default (with --no-subs-match-os-language it picks the German subs).

Edit: Yeah it's a bug. compare_track is giving priority to the language over the default tag which is understandable but it shouldn't in this case. forced isn't actually relevant. Thanks for noticing, I'll think of something later.

@Dudemanguy
Copy link
Member Author

Okay should be fixed now. All I needed to do was make compare_track aware of os_langs.

@aufkrawall
Copy link

aufkrawall commented Aug 27, 2023

Nice. Could it be that in your provided sample, the eng subtitle track is also flagged as forced and default at the same time? As mpv selects it by default. I tested some other videos that advertise only non-forced subtitle tracks, and with them, there is no subtitle track selected by default with just alang=eng,ger.

Edit: As it also selects it with

alang=eng,ger
slang=eng,ger
subs-fallback-forced=always

In other files when there are no forced tracks available, the subtitles are guarded behind the [F] in the UI. So to me, it looks like it indeed works as expected. :)

@Dudemanguy
Copy link
Member Author

In that example, the english track is tagged as default (not forced) but so is the fake Japanese track. The general rule of thumb with subtitle selection priority and no configuration is: forced subtitle track if it matches the audio language -> default subtitle track -> nothing. In master, this doesn't work for a variety of situations currently. And of course if you set various subtitle options, the selection changes (e.g. slang will always try to select a track matching the language regardless of how the track is tagged).

This proved to be too problematic. Depending on the value of
--subs-with-matching-audio, you could either end up with cases where
--slang wasn't respected and users didn't get subtitles or alternatively
cases where subtitles were given and the user didn't ask for them.
Fundamentally, the OS language functionality doesn't really map well to
slang (and for alang/vlang it makes zero sense; not that anyone actually
used it). Instead of trying to shove it in an option where it doesn't
belong, we should split this off into something else. So for now, just
remove the special handling of "auto" and flip slang back to NULL.
What was previously there is extremely complicated and really confusing.
Poorly named variables like "prefer_forced" that don't neccesarily have
anything to do with prefering forced tracks didn't help either. Try to
rewrite a few things to be saner. The idea is that after you loop
through the tracks, the special sub-specific options (like subs-fallback
and so on) should be handled and the track should be deselected if
appropriate. Another change is to remove the "prefered_forced" argument
in compare_track. This actually was both not neccessary and caused bad
behavior by always depriortizing forced tracks even when it didn't apply
(e.g. forced video tracks were never selected even though the flag
should simply be ignored for anything that's not a subtitle track).
This is the replacement for the previous auto option for slang. It
behaves similar however it never overrides slang if that is set and will
instead try to pick the subtitle that matches the user's language if
appropriately flagged by the file.
In general, forced tracks should only be shown if they match the
language of the audio. However some people do want them no matter what,
so add an always option to this so such tracks are always selected.
@Dudemanguy Dudemanguy merged commit 165f9e0 into mpv-player:master Aug 28, 2023
@Dudemanguy Dudemanguy deleted the sub-os-lang branch August 28, 2023 18:44
This was referenced Aug 28, 2023
@Dudemanguy
Copy link
Member Author

Dudemanguy commented Sep 1, 2023

@aufkrawall:
Sorry for the ping. I might break your config again possibly (???). I realized a problem that forced subtitle tracks completely ignore the slang settings (i.e. they are always shown if available and match the audio language) which is probably not so good. So for example in the clip you posted, if your config is this;

alang=eng,ger
slang=eng,ger

The change I'm planning would make the eng subtitle be selected because it's first in your list of slang languages (language priority wins over forced and default tags always). Is that alright with you? If your config is just:

alang=eng,ger

Then the german track is still selected by default since it's tagged as the default.

@aufkrawall
Copy link

Hm, in my clip, it selects just [eng] with

alang=eng,ger
slang=eng,ger
subs-fallback-forced=always

and latest mpv git-master.

And it also selects [eng] for your anime sample (no change, but I guess wasn't to be expected). Wouldn't it make more sense if subs-fallback-forced=always prevented any subs to be selected that aren't explicitly flagged as forced?

@Dudemanguy
Copy link
Member Author

No that option is, like it says, for falling back. When you don't have a sub that's selected in some other manner (like slang), then fallback on one of those. I'm going to add yet another option for this soon. Probably something like --subs-select-forced-only which would do like what people thought sub-forced-only did (awful name).

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.

4 participants