GNOME Bugzilla – Bug 745455
dashdemux: doesn't take the presentationTimeOffset into account.
Last modified: 2015-06-25 21:41:58 UTC
Disclaimer: I don't think these patches are ready for merging yet, submitting them for discussion's sake. The BBC offers various kinds of streams for testing at http://rdmedia.bbc.co.uk/dash/ondemand/testcard/ I want to make the streams containing presentationTimeOffsets work with dash demux, for now the sinks receive segments that make them wait for 10 minutes before starting playback with these streams, for example : gst-validate-1.0 playbin uri=http://rdmedia.bbc.co.uk/dash/ondemand/testcard/1/client_manifest-pto_both-events.mpd There also is a separate issue with the three streams listed there, in that their segment indices start at 1, which requires a patch to avoid getting a 404. I couldn't find anything in the specifications stating the default index to start from, I would expect it to be 0, in which case I wonder if dashdemux could maybe try 0 first, then if it gets a 404 try with 1? The second patch makes dashdemux take into account the presentationTimeOffset to calculate the fragment timestamp, it only does this for SegmentTemplates, I don't have a mpd available with individual segments, I suppose the patch should also be needed for that case. The third patch makes it so adaptivedemux sets a different start on the segments it sends for each stream, so that the presentationTimeOffset is correctly taken into account. With these patches, the behaviour matches what the BBC describes (audio and video are matched, playback starts at ~ 0 second (actually 3.84), and lasts for twenty minutes. Patches attached next
Created attachment 298287 [details] [review] Quick and dirty fix to play the linked streams
Created attachment 298288 [details] [review] Sets a different start on the segment for each stream
Created attachment 298289 [details] [review] Account for the presentation time offset
(In reply to Mathieu Duponchelle from comment #0) > Disclaimer: I don't think these patches are ready for merging yet, > submitting them for discussion's sake. > > The BBC offers various kinds of streams for testing at > http://rdmedia.bbc.co.uk/dash/ondemand/testcard/ > > I want to make the streams containing presentationTimeOffsets work with dash > demux, for now the sinks receive segments that make them wait for 10 minutes > before starting playback with these streams, for example : > > gst-validate-1.0 playbin > uri=http://rdmedia.bbc.co.uk/dash/ondemand/testcard/1/client_manifest- > pto_both-events.mpd > > There also is a separate issue with the three streams listed there, in that > their segment indices start at 1, which requires a patch to avoid getting a > 404. I couldn't find anything in the specifications stating the default > index to start from, I would expect it to be 0, in which case I wonder if > dashdemux could maybe try 0 first, then if it gets a 404 try with 1? From the spec, it seems the default is indeed 1. We should follow the spec. "if SegmentTemplate element is present the Template-based Segment URL construction in 5.3.9.4.4 shall be applied with the number of the Media Segment in the Media Segment list. The first number in the list is determined by the value of the SegmentTemplate@startNumber attribute, if present, or is 1 in case this attribute is not present." > > The second patch makes dashdemux take into account the > presentationTimeOffset to calculate the fragment timestamp, it only does > this for SegmentTemplates, I don't have a mpd available with individual > segments, I suppose the patch should also be needed for that case. > > The third patch makes it so adaptivedemux sets a different start on the > segments it sends for each stream, so that the presentationTimeOffset is > correctly taken into account. So, despite the presentationTimeOffset, all streams start from timestamp=0 as usual just like if the presentationTimeOffset was ignored? > > With these patches, the behaviour matches what the BBC describes (audio and > video are matched, playback starts at ~ 0 second (actually 3.84), and lasts > for twenty minutes. > > Patches attached next It seems the section 7.2.1 in the spec is the one that explains how presentationTimeOffset should be used. "The presentation times within each Period are relative to the PeriodStart time of the Period minus the value of the @presentationTimeOffset, To , of the containing Representation. This means for an access unit with a presentation time Tp signalled in the media stream, the Media Presentation time relative to the PeriodStart is Tm = Tp - To . Media Segments should not contain any presentation time Tp that is smaller than the value of the @presentationTimeOffset, To . However, if this is the case, then presentation of the Media Segment is expected to only take place for presentation times greater than or equal to To." It seems like the offset into the stream where playback should start from. It might be used to skip initial data. Maybe someone from BBC has a different interpretation?
(In reply to Thiago Sousa Santos from comment #4) > (In reply to Mathieu Duponchelle from comment #0) > > Disclaimer: I don't think these patches are ready for merging yet, > > submitting them for discussion's sake. > > > > The BBC offers various kinds of streams for testing at > > http://rdmedia.bbc.co.uk/dash/ondemand/testcard/ > > > > I want to make the streams containing presentationTimeOffsets work with dash > > demux, for now the sinks receive segments that make them wait for 10 minutes > > before starting playback with these streams, for example : > > > > gst-validate-1.0 playbin > > uri=http://rdmedia.bbc.co.uk/dash/ondemand/testcard/1/client_manifest- > > pto_both-events.mpd > > > > There also is a separate issue with the three streams listed there, in that > > their segment indices start at 1, which requires a patch to avoid getting a > > 404. I couldn't find anything in the specifications stating the default > > index to start from, I would expect it to be 0, in which case I wonder if > > dashdemux could maybe try 0 first, then if it gets a 404 try with 1? > > From the spec, it seems the default is indeed 1. We should follow the spec. > OK, should I then push my first patch with an amended message? > "if SegmentTemplate element is present the Template-based Segment URL > construction in 5.3.9.4.4 shall be applied with the number of the Media > Segment in the Media Segment list. The first number in the list is > determined by the value of the SegmentTemplate@startNumber attribute, if > present, or is 1 in case this attribute is not present." > > > > > The second patch makes dashdemux take into account the > > presentationTimeOffset to calculate the fragment timestamp, it only does > > this for SegmentTemplates, I don't have a mpd available with individual > > segments, I suppose the patch should also be needed for that case. > > > > The third patch makes it so adaptivedemux sets a different start on the > > segments it sends for each stream, so that the presentationTimeOffset is > > correctly taken into account. > > So, despite the presentationTimeOffset, all streams start from timestamp=0 > as usual just like if the presentationTimeOffset was ignored? > No, with the patches I proposed, the timestamps are left untouched, for example with http://rdmedia.bbc.co.uk/dash/ondemand/testcard/1/client_manifest-pto_video-events.mpd the first timestamp is 0:00:03.840000000 for audio and 0:10:03.840000000 for video. > > > > With these patches, the behaviour matches what the BBC describes (audio and > > video are matched, playback starts at ~ 0 second (actually 3.84), and lasts > > for twenty minutes. > > > > Patches attached next > > It seems the section 7.2.1 in the spec is the one that explains how > presentationTimeOffset should be used. > > "The presentation times within each Period are relative to the PeriodStart > time of the Period minus the value of the @presentationTimeOffset, To , of > the containing Representation. This means for an access unit with a > presentation time Tp signalled in the media stream, the Media Presentation > time relative to the PeriodStart is Tm = Tp - To . > Media Segments should not contain any presentation time Tp that is smaller > than the value of the @presentationTimeOffset, To . However, if this is the > case, then presentation of the Media Segment is expected to only take place > for presentation times greater than or equal to To." > > It seems like the offset into the stream where playback should start from. > It might be used to skip initial data. Maybe someone from BBC has a > different interpretation? I actually share BBC's interpretation, as far as I understand, taking the mpd I linked above as an example, I understand the first Tp to be 10:03.840000000 for video and 00:03.840000000 for audio, thus being compliant with the requirement of having Tp - To to be positive (in both cases it is 00:03.840000000). I'm not exactly sure about the use case this is supposed to serve, but I think the BBC's interpretation is correct here?
Review of attachment 298287 [details] [review]: ::: ext/dash/gstdashdemux.c @@ +604,2 @@ /* start playing from the first segment */ + gst_mpd_client_set_segment_index_for_all_streams (dashdemux->client, 1); This would make all streams start from fragment at the second position. For dashdemux it should still start counting from 0 as usual. It can happen that the dash MPD uses a segment list and not a segment template and dashdemux doesn't know about that. The fix here is to assume in gstmpdparser.c that the startNumber=1 as default and otherwise read the value from the xml. Currently the default is being set to 0.
I think I got what you mean now and it seems to make sense. It seems that the individual streams have their first internal timestamps (within the container) set to Po, so we need to subtract Po from all timestamps to get them starting from 0. In gstreamer terms, doing segment.start += Po; should work. Will now review the patches with that in mind.
Review of attachment 298289 [details] [review]: ::: ext/dash/gstmpdparser.c @@ +3626,3 @@ + offset /= stream->cur_seg_template->MultSegBaseType-> + SegBaseType->timescale; + } Why not store this somewhere easier already scaled to the timestamp to avoid this long if here? Just having a stream->presentationTimeOffset that you can simply add below makes this clearer.
Review of attachment 298288 [details] [review]: ::: gst-libs/gst/adaptivedemux/gstadaptivedemux.c @@ +745,3 @@ stream->segment = demux->segment; + stream->segment.start = stream->segment.position = stream->segment.time = + stream->fragment.timestamp; There is some other magic in other part of adaptivedemux to take care of live streams start time to avoid delaying playback start for them. I think just somehow doing "segment.start/.position += presentationOffset;" would be a good enough solution here. Perharps some deeper changes would be needed in adaptivedemux to expose this to subclasses. Also, I don't think 'time' should change here. The stream-time should start from 0 anyway.
Created attachment 298697 [details] [review] Fixes the setting of the default startNumber.
Created attachment 298698 [details] [review] Sets a different start and position for each stream
Created attachment 298699 [details] [review] Account for the presentation time offset
(In reply to Thiago Sousa Santos from comment #6) > Review of attachment 298287 [details] [review] [review]: > > ::: ext/dash/gstdashdemux.c > @@ +604,2 @@ > /* start playing from the first segment */ > + gst_mpd_client_set_segment_index_for_all_streams (dashdemux->client, 1); > > This would make all streams start from fragment at the second position. For > dashdemux it should still start counting from 0 as usual. It can happen that > the dash MPD uses a segment list and not a segment template and dashdemux > doesn't know about that. > > The fix here is to assume in gstmpdparser.c that the startNumber=1 as > default and otherwise read the value from the xml. Currently the default is > being set to 0. Indeed, the code already *almost* followed the spec, except it only did set the default value if the field was present, which was not completely productive, that's fixed :) Also, my previous patch indeed skipped the first fragment, which is why I thought the first timestamps started at 3.84 seconds, your comment fixes that, thanks :)
(In reply to Thiago Sousa Santos from comment #8) > Review of attachment 298289 [details] [review] [review]: > > ::: ext/dash/gstmpdparser.c > @@ +3626,3 @@ > + offset /= stream->cur_seg_template->MultSegBaseType-> > + SegBaseType->timescale; > + } > > Why not store this somewhere easier already scaled to the timestamp to avoid > this long if here? > > Just having a stream->presentationTimeOffset that you can simply add below > makes this clearer. Done, I also slightly refactored the code around the setting at the same time, makes it a bit easier to read and spares some inderections.
(In reply to Thiago Sousa Santos from comment #9) > Review of attachment 298288 [details] [review] [review]: > > ::: gst-libs/gst/adaptivedemux/gstadaptivedemux.c > @@ +745,3 @@ > stream->segment = demux->segment; > + stream->segment.start = stream->segment.position = stream->segment.time > = > + stream->fragment.timestamp; > > There is some other magic in other part of adaptivedemux to take care of > live streams start time to avoid delaying playback start for them. > > I think just somehow doing "segment.start/.position += presentationOffset;" > would be a good enough solution here. Perharps some deeper changes would be > needed in adaptivedemux to expose this to subclasses. > > Also, I don't think 'time' should change here. The stream-time should start > from 0 anyway. Indeed, the patch I thought I had submitted only modified the segment start and position already, I updated it. Thanks a lot for the review, I think we've got something correct now.
Review of attachment 298697 [details] [review]: I'd just add a more verbose commit message explaining that the spec asks for the default value of 1 so we can see that from the git log and won't need to look at the code. Otherwise, just push.
Review of attachment 298698 [details] [review]: ::: gst-libs/gst/adaptivedemux/gstadaptivedemux.c @@ +745,3 @@ stream->segment = demux->segment; + stream->segment.start = stream->segment.position = + stream->fragment.timestamp; I don't think this is correct because in the case of live streams we use the lowest pts of all streams as the segment.start (just a few lines above this). This might break sync of live streams. I'm not sure if presentationTimeOffset makes sense with live streams, but the solution above will cause issues. I think something like: if (stream_has_presentation_offset) { segment.start += offset } might be better.
Review of attachment 298699 [details] [review]: Good, but should only be committed after the other patch is ready as well.
Created attachment 298980 [details] [review] gstmpdparser: Really set the default value for startNumber. + The specs ask for a default of 1, the current code only did set a default when the field was present.
Created attachment 298981 [details] [review] adaptivedemux: [API]: get_presentation_offset virtual method. Asks the subclass for a potential time offset to apply to each separate stream, in dash streams can have "presentation time offsets", which can be different for each stream.
Created attachment 298982 [details] [review] dashdemux: implement get_presentation_offset. To account for presentationTimeOffset as per section 7.2.1 .
Review of attachment 298697 [details] [review]: obsoleted by more verbose commit.
Review of attachment 298698 [details] [review]: obsoleted by a commit defining a new API
Review of attachment 298699 [details] [review]: obsoleted by a commit implementing the new API
Created attachment 298996 [details] [review] adaptivedemux: [API]: get_presentation_offset virtual method. Asks the subclass for a potential time offset to apply to each separate stream, in dash streams can have "presentation time offsets", which can be different for each stream.
Review of attachment 298982 [details] [review]: Depends on the other patch, but looks good. Push together with the adaptivedemux one.
Review of attachment 298980 [details] [review]: commit 793b4bca93530a5e30b5ba8ea02983cbd11532db Author: Mathieu Duponchelle <mathieu.duponchelle@opencreed.com> Date: Fri Mar 6 12:24:44 2015 +0100 gstmpdparser: Really set the default value for startNumber. + The specs ask for a default of 1, the current code only did set a default when the field was present. https://bugzilla.gnome.org/show_bug.cgi?id=745455
Review of attachment 298982 [details] [review]: commit bd70c73a8a4ceebf40ba65ec65616d0a29bfc41e Author: Mathieu Duponchelle <mathieu.duponchelle@opencreed.com> Date: Mon Mar 2 14:00:03 2015 +0100 dashdemux: implement get_presentation_offset. To account for presentationTimeOffset as per section 7.2.1 . https://bugzilla.gnome.org/show_bug.cgi?id=745455
Review of attachment 298996 [details] [review]: commit b4d8c04f08dce9f720fd4df3237d4f9986f98c43 Author: Mathieu Duponchelle <mathieu.duponchelle@opencreed.com> Date: Mon Mar 2 13:53:03 2015 +0100 adaptivedemux: [API]: get_presentation_offset virtual method. Asks the subclass for a potential time offset to apply to each separate stream, in dash streams can have "presentation time offsets", which can be different for each stream. https://bugzilla.gnome.org/show_bug.cgi?id=745455
Review of attachment 298996 [details] [review]: You should provide a default implementation that just returns 0 so that mssdemux and hlsdemux won't crash/assert. ::: gst-libs/gst/adaptivedemux/gstadaptivedemux.c @@ +1396,3 @@ + if (GST_BUFFER_PTS (buffer) != GST_CLOCK_TIME_NONE) + GST_BUFFER_PTS (buffer) += offset; How about moving this to when the PTS is set on the buffer a few lines above? Makes easier to read the code as the PTS is set only once and is not modified anymore. You can also only call the get_presentation_offset function inside the 'first_fragment_buffer' if as it would only be used there.
Hey Sebastian, am I missing something here? Why did you reopen that bug as blocker ?
(In reply to Thiago Sousa Santos from comment #31) > Review of attachment 298996 [details] [review] [review]: > > You should provide a default implementation that just returns 0 so that > mssdemux and hlsdemux won't crash/assert. Because of that ↑
Ah sorry, I pushed a patch in -bad already but didn't mention it here : commit a8604bc4f8694fcf32295193e71b5d7f32052f86 Author: Mathieu Duponchelle <mathieu.duponchelle@opencreed.com> Date: Tue Mar 10 15:31:21 2015 +0100 adaptivedemux: fix get_presentation_offset check. And return 0 isntead of FALSE. https://bugzilla.gnome.org/show_bug.cgi?id=745455
(In reply to Thiago Sousa Santos from comment #17) > Review of attachment 298698 [details] [review] [review]: > > ::: gst-libs/gst/adaptivedemux/gstadaptivedemux.c > @@ +745,3 @@ > stream->segment = demux->segment; > + stream->segment.start = stream->segment.position = > + stream->fragment.timestamp; > > I don't think this is correct because in the case of live streams we use the > lowest pts of all streams as the segment.start (just a few lines above > this). This might break sync of live streams. > > I'm not sure if presentationTimeOffset makes sense with live streams, but > the solution above will cause issues. I think something like: > > if (stream_has_presentation_offset) { segment.start += offset } > > might be better. It seems like this comment was forgotten. And actually this commit breaks seeking if there are "new streams" after the seek, as happens when switching periods during seeking. Looking into this now.
commit 548ed60e86b21c55ab566a32eb60a109d4757587 Author: Sebastian Dröge <sebastian@centricular.com> Date: Thu Jun 25 23:32:10 2015 +0200 adaptivedemux: Properly handle presentationTimeOffset for seeking and multi-period streams Segment start/time/position/base should only be modified if this is the first time we send a segment, otherwise we will override values from the seek segment if new streams have to be exposed as part of the seek. Segment base should be calculated from the segment start based on the stream's own segment, not the demuxer's segment. Both might differ slightly because of the presentationTimeOffset. Always add the presentationTimeOffset (relative to the period start, not timestamp 0) to the segment start after resetting the stream's segment based on the demuxer's segment (i.e. after seeks or stream restart). Also make sure to keep the stream's segment up to date and not just send a new segment event without storing the segment in the stream. https://bugzilla.gnome.org/show_bug.cgi?id=745455 commit 626a8f0a74f8ea748b811b74ba9e7ae2baea2cca Author: Sebastian Dröge <sebastian@centricular.com> Date: Thu Jun 25 23:24:50 2015 +0200 dashdemux: Subtract the period start time from the presentation offset We're interested in the offset between the period start timestamp and the actual media timestamp so that we can properly correct for it. The absolute presentation offset to timestamp 0 is useless as the only thing we really care about is the offset between the current fragment timestamp and the media timestamp. commit 95eb1aa49c130bb2f58ec6712bd8cb8b61326b99 Author: Sebastian Dröge <sebastian@centricular.com> Date: Thu Jun 25 20:19:34 2015 +0200 dashdemux: Subtract the period start when seeking based on a template Otherwise we will look for segments after the period usually. The seek timestamp is relative to the start of the first period and we have to select a segment relative to the current period's start. commit e671ad25a989cb21c62c7a5867c2090890ce49ba Author: Sebastian Dröge <sebastian@centricular.com> Date: Thu Jun 25 20:09:14 2015 +0200 dashdemux: Include the period start in the fragment timestamps in all cases We didn't do this for fragments that are generated on demand from a template, only for the other cases when they were all generated upfront. This caused fragment timestamps to start from 0 again for each new period. commit 9e8e1c452d6264498f8cfc6b0fa8f92556c8c121 Author: Sebastian Dröge <sebastian@centricular.com> Date: Thu Jun 25 23:23:58 2015 +0200 dashdemux: Seek on the new streams if the seek caused a period switch Seeking on the old streams is pointless, they are going to be freed on the next opportunity.