Skip to content
This repository has been archived by the owner on Jan 12, 2019. It is now read-only.

Fix issue 963: error on SourceBuffer creation #1330

Closed
wants to merge 3 commits into from

Conversation

tchakabam
Copy link
Contributor

@tchakabam tchakabam commented Jan 29, 2018

Description

Fixes https://github.com/videojs/videojs-contrib-hls/issues/963, see history/triage there.

Defers creation of SourceBuffer and retries as necessary (see comments in code).

Trying to deal with apparent uncertainty coming from lower layers (MediaSource wrappers or actually browsers implementations).

TODO:

  • Adapt tests to check for creation of SourceBuffer asynchroneously

Specific Changes proposed

Please list the specific changes involved in this pull request.

Requirements Checklist

  • Feature implemented / Bug fixed
  • If necessary, more likely in a feature request than a bug fix
  • Reviewed by Two Core Contributors

// However when state is settled on the next tick,
// it seems safe to do so.
createSourceBufferDeferred =
(() => setTimeout(createSourceBuffer, ADD_SOURCE_BUFFER_RETRY_DEFER_MS));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

running this immediately (zero timeout) doesn't fix the issue.

we have experienced that this type of "delay" allows for a reliable behavior.

that proves this is a pretty "fuzzy" issue in the lower layers imho.

@tchakabam
Copy link
Contributor Author

tchakabam commented Jan 30, 2018

Some tests need to be adapted, since the behavior of SourceUpdater changes concerning creation of SourceBuffer.

This fix isn't nice, but it's a workaround for a very real issue (which may also be browser-related).

It happens very reproducibly when the media segments are cached, and thus there seems to be a very short period of time after creation of the MediaSource where calls to addSourceBuffer will lead to fatal errors.

Hls.js always has a few more ticks between creating MediaSource and adding buffers, which is maybe why we never have observed the issue there.

I am actually not exactly sure if the issue also happens when the call to addSourceBuffer happens from the sourceopen event dispatcher.

What I am sure is that most of the cases I've seen were when the MediaSource was advertising open ready-state, and thus no event-handler was attached and creation was attempted immediatly (and then failed).

Hope this may help to understand better what's going on.

@tchakabam tchakabam changed the title Fix/issue 963 Fix issue 963: error on SourceBuffer creation Jan 30, 2018
@tchakabam
Copy link
Contributor Author

Just another thought: This seems to happen actually only when we need to create several SourceBuffer objects. This leads me to the doubt wether somehow browsers implementations have introduced an intermediate state just after creation of SourceBuffers where we need to wait before the next call to addSourceBuffer. Maybe we need to check updating property, wait for an updateend event? Just checked the spec again, it's not mentioning any of that or giving any clues about repeated calls to addSourceBuffer, see https://developer.mozilla.org/en-US/docs/Web/API/MediaSource/addSourceBuffer

@gesinger
Copy link
Contributor

Thanks for another contribution and all the detailed notes!

I agree that the timeout solution, while simplest, is not the most pleasant. If the browsers aren't (or aren't consistently) managing the state when dealing with adding multiple source buffers in quick succession, it may be necessary for us to consider wrapping the media source itself (and/or making changes via videojs-contrib-media-sources) and making the functions async to ensure that we are waiting for each addSourceBuffer to appropriately finish (as you mentioned, listening for updating and updateend). That would at least be a little more deterministic than timeouts.

There are a few more details in https://www.w3.org/TR/media-source/#methods , but not complete there either.

@tchakabam
Copy link
Contributor Author

@gesinger Yes totally agree about that. I'd also want the fix/solution to be robust against anything, and for that we need to do it as you describe.

@Xesme
Copy link

Xesme commented Oct 9, 2018

Is there a status update or work around for this merge?

@TylerZubke
Copy link

I'm also wondering on the status of this. There are certain cases where the vast-vpaid plugin loads a preroll and this error gets thrown.

@trailofdad
Copy link

This is Issue still alive? I'm trying to solve this...

@forbesjo
Copy link
Contributor

Thank you for your PR but we will not be accepting new changes for this repo and will be archived very soon. I would advise you to open your PR against the next iteration of this project at https://github.com/videojs/http-streaming.

@forbesjo forbesjo closed this Jan 11, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants