Setting src is not synchronous - causes mobile bug

[jthomerson]

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#L19

Thus, this code would not work on a mobile device:

$('.play-button').click(function() {
   var itemEl = $(this).closest('.playlist-item'),
       src = itemEl.data('src'),
       poster = itemEl.find('img').attr('src');

   player.poster(poster);
   player.src({ src: src });
   player.play();
});

Steps to reproduce

1. On your mobile device (phone or most tablets) open this codepen: https://codepen.io/mluedke/full/ZaoQev/
2. Click one of the "Play" buttons

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 master still uses setTimeout, so it will still happen there as well.

This breaking change was introduced by @gkatsev in 34aab3f357b5810b69a8b170f7b3bcb0cd403183, 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 play must be initiated by a user event.

For example, the iOS Developer Library says this:

In Safari on iOS (for all devices, including iPad), where the user may be on a cellular network and be charged per data unit, preload and autoplay are disabled. No data is loaded until the user initiates it. This means the JavaScript play() and load() methods are also inactive until the user initiates playback, unless the play() or load() method is triggered by user action. In other words, a user-initiated Play button works, but an onLoad="play()" event does not.

[jthomerson]

tl;dr: I think that the fix for this issue is simply to remove the call to setTimeout at middleware.js#L19, and that both synchronous and asynchronous middleware will continue to "just work"™.

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 34aab3f357b5810b69a8b170f7b3bcb0cd403183, which added middleware, started forcing the setSource operation to be asynchronous by introducing the call to setTimeout at middleware.js#L19. That commit said:

This makes setting the source asynchronous, which aligns it with the spec a bit more. Methods like play can still be called synchronously on the player after setting the source and the player will play once the source has loaded.

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 setTimeout that appears in middleware.js. Here's what I mean:

Synchronous Scenario

This 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 setTimeout, the call to src ends up calling middleware.setSource, which then loops through an empty array of middleware and eventually calls next(src, acc) (see middleware.js#91).

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 player.src(...) to the eventual call to the setSrc method on the underlying tech is synchronous.

In either of these cases, the user can call player.src(...); player.play() and those two actions are synchronous, which is a huge win for their mobile users because the user action of clicking on a link/button/etc is then directly (synchronously) tied to the invocation of the HTML5 video player's play method, which allows the video to play without a second user interaction.

Asynchronous Scenario

If the user has plugged in a middleware that requires an asynchronous operation, the code all still works fine even without the setTimeout that appears in middleware.js. Whichever middleware has an async operation just does its op and postpones the call to next(...) until its async op is completed.

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-Compatible

I'm not sure if the change suggested above (removing the call to setTimeout) would be considered a breaking change or not ... it is changing an operation that was asynchronous to be synchronous (in most cases, unless you have async middleware plugged in), but I'm struggling to think of a real-world scenario where someone (since 6.0) could be dependent on the asynchronous nature of the call to src(...). Can anyone help come up with a usecase where someone could be dependent on it?

If someone was dependent on the call to src(...) being async and we made the change to remove setTimeout, I think they could easily get the async behavior back by doing this:

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.

[gkatsev]

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 (vid.src = 'video.mp4') and then you immediately, in the same frame, check what the currentSrc of the video is, you'll find that it's still set to the empty string or the last value. This is because actually setting the source and updating all the internal properties will be done once this current method yields to the browser. So, changing setting source to be asynchronous matches that more than what we were doing previously which was pretending that setting the source was synchronous. This actually caused other issues related to calling play immediately after changing a source. As part of this change, what we wanted to do was also make methods like play work correctly even if we are in the middle of changing a source, meaning that the new video should play. We have tried to do so and we recently just had a bug fix related to it ( #4743 ). So, I'd be inclined to say that we should fix it this way instead.

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. (https://github.com/videojs/video.js/pull/2188, https://github.com/videojs/video.js/issues/1667, https://github.com/videojs/video.js/issues/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 play() method does The Right Thing ™️ rather than making middleware work synchronously. How to do this, I'm not sure, partially because I'm not sure why exactly this is an issue, I'd want to investigate the timing more specifically. But perhaps, we'd want something like @alex-barstow's suggested fix for a safari related issue we had previously (https://github.com/videojs/video.js/pull/4639). Though, maybe making it more generic. Basically, priming the video element immediately in play() if source isn't set or something like that.

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.

[gkatsev]

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.

[jthomerson]

@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 setSource is asynchronous, the call to play thus doesn't happen in the same loop as the click. Thus, you're calling play as a callback to some asynchronous event - not as the result of a user action. Mobile browsers don't allow that because they don't want auto-play audio/video. Desktop browsers are starting to adopt the same security restrictions, which will only make this issue worse.

Please re-open this issue and reconsider not just the setSource / play usecase, but middleware in general. There are quite a number of issues with forcing everything to be asynchronous due to the security restrictions across different browsers, so if all middleware is forcibly async, it really messes up a lot of usecases. It's the number one reason we have to maintain our own fork of video.js, which we don't want to do.

Thanks!

[gkatsev]

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. I do think there's more we can do to handle this usecase that doesn't involve removing middleware or allow sources to be synchronous in some cases.

[stale[bot]]

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.

[yokuze]

@gkatsev thanks for removing the wontfix label and keeping this ticket up to date. I am still very interested in it.

[gkatsev]

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.

[jthomerson]

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:

Desktop browsers are starting to adopt the same security restrictions, which will only make this issue worse.

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.

[gkatsev]

I wonder if this PR makes this better: https://github.com/videojs/video.js/pull/5822

[gkatsev]

Also, we have time planned to fully address this in Q2, thanks for your patience in us getting around to this.

[AwokeKnowing]

what happened to Q2?

[Azq2]

Runtime fix

function patchVideoJsForSyncSetSource() {
  const Player = videojs.getComponent('Player');
  
  const originalSetTimeout = Player.prototype.setTimeout;
  const originalHandleSrc = Player.prototype.handleSrc_;
  let shouldSyncTimeout = false;

  Player.prototype.setTimeout = function (callback, timeout) {
    if (shouldSyncTimeout) {
      callback.call(this);
      return -1;
    } else {
      return originalSetTimeout.call(this, callback, timeout);
    }
  };

  Player.prototype.handleSrc_ = function (...args) {
    shouldSyncTimeout = true;
    const result = originalHandleSrc.apply(this, args);
    shouldSyncTimeout = false;
    return result;
  };
}

[yokuze]

Runtime fix

function patchVideoJsForSyncSetSource() { const Player = videojs.getComponent('Player');

const originalSetTimeout = Player.prototype.setTimeout; const originalHandleSrc = Player.prototype.handleSrc_; let shouldSyncTimeout = false;

Player.prototype.setTimeout = function (callback, timeout) { if (shouldSyncTimeout) { callback.call(this); return -1; } else { return originalSetTimeout.call(this, callback, timeout); } };

Player.prototype.handleSrc_ = function (...args) { shouldSyncTimeout = true; const result = originalHandleSrc.apply(this, args); shouldSyncTimeout = false; return result; }; }

Nice, thank you! If it helps, a more robust, production-tested changeset that we've run for several years is available here: https://github.com/videojs/video.js/commit/582419fd891584bca3ef5c7bf8fdc05903ac9bde

(Granted, that's not drop-in/runtime-only).

It's been a long time since I dug into this, but I think there's also something that needs to be tweaked to be sure that remote text tracks are also set correctly (as shown in that changeset).

We've had to maintain a fork of Video.js with those changes applied for years now because of this issue. 🤦🏻‍♂️

[Azq2]

@yokuze This code has been running in production for over 7 years. I just decided to check if they had fixed this bug. Turns out they haven’t. 🤦‍♂️

Tested with videojs 7/8