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 778426 - qtdemux: Properly handle edit list in push mode
qtdemux: Properly handle edit list in push mode
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-good
git master
Other Linux
: Normal normal
: 1.15.1
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2017-02-10 04:45 UTC by Seungha Yang
Modified: 2018-05-24 09:46 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
qtdemux: Properly handle edit list in push mode (3.06 KB, patch)
2017-02-10 04:46 UTC, Seungha Yang
none Details | Review
ng case log (2.71 MB, text/plain)
2017-02-10 04:52 UTC, Seungha Yang
  Details
qtdemux: Properly handle edit list in push mode (3.05 KB, patch)
2017-02-10 04:59 UTC, Seungha Yang
none Details | Review
empty_edit.mp4.7z.001 (3.40 MB, patch)
2017-02-10 11:23 UTC, Seungha Yang
none Details | Review
empty_edit.mp4.7z.002 (3.40 MB, application/octet-stream)
2017-02-10 11:23 UTC, Seungha Yang
  Details
empty_edit.mp4.7z.003 (3.40 MB, application/octet-stream)
2017-02-10 11:23 UTC, Seungha Yang
  Details
empty_edit.mp4.7z.004 (1.90 MB, application/octet-stream)
2017-02-10 11:24 UTC, Seungha Yang
  Details
[1/4] qtdemux: Clarify variable name (2.43 KB, patch)
2018-03-31 11:37 UTC, Seungha Yang
committed Details | Review
[2/4] qtdemux: Add support zero duration edit list (2.84 KB, patch)
2018-03-31 11:38 UTC, Seungha Yang
rejected Details | Review
[3/4] qtdemux: Don't try parse next edit list entry using invalid start time (2.31 KB, patch)
2018-03-31 11:39 UTC, Seungha Yang
none Details | Review
[4/4] qtdemux: Properly handle edit list in push mode (3.06 KB, patch)
2018-03-31 11:40 UTC, Seungha Yang
none Details | Review
qtdemux: Don't try parse next edit list entry using invalid start time (2.97 KB, patch)
2018-05-23 11:13 UTC, Seungha Yang
rejected Details | Review
qtdemux: Properly handle edit list in push mode (3.51 KB, patch)
2018-05-23 11:14 UTC, Seungha Yang
committed Details | Review
qtdemux: Don't send gaps bigger than 1 second (now in push mode too) (2.34 KB, patch)
2018-05-23 17:16 UTC, Alicia Boya García
committed Details | Review

Description Seungha Yang 2017-02-10 04:45:35 UTC
If there are empty segments in edit list, demux should
adjust "accumulated_base" to apply it into running/stream time.
Comment 1 Seungha Yang 2017-02-10 04:46:18 UTC
Created attachment 345398 [details] [review]
qtdemux: Properly handle edit list in push mode
Comment 2 Seungha Yang 2017-02-10 04:52:29 UTC
Created attachment 345399 [details]
ng case log
Comment 3 Seungha Yang 2017-02-10 04:59:53 UTC
Created attachment 345400 [details] [review]
qtdemux: Properly handle edit list in push mode
Comment 4 Sebastian Dröge (slomo) 2017-02-10 11:09:22 UTC
Review of attachment 345400 [details] [review]:

Seems to make sense, but can you provide a sample stream that shows how this fails?
Comment 5 Seungha Yang 2017-02-10 11:23:18 UTC
Created attachment 345420 [details] [review]
empty_edit.mp4.7z.001
Comment 6 Seungha Yang 2017-02-10 11:23:37 UTC
Created attachment 345421 [details]
empty_edit.mp4.7z.002
Comment 7 Seungha Yang 2017-02-10 11:23:55 UTC
Created attachment 345422 [details]
empty_edit.mp4.7z.003
Comment 8 Seungha Yang 2017-02-10 11:24:11 UTC
Created attachment 345423 [details]
empty_edit.mp4.7z.004
Comment 9 Seungha Yang 2017-02-10 11:25:29 UTC
(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 :)
Comment 10 Alicia Boya García 2018-01-31 14:08:51 UTC
I confirm the patch works for the test file provided.
Comment 11 Thiago Sousa Santos 2018-02-01 08:06:46 UTC
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.
Comment 12 Thiago Sousa Santos 2018-02-20 02:26:44 UTC
https://bugzilla.gnome.org/show_bug.cgi?id=793058#c2

Can you check this regression that your patch introduces and fix it? Thanks!
Comment 13 Seungha Yang 2018-02-20 05:43:23 UTC
(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 :)
Comment 14 Alicia Boya García 2018-03-30 16:49:54 UTC
(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.
Comment 15 Alicia Boya García 2018-03-30 17:15:57 UTC
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.)
Comment 16 Seungha Yang 2018-03-31 11:37:06 UTC
Created attachment 370368 [details] [review]
[1/4] qtdemux: Clarify variable name

As defined by spec, use "empty edit". It's more straightforward.
Comment 17 Seungha Yang 2018-03-31 11:38:19 UTC
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.
Comment 18 Seungha Yang 2018-03-31 11:39:06 UTC
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.
Comment 19 Seungha Yang 2018-03-31 11:40:06 UTC
Created attachment 370372 [details] [review]
[4/4] qtdemux: Properly handle edit list in push mode

rebase
Comment 20 Seungha Yang 2018-03-31 12:08:12 UTC
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 :)
Comment 21 Alicia Boya García 2018-03-31 12:25:53 UTC
Review of attachment 370368 [details] [review]:

Totally agree.
Comment 22 Alicia Boya García 2018-03-31 12:28:11 UTC
Review of attachment 370371 [details] [review]:

This seems an unrelated issue. Could you open a separate bug and explain it?
Comment 23 Seungha Yang 2018-03-31 12:40:44 UTC
(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.
Comment 24 Alicia Boya García 2018-03-31 12:41:37 UTC
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.
Comment 25 Seungha Yang 2018-03-31 13:14:14 UTC
(In reply to Alicia Boya García from comment #24)
> Review of attachment 370369 [details] [review] [review]:

I will. Thanks for your review
Comment 26 Thibault Saunier 2018-05-22 09:21:01 UTC
Review of attachment 370368 [details] [review]:

Lgtm
Comment 27 Thibault Saunier 2018-05-22 10:45:57 UTC
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.
Comment 28 Thibault Saunier 2018-05-22 11:55:24 UTC
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.
Comment 29 Thibault Saunier 2018-05-22 11:55:26 UTC
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.
Comment 30 Alicia Boya García 2018-05-22 16:46:15 UTC
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
Comment 31 Seungha Yang 2018-05-23 11:13:44 UTC
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)
Comment 32 Seungha Yang 2018-05-23 11:14:15 UTC
Created attachment 372359 [details] [review]
qtdemux: Properly handle edit list in push mode
Comment 33 Seungha Yang 2018-05-23 11:21:05 UTC
(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.
Comment 34 Seungha Yang 2018-05-23 11:26:13 UTC
(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.
Comment 35 Alicia Boya García 2018-05-23 14:29:36 UTC
(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.
Comment 36 Alicia Boya García 2018-05-23 16:06:08 UTC
(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.
Comment 37 Alicia Boya García 2018-05-23 17:16:02 UTC
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.
Comment 38 Thibault Saunier 2018-05-24 08:49:17 UTC
(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 :-)
Comment 39 Thibault Saunier 2018-05-24 08:52:06 UTC
Review of attachment 372358 [details] [review]:

The patch merged as part of #794858 already fixes that actually.
Comment 40 Thibault Saunier 2018-05-24 08:53:29 UTC
Review of attachment 372359 [details] [review]:

lgtm
Comment 41 Thibault Saunier 2018-05-24 09:45:56 UTC
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)