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

Don't wait for sessions to close on DrmEngine#destroy #1168

Merged

Conversation

chrisfillmore
Copy link
Contributor

This is a workaround for Chrome bug https://crbug.com/690583.

Resolves #1093

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. No reason we have to wait for the session to be closed, if we asked for it to be closed and we are reasonably confident that the browser will close it at some point. It's not an exclusive resource like the video element or MediaSource, so we should be able to move on and still open new sessions later.

@shaka-bot
Copy link
Collaborator

All tests passed!

@joeyparrish joeyparrish added the type: enhancement New feature or request label Dec 4, 2017
@joeyparrish joeyparrish added this to the v2.3.0 milestone Dec 4, 2017
@joeyparrish
Copy link
Member

Thanks for the PR! The build bot likes it, I like it, so I'm merging it now.

@joeyparrish joeyparrish merged commit e387e06 into shaka-project:master Dec 4, 2017
joeyparrish pushed a commit that referenced this pull request Dec 4, 2017
@joeyparrish
Copy link
Member

This is causing failures in offline storage scenarios. I'm going to try to fix it, but I may have to revert your PR.

The problem is that when close() is working correctly, not waiting for it to resolve can cause playback to fail. The Widevine CDM won't let you open the same persistent session twice.

Even when you wait for DrmEngine.destroy() to tell you you're done, the sessions haven't actually been closed yet, so subsequent playback using the same stored session fails (if you're fast enough).

@joeyparrish
Copy link
Member

I found a way to fix it. Instead of trusting close() or ignoring it, we can trust it for 1 second, then ignore it if it doesn't resolve in that time. This seems to work in my tests.

@chrisfillmore
Copy link
Contributor Author

Thanks!

shaka-bot pushed a commit that referenced this pull request Dec 6, 2017
An improved workaround for https://crbug.com/690583 and #1093, on top
of PR #1168.

If we don't wait at all, we end up trying to use the same session ID
too quickly, which causes a playback failure.

Change-Id: I8c9d1a0f09432054b862e033b80b0c2f47739c74
@joeyparrish
Copy link
Member

Should be fixed now.

joeyparrish added a commit that referenced this pull request Dec 6, 2017
An improved workaround for https://crbug.com/690583 and #1093, on top
of PR #1168.

If we don't wait at all, we end up trying to use the same session ID
too quickly, which causes a playback failure.

Change-Id: I8c9d1a0f09432054b862e033b80b0c2f47739c74
@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

3 participants