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

fix: handle initial duplicate webkitneedskey #134

Merged
merged 8 commits into from
May 18, 2021

Conversation

brandonocasey
Copy link
Contributor

@brandonocasey brandonocasey commented Apr 15, 2021

Fixes an issues where safari will trigger a webkitneedskey on a rendition and then switch renditions and trigger another one.

shaka-project/shaka-player#2426

src/plugin.js Outdated Show resolved Hide resolved
src/plugin.js Outdated Show resolved Hide resolved
src/plugin.js Outdated
player.eme.webkitneedkey.first = true;
player.eme.webkitneedkey.timeout = null;
handleFn(event);
}, 1000);
Copy link
Member

Choose a reason for hiding this comment

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

So, this means that safari+fairplay will always take at least 1s to start playback? Should we make this value configurable?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will add a configuration option but default it to 1000ms. How does that sound?

Copy link
Member

Choose a reason for hiding this comment

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

Sounds fine to me. Was there a smaller value that you found worked or is 1s the most reliable you've found?

src/plugin.js Outdated Show resolved Hide resolved
src/plugin.js Outdated Show resolved Hide resolved
// (also used in early Chrome or Chrome with EME disabled flag)
player.tech_.el_.addEventListener('webkitneedkey', (event) => {
// It's possible that at the start of playback a rendition switch
// on a small player in safari's HLS implementation will cause
Copy link
Contributor

Choose a reason for hiding this comment

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

Does it have to be a small player?

Copy link
Member

Choose a reason for hiding this comment

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

makes it more likely since they'll start with a lower rendition. Happens a lot less if you have the window maximized.

src/plugin.js Outdated Show resolved Hide resolved
src/plugin.js Outdated Show resolved Hide resolved
src/plugin.js Show resolved Hide resolved
// It's possible that at the start of playback a rendition switch
// on a small player in safari's HLS implementation will cause
// two webkitneedkey events to occur. We want to make sure to cancel
// our first existing request if we get another within 1 second. This
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there any reasoning behind 1 second that we can mention?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was typically happening from 250ms to 750ms for me

// prevents a non-fatal player error from showing up due to a
// request failure.
if (!player.eme.webkitneedkey_.seenFirstEvent) {
player.clearTimeout(player.eme.webkitneedkey_.timeout);
Copy link
Contributor

Choose a reason for hiding this comment

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

I know that there's not much we can do, but if Safari ever decides to keep firing webkitneedkey events within the 1 second duration we'll keep clearing and creating new timeouts. That would probably be a bigger issue for them to fix though.

// request failure.
if (!player.eme.webkitneedkey_.seenFirstEvent) {
player.clearTimeout(player.eme.webkitneedkey_.timeout);
player.eme.webkitneedkey_.timeout = player.setTimeout(() => {
Copy link
Contributor

Choose a reason for hiding this comment

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

We may want to clear out this timeout on source changes

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added a check for this.

Copy link
Member

@gkatsev gkatsev left a comment

Choose a reason for hiding this comment

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

Verified that it gets rid of the issue.

@brandonocasey brandonocasey merged commit 5ded675 into main May 18, 2021
@brandonocasey brandonocasey deleted the fix/fairplay-duplicate branch May 18, 2021 21:30
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.

3 participants