GNOME Bugzilla – Bug 774463
mpdparser: Allows invalid URI within xlink attributes
Last modified: 2016-11-21 07:44:34 UTC
mpdparser: Allows invalid URI within xlink attributes Section 5.3.3 in ISO/IEC 23009-1:2014 defines that invalid URI specified by "@xlink:href" attribute shall be removed. That means, we should play it without error, and just ignore the corresponding element. It's similar to "urn:mpeg:dash:resolve-to-zero:2013".
Created attachment 339908 [details] [review] mpdparser: Allows invalid URI within xlink attributes
This patch is to fix following mpd playback problems. http://dash.akamaized.net/dash264/TestCases/5c/nomor/4_1d.mpd http://dash.akamaized.net/dash264/TestCases/5c/nomor/5_1d.mpd In above mpd, external links for period xml are intentionally invalid.
Comment on attachment 339908 [details] [review] mpdparser: Allows invalid URI within xlink attributes Sorry for making noise. Not only 404 error, external xml parsing error also should be play-allowed. I'll prepare new patch.
Created attachment 339918 [details] [review] mpdparser: Handle invalid external xml link for Period element Section 5.3.3 in ISO/IEC 23009-1:2014 defines that invalid references (e.g., invalide URI or cannot be resolved) specified by "@xlink:href" attribute shall be removed. That means, we should play it without error, and just ignore the corresponding element. It's similar to "urn:mpeg:dash:resolve-to-zero:2013".
Review of attachment 339918 [details] [review]: ::: ext/dash/gstmpdparser.c @@ -4246,3 @@ period_node->xlink_href, err->message); g_clear_error (&err); - *error = TRUE; This means that error is never ever set to anything. Should just be removed from the parameters then @@ +4309,3 @@ + if (new_periods) { + g_list_free_full (new_periods, + (GDestroyNotify) gst_mpdparser_free_period_node); Shouldn't we keep all periods that were successfully parsed? Or is the spec saying to discard all if one can't be parsed?
(In reply to Sebastian Dröge (slomo) from comment #5) > Review of attachment 339918 [details] [review] [review]: > > ::: ext/dash/gstmpdparser.c > @@ -4246,3 @@ > period_node->xlink_href, err->message); > g_clear_error (&err); > - *error = TRUE; > > This means that error is never ever set to anything. Should just be removed > from the parameters then The error is still used for checking "downloader" https://cgit.freedesktop.org/gstreamer/gst-plugins-bad/tree/ext/dash/gstmpdparser.c#n4217 Could you give me suggestion about that, whether "removing the condition on function and checking downloader whenever before calling the function" or "leave it"? > > @@ +4309,3 @@ > + if (new_periods) { > + g_list_free_full (new_periods, > + (GDestroyNotify) gst_mpdparser_free_period_node); > > Shouldn't we keep all periods that were successfully parsed? Or is the spec > saying to discard all if one can't be parsed? Actually, there are a little uncertainty on that. In my interpretation of spec, however, all period on external xml should be removed. spec is saying that "if there is any error on remote xml, corresponding element on parent MPD should be removed". That is, although one of period could be correctly parsed, another error element will cause removal the external xml element on parent MPD. So, the correctly parsed period on external xml is no more validated.
(In reply to Seungha Yang from comment #6) > (In reply to Sebastian Dröge (slomo) from comment #5) > > Review of attachment 339918 [details] [review] [review] [review]: > > > > ::: ext/dash/gstmpdparser.c > > @@ -4246,3 @@ > > period_node->xlink_href, err->message); > > g_clear_error (&err); > > - *error = TRUE; > > > > This means that error is never ever set to anything. Should just be removed > > from the parameters then > > The error is still used for checking "downloader" > https://cgit.freedesktop.org/gstreamer/gst-plugins-bad/tree/ext/dash/ > gstmpdparser.c#n4217 > Could you give me suggestion about that, whether "removing the condition on > function and checking downloader whenever before calling the function" or > "leave it"? That remaining case seems more or less equivalent to not being able to download the external XML (for whatever reason). Seems more consistent to handle it the same. > > @@ +4309,3 @@ > > + if (new_periods) { > > + g_list_free_full (new_periods, > > + (GDestroyNotify) gst_mpdparser_free_period_node); > > > > Shouldn't we keep all periods that were successfully parsed? Or is the spec > > saying to discard all if one can't be parsed? > > Actually, there are a little uncertainty on that. In my interpretation of > spec, however, all period on external xml should be removed. > > spec is saying that "if there is any error on remote xml, corresponding > element on parent MPD should be removed". That is, although one of period > could be correctly parsed, another error element will cause removal the > external xml element on parent MPD. So, the correctly parsed period on > external xml is no more validated. Ok, let's go with your interpretation then :)
Created attachment 340214 [details] [review] mpdparser: Handle invalid external xml link for Period element
(In reply to Sebastian Dröge (slomo) from comment #7) > That remaining case seems more or less equivalent to not being able to > download the external XML (for whatever reason). Seems more consistent to > handle it the same. I agree your opinion :) whatever error related to external xml, we should play a mpd using remaining information without error. So, I removed the error for "downloader" also in the latest patchset.
Review of attachment 340214 [details] [review]: The *error is still there, for the case of having no downloader. I think exactly the same change is needed for gst_mpd_client_fetch_external_adaptation_set() btw (also the change to load multiple properly)
(In reply to Sebastian Dröge (slomo) from comment #10) > Review of attachment 340214 [details] [review] [review]: > I think exactly the same change is needed for > gst_mpd_client_fetch_external_adaptation_set() btw (also the change to load > multiple properly) Right. We may need same change for _fetch_external_segment_list() and _fetch_external_adaptation_set(). However, because I have no sample for verifying them yet, need more time to checking spec. Anyway, I'll report another patch for them soon.
Created attachment 340291 [details] [review] mpdparser: Handle invalid external xml link for Period element
(In reply to Sebastian Dröge (slomo) from comment #10) > Review of attachment 340214 [details] [review] [review]: > > The *error is still there, for the case of having no downloader. Sorry, that was my mistake :) I updated new patch which includes "no error for no downloader"
(In reply to Seungha Yang from comment #11) > (In reply to Sebastian Dröge (slomo) from comment #10) > > Review of attachment 340214 [details] [review] [review] [review]: > > > I think exactly the same change is needed for > > gst_mpd_client_fetch_external_adaptation_set() btw (also the change to load > > multiple properly) > > Right. We may need same change for _fetch_external_segment_list() and > _fetch_external_adaptation_set(). However, because I have no sample for > verifying them yet, need more time to checking spec. Anyway, I'll report > another patch for them soon. It should be exactly the same, everything else would be inconsistent and confusing :) I also don't have any sample streams though.
Created attachment 340296 [details] [review] mpdparser: Handle invalid external xml link for Period element Really sorry for my double mistake to waste your time.
Created attachment 340307 [details] [review] mpdparser: Handle invalid external xml link for AdaptationSet element Ignore invalid xml link for AdaptationSet likewise external Period without error.
Thanks, will review later :) I think the patch from bug #774357 is also necessary for AdaptationSets, right?
(In reply to Sebastian Dröge (slomo) from comment #17) > Thanks, will review later :) I think the patch from bug #774357 is also > necessary for AdaptationSets, right? My conclusion is that, multi-AdaptationSets and multi-SegmentList in external xml is not allowed and only multi-Period is allowed. In case of AdaptationSet, spec. is saying that exactly one AdaptationSet in external xml is allowed. For SegmentList, although spec is saying multi-SegmentList in external xml is allowed, it does not make sense, because Period/AdaptationSet/Representation can have only one SegmentList. Anyway, I'll report patch for SegmentList handle soon. :)
Created attachment 340340 [details] [review] mpdparser: Handle invalid external xml link for SegmentList element Ignore invalid xml link for SegmentList likewise external Period without error.
Created attachment 340341 [details] [review] mpdparser: Modify return of the function for loading external resources to void gst_mpd_client_fetch_on_load_external_resources() never ever return FALSE due to modified external xml loading functions.
(In reply to Sebastian Dröge (slomo) from comment #17) > Thanks, will review later :) I think the patch from bug #774357 is also > necessary for AdaptationSets, right? To be clear, we don't need it for AdaptationSet and SegmentList. I think almost things for handling external xml is done by using uploaded patch. In my opinion, the one thing left is that, the use of parent's SegmentList, if fetching was failed. (e.g., use of AdaptationSet or Period's SegmentList if there is error during downloading external SegmentList of Representation). Note that above comment is my logical assumption and there is no sample for it yet..
Attachment 340296 [details] pushed as e454694 - mpdparser: Handle invalid external xml link for Period element Attachment 340307 [details] pushed as ae8759c - mpdparser: Handle invalid external xml link for AdaptationSet element Attachment 340340 [details] pushed as 3c31c47 - mpdparser: Handle invalid external xml link for SegmentList element Attachment 340341 [details] pushed as 37e1349 - mpdparser: Modify return of the function for loading external resources to void