-
Notifications
You must be signed in to change notification settings - Fork 425
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: check sidx on sidxmapping, check that end > start on remove #1121
Conversation
@@ -140,7 +140,8 @@ export const updateMaster = (oldMaster, newMaster, sidxMapping) => { | |||
if (playlist.sidx) { | |||
const sidxKey = generateSidxKey(playlist.sidx); | |||
|
|||
if (sidxMapping && sidxMapping[sidxKey]) { | |||
// add sidx segments to the playlist if we have all the sidx info already | |||
if (sidxMapping && sidxMapping[sidxKey] && sidxMapping[sidxKey].sidx) { |
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.
in addSidxSegmentsToPlaylist
we try to use sidxMappind[sidxKey].sidx
but it can be undefined if the request for sidx and parsing is still in progress (at which point it will be added). So we need to add the additional key check here.
Codecov Report
@@ Coverage Diff @@
## main #1121 +/- ##
=======================================
Coverage 86.40% 86.40%
=======================================
Files 39 39
Lines 9628 9631 +3
Branches 2176 2177 +1
=======================================
+ Hits 8319 8322 +3
Misses 1309 1309
Continue to review full report at Codecov.
|
// skip removes that would throw an error | ||
// commonly happens during a rendition switch at the start of a video | ||
// from start 0 to end 0 | ||
if (end <= start) { |
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.
I was seeing a sourcebuffer remove error being thrown when trying to remove from 0 -> 0 at the start of a video, as end is <= start.
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.
Looks good. Would maybe be good to have a couple tests around the behavior.
loader.remove(0, 0, () => {}); | ||
assert.equal(audioRemoves, 0, 'no audio remove'); | ||
assert.equal(videoRemoves, 0, 'no video remove'); | ||
|
||
loader.remove(5, 4, () => {}); | ||
assert.equal(audioRemoves, 0, 'no audio remove'); | ||
assert.equal(videoRemoves, 0, 'no video remove'); |
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.
It may be worth having one positive case after to ensure that the removes would be incremented appropriately.
Description
Fixes two console errors that can be seen on dash sidx playlists right now during a rendition switch.
Fixes #1120