-
Notifications
You must be signed in to change notification settings - Fork 421
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
Conversation
There was a problem hiding this 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 { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
test added
There was a problem hiding this 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
.
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: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
tostopLoaders
foraudioLoader
but for themainLoader
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 onmediachanging
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.