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

allow setInitialMediaSettingsFor with fragmentedText media type #3245

Conversation

jeffcunat
Copy link
Contributor

This PR will make setInitialMediaSettingsFor compatible with fragmentedText (like it is done with audio and video)

For example :

 player.setInitialMediaSettingsFor('fragmentedText', { 
                lang: 'en',
                role: 'caption'
            });

This is useful to initialize the player for users that want to activate closed captions by default if they are present.

We are using the mediaController.matchSettings method inside the TextController to reuse the same mechanism than for audio and video.

We can keep the setTextDefaultLanguage method for backward compatibility but
player.setInitialMediaSettingsFor('fragmentedText', { lang: 'en' }); is equivalent to player.setTextDefaultLanguage('en')

@jeffcunat
Copy link
Contributor Author

@dsilhavy Note that today, dash.js player enables text by default if any.
We could also make a PR to disable text by default. Does it make sense ?

Moreover, localStorage is actually used to store audio and video media settings.
I have noticed that dashjs_fragmentedText_settings is also stored but never used anywhere.
We could allow to use it to set the same text language than the previous stream (or of the same stream if we reload). Would be another PR ?

@jeffcunat jeffcunat force-pushed the initial-media-settings-text branch from b06acf3 to d7b4d24 Compare May 5, 2020 17:00
@dsilhavy dsilhavy added this to the 3.1.2 milestone May 6, 2020
@dsilhavy dsilhavy self-requested a review May 6, 2020 08:32
@dsilhavy
Copy link
Collaborator

dsilhavy commented Jun 4, 2020

@jeffcunat Thanks for this PR, I added some minor comments. In addition:

  • Can you please add a warning to the setTextDefaultLanguage and getTextDefaultLanguage function that they are deprecated and will be removed in version 3.2.0. Might also be good to refer to the two functions which replace the aforementioned.
  • I agree that text tracks should be disabled by default. Can you please change the enabled parameter to false.
  • Would be nice to have the default text settings in the local storage as well. If you can implement that I would prefer a new PR for that.

@jeffcunat
Copy link
Contributor Author

I will provide another PR soon for keeping text settings in local storage.

@dsilhavy
Copy link
Collaborator

dsilhavy commented Jun 9, 2020

Perfect, thanks

@dsilhavy dsilhavy merged commit f29346d into Dash-Industry-Forum:development Jun 9, 2020
@bbert bbert deleted the initial-media-settings-text branch June 12, 2020 13:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants