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

Add robustness suggestions for Widevine #1008

Merged
merged 3 commits into from
Oct 3, 2017

Conversation

beaufortfrancois
Copy link
Contributor

This PR adds suggestions for audio and video robustness fields which may be obscure to newcomers especially if they didn't read https://shaka-player-demo.appspot.com/docs/api/tutorial-drm-config.html yet.

screenshot 2017-09-06 at 10 54 24 am

@theodab theodab self-requested a review September 6, 2017 17:22
Copy link
Collaborator

@theodab theodab left a comment

Choose a reason for hiding this comment

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

It's a little sad this can only happen after actually loading the asset, rather than after selecting the asset but before loading it. That wouldn't be possible, though, because this works by checking loaded data from the player. We could check the tags on the default assets to see if they contain Widevine, but none of the default assets require robustness configuration.

Anyway, LGTM.

@theodab theodab added type: enhancement New feature or request and removed needs triage labels Sep 6, 2017
@joeyparrish joeyparrish self-assigned this Sep 6, 2017
@@ -55,6 +55,25 @@ shakaDemo.setupConfiguration_ = function() {


/** @private */
shakaDemo.updateRobustnessSuggestions_ = function() {
var drmInfo = shakaDemo.player_.drmInfo();
Copy link
Member

Choose a reason for hiding this comment

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

I appreciate the idea, but this won't work as you would expect.

drmInfo() returns info about the currently-loaded stream. Before you load anything, it returns null.

The robustness settings, however, have no immediate effect. They effect the next content you load.

So using the key system of the current content to decide what values to suggest for the next content is not going to work the way it should in all cases.

An alternative would be to detect what key systems are supported by the browser, and suggest all known robustness settings for all supported key systems. (Currently, the only key system with well-known values is Widevine, but we hope to see that change.)

@joeyparrish joeyparrish added this to the v2.3.0 milestone Sep 6, 2017
@beaufortfrancois
Copy link
Contributor Author

beaufortfrancois commented Sep 7, 2017

@joeyparrish I've tried your approach in feb1d87. WDYT?

screenshot 2017-09-07 at 9 12 12 am

demo/index.html Outdated
</div>
<input id="drmSettingsAudioRobustness" type="text" class="flex-grow" list="robustnessSuggestions">
</div>
<datalist id="robustnessSuggestions">
Copy link
Contributor

Choose a reason for hiding this comment

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

Even though I would like to use this, caniuse says it is not well supported in all the browsers we support. You'll probably need to do this in JavaScript.

Copy link
Member

Choose a reason for hiding this comment

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

It will probably be fine.

The only browser with no support on that list is Safari, at which point Safari behaves the same as it did before.

Partial support on Firefox refers to text fields only, which works here.

Partial support on IE/Edge is a little more concerning, since it seems that some events may not fire. But if we don't need those events, it may still work. (I don't think we need change events on these fields.)

@@ -57,6 +58,14 @@ shakaDemo.setupAssets_ = function() {
if (asset.drm.length && !asset.drm.some(
function(keySystem) { return shakaDemo.support_.drm[keySystem]; })) {
option.disabled = true;
asset.drm.forEach(function(keySystem) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Joey's suggestion was to use the drm probe to detect browser support. asset.drm is the key systems available in the asset. You can use shakaDemo.support_.drm[keySystem] to detect if it is supported on the browser.

Copy link
Member

Choose a reason for hiding this comment

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

Correct. We are less interested in what key systems the selected asset uses. If the "custom" field is used, we don't know. But if the browser supports Widevine, it is reasonable to suggest Widevine-specific values here regardless of what asset is selected.

@beaufortfrancois
Copy link
Contributor Author

beaufortfrancois commented Sep 21, 2017

@TheModMaker @joeyparrish WDYT of c8b5c07?

Copy link
Member

@joeyparrish joeyparrish left a comment

Choose a reason for hiding this comment

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

Looks good to me. Sorry for the delay. I'll run it through the build bot one more time.

@shaka-bot
Copy link
Collaborator

All tests passed!

@joeyparrish joeyparrish merged commit 7a9fcbf into shaka-project:master Oct 3, 2017
@beaufortfrancois beaufortfrancois deleted the fix-robustness branch October 4, 2017 09:57
@joeyparrish
Copy link
Member

Cherry-picked to v2.2.3.

@github-actions github-actions bot added the status: archived Archived and locked; will not be updated label Jul 25, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 25, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
status: archived Archived and locked; will not be updated type: enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants