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 752409 - dashdemux: gst_mpd_parser_get_stream_presentation_offset returns a wrong value
dashdemux: gst_mpd_parser_get_stream_presentation_offset returns a wrong value
Product: GStreamer
Classification: Platform
Component: gst-plugins-bad
git master
Other Linux
: Normal normal
: 1.7.1
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Reported: 2015-07-15 09:35 UTC by Florin Apostol
Modified: 2015-10-02 14:10 UTC
See Also:
GNOME target: ---
GNOME version: ---

proposed fix and unit tests (2.92 KB, patch)
2015-07-15 09:38 UTC, Florin Apostol
none Details | Review
added unit test for presentation time (1.76 KB, patch)
2015-09-07 15:32 UTC, Florin Apostol
committed Details | Review

Description Florin Apostol 2015-07-15 09:35:41 UTC
The gst_mpd_parser_get_stream_presentation_offset function compares the presentationTimeOffset with the period start and returns the difference. It should not do that. The presentationTimeOffset is an offset that will be subtracted from the segment start to determine the segment's MPD time. The period's start is the MPD start time of the period. The 2 are unrelated.
Comment 1 Florin Apostol 2015-07-15 09:38:10 UTC
Created attachment 307456 [details] [review]
proposed fix and unit tests

attached proposed fix and a unit test reproducing the problem
Comment 2 Thiago Sousa Santos 2015-07-27 16:51:31 UTC
Reading the spec again and it seems to make sense.

As this part is a bit confusing and there aren't much examples around I'm adding some people on CC to check if they agree.
Comment 3 Chris Bass 2015-07-31 10:33:52 UTC
I think the patch is correct. presentationTimeOffset is used to apply an offset to the presentation times of samples as indicated within the media (i.e., the presentation times as given in the mp4 headers) so that different AdaptationSets align with each other on the overall media presentation timeline.

Subtracting presentationTimeOffset from the PTS of a sample in a media segment will give you the timestamp of that sample relative to the start of its Period. Where PeriodStart comes into the equation is in working out what the timestamp of that sample should be relative to start of the overall media timeline: to find the timestamp of that sample on the overall media timeline you would need to do (PeriodStart + (sample_PTS - presentationTimeOffset)).

It looks like what the original code might possibly have been trying to do is to return an offset that would allow you to directly calculate the timestamp of a sample on the overall media timeline, rather than relative to PeriodStart(?). However, it's based on an assumption that presentationTimeOffset should be > PeriodStart, which isn't necessarily the case: it's perfectly acceptable for presentationTimeOffset to be <= PeriodStart. For example, you could have a case where a Period starts at 30s, but the PTSs in the first segment of that Period start at 10s; in this case, PeriodStart would be 30s, and presentationTimeOffset would be 10s.

You *could* have this function return the offset from sample PTSs to their position on the overall media timeline, i.e., return (presentationTimeOffset - PeriodStart) as a signed value. But I think that would probably be a bit confusing; so probably better to return the presentationTimeOffset, as in the patch. 

I'm guessing the returned value will probably be used to populate a newsegment event pushed downstream at the start of a new Period. If so, I would have thought you'd want segment.start to be presentationTimeOffset, and segment.time to be PeriodStart...(?)
Comment 4 Sebastian Dröge (slomo) 2015-08-07 11:29:22 UTC
There are some streams in the DASH-IF test vectors. Whatever you do here, make sure these streams still work. Also see the related changes I did to dashdemux a while ago.

The patch as is, is going to break it again so don't merge it without also changing dashdemux. I think it would be better to just rename the function to something else. The value that dashdemux really only needs is this difference between the period's start and the offset, dashdemux does not care about the presentation offset itself.
Comment 5 Sebastian Dröge (slomo) 2015-08-07 11:32:47 UTC
Also in general, please don't just change gstmpdparser.c without making sure that your changes make sense in the way how dashdemux uses it. gstmpdparser.c does not exist alone
Comment 6 Sebastian Dröge (slomo) 2015-09-02 09:34:53 UTC
I'm cleaning this up currently btw
Comment 7 Sebastian Dröge (slomo) 2015-09-02 15:39:31 UTC
See bug #754222
Comment 8 Sebastian Dröge (slomo) 2015-09-02 16:07:47 UTC
Florin, can you update your patch on top of the patches there? I.e., only add the new test and update the test to also check for period start times?
Comment 9 Florin Apostol 2015-09-07 15:32:01 UTC
Created attachment 310838 [details] [review]
added unit test for presentation time

as requested, updated the patch. It now contains only the unit test update and is supposed to be committed on top of changes
Comment 10 Sebastian Dröge (slomo) 2015-10-02 14:10:24 UTC
commit ebf2a0092673cb224a202fb3cfd36bfdf1950867
Author: Florin Apostol <>
Date:   Mon Sep 7 16:20:42 2015 +0100

    dashdemux: test: added unit test for presentation time offset