-
Notifications
You must be signed in to change notification settings - Fork 422
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
feat: add support for dash manifests describing sidx boxes #455
Conversation
- start DashPlaylistLoader in HAVE_NOTHING state - enter HAVE_METADATA in media() method - set media automatically only for child playlist loader as masterPlaylistLoader is set from masterPlaylistController update tests with desired behavior ensure loadedmetadata is triggered after loadedplaylist remove unnecessary media playlist selection for master loader
…urns and once when the initial media is selected
- delay triggering selectedinitialmedia event until the active audio playlist loader is finished setting media - ensure that HLS playlists without alternate audio are not affected
- moving haveMetadata to a class-level method, more comments. - remove setTimeouts as they were not performing a necessary function. XHR requests can return in any amount of time and HLS PlaylistLoader handles any requests within a callback, or returns if a request need not be made. DASH should do the same - cleanup the minimumUpdatePeriod setup - remove unnecessary clock.ticks from tests
…mention hasPendingRequest for fallback media selection
- have parseMasterXml use previously downloaded sidxes when refreshing media
TODO:
|
Another test stream that works: http://download.tsi.telecom-paristech.fr/gpac/DASH_CONFORMANCE/TelecomParisTech/mp4-onDemand/mp4-onDemand-mpd-AV.mpd https://gpac.wp.imt.fr/2012/02/23/dash-sequences/ This was originally failing, but then works after doing a null check for segments when setting discontinuity. http://yt-dash-mse-test.commondatastorage.googleapis.com/media/car-20120827-manifest.mpd, from http://dash-mse-test.appspot.com/media.html. |
This one from the dash-mse-test is also working: http://yt-dash-mse-test.commondatastorage.googleapis.com/media/motion-20120802-manifest.mpd |
…responseType was set to arraybuffer
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.
Mostly looks good, just some style changes really. Side note. I find it extremely confusing that there are so many playlists
objects (are some of them arrays?). Seems like we could use a small refactor just to be more descriptive with names where we can.
src/dash-playlist-loader.js
Outdated
const equivalentSidx = (a, b) => { | ||
let equivalentMap = true; | ||
|
||
if (a.map && b.map) { |
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.
What if one of them has a map and the other does not? Doesn't that make them not equivalent?
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.
yes, the map
here is the init segment for the playlist
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.
So should the check be (a.map || b.map)
so that we check if either has a map?
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 suppose the if condition is unnecessary as the value of equivalentMap
would be falsy if either a or b did not have map
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.
equivalentMap already checks that both have .map, so, I'm going to remove the if statement.
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.
Improved
src/dash-playlist-loader.js
Outdated
@@ -178,7 +316,48 @@ export default class DashPlaylistLoader extends EventTarget { | |||
this.trigger('mediachanging'); | |||
} | |||
|
|||
// TODO: check for sidx here | |||
if (playlist.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.
perhaps it would make more sense to more the mediaRequest below this if statement into if (!playlist.sidx)
and then return inside of it? That would keep similar code together and get rid of a big if scope.
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.
Makes sense to me
src/dash-playlist-loader.js
Outdated
}; | ||
|
||
// exported for testing | ||
export const filterSidxMapping = (masterXml, srcUrl, clientOffset, oldSidxMapping) => { |
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 don't quite understand what this function is filtering. It seems to be merging a new sidx mapping with an old one?
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's removing sidx mappings that have changed (either url or indexRange) from the video and audio playlists
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.
Maybe rename it to removeChangedMappings
for clarity?
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 think filter is fine there because it parallels Array#filter. However, a comment can be added to clarify.
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.
Updated the name a bit as well.
The playlist objects are mimicking HLS playlists, where we have an array of playlists, but also put keys on them (which I agree is confusing). Changing this would be a large refactor |
Description
Attempt to fix #162.
Requires videojs/mpd-parser#41
Specific Changes proposed
Dash playlist loader requests the sidx after parsing the master manifest once for the sidx byte range information. Then, the master playlist is mutated.
Current Status
Sources:
Known Issues
there appears to be a bug where the
ended
event is not fired sometimes. This may be an existing issues with DASH.Repro by using https://dash.akamaized.net/dash264/TestCases/10a/1/iis_forest_short_poem_multi_lang_480p_single_adapt_aaclc_sidx.mpd, seeking to > 25s, changing audio selection, seeking near the end.
The skipped playback test
DASH sidx with alt audio should end
should also reproduce the issueif sidx ranges change in between Periods in a live stream, we do not clean up sidxMapping.
This will not be handled at this point as there is further multiperiod work that is required to be able to detect a period change. There is a TODO in the code for this, but no Github issues yet
Requirements Checklist