GNOME Bugzilla – Bug 763454
dash: Fix the bug when inheriting segmentbase, segmentTemplate or segmentList from grandparent
Last modified: 2018-11-03 13:47:45 UTC
Created attachment 323611 [details] [review] proposed solution Fix the bug when inheriting SegmentBase, SegmentTemplate and SegmentList from upper layer. The detail is given in the attached = report. The issued MPD is also given in the attached. Also a proposed patch is given. A brief description is as below but for the detail, please refer to report. From the attached MPD, it is expected to generate resource URLs by the SegmentTemplate table below. Unfortunately, it is not so now. For current Gstmpdparser we will get illegal address & make playback fail. The root cause is: Firstly We call gst_mpdparser_parse_period_node() and store the result in struct GstSegmentTemplateNode by calling gst_mpdparser_parse_segment_template_node(). Then we parse the children node and call gst_mpdparser_parse_adaptation_set_node(). At this time "SegmentTemplate" DOES NOT appear. So we have no chance to call gst_mpdparser_parse_segment_template_node(). As the result, the adaption set carries no info about "SegmentTemplate". Finally, we parse the representation node but parent->SegmentTemplate is NULL (parent = adaption set). So representation node’s SegmentTemplate inherits nothing and only with timescale from itself. As the result, the generated stream->cur_seg_template will only have timescale as the logic below. It misses the attribute of @index and @ initialization so we CANNOT generate a legal URL address. [Proposed solution] From the description above, the root cause is that we only do inherit from “parent”. However, if the desired attribute exists at the grandparent (as period’s role to representation), we will get miss. The spec says: “…SegmentBase, SegmentTemplate and SegmentList shall inherit attributes and elements from the same element on a higher level. If the same attribute or element is present on both levels, the one on the lower level shall take precedence over the one on the higher level. …” According to the spec, the proposed solution is: When we parse an adaption set and find that either one of the three elements = [ SegmentBase, SegmentTemplate and SegmentList ] DOES NOT exist but the parent (period node) holds one, we FORCE to inherit the element from period node.
Created attachment 323614 [details] report report of this issue & proposed solution
Created attachment 323615 [details] issued MPD file issued DASH MPD file
I can see that we have a problem here. Still haven't fully reviewed the patch, just took a quick glance. In any case, could you also write some unit tests in tests/check/elements/dash_mpd.c illustrating the cases where your patch improves things?
Created attachment 323836 [details] [review] check flow for verifying the issue of inheritance from grandparent node (from period to representation) For this test case, we must insure that the segmentTemplate's media & initialization which should be inherited from period node should not be NULL. Otherwise, when set URI address, we will request an invalid one and get connection fail.
Created attachment 323837 [details] [review] Remove some unnecessary check Remove some unnecessary check
Dear Thiago: I have created a check program according to your suggestion, as the last two patches. It claims: "For this test case, we should insure that the segmentTemplate's media & initialization which must be inherited from period node will not be NULL." The first has some unnecessary checks so I removed them in the second. Thank you~
Review of attachment 323836 [details] [review]: Thanks for the patch, still some things to fix. Also, I manually added the test here and it failed. It also gave me some warnings during compilation. ::: tests/check/elements/dash_mpd.c @@ +796,3 @@ + * + */ +GST_START_TEST (dash_mpdparser_inherit_segmentTemplate_from_period) You should add the function to the test cases in the main at the bottom of the file otherwise it won't be run. @@ +2718,3 @@ * * Description of bistreamSwitching attribute in Period: + * "When set to ¡¥true¡¦, this is equivalent as if the This seems to be unrelated to the rest of the patch.
Any updates? Also, can you get a proper signature on your patches for the git history?
Is this still valid with latest master?
-- GitLab Migration Automatic Message -- This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity. You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/gstreamer/gst-plugins-bad/issues/359.