After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 780570 - mpdparser: Allow inherit Segment{Base,Template} from Period
mpdparser: Allow inherit Segment{Base,Template} from Period
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-bad
git master
Other Linux
: Normal normal
: 1.11.91
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2017-03-27 01:18 UTC by Seungha Yang
Modified: 2017-04-16 08:26 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
mpdparser: Allow inherit Segment{Base,Template} from Period (1.55 KB, patch)
2017-03-27 01:21 UTC, Seungha Yang
committed Details | Review
mpdparser: Relex sanity condition of Segment Base Information (7.25 KB, patch)
2017-03-27 01:24 UTC, Seungha Yang
none Details | Review
mpdparser: Do sanity check of Segment Base Information only at Repesentation level (3.74 KB, patch)
2017-04-12 08:06 UTC, Seungha Yang
committed Details | Review
tests: dash: add tests for period segmenttemplate inheritance (6.42 KB, patch)
2017-04-13 05:43 UTC, Thiago Sousa Santos
none Details | Review

Description Seungha Yang 2017-03-27 01:18:54 UTC
Similar to SegmentList, Representation can inherit Segment{Base,Template}
from Period
Comment 1 Seungha Yang 2017-03-27 01:21:23 UTC
Created attachment 348759 [details] [review]
mpdparser: Allow inherit Segment{Base,Template} from Period

We handled this for SegmentList in bug #768460. SegmentBase and SegmentTemplate also could be inherited from Period level
Comment 2 Seungha Yang 2017-03-27 01:24:53 UTC
Created attachment 348760 [details] [review]
mpdparser: Relex sanity condition of Segment Base Information

The existence condition of duration attribute and SegmentTimeline
should be checked in Representation level. Higher level elements
might not have them.
Comment 3 Seungha Yang 2017-03-27 01:33:04 UTC
This bug seems to duplication of bug #763454, but the previous one seems to no action for a long time, and we can simply inherit them like bug #768460.
Comment 4 Sebastian Dröge (slomo) 2017-04-09 08:00:51 UTC
Thiago, can you take a look at this? You reviewed/wrote patches for the above bugs, do these patches here now fix everything?
Comment 5 Thiago Sousa Santos 2017-04-12 06:20:26 UTC
Review of attachment 348760 [details] [review]:

I don't like how we need to modify and add this extra parameter everywhere to achieve this. Maybe it would be simpler to just split parsing and validating into 2 steps. Then we only need to do the validation at the end when the final structures are formed and we can check for any missing information required for playback.
Comment 6 Seungha Yang 2017-04-12 08:06:34 UTC
Created attachment 349714 [details] [review]
mpdparser: Do sanity check of Segment Base Information only at Repesentation level

Spec 5.3.9.2 is saying about the existence of duration and SegmentTimeline
only for Representation level. Other level such as Period or AdaptationSet
might not have the attributes.
Comment 7 Seungha Yang 2017-04-12 08:08:19 UTC
(In reply to Thiago Sousa Santos from comment #5)
> Review of attachment 348760 [details] [review] [review]:
> 

I found much simpler way for checking it :) Please review new patch set.
Comment 8 Thiago Sousa Santos 2017-04-13 05:43:06 UTC
Created attachment 349762 [details] [review]
tests: dash: add tests for period segmenttemplate inheritance

This relaxing of the check doesn't catch the case where the segmenttemplate is completely specified under the Period as the test attached depicts. It would be nice to also provide more unit tests so we can confirm this works and keeps working in the long run.
Comment 9 Thiago Sousa Santos 2017-04-13 05:44:03 UTC
My impression is that it is getting too complicated to check this during parsing. I'd just eliminate the checking during parsing (unless it is something mandatory that can't be inherited) and instead of check for the sanity of the values afterwards as a post-processing step.
Comment 10 Seungha Yang 2017-04-13 11:30:49 UTC
(In reply to Thiago Sousa Santos from comment #9)
> My impression is that it is getting too complicated to check this during
> parsing. I'd just eliminate the checking during parsing (unless it is
> something mandatory that can't be inherited) and instead of check for the
> sanity of the values afterwards as a post-processing step.

I agree that parser seems to become complicated more and more. 
If you are willing to work about this, that sounds really great :)
Comment 11 Thiago Sousa Santos 2017-04-13 15:35:14 UTC
I'm extending the unit tests at the moment to make sure we cover more cases, will try to get that out by the end of this week.
Comment 12 Edward Hervey 2017-04-14 05:58:46 UTC
I'm currently working on a dash-specific validate testsuite, if you have streams that expose this issue, please tell me so I can add it to the list.
Comment 13 Thiago Sousa Santos 2017-04-16 02:01:06 UTC
commit 22c037df6c6511859e6dfa0c627a9e768151425e
Author: Thiago Santos <thiagossantos@gmail.com>
Date:   Sat Apr 15 18:17:29 2017 -0700

    tests: dash_mpd: add some inheritance tests
    
    Tests regarding inheritance of segment template attributes

commit 68ac72431c3e68346ee918580bf41d7f0f90929d
Author: Seungha Yang <sh.yang@lge.com>
Date:   Wed Apr 12 16:58:10 2017 +0900

    mpdparser: Do sanity check of Segment Base Information only at Repesentation level
    
    Spec 5.3.9.2 is saying about the existence of duration and SegmentTimeline
    only for Representation level. Other level such as Period or AdaptationSet
    might not have the attributes.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=780570

commit 6167dab39ee0f8c683f0ab840ceaa005780cfa5e
Author: Seungha Yang <sh.yang@lge.com>
Date:   Mon Mar 27 10:06:30 2017 +0900

    mpdparser: Allow inherit Segment{Base,Template} from Period
    
    Similar to SegmentList, Representation can inherit Segment{Base,Template}
    from Period
    
    https://bugzilla.gnome.org/show_bug.cgi?id=780570
Comment 14 Edward Hervey 2017-04-16 07:42:09 UTC
Is there a public dashstream that uses this so we can it to the dash regression suite ?
Comment 15 Seungha Yang 2017-04-16 08:26:50 UTC
(In reply to Edward Hervey from comment #14)
> Is there a public dashstream that uses this so we can it to the dash
> regression suite ?

My issue was tested on private stream and related bug #763454 seems to be also private one.

Anyway bug #778237 seems to be similar to this issue but not exactly same. 
If we can use the url which was opened by the reporter, I expect it's also useful :)

http://akamai-progressive.irt.de/irt_avstandard_2014/M4S/1280x720p50/V-1280x720p50_libx264_high_yuv420p_gop100_bit3584k_term5-2-5-14-1/seg2_frag2_hbbtv15_live_multi/V-1280x720p50_libx264_high_yuv420p_gop100_bit3584k_term5-2-5-14-1.mpd