Setting src is not synchronous - causes mobile bug #4765
Comments
|
tl;dr: I think that the fix for this issue is simply to remove the call to For example: diff --git a/src/js/tech/middleware.js b/src/js/tech/middleware.js
index 8cf901b4..4073dec6 100644
--- a/src/js/tech/middleware.js
+++ b/src/js/tech/middleware.js
@@ -16,7 +16,7 @@ export function getMiddleware(type) {
}
export function setSource(player, src, next) {
- player.setTimeout(() => setSourceHelper(src, middlewares[src.type], next, player), 1);
+ setSourceHelper(src, middlewares[src.type], next, player);
}
export function setTech(middleware, tech) {Longer Explanation:Commit 34aab3f, which added middleware, started forcing the
But neither the commit or the pull request (#3788) actually clarified why setting the source must be asynchronous, and I'm not sure which "spec" is being referred to. Obviously some middleware may need to perform an asynchronous operation before it can determine the source to be used (see below). I think that the middleware system (as currently written) would allow for either the synchronous or asynchronous usecase based on the actual middleware being used, if we simply remove the forced call to Synchronous ScenarioThis is likely the most common scenario - most users that simply want to play video may not have any middleware "installed". By removing the call to Or, if a user does have some middleware plugged in (for example, the middleware we provide in silvermine/videojs-quality-selector), and that middleware runs synchronously, the path from the user calling In either of these cases, the user can call Asynchronous ScenarioIf the user has plugged in a middleware that requires an asynchronous operation, the code all still works fine even without the A usecase for this type of asynchronous middleware is documented on the video.js blog at: http://blog.videojs.com/feature-spotlight-middleware/. Making This Change Backwards-CompatibleI'm not sure if the change suggested above (removing the call to If someone was dependent on the call to videojs.use('*', function(player) {
// this re-introduces a no-op asynchronous operation when src(...)
// is called so that if someone depended on the async nature of src(...)
// previously, they can still achieve it after the proposed change
return {
setSource: function(srcObj, next) {
setTimeout(function() {
next(null, srcObj);
}, 1);
}
};
});If there was a way to remove middleware, the middleware shown above could even be added by default (to maintain backwards-compatibility on the 6.x release line), and then removed by users like me who do not want async behavior. That said, I think the default should be to remove the async behavior altogether, but I'm just mentioning this in case that could not be done in the 6.x line. |
Previously, calls to middleware.setSource always set the source asynchronously.
However, there are common use cases for mobile devices where you need to be able to set
the source synchronously. On mobile browsers, auto-playing videos are stopped, and so are
calls to the HTML5 Video element's play function unless the browser can synchronously
associate that function call with a user-initiated browser event (such as a "click" event).
Mobile browsers currently cannot associate play requests with a user-initiated browser
event if that play request is invoked from an asynchronous callback (setTimeout, Promise,
etc).
For example, consider a page that has a video player and a list of videos with a "play"
button next to each video. When the user clicks a "play" button next to one of the videos,
the click event handler will call `player.src({ src: 'example' })` on the video.js player
instance, and then call `player.play()` immediately after. This works fine on desktop
browsers because the current implementation of the play function waits for the sources to
be set before playing. On mobile browsers, however, playback is blocked because the
browser cannot associate the user's click event with the call to `play()` because the
source-setting process is asynchronous, which causes the play() request to be
asynchronous.
This fix allows the call to `player.src({ src: 'example' })` (and the resulting call to
`middleware.setSource`) to be synchronous, given that there is no middleware that calls
its `next` callback asynchronously. That may not always be the case, but this fix is
focused on supporting the scenario described above in the case where there is no
asynchronous middleware.
Fixes videojs#4765
|
Hey, thanks for your patience, and the great and details bug report! I had the week off and combined with Thanksgiving, it took a minute to get the time to reply. There are two reasons why middleware is made async by default, why the spec was mentioned, and why I would be very wary of changing it. If you look at the video element spec for how it sets the source, and actually just about any operations on it, ultimately, it's all done asynchronously. For example, if you have a video element and then you set a source on it ( As for why not allow synchronous middleware? This is because having methods that sometimes function synchronously and something asynchronously is a recipe for disaster. As an example just in Video.js. A long time ago, in v4 land, Html5 and Flash tech were set to ready whenever they were ready. Meaning that because Html5 just synchronously created a video element, or just re-used the existing one, it was ready synchronously. Then, Flash had to be ready asynchronously, because we had to wait for a message from within the swf to send back a message telling us that it finished loading fine. For a while, this was fine, but as we started doing more complicated things with Video.js, we quickly ran into problems because we had code that mistakenly assumed that the techs were ready synchronously or that they were only ready asynchronously. So, we had to end up writing code for each tech. In v5, we made sure that techs are always only ready asynchronously. (#2188, #1667, #2326). Having APIs that are sometimes synchronous and sometimes asynchronous are often said to release Zalgo. Isaac, from npm fame, has a good post on the subject. Our past experience in this subject and that it's generally something to be avoided is why we chose this route and why I would be wary to change it. I test a simple example and I found that it worked in Chrome for Android but didn't work on my iOS (granted it was iOS 9) device. I think the way forward here is probably to make sure that the Also, it's great to hear that you're using middleware! Would be super interested in any (other) feedback you have on it as well as how it is working with it. Also, anything else you want to see in middleware. |
|
Closing this issue due to lack of activity. If it is still a problem and we get more information and a reduced test case, we will happily re-open the issue. |
|
@gkatsev this is still an issue with the latest version of video.js. See this updated codepen which uses 6.8.0: https://codepen.io/jthomerson/pen/WJZNWw (compare a desktop browser with your mobile browser). Notice that on desktop when you click "play" on one of the playlist items below the player, it queues it and plays it, but on your mobile browser you have to click play on the player itself after clicking play on the playlist item. This happens because mobile devices require that calling play on a player synchronously results from a user action such as a click. But because the call to Please re-open this issue and reconsider not just the Thanks! |
|
I may have been a bit quick to close this, was kind of in a closey mood. Thanks for the test case. Testing it locally on my phone, an android device, things play properly. I don't have an iOS device with to test currently. |
|
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
gkatsev
added
confirmed
and removed
outdated
|
@gkatsev thanks for removing the |
|
Yeah, this is definitely something we want to address. We're working on trying to make addressing large issues like these more manageable and timely. |
|
Thanks @gkatsev for updating this. We are definitely in need of a fix for this. We're upgrading to 7.x and are going to have to continue maintaining our own fork that incorporates (essentially) the changes from #4769 so that the usecase of setting source and playing from a single user event works on mobile devices. Also, in the original issue I noted:
Here is an example to reinforce that: https://developers.google.com/web/updates/2017/09/autoplay-policy-changes This, of course, makes this issue even more important to resolve. |
Previously, calls to middleware.setSource always set the source asynchronously.
However, there are common use cases for mobile devices where you need to be able to set
the source synchronously. On mobile browsers, auto-playing videos are stopped, and so are
calls to the HTML5 Video element's play function unless the browser can synchronously
associate that function call with a user-initiated browser event (such as a "click" event).
Mobile browsers currently cannot associate play requests with a user-initiated browser
event if that play request is invoked from an asynchronous callback (setTimeout, Promise,
etc).
For example, consider a page that has a video player and a list of videos with a "play"
button next to each video. When the user clicks a "play" button next to one of the videos,
the click event handler will call `player.src({ src: 'example' })` on the video.js player
instance, and then call `player.play()` immediately after. This works fine on desktop
browsers because the current implementation of the play function waits for the sources to
be set before playing. On mobile browsers, however, playback is blocked because the
browser cannot associate the user's click event with the call to `play()` because the
source-setting process is asynchronous, which causes the play() request to be
asynchronous.
This fix allows the call to `player.src({ src: 'example' })` (and the resulting call to
`middleware.setSource`) to be synchronous, given that there is no middleware that calls
its `next` callback asynchronously. That may not always be the case, but this fix is
focused on supporting the scenario described above in the case where there is no
asynchronous middleware.
Fixes videojs#4765
|
I wonder if this PR makes this better: #5822 |
|
Also, we have time planned to fully address this in Q2, thanks for your patience in us getting around to this. |
|
what happened to Q2? |
Previously, calls to middleware.setSource always set the source asynchronously.
However, there are common use cases for mobile devices where you need to be able to set
the source synchronously. On mobile browsers, auto-playing videos are stopped, and so are
calls to the HTML5 Video element's play function unless the browser can synchronously
associate that function call with a user-initiated browser event (such as a "click" event).
Mobile browsers currently cannot associate play requests with a user-initiated browser
event if that play request is invoked from an asynchronous callback (setTimeout, Promise,
etc).
For example, consider a page that has a video player and a list of videos with a "play"
button next to each video. When the user clicks a "play" button next to one of the videos,
the click event handler will call `player.src({ src: 'example' })` on the video.js player
instance, and then call `player.play()` immediately after. This works fine on desktop
browsers because the current implementation of the play function waits for the sources to
be set before playing. On mobile browsers, however, playback is blocked because the
browser cannot associate the user's click event with the call to `play()` because the
source-setting process is asynchronous, which causes the play() request to be
asynchronous.
This fix allows the call to `player.src({ src: 'example' })` (and the resulting call to
`middleware.setSource`) to be synchronous, given that there is no middleware that calls
its `next` callback asynchronously. That may not always be the case, but this fix is
focused on supporting the scenario described above in the case where there is no
asynchronous middleware.
Fixes videojs#4765
Previously, calls to middleware.setSource always set the source asynchronously.
However, there are common use cases for mobile devices where you need to be able to set
the source synchronously. On mobile browsers, auto-playing videos are stopped, and so are
calls to the HTML5 Video element's play function unless the browser can synchronously
associate that function call with a user-initiated browser event (such as a "click" event).
Mobile browsers currently cannot associate play requests with a user-initiated browser
event if that play request is invoked from an asynchronous callback (setTimeout, Promise,
etc).
For example, consider a page that has a video player and a list of videos with a "play"
button next to each video. When the user clicks a "play" button next to one of the videos,
the click event handler will call `player.src({ src: 'example' })` on the video.js player
instance, and then call `player.play()` immediately after. This works fine on desktop
browsers because the current implementation of the play function waits for the sources to
be set before playing. On mobile browsers, however, playback is blocked because the
browser cannot associate the user's click event with the call to `play()` because the
source-setting process is asynchronous, which causes the play() request to be
asynchronous.
This fix allows the call to `player.src({ src: 'example' })` (and the resulting call to
`middleware.setSource`) to be synchronous, given that there is no middleware that calls
its `next` callback asynchronously. That may not always be the case, but this fix is
focused on supporting the scenario described above in the case where there is no
asynchronous middleware.
Fixes videojs#4765
Previously, calls to middleware.setSource always set the source asynchronously.
However, there are common use cases for mobile devices where you need to be able to set
the source synchronously. On mobile browsers, auto-playing videos are stopped, and so are
calls to the HTML5 Video element's play function unless the browser can synchronously
associate that function call with a user-initiated browser event (such as a "click" event).
Mobile browsers currently cannot associate play requests with a user-initiated browser
event if that play request is invoked from an asynchronous callback (setTimeout, Promise,
etc).
For example, consider a page that has a video player and a list of videos with a "play"
button next to each video. When the user clicks a "play" button next to one of the videos,
the click event handler will call `player.src({ src: 'example' })` on the video.js player
instance, and then call `player.play()` immediately after. This works fine on desktop
browsers because the current implementation of the play function waits for the sources to
be set before playing. On mobile browsers, however, playback is blocked because the
browser cannot associate the user's click event with the call to `play()` because the
source-setting process is asynchronous, which causes the play() request to be
asynchronous.
This fix allows the call to `player.src({ src: 'example' })` (and the resulting call to
`middleware.setSource`) to be synchronous, given that there is no middleware that calls
its `next` callback asynchronously. That may not always be the case, but this fix is
focused on supporting the scenario described above in the case where there is no
asynchronous middleware.
Fixes videojs#4765
Previously, calls to middleware.setSource always set the source asynchronously.
However, there are common use cases for mobile devices where you need to be able to set
the source synchronously. On mobile browsers, auto-playing videos are stopped, and so are
calls to the HTML5 Video element's play function unless the browser can synchronously
associate that function call with a user-initiated browser event (such as a "click" event).
Mobile browsers currently cannot associate play requests with a user-initiated browser
event if that play request is invoked from an asynchronous callback (setTimeout, Promise,
etc).
For example, consider a page that has a video player and a list of videos with a "play"
button next to each video. When the user clicks a "play" button next to one of the videos,
the click event handler will call `player.src({ src: 'example' })` on the video.js player
instance, and then call `player.play()` immediately after. This works fine on desktop
browsers because the current implementation of the play function waits for the sources to
be set before playing. On mobile browsers, however, playback is blocked because the
browser cannot associate the user's click event with the call to `play()` because the
source-setting process is asynchronous, which causes the play() request to be
asynchronous.
This fix allows the call to `player.src({ src: 'example' })` (and the resulting call to
`middleware.setSource`) to be synchronous, given that there is no middleware that calls
its `next` callback asynchronously. That may not always be the case, but this fix is
focused on supporting the scenario described above in the case where there is no
asynchronous middleware.
Fixes videojs#4765
Description
Setting src on a player is not synchronous. Because it's not synchronous, if your play button sets the source on the player and then calls play, this will not work in mobile browsers where they require the click event to directly invoke the call to play on the player (not some code that runs later after an asynchronous callback). The code in video.js that causes setting
.src(...)to be asynchronous is https://github.com/videojs/video.js/blob/v6.2.0/src/js/tech/middleware.js#L19Thus, this code would not work on a mobile device:
Steps to reproduce
Thanks @yokuze for creating the codepen!
Results
Expected
The player should start playing the video.
Actual
On desktops, the video starts playing. On mobile devices, it does not; instead, the item is queued up in the player and the user must click the play button on the player itself to then cause the player to actually start playing.
Error Output
There are no errors.
Additional Information
Versions
Video.js
We are using version 6.2.0, but the latest code in
masterstill usessetTimeout, so it will still happen there as well.This breaking change was introduced by @gkatsev in 34aab3f, which first appears in v6.0.0, so you should be able to reproduce the bug with any version from 6.0.0-on.
Browsers and Operating Systems
Mobile browsers where they enforce that a call to
playmust be initiated by a user event.For example, the iOS Developer Library says this:
The text was updated successfully, but these errors were encountered: