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

Subtitle autoselect improvements #8865

Merged
merged 15 commits into from
Jun 25, 2023
Merged

Subtitle autoselect improvements #8865

merged 15 commits into from
Jun 25, 2023

Conversation

rcombs
Copy link
Contributor

@rcombs rcombs commented May 26, 2021 •
edited
Loading

Addresses #8863

  • (Some platform-specific infrastructural tweaks to provide needed routines)
  • Support for matching 2-character language tags against 3-character ones, and vice versa
  • Minimal IETF language tag matching support, with support for 2-subtag tags (language+region)
  • Full subtitles off by default when audio language matches preferred subtitle language
  • slang defaults to the user's OS-level preferred language list
  • Additional controls for subtitle autoselection behavior
  • Fix various issues with forced-only autoselection
  • Expose forced-only autoselection in CLI
  • Expose forced-only autoselection and provide toggle in OSC

@rcombs rcombs force-pushed the sub-autoselect-improvements branch 4 times, most recently from cd129e2 to b265bd3 Compare May 27, 2021 06:09
@Akemi
Copy link
Member

Akemi commented May 27, 2021

Swift: avoid unnecessary unsafe conversions; fixes crash in debug builds is an unrelated commit and should not be included in this PR. i will comment on the actual change there.

@rcombs
Copy link
Contributor Author

rcombs commented May 27, 2021

Yeah, I just can't debug without it.

Akemi
Akemi reviewed May 27, 2021
View reviewed changes
@rcombs rcombs mentioned this pull request May 28, 2021
Closed
@rcombs rcombs force-pushed the sub-autoselect-improvements branch from b265bd3 to b486b89 Compare May 29, 2021 02:25
@rcombs rcombs force-pushed the sub-autoselect-improvements branch 3 times, most recently from 700ccbb to edcf71f Compare June 25, 2021 08:44
@rcombs
Copy link
Contributor Author

rcombs commented Jun 26, 2021

Updated with some changes:

  • User language detection now works correctly on Windows (and has been tested there)
  • Forced-only autodetection behavior is now better-exposed in properties
  • Forced-only sub mode is now exposed, and toggleable, in the OSC (though the indicator is currently very simple and text-based)

@rcombs rcombs force-pushed the sub-autoselect-improvements branch from edcf71f to 3adca78 Compare June 27, 2021 06:26
@Doofussy2 Doofussy2 mentioned this pull request Nov 8, 2021
Closed
@rcombs rcombs force-pushed the sub-autoselect-improvements branch from 3adca78 to 5bf6d44 Compare November 28, 2021 23:00
@rcombs rcombs requested a review from Akemi November 28, 2021 23:00
@rcombs rcombs force-pushed the sub-autoselect-improvements branch 2 times, most recently from eb0ccf1 to f617bfb Compare November 28, 2021 23:54
@rcombs
Copy link
Contributor Author

rcombs commented Nov 29, 2021

(Re-pushes fix meson build)

@dyphire dyphire mentioned this pull request Feb 1, 2022
Closed
@rcombs rcombs force-pushed the sub-autoselect-improvements branch from f617bfb to a6a338c Compare April 5, 2022 02:18
@rcombs
Copy link
Contributor Author

rcombs commented Apr 5, 2022

Rebased on latest master.

@NaruZosa
Copy link

I'm still using version 0.32.0 MPV from January 2020 due to #8396, since this has yet to be merged.

@rcombs rcombs force-pushed the sub-autoselect-improvements branch from a6a338c to 0877fc8 Compare February 20, 2023 06:13
@NaruZosa
Copy link

Thank you ❤

@rcombs rcombs force-pushed the sub-autoselect-improvements branch from 0877fc8 to d118d12 Compare April 5, 2023 03:20
@sfan5 sfan5 self-requested a review April 29, 2023 12:58
sfan5
sfan5 reviewed May 21, 2023
View reviewed changes
DOCS/man/options.rst Outdated
When autoselecting a subtitle track, select a full/non-forced one even if the selected
audio stream matches your preferred subtitle language (default: no).

``--subs-fallback=<always|default|no>``
Copy link
Member

Choose a reason for hiding this comment

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

yes / default / no would be better here IMO

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure.

@sfan5
Copy link
Member

sfan5 commented May 21, 2023

So I tested this and here are my thoughts:

  • the automatic selection changes work well in general
  • slang and subs-with-matching-audio default change needs to be documented in interface changes
  • maybe there could be some smart rules to hide the [F], when watching non-dubbed content it just acts as a funny alternative to pressing v
  • when no sub tracks are present the [ ] never appears, if no track is selected it appears but is disabled. intentional?
  • I couldn't get the signs track to be properly selected with slang=auto on this file, but I think that's a file issue -> https://0x0.st/HqXu.mkv

@rcombs
Copy link
Contributor Author

rcombs commented May 21, 2023

Just as a note, I'm kinda hoping someone can eventually come up with an icon pair to replace the [F].

Re: docs, sure, I'll write 'em up.
Re: automatically hiding [F] when not relevant, I could probably decide based on sub format for now (forced events only really apply to vobsub and PGS right now); maybe long-term this should be exposed by lavc as a flag in AVCodecDescriptor.
Re: inconsistent display of [ ], I'll take a look; I don't recall the details there.
Re: that file, yeah, it's improperly tagged (nothing's marked as forced).

@rcombs
Copy link
Contributor Author

rcombs commented May 27, 2023

Re: [ ] display, what behavior would you prefer? I could hide it altogether whenever the currently-selected track isn't either dvdsub or PGS?

@rcombs rcombs force-pushed the sub-autoselect-improvements branch 2 times, most recently from 42de279 to 1ebe94f Compare May 27, 2023 23:21
@rcombs
Copy link
Contributor Author

rcombs commented May 27, 2023

Rebased and re-pushed; I believe all comments are addressed. Forced-only is no longer used for codecs that cannot include forced events, and the [ ] box is hidden when such a codec is not in use.

@sfan5
Copy link
Member

sfan5 commented Jun 9, 2023

one more: I'd rename

Add more precise sub fallback options

to

player: add more precise sub fallback options

@sfan5
Copy link
Member

sfan5 commented Jun 23, 2023

needs rebase

rcombs added 15 commits June 24, 2023 05:32
@rcombs
options: set subs-with-matching-audio to off by default
c93da55 
This means that subtitles won't be displayed unless the user asks for them
@rcombs
misc: add language-matching utilities
cf90715
@rcombs
osdep: move cfstr<->cstr conversions to a new apple_utils.c file
6977fb5
@rcombs
osdep: add mp_get_user_langs
e971327
@rcombs
loadfile: short-circuit early on null stream language
de785da
@rcombs
loadfile: take a const char* in match_lang
3bbefe6
@rcombs
loadfile: use mp_match_lang_single
8460b41 
This adds basic support for IETF language tags,
as well as matching 2-letter language codes against 3-letter ones (and vice versa).
@rcombs
loadfile: support "auto" as a language option
9b0186d 
This uses the user's OS-level preferred set of languages
@rcombs
options: default slang to auto
900462b
@rcombs
player: add more precise sub fallback options
64ca873
@rcombs
sub: rewrite auto-forced-only support
014e1d4 
- No longer has a fake "option" used only internally (which didn't always get propagated properly)
- Always overrideable during playback
@rcombs
command: report auto-forced-only flag in track desc
9f37a2a
@rcombs
command: add sub-forced-only-cur prop
84466be
@rcombs
loadfile: expose auto-forced-only when printing stream info
51dfa82
@rcombs
osc: add toggle for forced-only subs
333b9a5
@rcombs rcombs force-pushed the sub-autoselect-improvements branch from 1ebe94f to 333b9a5 Compare June 24, 2023 10:32
@rcombs
Copy link
Contributor Author

rcombs commented Jun 24, 2023

Rebased and addressed commit-message issue; are we good to merge?

@sfan5 sfan5 merged commit 945d7c1 into mpv-player:master Jun 25, 2023
@hooke007 hooke007 mentioned this pull request Jun 29, 2023
Closed
@sfan5 sfan5 mentioned this pull request Jul 16, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sfan5 sfan5 sfan5 approved these changes

@Akemi Akemi Awaiting requested review from Akemi

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@rcombs @Akemi @NaruZosa @sfan5