GNOME Bugzilla – Bug 752230
mpdparser: Parse xlink attributes from Period, AdaptationSet and SegmentList
Last modified: 2015-10-02 08:02:40 UTC
We still have to do something useful with them, like actually loading the content behind the URL.
Created attachment 307234 [details] [review] mpdparser: Parse xlink attributes from Period, AdaptationSet and SegmentList
FWIW, I'm going to implement the actual usage of these things :)
Created attachment 309816 [details] [review] mpdparser: Parse xlink attributes from Period, AdaptationSet and SegmentList We still have to do something useful with them, like actually loading the content behind the URL.
Created attachment 309817 [details] [review] mpdparser: Store an URI downloader in the parser for downloading additional MPD resources if needed
Created attachment 309818 [details] [review] mpdparser: Implement loading of external Period nodes The same has to be done for AdaptationSet and SegmentList nodes still. Also this does not correctly implement the semantics: by default Period (and other nodes) should only be loaded when needed, not in the very beginning. We need to implement lazy loading for them, which means adjusting gst_mpd_client_setup_media_presentation().
Review of attachment 309816 [details] [review]: bonus points for unit tests!
Review of attachment 309817 [details] [review]: Looks good
(In reply to Thiago Sousa Santos from comment #6) > Review of attachment 309816 [details] [review] [review]: > > bonus points for unit tests! With external e.g. Periods and a mock downloader? I'd rather put a stream with multiple periods in gst-validate ;)
Review of attachment 309818 [details] [review]: ::: ext/dash/gstmpdparser.c @@ +3540,3 @@ + client->mpd_node->Periods = + g_list_insert_before (client->mpd_node->Periods, tmp, l->data); + g_list_free (new_periods); My main concern here is about how the timing of period start will be affected. And also about what happens with seeking across periods?
(In reply to Sebastian Dröge (slomo) from comment #8) > (In reply to Thiago Sousa Santos from comment #6) > > Review of attachment 309816 [details] [review] [review] [review]: > > > > bonus points for unit tests! > > With external e.g. Periods and a mock downloader? I'd rather put a stream > with multiple periods in gst-validate ;) I was meaning just simple parsing tests but gst-validate would be even better, indeed.
(In reply to Thiago Sousa Santos from comment #9) > Review of attachment 309818 [details] [review] [review]: > > ::: ext/dash/gstmpdparser.c > @@ +3540,3 @@ > + client->mpd_node->Periods = > + g_list_insert_before (client->mpd_node->Periods, tmp, l->data); > + g_list_free (new_periods); > > My main concern here is about how the timing of period start will be > affected. And also about what happens with seeking across periods? Would there be a chance that a stream's period_idx variable would go inconsistent because of those period add/removal?
(In reply to Thiago Sousa Santos from comment #11) > (In reply to Thiago Sousa Santos from comment #9) > > Review of attachment 309818 [details] [review] [review] [review]: > > > > ::: ext/dash/gstmpdparser.c > > @@ +3540,3 @@ > > + client->mpd_node->Periods = > > + g_list_insert_before (client->mpd_node->Periods, tmp, l->data); > > + g_list_free (new_periods); > > > > My main concern here is about how the timing of period start will be > > affected. And also about what happens with seeking across periods? That's solveable, I have a plan that has minimal impact on the code ;) With the current patch there is no problem though, they are loaded before start/etc are calculated. > Would there be a chance that a stream's period_idx variable would go > inconsistent because of those period add/removal? Not with the current patch, the later patches (to be done) will make sure they don't become inconsistent.
Basically, nodes are going to be replaced by the full version from the external resource before their first use. For AdaptationSet/SegmentList that can easily be done, for Period we have to refactor things a little bit. But not much needed there either :)
The streams are playing fine now after applying the patch from bug #754222
Created attachment 310887 [details] [review] mpdparser: Add support for loading external AdaptationSets
Created attachment 310888 [details] [review] mpdparser: Add support for loading external SegmentLists
AdaptationSet/SegmentList code is not tested as I don't have any example streams with this.
Created attachment 311799 [details] [review] mpdparser: Implement loading of external Period nodes The same has to be done for AdaptationSet and SegmentList nodes still. Also this does not correctly implement the semantics: by default Period (and other nodes) should only be loaded when needed, not in the very beginning. We need to implement lazy loading for them, which means adjusting gst_mpd_client_setup_media_presentation().
Created attachment 311800 [details] [review] mpdparser: Add support for loading external AdaptationSets
Created attachment 311801 [details] [review] mpdparser: Add support for loading external SegmentLists
Created attachment 311802 [details] [review] mpdparser: Load OnLoad external resources immediately instead of on demand
Created attachment 311865 [details] [review] mpdparser: Parse xlink attributes from Period, AdaptationSet and SegmentList We still have to do something useful with them, like actually loading the content behind the URL.
Created attachment 311866 [details] [review] mpdparser: Store an URI downloader in the parser for downloading additional MPD resources if needed
Created attachment 311867 [details] [review] mpdparser: Implement loading of external Period nodes The same has to be done for AdaptationSet and SegmentList nodes still. Also this does not correctly implement the semantics: by default Period (and other nodes) should only be loaded when needed, not in the very beginning. We need to implement lazy loading for them, which means adjusting gst_mpd_client_setup_media_presentation().
Created attachment 311868 [details] [review] mpdparser: Add support for loading external AdaptationSets
Created attachment 311869 [details] [review] mpdparser: Add support for loading external SegmentLists
Created attachment 311870 [details] [review] mpdparser: Load OnLoad external resources immediately instead of on demand
Created attachment 311871 [details] [review] dashdemux: Implement lazy-loading of external periods
Sample streams are here: http://dashif.org/test-vectors/#MP , 14 to 17
commit 93d85bd361a6fabd830ea3f797cebb110daa3fb5 Author: Sebastian Dröge <sebastian@centricular.com> Date: Tue Sep 22 16:17:38 2015 +0200 dashdemux: Implement lazy-loading of external periods https://bugzilla.gnome.org/show_bug.cgi?id=752230 commit b70dab8f27c708213f44b357dd09273192617c43 Author: Sebastian Dröge <sebastian@centricular.com> Date: Mon Sep 21 21:05:03 2015 +0200 mpdparser: Load OnLoad external resources immediately instead of on demand https://bugzilla.gnome.org/show_bug.cgi?id=752230 commit 49931eda69009249283acf4a42555ebf0053f1ed Author: Sebastian Dröge <sebastian@centricular.com> Date: Tue Sep 8 13:36:23 2015 +0300 mpdparser: Add support for loading external SegmentLists https://bugzilla.gnome.org/show_bug.cgi?id=752230 commit 51e8624d561251223f675905a1186b181ffb37d5 Author: Sebastian Dröge <sebastian@centricular.com> Date: Tue Sep 8 13:04:11 2015 +0300 mpdparser: Add support for loading external AdaptationSets https://bugzilla.gnome.org/show_bug.cgi?id=752230 commit 97f5b82bf6352bf545800bd333948b4a3b2b87e5 Author: Sebastian Dröge <sebastian@centricular.com> Date: Fri Aug 21 16:40:10 2015 +0300 mpdparser: Implement loading of external Period nodes The same has to be done for AdaptationSet and SegmentList nodes still. Also this does not correctly implement the semantics: by default Period (and other nodes) should only be loaded when needed, not in the very beginning. We need to implement lazy loading for them, which means adjusting gst_mpd_client_setup_media_presentation(). https://bugzilla.gnome.org/show_bug.cgi?id=752230 commit 572e54574bb3600bb40a8b62c267c537d4b411f0 Author: Sebastian Dröge <sebastian@centricular.com> Date: Fri Aug 21 12:06:07 2015 +0300 mpdparser: Store an URI downloader in the parser for downloading additional MPD resources if needed https://bugzilla.gnome.org/show_bug.cgi?id=752230 commit 07ee57228b77c2c06cac232311388ef638e87721 Author: Sebastian Dröge <sebastian@centricular.com> Date: Fri Jul 10 18:56:29 2015 +0300 mpdparser: Parse xlink attributes from Period, AdaptationSet and SegmentList We still have to do something useful with them, like actually loading the content behind the URL. https://bugzilla.gnome.org/show_bug.cgi?id=752230