GNOME Bugzilla – Bug 778426
qtdemux: Properly handle edit list in push mode
Last modified: 2018-05-24 09:46:55 UTC
If there are empty segments in edit list, demux should adjust "accumulated_base" to apply it into running/stream time.
Created attachment 345398 [details] [review] qtdemux: Properly handle edit list in push mode
Created attachment 345399 [details] ng case log
Created attachment 345400 [details] [review] qtdemux: Properly handle edit list in push mode
Review of attachment 345400 [details] [review]: Seems to make sense, but can you provide a sample stream that shows how this fails?
Created attachment 345420 [details] [review] empty_edit.mp4.7z.001
Created attachment 345421 [details] empty_edit.mp4.7z.002
Created attachment 345422 [details] empty_edit.mp4.7z.003
Created attachment 345423 [details] empty_edit.mp4.7z.004
(In reply to Sebastian Dröge (slomo) from comment #4) > Review of attachment 345400 [details] [review] [review]: > > Seems to make sense, but can you provide a sample stream that shows how this > fails? Please unzip attached files... Frankly I'm not sure that uploading that content on public is allowed or not.... but I did :)
I confirm the patch works for the test file provided.
Indeed it removes warnings about buffers being dropped. Also seems to make sense overall. Going to run this through validate to see if any regressions pop up.
https://bugzilla.gnome.org/show_bug.cgi?id=793058#c2 Can you check this regression that your patch introduces and fix it? Thanks!
(In reply to Thiago Sousa Santos from comment #12) > https://bugzilla.gnome.org/show_bug.cgi?id=793058#c2 > > Can you check this regression that your patch introduces and fix it? Thanks! Sure, I'll look at it :)
(In reply to Seungha Yang from comment #13) > (In reply to Thiago Sousa Santos from comment #12) > > https://bugzilla.gnome.org/show_bug.cgi?id=793058#c2 > > > > Can you check this regression that your patch introduces and fix it? Thanks! > > Sure, I'll look at it :) There is no regression caused by this patch AFAIK. The one discussed in the link occurs after making a modification to this one, but -- as described in my last comment, it's a consequence of a different bug, unrelated to this patch.
Review of attachment 345400 [details] [review]: r+ ::: gst/isomp4/qtdemux.c @@ +6431,3 @@ demux->got_moov = TRUE; + if (demux->fragmented) { + /* fragmented streams headers shouldn't contain edts atoms */ Fragmented streams can (and often should) contain edts atoms. Nevertheless, that is better changed in a different patch, after edts.duration=0 support is added, which is often used in fragmented files. @@ +6434,3 @@ + gst_qtdemux_check_send_pending_segment (demux); + } else { + gst_event_replace (&demux->pending_newsegment, NULL); This line makes me wonder... Why are we setting demux->pending_newsegment in the first place? demux->pending_newsegment is loaded with the movie (unedited) playback range, which should never be emitted by the srcpads. Instead, the requested movie position should be mapped to an edit, like the following line does. (This is a reflection about qtdemux, not a problem with this particular patch.)
Created attachment 370368 [details] [review] [1/4] qtdemux: Clarify variable name As defined by spec, use "empty edit". It's more straightforward.
Created attachment 370369 [details] [review] [2/4] qtdemux: Add support zero duration edit list An edit list with zero duration does not imply that actual duration is zero, but just used for indicating presentation time offset.
Created attachment 370371 [details] [review] [3/4] qtdemux: Don't try parse next edit list entry using invalid start time Invalid start time can break presentation timeline.
Created attachment 370372 [details] [review] [4/4] qtdemux: Properly handle edit list in push mode rebase
Sorry for late response Thiago :( I fixed zero-duration edit list issue. Previously, the reported file http://yt-dash-mse-test.commondatastorage.googleapis.com/unit-tests/media/car-20120827-86.mp4 was not playable in pull mode regardless of my patch, since we've not handled zero-duration edit list as Thiago said in bug #793058. (In reply to Alicia Boya García from comment #15) > Review of attachment 345400 [details] [review] [review]: > > r+ > > ::: gst/isomp4/qtdemux.c > @@ +6431,3 @@ > demux->got_moov = TRUE; > + if (demux->fragmented) { > + /* fragmented streams headers shouldn't contain edts atoms */ > > Fragmented streams can (and often should) contain edts atoms. Nevertheless, > that is better changed in a different patch, after edts.duration=0 support > is added, which is often used in fragmented files. Agree :) Fragmented mp4 can have it. > @@ +6434,3 @@ > + gst_qtdemux_check_send_pending_segment (demux); > + } else { > + gst_event_replace (&demux->pending_newsegment, NULL); > > This line makes me wonder... Why are we setting demux->pending_newsegment in > the first place? > > demux->pending_newsegment is loaded with the movie (unedited) playback > range, which should never be emitted by the srcpads. Instead, the requested > movie position should be mapped to an edit, like the following line does. > > (This is a reflection about qtdemux, not a problem with this particular > patch.) I guess it could be related dashdemux use case. Is there any reference for relation between edit list and mpd timeline? I don't mean that code is optimal though :)
Review of attachment 370368 [details] [review]: Totally agree.
Review of attachment 370371 [details] [review]: This seems an unrelated issue. Could you open a separate bug and explain it?
(In reply to Alicia Boya García from comment #22) > Review of attachment 370371 [details] [review] [review]: > > This seems an unrelated issue. Could you open a separate bug and explain it? We can reject the patch for now. The purpose was possible invalid edit handling, e.g., dropping any edit list entries following a zero duration edit list entry, since we don't know how to handle them.
Review of attachment 370369 [details] [review]: This should be in a separate bug. I'm already working on this same issue by the way. Please upload it here instead. https://bugzilla.gnome.org/show_bug.cgi?id=794858 ::: gst/isomp4/qtdemux.c @@ +9087,3 @@ segment->time = stime; /* add non scaled values so we don't cause roundoff errors */ + if (G_UNLIKELY (!duration)) { Please avoid using !negations for time values, as intuition will bite the reader, no matter how familiar with C they are. (!time) is often misread as "there is no time and as such time == GST_CLOCK_TIME_NONE", but it actually means time != 0, which is a completely different case. @@ +9094,3 @@ + qtdemux->duration > 0) { + /* If we can guess duration, use it */ + stime = QTTIME_TO_GSTTIME (qtdemux, qtdemux->duration); qtdemux->duration is not particularly reliable, as all three fields containing durations may use (slightly) bogus values. I think it would be safer to just use GST_CLOCK_TIME_NONE there. This value is not used for duration reporting after all. @@ +9118,3 @@ segment->media_start = media_start; + segment->media_stop = GST_CLOCK_TIME_IS_VALID (segment->duration) ? + segment->media_start + segment->duration : GST_CLOCK_TIME_NONE; Good catch, that was a subtle bug.
(In reply to Alicia Boya García from comment #24) > Review of attachment 370369 [details] [review] [review]: I will. Thanks for your review
Review of attachment 370368 [details] [review]: Lgtm
Review of attachment 370371 [details] [review]: ::: gst/isomp4/qtdemux.c @@ +9158,3 @@ buffer += entry_size; + + if (!GST_CLOCK_TIME_IS_VALID (stime)) { In what case is `stime` going to be CLOCK_TIME_NONE? I do not have the impression that this is going to happen with the file you provided. Otherwise I understand the idea, and it seems sensible.
Review of attachment 370372 [details] [review]: I think we should be sending the segment then the GAP event, making sure to correct the base time of the following segment.
Review of attachment 370372 [details] [review]: Starting a stream with a GAP event is problematic. See https://github.com/GStreamer/gst-plugins-good/commit/2e45926a96ec5298c6ef29bf912e5e6a06dc3e0e
Created attachment 372358 [details] [review] qtdemux: Don't try parse next edit list entry using invalid start time Invalid start time can break presentation timeline. (e.g., any segment following empty edit list, which is invalid case)
Created attachment 372359 [details] [review] qtdemux: Properly handle edit list in push mode
(In reply to Thibault Saunier from comment #27) > Review of attachment 370371 [details] [review] [review]: > > ::: gst/isomp4/qtdemux.c > @@ +9158,3 @@ > buffer += entry_size; > + > + if (!GST_CLOCK_TIME_IS_VALID (stime)) { > > In what case is `stime` going to be CLOCK_TIME_NONE? I do not have the > impression that this is going to happen with the file you provided. > > Otherwise I understand the idea, and it seems sensible. Not sure whether it's possible or not, but in that function, "qtdemux->duration == -1" can cause CLOCK_TIME_NONE. As I mentioned before, this patch is not required for this bug.
(In reply to Thibault Saunier from comment #29) > Review of attachment 370372 [details] [review] [review]: > > I think we should be sending the segment then the GAP event, making sure to > correct the base time of the following segment. If my understanding is correct, gst_qtdemux_map_and_push_segments() makes GAP event after sending segment event.
(In reply to Seungha Yang from comment #34) > (In reply to Thibault Saunier from comment #29) > > Review of attachment 370372 [details] [review] [review] [review]: > > > > I think we should be sending the segment then the GAP event, making sure to > > correct the base time of the following segment. > > If my understanding is correct, gst_qtdemux_map_and_push_segments() makes > GAP event after sending segment event. gst_qtdemux_map_and_push_segments() (which is used only in push mode) sends GAP events only for "spare" streams like subtitle tracks. This does not match the behavior in pull mode, where gaps are sent for all types of streams, but only if they are at least 1 second long.
(In reply to Alicia Boya García from comment #35) > gst_qtdemux_map_and_push_segments() (which is used only in push mode) sends > GAP events only for "spare" streams like subtitle tracks. This does not > match the behavior in pull mode, where gaps are sent for all types of > streams, but only if they are at least 1 second long. Correction: the GAP events are sent in push mode too. The spare stream check is used for a completely different thing, I got that mixed up. Therefore in the end the only difference between the behavior in pull mode and push mode after the patch is the 1 second check.
Created attachment 372368 [details] [review] qtdemux: Don't send gaps bigger than 1 second (now in push mode too) This applies the same workaround to gaps that is being used in pull mode.
(In reply to Seungha Yang from comment #34) > (In reply to Thibault Saunier from comment #29) > > Review of attachment 370372 [details] [review] [review] [review]: > > > > I think we should be sending the segment then the GAP event, making sure to > > correct the base time of the following segment. > > If my understanding is correct, gst_qtdemux_map_and_push_segments() makes > GAP event after sending segment event. It actually does, I missed that, sorry :-)
Review of attachment 372358 [details] [review]: The patch merged as part of #794858 already fixes that actually.
Review of attachment 372359 [details] [review]: lgtm
Attachment 372359 [details] pushed as f61c2bc - qtdemux: Properly handle edit list in push mode Attachment 372368 [details] pushed as d35f893 - qtdemux: Don't send gaps bigger than 1 second (now in push mode too)