feat: New autoShowText config to change initial text visibility behavior

[valotvince]

Description

Enforce user text preferences without taking audio into account. An user could like to have French audio, with French subtitles (or captions for hearing impaired) at the same time, who are we to judge ? 😄 My use case is that we save what the user chose as audio / text languages, and then we always start with those languages (and / or roles)

I tried to make some archeology on the codebase to see why the feature was developed like that, but I wasn't able to find an explanation.

Screenshots (optional)

Type of change

• [ ] Bug fix (non-breaking change which fixes an issue)
• [x] New feature (non-breaking change which adds functionality)
• [ ] Breaking change (fix or feature that would cause existing functionality to not work as expected)
• [x] This change requires a documentation update

Checklist:

• [x] I have signed the Google CLA https://cla.developers.google.com
• [x] My code follows the style guidelines of this project
• [x] I have made corresponding changes to the documentation
• [x] I have added tests that prove my fix is effective or that my feature works
• [x] I have verified my change on multiple browsers on different platforms
• [x] I have run ./build/all.py and the build passes
• [x] I have run ./build/test.py and all tests pass

[valotvince]

@joeyparrish 👋 WYT of this PR ?

[valotvince]

Hey @theodab, I must concede you bested me in term of archeology 😄

Well in our Shaka-Player usage, we struggled to understand why our preferredTextLanguage / preferredTextRole was never applied and that was causing it, I came to think that we are not the only ones (ok, we could have read the doc)

I guess if this is properly advertised in the changelog of the release this commit is going into, that wouldn't be a problem ! I think the "only" thing needed to get the previous behaviour is to do something like:

preferredAudioLanguage: audioLanguage,
preferredTextLanguage: textLanguage === audioLanguage ? '' : textLanguage,

Let me know if you want to go through with it, or if I should add a new configuration key to keep the existing behaviour :)

[joeyparrish]

I suppose I don't understand the problem. Perhaps I misunderstand or misremember the state of the player, but when I read this:

Enforce user text preferences without taking audio into account. An user could like to have French audio, with French subtitles (or captions for hearing impaired) at the same time, who are we to judge ? 😄 My use case is that we save what the user chose as audio / text languages, and then we always start with those languages (and / or roles)

I feel like this is a non-issue from my perspective. An application may:

1. Set a preferred audio language and text language in the player configuration to influence the initial choices of the player
2. Choose specific audio or text languages after calling load() to override those initial choices
3. Choose to enable text display before or after calling load()
4. Choose to disable auto-text display after calling load()

It looks to me like you're changing the auto-text display rules with this PR, but I have a hard time connecting that to how I understood your goal:

An user could like to have French audio, with French subtitles (or captions for hearing impaired) at the same time, who are we to judge ?

We don't judge the choice! If you ask for those via preference or via select*Track() calls, we don't override the application. Those will be the selected tracks.

My use case is that we save what the user chose as audio / text languages, and then we always start with those languages (and / or roles)

I don't see how the automatic decision to enable text breaks this for you. We don't override the choice of languages at all, we just choose based on the languages whether or not text should be enabled by default. The alternative would be to require apps to enable text streaming/display explicitly in addition to selecting languages or tracks.

Currently, the automatic decision to enable text is if:

1. It's not already enabled by the app and
2. The text track matches the text preference and
3. The audio track doesn't match the text track

So if you want to always start with the user's preferred text language, you would:

1. Configure the audio & text preferences
2. Call setTextTrackVisibility(true) on the player to enable text streaming and display
3. Call load() on the player

If the user has no text preference, don't configure one and don't call setTextTrackVisibility(true), and we won't stream or display text.

It's possible I misunderstood you. If so, please forgive me and try to explain it to me so I can understand better.

It's also possible that app developers would prefer not having any such automatic choices made like this. For example, we used to automatically disable ABR when someone selected an explicit track, but developers complained that this implicit behavior was surprising. So we changed it, and now if you want to select a specific track and not have it changed by ABR, you have to also explicitly disable ABR. Perhaps auto-enabling text display is one of these things that is surprising and confusing, and we would be open to feedback and possibly removing it. Please let us know if this is the case!

However, just removing it would break compatibility, which we wouldn't do until we're ready for Shaka Player v4. Before that, we would need an opt-in config to disable the auto-enabling of text streams, and possibly a warning log about the coming change to give developers time to adjust.

[theodab]

I think the "only" thing needed to get the previous behaviour is to do something like:

preferredAudioLanguage: audioLanguage,
preferredTextLanguage: textLanguage === audioLanguage ? '' : textLanguage,

Let me know if you want to go through with it, or if I should add a new configuration key to keep the existing behaviour :)

That's not quite what we do. We actually ignore the region-component of the language, so that (for example) we consider fr-CA and fr to be equivalent, and don't show French Canadian people French subtitles (or US English-speaking people English subtitles, etc). That a case that some developers might not think about, and could lead to unexpected problems when updating to the newest Shaka Player version.

[valotvince]

👋 I think I wasn't that clear in my first explanation. I have a stream with the following tracks:

• audio
  • A1: fre
  • A2: eng
• text
  • T1: fre (role subtitle)
  • T2: fre (role caption)

I set the following configuration through Shaka configure:

preferredAudioLanguage: 'fre',
preferredTextLanguage: 'fre',
preferredTextRole: 'caption'

When I load the stream, I was expecting to see A1 and T2 to be automatically selected and shown to the user. Instead, because preferredTextLanguage is the same as preferredAudioLanguage, Shaka starts only with A1 and not showing the text track.

Set a preferred audio language and text language in the player configuration to influence the initial choices of the player
Choose specific audio or text languages after calling load() to override those initial choices
Choose to enable text display before or after calling load()
Choose to disable auto-text display after calling load()

So If I understand what you are saying, while wanting to enforce a user preference, I should write a block a code to get the actual textTracks of the stream, and see if any matches my actual user preference (and write a specific code to ignore region-component @theodab is speaking about) to be able to call / or not setTextTrackVisibility(true).

Another problematic example with this logic: I could prefer to watch most of my content with the es texts, but for some reason are not present on another video. If I configure Shaka with that preferredTextLanguage: es and call setTextTrackVisibility(true), it will select a random text track in the list (following the logic in https://github.com/google/shaka-player/blob/master/lib/util/stream_utils.js#L1211), which could be fr, which makes no sense to the user.

Auto-enabling textTracks is alright for me, what was surprising is the implicit choice of not auto-displaying user preferences when configured.

[valotvince]

@joeyparrish 👋 Any thoughts on my previous message ? Thanks :)

[joeyparrish]

In retrospect, it seems that the logic to auto-enable text is somewhat magical and confusing. Perhaps the best thing is just to remove it. The selected text and audio tracks would be chosen based on preferences, but text wouldn't be shown unless requested explicitly. It would be up to the app to look at tracks, and create their own logic for enabling based on user preferences.

Alternately, we could have an explicit config to choose between the old and new behavior (boolean).

Yet another option would be to have an enum-based config to choose behavior for auto-enabling text:

1. legacy - auto-enable only if the text matches user prefs and audio differs
2. on text match - auto-enable only if the text matches user prefs
3. always - auto-enable text if any text is available
4. never - text must be explicitly enabled by the app

This last option could be overkill, but would give both maximum flexibility and backward compatibility.

@valotvince, thoughts?

[valotvince]

Hi @joeyparrish ,

From my point of view, it is better to have the text preferences do their job (auto-enabled if existing), let the app change the text track by itself and not having some magic happening when switching of audio track. That being said, having the audio being taken into account could answer to some use cases (that I am not familiar with).

The text track handling is something we completely manage in our app because of Shaka today's behaviour, and we specifically don't use the text track prefs.

If you're okay with it I could rework that PR having:

• a configuration switch between old & "new" behaviour => the switch would be on the old behaviour per default
• keep the existing PR code for the "new" behaviour

[joeyparrish]

@valotvince, sorry for the delay in my reply. That sounds like a great plan. Thanks!

[valotvince]

@joeyparrish @avelad At last had the opportunity to rewrite the PR according to reviews. Let me know what you think!

[github-actions[bot]]

Incremental code coverage: 89.06%

[avelad]

@theodab can you review it? Thanks!

[joeyparrish]

I may have gone a little overboard. I felt that I couldn't make the setting easy to explain as a boolean, so I both renamed it and made it into an enum. Please let me know what you think.

[valotvince]

@joeyparrish Works for me, thank you !