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

Order of attachMedia and loadSource calls is inconsistent in docs and causes inconsistent results #4952

Closed
jeffometer opened this issue Oct 6, 2022 · 7 comments · Fixed by #5206

Comments

@jeffometer
Copy link

What do you want to do with Hls.js?

The docs in https://github.com/video-dev/hls.js/blob/v1.2.3/docs/API.md#third-step-load-a-manifest show indicates a nesting of attachMedia and loadSource based on the MEDIA_ATTACHED event. But the version for 1.2.4 calls these methods synchronously.

At first I though this indicated a breaking change, but what I observed seemed to indicate some general confusing flakiness. Here is what I observed:

Under 1.2.3 nesting as in the docs as in the docs sometimes worked but sometimes did not reach the MEDIA_ATTACHED event.
Under 1.2.4 calling synchronously as in the docs sometimes worked but also sometimes did not reach the MEDIA_ATTACHED event.
Under 1.2.4 nesting the attachMedia under the MANIFEST_PARSED event seems to work every time (note this is different from the docs for both 1.2.3. and 1.24)

Should it matter how these get called? Is the flakiness an internal error or something weird in my setup? Thanks for helping clarify.

What have you tried so far?

No response

@jeffometer jeffometer added Needs Triage If there is a suspected stream issue, apply this label to triage if it is something we should fix. Question labels Oct 6, 2022
@oliverswitzer
Copy link

Just to provide code examples for the scenarios outlined by @jeffometer above...

When using 1.2.3, we tried using what was outlined in the docs for the 1.2.3 version (binding the manifest parsed listener from within the MEDIA_ATTACHED event and also calling hls.loadSource). This did not work (thus we chose to upgrade to 1.2.4):

 if (Hls.isSupported()) {
    var video = document.getElementById('video');
    var hls = new Hls();
    // bind them together
    hls.attachMedia(video);
    hls.on(Hls.Events.MEDIA_ATTACHED, function () {
      console.log('video and hls.js are now bound together !');
      hls.loadSource('http://my.streamURL.com/playlist.m3u8');
      hls.on(Hls.Events.MANIFEST_PARSED, function (event, data) {
        console.log(
          'manifest loaded, found ' + data.levels.length + ' quality level'
        );
      });
    });
  }

When using 1.2.4 we also tried what was outlined in the docs. This did not work:

if (Hls.isSupported()) {
    var video = document.getElementById('video');
    var hls = new Hls();
    hls.on(Hls.Events.MEDIA_ATTACHED, function () {
      console.log('video and hls.js are now bound together !');
    });
    hls.on(Hls.Events.MANIFEST_PARSED, function (event, data) {
      console.log(
        'manifest loaded, found ' + data.levels.length + ' quality level'
      );
    });
    hls.loadSource('http://my.streamURL.com/playlist.m3u8');
    // bind them together
    hls.attachMedia(video);
  }

This is the solution that worked every time with 1.2.4 (calling attachMedia from within event listener for manifest parsed event):

  var video = document.querySelector("video");
  videoSrc = video.dataset.url;

  if (window.Hls.isSupported()) {
    var hls = new window.Hls();
    hls.on(Hls.Events.MANIFEST_PARSED, function (event, data) {
      hls.attachMedia(video);
    });
    hls.loadSource(videoSrc)
  } else if (video.canPlayType("application/vnd.apple.mpegurl")) {
    video.src = videoSrc;
  }

@robwalch
Copy link
Collaborator

robwalch commented Oct 6, 2022

Can you clarify what is meant by not reaching media attached?

The nesting was removed from the docs because:

  1. It is unnecessary
  2. It results in errors when run a second time to load a new stream since detach and then attach will be called recursively in the attached event callback

The media element does not need to be attached for a stream to begin loading. Media should be detached however before loading a new stream in order to reset player state. The player does this automatically in loadSource. So, it is best practice to not call loadSource in a player event callback.

@robwalch robwalch added Need info and removed Needs Triage If there is a suspected stream issue, apply this label to triage if it is something we should fix. labels Oct 8, 2022
@jeffometer
Copy link
Author

Hi Rob. I mean that the MEDIA_ATTACHED event never fires, the video source never gets set, and the readyState remains at 0. FWIIW, we do not get any error events, so it seems something failed to occur as opposed to something going wrong.

Your explanation makes perfect sense. However we still were only able to get consistent correct results by putting the attachMedia call in the MANIFEST_PARSED callback. Perhaps there is another issue causing a race condition? But we were unable to discover it.

Unfortunately we don't have an easily reproducible sandbox example, but you can see it in a public debugging room on our product at https://standups.staging.geometer.dev/geometer-io/public-debugging-room/recordings/2230. You'll have to log in via Google to see it though. Directly under the video element is a script tag with the following content:

<script>
              var loadVideo = () => {
                var video = document.querySelector("video");
                videoSrc = video.dataset.url;

                if (window.Hls.isSupported()) {
                  var hls = new window.Hls();
                  hls.on(Hls.Events.ERROR, function (event, data) {
                    console.error(event)
                  })
                  hls.on(Hls.Events.MANIFEST_PARSED, function (event, data) {
                    hls.attachMedia(video);
                  });
                  hls.loadSource(videoSrc)
                } else if (video.canPlayType("application/vnd.apple.mpegurl")) {
                  video.src = videoSrc;
                }
              }
              var tryLoadVideo = () => {
                if (window.Hls) {
                  setTimeout(() => loadVideo(), 0)
                } else {
                  requestAnimationFrame(tryLoadVideo)
                }
              }
              tryLoadVideo();
            </script>

We also have other js on that page (via alpine.js) which watches the video readyState, and the whole page is server rendered (via Elixir LiveView).

Thanks!

@robwalch
Copy link
Collaborator

robwalch commented Jan 6, 2023

I would need a sandbox example to reproduce. Thanks.

@kdamken
Copy link

kdamken commented Feb 2, 2023

Wanted to comment that I was seeing a similar issue. Here are some details.

  • It's worth noting that I only had this issue in Firefox, using hls.js 1.2.1
  • No errors would be thrown but the video element would not have it's src attribute updated.
  • Jeffometer's solution of putting the hls.attachMedia call inside of the listener for MANIFEST_PARSED seemed to do the trick (thank you!!!)
  • Specific to my case, but I have an ad library running before my video getting loaded by hls and it seemed like the ad library wasn't properly tearing its elements down in time when it should - I was occasionally seeing mediaAbort events firing.

@robwalch
Copy link
Collaborator

robwalch commented Feb 3, 2023

Check that the internal onMediaAttaching listeners are not throwing somewhere. MEDIA_ATTACHED is triggered on MediaSource 'sourceopen'. So if it never fires, then MSE failed somewhere here in buffer-controller:

  protected onMediaAttaching(
    event: Events.MEDIA_ATTACHING,
    data: MediaAttachingData
  ) {
    const media = (this.media = data.media);
    if (media && MediaSource) {
      const ms = (this.mediaSource = new MediaSource());
      // MediaSource listeners are arrow functions with a lexical scope, and do not need to be bound
      ms.addEventListener('sourceopen', this._onMediaSourceOpen);
      // link video and media Source
      media.src = self.URL.createObjectURL(ms);

// Setting the src should invoke this callback unless replaced:

  private _onMediaSourceOpen = () => {
    const { hls, media, mediaSource } = this;
    logger.log('[buffer-controller]: Media source opened');
    if (media) {
      this.updateMediaElementDuration();
      hls.trigger(Events.MEDIA_ATTACHED, { media });
    }

You probably want to make sure that something else isn't setting (or clearing) src on your media element after you call attach. That would likely prevent the 'sourceopen' callback from completing. If you are able to reproduce the issue, try adding 'emptied' and 'loadstart' event listeners to the media element you are attaching. The expected order of events here (when using a media element that already has something in "src") is:

[log] > attachMedia
> video event: "emptied" // src was removed or replayed and load() was called or invoked automatically 
> video event: "loadstart" // new src is loading
[log] > [buffer-controller]: Media source opened (MEDIA_ATTACHED)

So if you see another "emptied" after "loadstart" then you've got something else in your application interfering with the video element. So you might want to attach on "emptied" or MANIFEST_PARSED (which ever comes first).

@robwalch
Copy link
Collaborator

robwalch commented Feb 8, 2023

v1.3.3 includes changes in #5206 based on the concept above. If the media element dispatches "emptied" while attaching, and media.src does not match the MediaSource blob object URL being attached, then HLS.js will log an error that attach was interrupted. Check the console for this error to determine if something is setting the media element src outside of HLS.js while hls.attachMedia is setting up:

Media element src was set while attaching MediaSource (OBJECT_URL > NEW_SRC_VALUE)

This won't "fix" the issue, because the issue is not with HLS.j when something external modifies the src attribute on the media element. Add your own "emptied" listener or defer attaching until your app and its libs are done with the media element.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants