Fix bug where mp4 autoplay videos weren't paused until all streams loaded

[LukasKalbertodt]

When starting a dual stream video, it could regularly happen that the user already heard the audio from one stream, but the loading spinner was still shown. That's because MP4 streams are included with a <video> tag that has autoplay. This is unfortunately required for some browsers to properly load. Paella needs to immediately pause the video when it loaded. It is only supposed to start playing once all streams have loaded and a "play" command is coming from the StreamProvider.

This behavior got broken in bc37279f4: a new branch was added to waitForLoaded that could lead to resolve() being called, but there the pause was forgotten. I couldn't just add the pause as waitForLoaded is used in many places, and we don't always want to pause. So I added a boolean parameter to only do it when called from loadStream.

---

If you need a test deployment to test this bug and the fix, let me know.

[karendolan]

I concur with @LukasKalbertodt. Doing the video.pause based on the video's readyState is more reliable than doing the video.paused based on the video's loadeddata event.

I don't know if it's an issue that it will happen in both places if this pull is merged? @ferserc1 , what was the reason that the video.pause was moved into the loadeddata event, commit https://github.com/polimediaupv/paella-core/commit/bc37279f47fa19d0513969999eb84cbac8f7ff9c, and out of the readyState check?

[EDIT] I re-read @LukasKalbertodt description, and I suspect the pause was moved because this function is called at other times as well. I retract my question above to @ferserc1

Related question, our site added the following to waitForLoaded(), to short circuit the call and resolve the issue from this pull in a different way. @ferserc1 and @LukasKalbertodt are there issues doing this short circuit as apposed to the solution from this pull?

In mhVideoFormat.js

  async waitForLoaded() {
    if (getHlsSupport(this.forceNative) === HlsSupport.NATIVE) {
      return super.waitForLoaded();
    }
    else {
      await (new Promise((resolve) => {
        // #DCE Override, bypass the check if already ready at start
        if (this._ready) {
          resolve();
          return;
        }
        ... the rest of the code ...

In mh4VideoFormat.js

  waitForLoaded() {
    return new Promise((resolve,reject) => {
      if (this.ready) {
        resolve();
      }
      .. the rest of the code ..

[LukasKalbertodt]

Related question, our site added the following to waitForLoaded(), to short circuit the call and resolve the issue from this pull in a different way. @ferserc1 and @LukasKalbertodt are there issues doing this short circuit as apposed to the solution from this pull?

I'm not sure I understand :/ The code you posted contains no pause() call, but the issue I'm fixing here is that the video is not paused after being loaded. We need to pause it due to adding the autoplay attribute, which isn't done for actual "auto play this please" reasons, but just to work around other browser bugs. So I suppose from my point of view the answer is "no, the posted code does not solve the issue this PR is fixing". But maybe I'm missing the point.

[karendolan]

I'm not sure I understand :/ The code you posted contains no pause() call

I concur with the part of this pull about adding the video.pause back into the readyState check. A revert of that part of the "update" commit you pointed to from 2 weeks ago.

The short-circuit code is to bypass the waitForLoaded function after the video has loaded for the first time (near the start of the function, before the pause()). So that it would only be fully used on first load of the video.

Why does waitForLoaded needs to be run after the video has already loaded successfully the first time? I assumed the reason for putting waitForLoaded everywhere was just a precaution to prevent the video object from being called prior to being fully loaded for the first time. Once the video has loaded, I have never seen the mp4VideoFormat class go from _ready = true to _ready = false. However, we might not have all the uses cases of other sites.

[LukasKalbertodt]

Yes fair, I'm also unsure about all the places calling waitForLoaded. In fact, the method is called every couple hundred milliseconds. That should likely be avoidable, but I wasn't really interested in looking into this in detail. So I just fixed the bug :)

[github-actions[bot]]

This pull request has conflicts ☹ Please resolve those so we can review the pull request. Thanks.

[ferserc1]

This behavior got broken in bc37279: a new branch was added to waitForLoaded that could lead to resolve() being called, but there the pause was forgotten. I couldn't just add the pause as waitForLoaded is used in many places, and we don't always want to pause. So I added a boolean parameter to only do it when called from loadStream.

The call to pause was deliberately removed, because it prevented the video from loading on iOS.

[EDIT] I re-read @LukasKalbertodt description, and I suspect the pause was moved because this function is called at other times as well. I retract my question above to @ferserc1

In fact, the video.pause() function was called inside loadStreamData() right after waitForLoaded. It was removed because it was causing the problems on iOS. The problem was solved by modifying waitForLoaded to handle the ready flag of the video with the event, but I didn't detect problems by not calling the pause function again after waitForLoaded. Evidently there are problems, but to solve it, just add the call to video.pause() again. In the next commit, the call to pause() has been restored, as well as introducing a new set of tests to detect this bug, should it occur again:

https://github.com/polimediaupv/paella-core/commit/d07152ad4bf1bdfd84aef328661ca6fa1c852a59

Why does waitForLoaded needs to be run after the video has already loaded successfully the first time? I assumed the reason for putting waitForLoaded everywhere was just a precaution to prevent the video object from being called prior to being fully loaded for the first time. Once the video has loaded, I have never seen the mp4VideoFormat class go from _ready = true to _ready = false. However, we might not have all the uses cases of other sites.

It is necessary to call waitForLoaded whenever any call is made to the native video API because a video can be unloaded at any time. This problem is mainly due to the fact that paella-core supports the possibility of dealing with multiple qualities in non-MediaSourceExtensions video formats (such as mp4), coupled with the need to synchronise multiple streams.