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

Waited for canplay event before playing after seek #7

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

mauro1855
Copy link

PR to address #5

This solution doesn't work in Safari, but the previous code also didn't work there, so nothing lost.
I checked a couple of STBs and confirmed this addresses the issue of not resuming playback after seek.

(ApplicationInstance &&
ApplicationInstance.stage &&
ApplicationInstance.stage.getRenderPrecision()) ||
precision
Copy link
Collaborator

Choose a reason for hiding this comment

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

This looks like just linebreaks which has nothing to do with the initial issue, correct?

Copy link
Author

@mauro1855 mauro1855 Nov 30, 2022

Choose a reason for hiding this comment

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

Seems you are correct.

@@ -69,7 +69,8 @@ const hooks = {
state.playing = false
},
seeked() {
state.playAfterSeek === true && videoPlayerPlugin.play()
state.playAfterSeek &&
videoEl.addEventListener('canplay', () => videoPlayerPlugin.play(), { once: true })
Copy link
Collaborator

Choose a reason for hiding this comment

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

Are we sure all browsers actually fire the correct 'canplay' event?
Don't we need some fallback? like firing play immediately, but also on canplay?

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Author

@mauro1855 mauro1855 Nov 30, 2022

Choose a reason for hiding this comment

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

The original SDK code replaced in this PR doesn't really work as expected (as indicated in #5) according to some tests I did in the past. This fix solved the issue, but I didn't test it exhaustively. I would suggest double-checking this in some STBs.

Copy link
Contributor

@robbertvancaem robbertvancaem 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 👍 @Thomvl I'm waiting for your approval before merging it

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.

4 participants