GNOME Bugzilla – Bug 736318
qtdemux: reset qtdemux parsing state on buffer discontinuity
Last modified: 2017-04-14 12:21:15 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.
Created attachment 285726 [details] [review] qtdemux: reset qtdemux parsing state on buffer discontinuity
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.
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.
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.
(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.
One could also send a buffer of size 0 with the DISCONT flag set.
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 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
Why is this a blocker?
(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 ?
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.
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
(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.
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;
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.
I tried attachment #290197 [details] on top of gstreamer master, but it breaks seeking in a mp4 file over http or pushfile.
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.
Created attachment 290954 [details] [review] qtdemux: rework segment event handling Cleaned up the previous patch a bit
Is this still an issue ? I thought we'd fixed all adaptive/fragmented use-cases.
Closing since I'm pretty certain this is fixed. Reopen if issue still happens.