GNOME Bugzilla – Bug 754222
adaptivedemux: Timestamping of multi-period streams is not correct
Last modified: 2015-09-14 17:54:50 UTC
See commit message for more details :)
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
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
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
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 :)
The new bug: https://bugzilla.gnome.org/show_bug.cgi?id=754230
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?
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
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
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.
Created attachment 310520 [details] [review] dashdemux: Export the period start time to the base class
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)
(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.
(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.
Review of attachment 310517 [details] [review]: Good
Review of attachment 310518 [details] [review]: Looks good.
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.
Review of attachment 310520 [details] [review]: Look good.
I believe the dash unit tests failed after this patch. Likely needing some small adjustment
Created attachment 310563 [details] [review] dashdemux: Fix unit test that assumed that fragment timestamps should include the period start timestamp
Thanks for reviewing, if you have any suggestions how to make the comment about the segment handling easier to understand please let me know :)
Created attachment 310580 [details] [review] mpdparser: Fix unit test that assumed that fragment timestamps should include the period start timestamp
Created attachment 310581 [details] [review] mpdparser: Don't consider period start times in periods with segment lists either
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?
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.
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