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

fix: stop alt loaders on main mediachanging to prevent append race #895

Merged
merged 3 commits into from
Jul 9, 2020

Conversation

brandonocasey
Copy link
Contributor

Description

This is most noticeable right now on safari on the hevc fmp4 stream that we have for testing. You can reproduce it in main by seeking around and force changing the representation to one with another codec:

vhs.representations().forEach((r, i) => r.enabled(i ===49))
vhs.representations().forEach((r, i) => r.enabled(i ===53))
vhs.representations().forEach((r, i) => r.enabled(i ===20))

Usually it will fail with an append error, because we change the codec and then try to append old codec data. This happens because we previously waited until mediachange to stopLoaders for audioLoader but for the mainLoader we stop right away. This causes a race condition where old audio gets appended before the media has fully changed, but after codecs have. The fix here is to stop/pause all segment loaders on mediachanging

Why didn't we see it sooner

Its not an issue to append data of the same codec to a sourcebuffer even if it isn't relevant. That and we always reset when starting to load that data append never caused an issue. Now that we switch codecs such an append will throw an error.

Copy link
Contributor

@gesinger gesinger left a comment

Choose a reason for hiding this comment

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

Would be good to add a test

@@ -105,6 +105,17 @@ export const onGroupChanged = (type, settings) => () => {
startLoaders(activeGroup.playlistLoader, mediaType);
};

const onGroupChanging = (type, settings) => () => {
const {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this use stopLoaders? And should onGroupChanged no longer stopLoaders?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Originally I tried using stopLoaders but that caused an issue because we abort the current audio playlist loader request when starting playback even though we aren't changing audio playlists. I will check if mediachange should just stop the audio playlist loader, since the segmentLoader will be stopped.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK after testing out changing onGroupChanged to only stop the activePlaylistLoader is doesn't work. Loaders are started and stopped frequently during the start of playback and if we don't stop them in mediachange too it can cause playback to hang because the segment loader is trying to load, but there isn't an activePlaylistlistLoader.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

test added

@gkatsev gkatsev added this to the 2.1 milestone Jul 8, 2020
Copy link
Member

@gkatsev gkatsev left a comment

Choose a reason for hiding this comment

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

Had a issue with safari but it may be better once all the other PRs are merged. Approving preliminarily and we'll be testing in main.

@gkatsev gkatsev merged commit 8690c78 into main Jul 9, 2020
@gkatsev gkatsev deleted the fix/mediachanging-stop-loader branch July 9, 2020 15:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants