GNOME Bugzilla – Bug 774196
mpdparser: Fix timestamp setting on fragment
Last modified: 2016-11-15 12:49:32 UTC
mpdparser: Fix timestamp setting on fragment segment_index represents relative indexed from period start, and therefore "stream->segment_index * fragment->duration" is relative value. To make fragment's timestamp absolute value, period start should be compensated.
Created attachment 339472 [details] [review] mpdparser-Fix-timestamp-setting-on-fragment
How can we reproduce the problem you're seeing? The timestamp should be correct (i.e. the PTS/DTS as inside the MP4 container), and together with the segment it should lead to the correct running and stream time. It does not for you?
Issue can be reproduced with following live mpd. Live case, adaptive demux do segment seek to the first fragment's timestamp, and the timestamp must be in period's timeline. Because current codes did not seems to be non-zero period start considered, relative timestame was calculated, and it resulted to incorrect segment event. http://vm2.dashif.org/livesim-dev/periods_60/xlink_30/insertad_1/testpic_2s/Manifest.mpd http://vm2.dashif.org/livesim-dev/periods_60/xlink_30/insertad_5/testpic_2s/Manifest.mpd
Check this code: https://cgit.freedesktop.org/gstreamer/gst-plugins-bad/tree/gst-libs/gst/adaptivedemux/gstadaptivedemux.c#n1010 The problem is probably here. I assume the timestamps in the MP4 are in your case still approximately the same as the fragment start timestamp, but stream/running time is wrong?
(In reply to Sebastian Dröge (slomo) from comment #4) > Check this code: > https://cgit.freedesktop.org/gstreamer/gst-plugins-bad/tree/gst-libs/gst/ > adaptivedemux/gstadaptivedemux.c#n1010 > > The problem is probably here. I think you are correct :) but some other points should be more fixed. when I saw current usage of fragment.timestame, it seems to be relative timestamp. https://cgit.freedesktop.org/gstreamer/gst-plugins-bad/tree/gst-libs/gst/adaptivedemux/gstadaptivedemux.c#n1975 > I assume the timestamps in the MP4 are in your case still approximately the > same as the fragment start timestamp, but stream/running time is wrong? I'm not sure segment.start value should be as follows, but running time must be problem, I think. please refer to following log (Without patch) the first buffer PTS = 410772:47:00.010666666 segment start = 410772:45:00.000000000 (1478781900000000000) 0:00:03.046399410 21302 0x7f0ab4003800 DEBUG GST_EVENT gstpad.c:5550:gst_pad_send_event_unchecked:<multiqueue0:sink_0> have event type segment event: 0x7f0acc00edd0, time 99:99:99.999999999, seq-num 37, GstEventSegment, segment=(GstSegment)"GstSegment, flags=(GstSegmentFlags)GST_SEGMENT_FLAG_RESET, rate=(double)1, applied-rate=(double)1, format=(GstFormat)GST_FORMAT_TIME, base=(guint64)1478781844000000000, offset=(guint64)0, start=(guint64)1478781900000000000, stop=(guint64)18446744073709551615, time=(guint64)1478781900000000000, position=(guint64)1478781900000000000, duration=(guint64)18446744073709551615;"; 0:00:03.712068011 21361 0x7f8080003680 DEBUG GST_SCHEDULING gstpad.c:4204:gst_pad_chain_data_unchecked:<multiqueue0:sink_0> calling chainfunction &gst_multi_queue_chain with buffer buffer: 0x7f808c0116f0, pts 410772:47:00.010666666, dts 410772:47:00.010666666, dur 0:00:00.021333334, size 137, offset none, offset_end none, flags 0x4040
Created attachment 339490 [details] [review] adaptivedemux: Fix startup SEGMENT seeking and setting for live adaptivedemux: Fix startup SEGMENT seeking and setting for live Because fragment.timestamp is relative value to period_start, startup SEGMENT seeking should be pointed to "fragment.timestamp + period_start"
Comment on attachment 339490 [details] [review] adaptivedemux: Fix startup SEGMENT seeking and setting for live This seems to make sense, I'll have to give it a closer review later though. Can you also check if it still works with the other test vectors here: http://testassets.dashif.org/#feature/details/57cd83dfb626efae4d44d44c (the multi period ones), including 1) just playback over period boundaries, 2) seeking over period boundaries (back and forth), 3) seeking over period boundaries and then advancing (during playback) to the next period?
(In reply to Sebastian Dröge (slomo) from comment #7) > Comment on attachment 339490 [details] [review] [review] > adaptivedemux: Fix startup SEGMENT seeking and setting for live > > This seems to make sense, I'll have to give it a closer review later though. > > Can you also check if it still works with the other test vectors here: > http://testassets.dashif.org/#feature/details/57cd83dfb626efae4d44d44c (the > multi period ones), including 1) just playback over period boundaries, 2) > seeking over period boundaries (back and forth), 3) seeking over period > boundaries and then advancing (during playback) to the next period? I'll report test results for regression check soon :)
Created attachment 339618 [details] TEST results Test results is attached. In summary, there is no regression. (Actually, contents which have 3 periods did not work well.... with/without my patch) For contents which have 2 periods, there are no PTS discontinuity at period boundaries, and seeking is working well. However, some contents show MPD parsing error, and poor seek performance on the other contents.
Seek problem of 3 periods MPD's are strongly related to bug #767071 From my debugging, adaptivedemux driven SEGMENT events (especially segment.time) are broken by following code https://cgit.freedesktop.org/gstreamer/gst-plugins-good/tree/gst/isomp4/qtdemux.c#n2164 And it caused inaccurate query_position at basesink. Is this regression?
Did it ever work before? I'm pretty sure, 3+ period DASH streams worked at some point (when I added the above linked code to dashdemux). That qtdemux code should be older though. Do you know what needs fixing, and how?
(In reply to Sebastian Dröge (slomo) from comment #11) > Do you know what needs fixing, and how? simply, 3 periods mpd's are working when I disable the codes which are modifying dashdemux driven TIME FORMAT segment at https://cgit.freedesktop.org/gstreamer/gst-plugins-good/tree/gst/isomp4/qtdemux.c#n2164 However, my approach may not be correct. I need to understand more what was discussed at bug #767071
Anyway, as I mentiond above, modification of segment.time at qtdemux seems to be problem. It should be forwarded to sink without modification.
Created attachment 339678 [details] [review] qtdemux: Forward TIME segment without change on fragmented stream Demux couldn't know about total stream timeline of fragmented stream. Moreover, upstream driven time format stream/running time shouldn't be modified in this case. This patch is for fixing seek in DASH streaming.
Thiago, what do you think about this change, the qtdemux one? Can you think of any case where this would cause problems?
Review of attachment 339678 [details] [review]: Looks ok to me but I'm unaware of how DLNA works in detail. I guess it also sends TIME segments from the source? ::: gst/isomp4/qtdemux.c @@ +2168,3 @@ + if (!(demux->upstream_format_is_time && demux->fragmented)) { + /* accept upstream's notion of segment and distribute along */ + segment.format = GST_FORMAT_TIME; My main concern is ending up with segment.format != TIME for qtdemux as it always expect it to be time but I guess the upstream_format_is_time check will get that right.
(In reply to Thiago Sousa Santos from comment #16) > Review of attachment 339678 [details] [review] [review]: > > Looks ok to me but I'm unaware of how DLNA works in detail. I guess it also > sends TIME segments from the source? We have no DLNA support, and if something sends TIME segments these should be considered in any case instead of overriding them with something parsed from the stream. > ::: gst/isomp4/qtdemux.c > @@ +2168,3 @@ > + if (!(demux->upstream_format_is_time && demux->fragmented)) { > + /* accept upstream's notion of segment and distribute along */ > + segment.format = GST_FORMAT_TIME; > > My main concern is ending up with segment.format != TIME for qtdemux as it > always expect it to be time but I guess the upstream_format_is_time check > will get that right. Yeah
(In reply to Sebastian Dröge (slomo) from comment #17) > (In reply to Thiago Sousa Santos from comment #16) > > Review of attachment 339678 [details] [review] [review] [review]: > > > > Looks ok to me but I'm unaware of how DLNA works in detail. I guess it also > > sends TIME segments from the source? > > We have no DLNA support, and if something sends TIME segments these should > be considered in any case instead of overriding them with something parsed > from the stream. "no DLNA support *upstream*" I mean. If such a 3rd party element depends on qtdemux "ignoring" its own TIME segment events, that would be a bug IMHO
Review of attachment 339678 [details] [review]: ::: gst/isomp4/qtdemux.c @@ +2166,3 @@ + * because what demux could know is about upto current moof, + * and demux has limited understanding of total presentation timeline */ + if (!(demux->upstream_format_is_time && demux->fragmented)) { Why would we only want this on fragmented streams? I think we actually want this on all streams, no? If upstream is in TIME, there's probably a reason for that and we should not just ignore that information. Or am I missing something here?
Except for comment 19, I think we should get it in then. Any opinions on that one, Thiago? :)
(In reply to Sebastian Dröge (slomo) from comment #19) > Review of attachment 339678 [details] [review] [review]: The reason why I added "demux->fragmented" was just for safety reason, I mean to minimize possible regression :) Actually, I fully agree with your opinion that we shouldn't ignore upstream driven TIME-FORMAT segment, although it's non-fragmented media. I'd like to listen other experts' opinions.
If you update the patch, I'll merge it as is. It should not break anything I'm aware of and seems more correct, so let's get it in and give it some testing.
Created attachment 339923 [details] [review] qtdemux: Don't modify upstream TIME segment Patch updated :)
Comment on attachment 339923 [details] [review] qtdemux: Don't modify upstream TIME segment Attachment 339923 [details] pushed as e5b3d92 - qtdemux: Don't modify upstream TIME segment
Attachment 339490 [details] pushed as e2dbc2a - adaptivedemux: Fix startup SEGMENT seeking and setting for live