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 754222 - adaptivedemux: Timestamping of multi-period streams is not correct
adaptivedemux: Timestamping of multi-period streams is not correct
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-bad
unspecified
Other All
: Normal normal
: 1.5.91
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2015-08-28 08:50 UTC by Sebastian Dröge (slomo)
Modified: 2015-09-14 17:54 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
qtdemux: Only use the tfdt instead of the fragment timestamp if they don't differ too much (2.53 KB, patch)
2015-08-28 08:50 UTC, Sebastian Dröge (slomo)
none Details | Review
qtdemux: Only use the tfdt instead of the fragment timestamp if they don't differ too much (2.85 KB, patch)
2015-08-28 08:52 UTC, Sebastian Dröge (slomo)
rejected Details | Review
Revert "dashdemux: Include the period start in the fragment timestamps in all cases" (2.40 KB, patch)
2015-09-02 15:37 UTC, Sebastian Dröge (slomo)
committed Details | Review
Revert "dashdemux: Subtract the period start time from the presentation offset" (1.50 KB, patch)
2015-09-02 15:37 UTC, Sebastian Dröge (slomo)
committed Details | Review
adaptivedemux: Properly implement timestamping of multi-period streams (16.57 KB, patch)
2015-09-02 15:37 UTC, Sebastian Dröge (slomo)
committed Details | Review
dashdemux: Export the period start time to the base class (3.32 KB, patch)
2015-09-02 15:37 UTC, Sebastian Dröge (slomo)
committed Details | Review
dashdemux: Fix unit test that assumed that fragment timestamps should include the period start timestamp (1.36 KB, patch)
2015-09-03 07:27 UTC, Sebastian Dröge (slomo)
none Details | Review
mpdparser: Fix unit test that assumed that fragment timestamps should include the period start timestamp (1.40 KB, patch)
2015-09-03 11:21 UTC, Sebastian Dröge (slomo)
committed Details | Review
mpdparser: Don't consider period start times in periods with segment lists either (7.76 KB, patch)
2015-09-03 11:21 UTC, Sebastian Dröge (slomo)
committed Details | Review

Description Sebastian Dröge (slomo) 2015-08-28 08:50:01 UTC
See commit message for more details :)
Comment 1 Sebastian Dröge (slomo) 2015-08-28 08:50:05 UTC
Created attachment 310158 [details] [review]
qtdemux: Only use the tfdt instead of the fragment timestamp if they don't differ too much

There are DASH streams out there, that have completely wrong tfdt information
compared to the timestamps from the MPD (multiple minute differences). Using
the tfdt instead of the MPD timestamps breaks playback of these streams.

The streams are not according to the spec, but we can still try to work around
that by ignoring the tfdt information if the difference is more than 2
seconds. For everything smaller than two seconds we prefer using the tfdt as
it potentially gives more accurate timing information than the values from the
MPD.

See also https://github.com/Dash-Industry-Forum/Test-Vectors/issues/29
Comment 2 Sebastian Dröge (slomo) 2015-08-28 08:52:50 UTC
Created attachment 310159 [details] [review]
qtdemux: Only use the tfdt instead of the fragment timestamp if they don't differ too much

There are DASH streams out there, that have completely wrong tfdt information
compared to the timestamps from the MPD (multiple minute differences). Using
the tfdt instead of the MPD timestamps breaks playback of these streams.

The streams are not according to the spec, but we can still try to work around
that by ignoring the tfdt information if the difference is more than 2
seconds. For everything smaller than two seconds we prefer using the tfdt as
it potentially gives more accurate timing information than the values from the
MPD.

See also https://github.com/Dash-Industry-Forum/Test-Vectors/issues/29
Comment 3 Enrique Ocaña González 2015-08-28 10:25:39 UTC
Sorry if this comment is a bit out of context for this particular bug and doesn't belong here, but I think that it can open an interesting debate:

We're using qtdemux in WebKit to process data coming from Media Source Extensions[1] appends.  At appsrc push time, we don't have any info about what time the data we're appending belongs to. We rely on what qtdemux detects.

When our tests and use cases append data linearly from the begining, qtdemux keeps timing info internally and generates the proper timestamps without problems.

However, there are problems for sparse ranges. If the user seeks forward and the upper layer (out of our control) decides to append [2:00, 2:30] right after a former append of [0:00, 1:00], then the internal demuxer status keeps generating PTSs in the range [1:00, 1:30]. Note that in our layer we don't really know the time range being appended, so we can't hint appsrc in any way.

We have a patch[2] for qtdemux_parse_trun() to use decode_ts (which comes from the TFDT atom) if it's significantly higher than the time in the last sample (just the contrary of what you propose in your patch). This rule allows us to blindly feed sparse ranges and have qtdemux generating the right times.

Is it wrong to rely on TFDT as much as possible? What alternatives do we have to support sparse ranges in the appended data?


[1] https://w3c.github.io/media-source/
[2] http://pastebin.com/612D3kyU
Comment 4 Sebastian Dröge (slomo) 2015-08-28 10:40:15 UTC
Why don't you know the timestamps on your side, i.e. that you are appending [2:00, 2:30] now instead of [1:00, 1:30]? How would you distinguish these two cases if you don't know the timestamps? Also this seems completely unrelated to my change and the bug, please open another bug for this :)
Comment 5 Enrique Ocaña González 2015-08-28 12:13:50 UTC
The new bug: https://bugzilla.gnome.org/show_bug.cgi?id=754230
Comment 6 Sebastian Dröge (slomo) 2015-08-31 06:48:58 UTC
The alternative would be to always ignore tfdt inside qtdemux if we have a fragment timestamp, and let dashdemux parse the tfdt. Then we could take the start timestamp of each period as is, remember the offset between that and the tfdt and for all other fragments of that period we could take the tfdt and the offset as timestamp instead of what the mpd says. Any thoughts?
Comment 7 Sebastian Dröge (slomo) 2015-09-02 15:37:06 UTC
Created attachment 310517 [details] [review]
Revert "dashdemux: Include the period start in the fragment timestamps in all cases"

This reverts commit e671ad25a989cb21c62c7a5867c2090890ce49ba.

The timestamps should restart at 0 again for each period, but we have to
adjust the segment to map those timestamps to the actual stream time and
running time of that period.

Otherwise we would have timestamps that conflict with the ones from the tfdt
inside the MP4 container, which are restarting at 0 for each period.

https://bugzilla.gnome.org/show_bug.cgi?id=752409
Comment 8 Sebastian Dröge (slomo) 2015-09-02 15:37:13 UTC
Created attachment 310518 [details] [review]
Revert "dashdemux: Subtract the period start time from the presentation offset"

This reverts commit 626a8f0a74f8ea748b811b74ba9e7ae2baea2cca.

This allows us to get the plain presentation offset and the period start time
separately. We have to adjust the timestamp by the presentation offset, but
the period start time should only adjust the stream time and running time.

https://bugzilla.gnome.org/show_bug.cgi?id=752409
Comment 9 Sebastian Dröge (slomo) 2015-09-02 15:37:20 UTC
Created attachment 310519 [details] [review]
adaptivedemux: Properly implement timestamping of multi-period streams

Each period will start again with pts 0 + period presentation offset, which is
also going to be the presentation time inside the container stream if any.
However all periods together should form a continuous timeline, with regard to
stream time and running time.

For making this possible we keep track of the "user requested segment", i.e.
the seek events, inside the demuxer without adjusting anything and taking this
demuxer segment only as orientation for modified segments per stream.

This per stream segments will have their segment.start at pts that would be
produced for this stream in this period, and the segment.base/time adjusted so
that this pts maps to the running and stream time this period should have in
the context of all other periods.
Comment 10 Sebastian Dröge (slomo) 2015-09-02 15:37:27 UTC
Created attachment 310520 [details] [review]
dashdemux: Export the period start time to the base class
Comment 11 Thiago Sousa Santos 2015-09-02 16:33:31 UTC
Review of attachment 310519 [details] [review]:

::: gst-libs/gst/adaptivedemux/gstadaptivedemux.c
@@ +830,3 @@
       stream->segment.base =
+          gst_segment_to_running_time (&demux->segment, GST_FORMAT_TIME,
+          period_start);

Suppose you have a dash mpd with 2 periods, P0 and P1 with 10s each. No offsets.

The user does a flushing seek to 7s, that lands within P0 and the segment will have: base=0, start=7s - 0.
It plays P0 until the end and moves to P1, now the segment will have: base=10s, start=0.

The base at the second period will be wrong as it should be 3s, that is the total that was played for the P0 in this case.
Maybe what we need is actually to go accumulating the amount of time played for each period on base and time when moving to another period.

(I haven't actually reproduced this, it is just an analysis from the code itself)
Comment 12 Sebastian Dröge (slomo) 2015-09-02 17:42:29 UTC
(In reply to Thiago Sousa Santos from comment #11)
> Review of attachment 310519 [details] [review] [review]:
> 
> ::: gst-libs/gst/adaptivedemux/gstadaptivedemux.c
> @@ +830,3 @@
>        stream->segment.base =
> +          gst_segment_to_running_time (&demux->segment, GST_FORMAT_TIME,
> +          period_start);
> 
> Suppose you have a dash mpd with 2 periods, P0 and P1 with 10s each. No
> offsets.
> 
> The user does a flushing seek to 7s, that lands within P0 and the segment
> will have: base=0, start=7s - 0.
> It plays P0 until the end and moves to P1, now the segment will have:
> base=10s, start=0.

No, it will have base=3s because the running time is calculated based on the demuxer segment. The demuxer segment will be start=7s,base=0s which leads to a running time of 3s at P1's start time (10s).

I tested this case btw, but with presentation time offsets.
Comment 13 Thiago Sousa Santos 2015-09-02 21:41:56 UTC
(In reply to Sebastian Dröge (slomo) from comment #12)
> (In reply to Thiago Sousa Santos from comment #11)
> > Review of attachment 310519 [details] [review] [review] [review]:
> > 
> > ::: gst-libs/gst/adaptivedemux/gstadaptivedemux.c
> > @@ +830,3 @@
> >        stream->segment.base =
> > +          gst_segment_to_running_time (&demux->segment, GST_FORMAT_TIME,
> > +          period_start);
> > 
> > Suppose you have a dash mpd with 2 periods, P0 and P1 with 10s each. No
> > offsets.
> > 
> > The user does a flushing seek to 7s, that lands within P0 and the segment
> > will have: base=0, start=7s - 0.
> > It plays P0 until the end and moves to P1, now the segment will have:
> > base=10s, start=0.
> 
> No, it will have base=3s because the running time is calculated based on the
> demuxer segment. The demuxer segment will be start=7s,base=0s which leads to
> a running time of 3s at P1's start time (10s).
> 
> I tested this case btw, but with presentation time offsets.

Cool, got how it works and also tested it.
Comment 14 Thiago Sousa Santos 2015-09-02 21:43:20 UTC
Review of attachment 310517 [details] [review]:

Good
Comment 15 Thiago Sousa Santos 2015-09-02 21:43:37 UTC
Review of attachment 310518 [details] [review]:

Looks good.
Comment 16 Thiago Sousa Santos 2015-09-02 21:49:11 UTC
Review of attachment 310519 [details] [review]:

Thanks for handling this one, once all the dash samples pass and we fix gst-validate a bit we can add more of those to gst-validate and keep it working.

Tested a bit here with multiple streams and it works.
Comment 17 Thiago Sousa Santos 2015-09-02 21:50:28 UTC
Review of attachment 310520 [details] [review]:

Look good.
Comment 18 Thiago Sousa Santos 2015-09-03 02:18:15 UTC
I believe the dash unit tests failed after this patch. Likely needing some small adjustment
Comment 19 Sebastian Dröge (slomo) 2015-09-03 07:27:10 UTC
Created attachment 310563 [details] [review]
dashdemux: Fix unit test that assumed that fragment timestamps should include the period start timestamp
Comment 20 Sebastian Dröge (slomo) 2015-09-03 07:27:28 UTC
Thanks for reviewing, if you have any suggestions how to make the comment about the segment handling easier to understand please let me know :)
Comment 21 Sebastian Dröge (slomo) 2015-09-03 11:21:17 UTC
Created attachment 310580 [details] [review]
mpdparser: Fix unit test that assumed that fragment timestamps should include the period start timestamp
Comment 22 Sebastian Dröge (slomo) 2015-09-03 11:21:55 UTC
Created attachment 310581 [details] [review]
mpdparser: Don't consider period start times in periods with segment lists either
Comment 23 Thiago Sousa Santos 2015-09-04 19:11:16 UTC
Review of attachment 310580 [details] [review]:

::: tests/check/elements/dash_mpd.c
@@ +4090,3 @@
+   * time equal to the period start time.
+   */
+  expectedTimestamp = duration_to_ms (0, 0, 0, 0, 0, 0, 0);

What happens with the real world stream that you were testing where the first Period start was not 0? The stream timestamps start at 0 anyway or do they start with the period start value?
Comment 24 Sebastian Dröge (slomo) 2015-09-06 10:01:41 UTC
I didn't see a single real world stream with the first period starting at > 0. It's only in the unit tests :)

I would assume that their timestamps start at 0 (+ presentation time offset), like they do for all other periods. Otherwise this would be inconsistent behaviour and I see nothing in the spec that would support this.
Comment 25 Sebastian Dröge (slomo) 2015-09-14 17:54:00 UTC
commit c9f60db2d489cda27629e91fe3009e7c90769460
Author: Sebastian Dröge <sebastian@centricular.com>
Date:   Thu Sep 3 14:20:00 2015 +0300

    mpdparser: Don't consider period start times in periods with segment lists either
    
    https://bugzilla.gnome.org/show_bug.cgi?id=754222

commit d9c45e918fd70cfbb2dea076c0320f1b69146c3a
Author: Sebastian Dröge <sebastian@centricular.com>
Date:   Thu Sep 3 10:26:03 2015 +0300

    mpdparser: Fix unit test that assumed that fragment timestamps should include the period start timestamp
    
    https://bugzilla.gnome.org/show_bug.cgi?id=754222

commit 0d0455e346438c6bf3bc68c3510587016f1a5bdc
Author: Sebastian Dröge <sebastian@centricular.com>
Date:   Wed Sep 2 18:33:51 2015 +0300

    dashdemux: Export the period start time to the base class
    
    https://bugzilla.gnome.org/show_bug.cgi?id=754222

commit d1c5669a384dfb7bc451eb68ba1dbfe11675a149
Author: Sebastian Dröge <sebastian@centricular.com>
Date:   Wed Sep 2 18:29:43 2015 +0300

    adaptivedemux: Properly implement timestamping of multi-period streams
    
    Each period will start again with pts 0 + period presentation offset, which is
    also going to be the presentation time inside the container stream if any.
    However all periods together should form a continuous timeline, with regard to
    stream time and running time.
    
    For making this possible we keep track of the "user requested segment", i.e.
    the seek events, inside the demuxer without adjusting anything and taking this
    demuxer segment only as orientation for modified segments per stream.
    
    This per stream segments will have their segment.start at pts that would be
    produced for this stream in this period, and the segment.base/time adjusted so
    that this pts maps to the running and stream time this period should have in
    the context of all other periods.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=754222


commit b5697b8ced15b995382a9dd5d899eb6c10e7f775
Author: Sebastian Dröge <sebastian@centricular.com>
Date:   Tue Sep 1 13:13:58 2015 +0300

    Revert "dashdemux: Subtract the period start time from the presentation offset"
    
    This reverts commit 626a8f0a74f8ea748b811b74ba9e7ae2baea2cca.
    
    This allows us to get the plain presentation offset and the period start time
    separately. We have to adjust the timestamp by the presentation offset, but
    the period start time should only adjust the stream time and running time.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=752409

commit bb05bbafd55833f18722ffc828186fa7710dc352
Author: Sebastian Dröge <sebastian@centricular.com>
Date:   Tue Sep 1 13:12:45 2015 +0300

    Revert "dashdemux: Include the period start in the fragment timestamps in all cases"
    
    This reverts commit e671ad25a989cb21c62c7a5867c2090890ce49ba.
    
    The timestamps should restart at 0 again for each period, but we have to
    adjust the segment to map those timestamps to the actual stream time and
    running time of that period.
    
    Otherwise we would have timestamps that conflict with the ones from the tfdt
    inside the MP4 container, which are restarting at 0 for each period.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=752409