feat: add support for dash manifests describing sidx boxes #455

ldayananda · 2019-04-05T21:16:00Z

Description

Attempt to fix #162.

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

Need to investigate ended issue (known issue 1)

Sources:

{ 
  src: "https://dash.akamaized.net/dash264/TestCases/10a/1/iis_forest_short_poem_multi_lang_480p_single_adapt_aaclc_sidx.mpd",
  type: "application/dash+xml"
}

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 issue
if 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

Feature implemented / Bug fixed
Unit Tests updated or fixed
Reviewed by Core Contributor(s)

- 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

- don't autoselect media for the masterPlaylistLoader as that is done by MasterPlaylistController - ensure updateMaster returns null if there are no changes in the playlist

- trigger loadedmetadata as part of haveMetadata - add some comments to explain why we trigger certain events and set media at points

- 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

… playlist

…and media

- fix up a comment - add idea for media groups refactor

…mention hasPendingRequest for fallback media selection

- correcting merge mistake - moving check for sidx to correct place

- have parseMasterXml use previously downloaded sidxes when refreshing media

gkatsev · 2019-04-05T21:37:37Z

TODO:

gkatsev · 2019-04-08T19:08:51Z

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.

gkatsev · 2019-04-08T19:10:45Z

This one from the dash-mse-test is also working: http://yt-dash-mse-test.commondatastorage.googleapis.com/media/motion-20120802-manifest.mpd

gkatsev · 2019-04-08T19:11:35Z

Also, this one: http://yt-dash-mse-test.commondatastorage.googleapis.com/media/oops-20120802-manifest.mpd

…responseType was set to arraybuffer

brandonocasey

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.

package.json

brandonocasey · 2019-04-08T20:59:05Z

src/dash-playlist-loader.js

+const equivalentSidx = (a, b) => {
+  let equivalentMap = true;
+
+  if (a.map && b.map) {


What if one of them has a map and the other does not? Doesn't that make them not equivalent?

yes, the map here is the init segment for the playlist

So should the check be (a.map || b.map) so that we check if either has a map?

I suppose the if condition is unnecessary as the value of equivalentMap would be falsy if either a or b did not have map

equivalentMap already checks that both have .map, so, I'm going to remove the if statement.

src/dash-playlist-loader.js

brandonocasey · 2019-04-08T21:17:10Z

src/dash-playlist-loader.js

@@ -178,7 +316,48 @@ export default class DashPlaylistLoader extends EventTarget {
      this.trigger('mediachanging');
    }

-    // TODO: check for sidx here
+    if (playlist.sidx) {


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.

Makes sense to me

brandonocasey · 2019-04-08T21:22:44Z

src/dash-playlist-loader.js

+};
+
+// exported for testing
+export const filterSidxMapping = (masterXml, srcUrl, clientOffset, oldSidxMapping) => {


I don't quite understand what this function is filtering. It seems to be merging a new sidx mapping with an old one?

it's removing sidx mappings that have changed (either url or indexRange) from the video and audio playlists

Maybe rename it to removeChangedMappings for clarity?

I think filter is fine there because it parallels Array#filter. However, a comment can be added to clarify.

Updated the name a bit as well.

ldayananda · 2019-04-08T21:45:09Z

I find it extremely confusing that there are so many playlists objects (are some of them arrays?).

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

ldayananda added 30 commits March 11, 2019 16:52

Adding initial tests to ensure coverage of states and events

3a9a95d

fixes live manifests

5aa5d16

loadedplaylist should happen twice, once when the master manifest ret…

0fd7f87

…urns and once when the initial media is selected

Fix eme setup:

85049e8

- delay triggering selectedinitialmedia event until the active audio playlist loader is finished setting media - ensure that HLS playlists without alternate audio are not affected

Fixes:

ff40727

- don't autoselect media for the masterPlaylistLoader as that is done by MasterPlaylistController - ensure updateMaster returns null if there are no changes in the playlist

Cleanup:

67c92cc

- trigger loadedmetadata as part of haveMetadata - add some comments to explain why we trigger certain events and set media at points

only trigger loadedplaylist and loadedmetadata when loading a new VOD…

1bb3dd4

… playlist

Fix: return to using zero delays to mimic asynchronous code in start …

9aab16d

…and media

docs:

65b9d0e

- fix up a comment - add idea for media groups refactor

remove comment

02c8595

fix: media selection fallback

6723894

fix: mock loader.hasPendingRequest for functional tests

6ebf1d2

both haveMaster and haveMetadata can use zero delays. Update docs to …

fafa47e

…mention hasPendingRequest for fallback media selection

initial work

b5f24dd

add scaffolding

8a34323

move sidx requesting into the dash playlist loader

f7195dc

first master playlist load complete

ecd9699

make parseMasterXml async

e3109dc

fix test

55e02c2

slight change to make things clearer

38c11a3

request segment base sidxes for media groups also

e320129

clean up code a bit

c3d39f1

CR comments

97e37d6

fix:

31c8ad8

- correcting merge mistake - moving check for sidx to correct place

- update to request sidx on a playlist basis

420fe73

- have parseMasterXml use previously downloaded sidxes when refreshing media

fix: store sidxInfo reference before making request for sidx

77a8e9f

clean up request code

7c74d4b

add integration tests for sidx

e80ad0a

gkatsev mentioned this pull request Apr 5, 2019

feat: add support for dash manifests describing sidx boxes #303

Closed

3 tasks

use pre-release of mpd-parser

3bf1d12

omarroth mentioned this pull request Apr 5, 2019

1080p and other video support iv-org/invidious#34

Closed

gkatsev mentioned this pull request Apr 8, 2019

VOD Dash stream not working #209

Closed

store response, if any. We aren't allowed to read responseText since …

7454652

…responseType was set to arraybuffer

gkatsev previously approved these changes Apr 8, 2019

View reviewed changes

switch test to use response as well

0dafa87

gkatsev dismissed their stale review via 0dafa87 April 8, 2019 20:36

gkatsev previously approved these changes Apr 8, 2019

View reviewed changes

fix erroring response test case

0fe8dbb

gkatsev dismissed their stale review via 0fe8dbb April 8, 2019 20:50

brandonocasey reviewed Apr 8, 2019

View reviewed changes

update mpd-parser to latest

4291943

gkatsev mentioned this pull request Apr 11, 2019

Update mpd-parser to the latest version 🚀 #467

Closed

gkatsev added 6 commits April 11, 2019 17:40

exit early if not sidx, one less block

9e00023

only one will

a9fc3b0

use boolean operations for equivalentSidx

ef6f39c

comment and rename filterSidxMapping to filterChangedSidxMappings

60c009e

rename handleSidxResponse to sidxRequestFinished to match segment loader

daa8c0f

Merge branch 'master' into request-sidx

e4155c6

brandonocasey approved these changes Apr 12, 2019

View reviewed changes

gkatsev merged commit 80dde16 into master Apr 12, 2019

gkatsev deleted the request-sidx branch April 12, 2019 14:31

gkatsev mentioned this pull request Apr 29, 2019

MPEG-DASH not loading (Error: CODE:4 MEDIA_ERR_SRC_NOT_SUPPORTED) videojs/video.js#5610

Closed

brandonocasey pushed a commit that referenced this pull request May 2, 2019

feat: add support for dash manifests describing sidx boxes (#455)

03e1822

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add support for dash manifests describing sidx boxes #455

feat: add support for dash manifests describing sidx boxes #455

ldayananda commented Apr 5, 2019

gkatsev commented Apr 5, 2019 •

edited

gkatsev commented Apr 8, 2019

gkatsev commented Apr 8, 2019

gkatsev commented Apr 8, 2019

brandonocasey left a comment

brandonocasey Apr 8, 2019

ldayananda Apr 8, 2019

brandonocasey Apr 9, 2019

ldayananda Apr 9, 2019

gkatsev Apr 11, 2019

gkatsev Apr 11, 2019

brandonocasey Apr 8, 2019

gkatsev Apr 11, 2019

brandonocasey Apr 8, 2019

ldayananda Apr 8, 2019 •

edited

brandonocasey Apr 9, 2019

gkatsev Apr 11, 2019

gkatsev Apr 11, 2019

ldayananda commented Apr 8, 2019

feat: add support for dash manifests describing sidx boxes #455

feat: add support for dash manifests describing sidx boxes #455

Conversation

ldayananda commented Apr 5, 2019

Description

Specific Changes proposed

Current Status

Sources:

Known Issues

Requirements Checklist

gkatsev commented Apr 5, 2019 • edited

gkatsev commented Apr 8, 2019

gkatsev commented Apr 8, 2019

gkatsev commented Apr 8, 2019

brandonocasey left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ldayananda Apr 8, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ldayananda commented Apr 8, 2019

gkatsev commented Apr 5, 2019 •

edited

ldayananda Apr 8, 2019 •

edited