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 736318 - qtdemux: reset qtdemux parsing state on buffer discontinuity
qtdemux: reset qtdemux parsing state on buffer discontinuity
Status: RESOLVED OBSOLETE
Product: GStreamer
Classification: Platform
Component: gst-plugins-good
1.4.1
Other Linux
: Normal major
: git master
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2014-09-09 10:40 UTC by Matthieu Bouron
Modified: 2017-04-14 12:21 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
qtdemux: reset qtdemux parsing state on buffer discontinuity (3.73 KB, patch)
2014-09-09 10:43 UTC, Matthieu Bouron
none Details | Review
qtdemux: reset qtdemux parsing state on gap event (3.40 KB, patch)
2014-09-09 13:08 UTC, Matthieu Bouron
rejected Details | Review
qtdemux: rework segment event handling (4.07 KB, patch)
2014-09-20 01:26 UTC, Thiago Sousa Santos
needs-work Details | Review
V2: qtdemux: rework segment event handling (6.92 KB, patch)
2014-11-07 17:01 UTC, Matthieu Bouron
none Details | Review
qtdemux: rework segment event handling (5.00 KB, patch)
2014-11-19 02:49 UTC, Thiago Sousa Santos
none Details | Review
qtdemux: rework segment event handling (5.34 KB, patch)
2014-11-19 03:57 UTC, Thiago Sousa Santos
none Details | Review

Description Matthieu Bouron 2014-09-09 10:40:32 UTC
When qtdemux is used in a fragmented scenario, downstream may not be linked, and qtdemux may not receive entirely the fragment as the adaptive demuxer will stop its task if he sees that downstream is not linked.

If a downstream pad is linked later, the adaptive demuxer will restart the related download task and push a new segment (with headers) and qtdemux needs to clear the adapter and reset its state to properly parse it. This is done by checking the discont flag on the incoming buffer and reset the qtdemux state accordingly so it can handle the new fragment.

The adaptive demuxer is then, responsible for setting the discont flag on the first buffer after it has restart its download task.
Comment 1 Matthieu Bouron 2014-09-09 10:43:10 UTC
Created attachment 285726 [details] [review]
qtdemux: reset qtdemux parsing state on buffer discontinuity
Comment 2 Matthieu Bouron 2014-09-09 12:33:32 UTC
Sorry about the noise, this patch does not apply on master and conflicts with https://bugzilla.gnome.org/show_bug.cgi?id=734443.

I need to find another way of doing this.
Comment 3 Matthieu Bouron 2014-09-09 13:08:14 UTC
Created attachment 285738 [details] [review]
qtdemux: reset qtdemux parsing state on gap event

Patch updated. The qtdemux state is now reset when the GAP event is received.
Comment 4 Tim-Philipp Müller 2014-09-09 13:18:03 UTC
It's not clear to me that any element should reset anything when it receives a GAP event, it seems like an abuse of the event at first glance.
Comment 5 Matthieu Bouron 2014-09-09 14:27:44 UTC
(In reply to comment #4)
> It's not clear to me that any element should reset anything when it receives a
> GAP event, it seems like an abuse of the event at first glance.

You're right, according to the documentation the GAP event is used to notify downstream that there will be no data for a certain amount of time and *not* that there is a discontinuity in the bytestream.

To clarify a bit the issue, here is what's happening.

Let's say our dash stream as 2 audios streams and 1 video stream.

1. When dashdemux exposes its pad, we link a new qtdemux element for each pad.

2. When the qtdemux elements exposes their pad, we link one video pad and one audio pad, so one of the qtdemux src pad is left unlinked.

At this point, all the qtdemux has received the fragment headers and we have the following pipeline.

            [qtdemux0] [videodecoder0] [videosink0] 
[dashdemux] [qtdemux1] [audiodecoder0] [audiosink0]
            [qtdemux2]


3. The pipeline starts playing, all qtdemux elements receive the first fragment.

4. dashdemux receives not-linked on the pad related to the qtdemux2 element and it stops the related download task.

5. The pipeline plays fine and we decide to switch our audio stream to the one related to qtdemux2. We remove the branch (decoder+sink) related to qtdemux1.

6. When the src pad of qtdemux2 is linked, a reconfigure event is sent upstream, and dashdemux decides to restart the related download task and download the segment (+ headers) corresponding to the current position. It also sends a gap event which is not correct since the gap event is supposed to tell that no data will arrrive for a certain amount of time.

7. qtdemux2 receives data but it expects the data to be the continuation of the old fragment (step 3).

So when we restart the download task and push the new fragment, we need to find a way to tell downstream to reset its internal state to be able to parse properly the new segment.
Comment 6 Tim-Philipp Müller 2014-09-09 14:37:40 UTC
One could also send a buffer of size 0 with the DISCONT flag set.
Comment 7 Edward Hervey 2014-09-09 14:43:55 UTC
so dashdemux knows that it's about to:
1) send data on that pad/demuxer which is no longer contiguous with what it sent previously
2) even more so in that it's going to be the beginning of a fragment

The only sane way I can think about is pushing a new TIME SEGMENT to notify that what is about to follow is:
1) topologically the same thing (i.e. same audio/video/formats/...)
2) but it will start with a new header
Comment 8 Sebastian Dröge (slomo) 2014-09-12 12:18:19 UTC
Comment on attachment 285738 [details] [review]
qtdemux: reset qtdemux parsing state on gap event

This is not what the GAP event is meaning. Use the DISCONT flag
Comment 9 Sebastian Dröge (slomo) 2014-09-17 17:24:53 UTC
Why is this a blocker?
Comment 10 Matthieu Bouron 2014-09-18 09:44:09 UTC
(In reply to comment #8)
> (From update of attachment 285738 [details] [review])
> This is not what the GAP event is meaning. Use the DISCONT flag

The discont flag is already used for reverse playback in dashdemux, meaning that for each first buffer of a new fragment the discont flag is set and forwarded downstream. Still in reverse playback the new fragment is pushed without the headers and I'm not sure we want to reset the parsing state of qtdemux each time we receive a new fragment in this scenario, what do you think ?
Anyway I should do some experiment and see if it works that way otherwise I will use the TIME SEGMENT approach.

(In reply to comment #9)
> Why is this a blocker?

Sorry about that, I misunderstood the meaning of blocker (and I finally read the documentation of priority/severity). I guess it can be set to major, right ?
Comment 11 Sebastian Dröge (slomo) 2014-09-18 10:13:13 UTC
Unless it's blocking a release (set target milestone for that) it shouldn't be a blocker. A blocker would be a regression for example.
Comment 12 Thiago Sousa Santos 2014-09-20 01:26:12 UTC
Created attachment 286666 [details] [review]
qtdemux: rework segment event handling

I'm currently using this to test the DASH stream switching scenarios.
I still need to make sure that it doesn't break other regular use cases:
seeking and reverse playback, mostly
Comment 13 Matthieu Bouron 2014-11-07 14:08:55 UTC
(In reply to comment #12)
> Created an attachment (id=286666) [details] [review]
> qtdemux: rework segment event handling
> 
> I'm currently using this to test the DASH stream switching scenarios.
> I still need to make sure that it doesn't break other regular use cases:
> seeking and reverse playback, mostly

qtdemux does not seem to push the new segment downstream breaking trick play.
The demuxer is correctly reset when dashdemux restart a stream though.
Comment 14 Matthieu Bouron 2014-11-07 15:22:17 UTC
Review of attachment 286666 [details] [review]:

::: gst/isomp4/qtdemux.c
@@ +1991,3 @@
+
+      } else if (segment.format == GST_FORMAT_TIME) {
+        } else {

This call reset qtdemux->upstream_segment to FALSE as well as qtdemux->pending_newsegment to NULL.

This prevents the new upstream segment to be pushed downstream later on.

A quick ugly fix would to add the following code after the reset call:

gst_event_replace (&demux->pending_newsegment, event);
demux->upstream_newsegment = TRUE;
Comment 15 Matthieu Bouron 2014-11-07 17:01:37 UTC
Created attachment 290197 [details] [review]
V2: qtdemux: rework segment event handling

I've updated the patch to make qtdemux avoid discarding the new upstream time segment so it is pushed later on.

I've replaced the gboolean parameter of gst_qtdemux_reset with an enum so the caller can choose what kind of reset he wants. Also gst_qtdemux_reset does not rely on qtdemux internal state (qtdemux->upstream_newsegment) to adapt his behaviour but instead rely only on the new reset parameter. It feels clearer IMHO.
Comment 16 Xavier Claessens 2014-11-13 19:15:03 UTC
I tried attachment #290197 [details] on top of gstreamer master, but it breaks seeking in a mp4 file over http or pushfile.
Comment 17 Thiago Sousa Santos 2014-11-19 02:49:15 UTC
Created attachment 290953 [details] [review]
qtdemux: rework segment event handling

For time segments, don't try to push something different than what
was received by upstream. Just store it and reset the internal state
to get ready to receive data again.


Updated version that fixes the push based seeking issue.

But I'd still prefer to split this patch into the segment
handling reworking and the refactoring of the _reset function.
I have no time to do it now but should get to do it before pushing.
Comment 18 Thiago Sousa Santos 2014-11-19 03:57:19 UTC
Created attachment 290954 [details] [review]
qtdemux: rework segment event handling

Cleaned up the previous patch a bit
Comment 19 Edward Hervey 2017-01-16 13:08:52 UTC
Is this still an issue ? I thought we'd fixed all adaptive/fragmented use-cases.
Comment 20 Edward Hervey 2017-04-14 12:21:15 UTC
Closing since I'm pretty certain this is fixed. Reopen if issue still happens.