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 safari play() after source change #4639

Conversation

alex-barstow
Copy link
Contributor

Description

Programmatically calling player.play() immediately following a source change will fail in Safari (desktop and iOS) if the video element's preload attribute is set to none.

Open the following links in Safari, start playback, then press the "Play new source" button. These samples demonstrate the problem using a vanilla <video> element.

Expected behavior: the source changes and starts to play.
Actual behavior: playback stalls

One observed effect of this problem is with the implementation of playlists. With preload: "none", playlists, whether configured to autoadvance or click-to-play, do not work in Safari because playback stalls when switching sources.

Note: This bug has apparently been fixed in Safari 11.

Specific additions

Modify the player.play() method so that if it is called while the content source is still changing in Safari, load() will be called before play().

@serv
Copy link

serv commented Oct 3, 2017

I have a similar problem that's happening with Safari 11 for video.js v5.20.3.

In Safari 11 > Preferences > Websites, I have the website set to Never Auto-Play.

screen shot 2017-10-03 at 4 06 01 pm

When I try to play any content, the videojs pauses the content. I can check this by seeing that videojs triggers pause event.

Do you think this is the same issue? Or should I file another issue.?

@gkatsev
Copy link
Member

gkatsev commented Oct 4, 2017

@serv that's another unrelated issue that's already been addressed as much as we can in video.js. See #4562 and #4582. Also, https://www.brightcove.com/en/blog/2017/09/autoplay is a good overview, I think of the issues.

@serv
Copy link

serv commented Oct 13, 2017

Sorry to resurface this issue one more time.

After reading, https://webkit.org/blog/6784/new-video-policies-for-ios/, I think the problem I am having is coming from this.

If a <video> element gains an audio track or becomes un-muted without a user gesture, playback will pause.

Wouldn't clicking a button, that triggers .play() be sufficient for a user gesture?

I think there's definitely something funky happening with video.js still because jumping to a middle of the track using .currentTime() plays the track successfully, while simply .play() a track immediately triggers a pause.

I am currently using

    "video.js": "5.20.4",
    "videojs-contrib-dash": "2.9.1",
    "videojs-contrib-hls": "5.11.1",

Copy link
Member

@misteroneill misteroneill left a comment

Choose a reason for hiding this comment

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

We were talking about this issue this morning and @gkatsev's concerns about unintended consequences. We wonder if we ought to make the if condition more specific - to target only those Safari versions that are known to have this issue and only when preload is actually none?

@alex-barstow
Copy link
Contributor Author

@misteroneill Yup, minimizing the scope makes sense. I'll make the changes.

@alex-barstow alex-barstow force-pushed the fix-safari-play-after-source-change branch from 4e90bc9 to 42cc43c Compare October 23, 2017 20:17
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.

I had a question about DESKTOP_SAFARI_VERSION

@@ -85,6 +85,14 @@ export const IE_VERSION = (function() {

export const IS_SAFARI = (/Safari/i).test(USER_AGENT) && !IS_CHROME && !IS_ANDROID && !IS_EDGE;
export const IS_ANY_SAFARI = IS_SAFARI || IS_IOS;
export const DESKTOP_SAFARI_VERSION = (function() {
Copy link
Member

Choose a reason for hiding this comment

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

Maybe this should just be SAFARI_VERSION? on iOS it'll be equivalent to IOS_VERSION?
I.e., if you return IOS_VERSION instead of null from this iife.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Personally I would find it clearer to have IOS_VERSION solely responsible for mobile and DESKTOP_SAFARI_VERSION solely responsible for desktop, rather than a multi-purpose SAFARI_VERSION, but that's just me. Happy to make this change if you want me to.

Copy link
Member

Choose a reason for hiding this comment

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

@videojs/core-committers any thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think having a separate Desktop Safari version would be more clear.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah I think having desktop is better as well

@gkatsev
Copy link
Member

gkatsev commented Oct 27, 2017

Looks like tests are still failing.

@gkatsev gkatsev added the needs: LGTM Needs one or more additional approvals label Nov 13, 2017
@misteroneill misteroneill added confirmed and removed needs: LGTM Needs one or more additional approvals labels Nov 13, 2017
@gkatsev gkatsev added the patch This PR can be added to a patch release. label Nov 16, 2017
@alex-barstow
Copy link
Contributor Author

alex-barstow commented Nov 16, 2017

Recent changes to Player#play have apparently addressed the underlying issue here, so this PR is no longer necessary. Closing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
confirmed patch This PR can be added to a patch release.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants