Created attachment 307456 [details] [review]
proposed fix and unit tests

attached proposed fix and a unit test reproducing the problem

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.

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...(?)

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.

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

I'm cleaning this up currently btw

See bug #754222

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?

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 https://bugzilla.gnome.org/show_bug.cgi?id=754222 changes

commit ebf2a0092673cb224a202fb3cfd36bfdf1950867
Author: Florin Apostol <florin.apostol@oregan.net>
Date:   Mon Sep 7 16:20:42 2015 +0100

    dashdemux: test: added unit test for presentation time offset
    
    https://bugzilla.gnome.org/show_bug.cgi?id=752409