GNOME Bugzilla – Bug 757951
adaptivedemux: set src element to ready before flushing the input pad
Last modified: 2016-01-15 15:05:04 UTC
Since commit ccff3be3ab2e5bffcefc12c80a5edb225801f1b9 introduced asynchronous input to adaptivedemux (by using a queue2 element that intermediates data generated by uri handler) the adaptive demux incorrectly shuts down the uri handler.
Previously, after the uri handler generated an error, it stopped sending data, so when adaptive demux sees the error message, no more data will be received. Now, due to the queue2 element, adaptive demux can see the error message and then receive some more data.
During the uri handler shut down sequence, adaptive demux will send a flush start event, change the state of src to ready and then send the flush stop event. But after the send start event, the queue2 element can try to push new data and this will fail because no segment start event was sent. Adaptive demux should first set the src element to ready and only then send the flush start and flush stop events.
Created attachment 315269 [details] [review]
Why should it send the FLUSH_* events after setting the element to READY? Changing the state to READY should already have the same effect as flushing... and the events should be just dropped in READY.
I think the first event was sent before changing the state to make sure everything shuts down ASAP.
Imagine this scenario:
- uri handler pushes to queue2 some segments
- uri handler detects a problem and posts an error message on the bus
- adaptive demux handles the error message before the segments from the queue. It will send a flush start and flush stop event to its input pad
- the queue2 will try to push a segment to adaptive demux input pad. This will generate the error "Unexpected critical/warning: gstpad.c:4332:gst_pad_push_data:<bin0:src> Got data flow before segment event"
Just don't send a flush-stop event then ;) Not sure why we send flush events at all here.
The flush event is sent to clear the EOS state. In normal scenarios, the uri handler will send an eos event and adaptive demux will detect that, change the state of uri handler to ready and must also clear the EOS state on its input pad. To do that, it sends itself a flush start/flush stop event.
So, we need the flush event because solely changing the state on uri handler will not generate it. But are you suggesting to send only a flush start and no flush stop event? I didn't knew this was possible.
If you go to READY, the EOS flag is also cleared. But this is about the ghostpad? You could deactivate/activate that one instead of flushing (has the same effect as setting the state to READY).
Flush events here seem a bit weird.
Seems that the function gst_adaptive_demux_stream_clear_eos_and_flush_state() already cleans the EOS flag and the flush state on the input pad. I don't know if flushing will do anything extra.
I now think that the flushing is not necessary. The tests are working fine without it.
Created attachment 315290 [details] [review]
removed flushing on input pad
Comment on attachment 315290 [details] [review]
Makes sense to me, let's see if Thiago remembers why the flushing is here
It was related to the internal pad of the ghostpad. IIRC just setting the ghost pad to inactive and bringing it back wasn't doing the same on its internal proxy pad and some bad status (flushing/eos) was left there, causing stream to fail.
Looking at gst_adaptive_demux_stream_clear_eos_and_flush_state gives some hints on the states that weren't properly flushed.
Are flushing seeks still working after this patch?
Instead of flushing, deactivating and activating the ghostpad should have the same effect for this.
If it all still works, feel free to push it. I might give it some testing myself later today.
Not sure if it was related, but I remember some issues with HLS live streams when hlsdemux tried to fetch a segment that was not available on the server.
In any case, I think it would be a good idea to also do an activation-cycle of the ghostpad when switching the sources :)
Florin, did you test if after your patch flushing seeks and other things are still working?
yes, seek operations work fine and the tests are passing. I haven't seen any problems.
given the fact that gst_adaptive_demux_stream_clear_eos_and_flush_state clears the flags Thiago was talking about, do you still feel that deactivating the ghost pad is still required? I could not see any problems while testing dash but I cannot say anything about hls.
Instead of unsetting the flag manually, I would deactivate and activate the pad. Because that might do more things than just unsetting the flags, and in other places we already had problems with code that was only unsetting the caps and not deactivating the pad.
I tried to deactivate and reactivate the pad but it does not work.
Deactivating and reactivating the stream pad will erase all its events. Next time when adaptive demux will try to send data on the pad it will fail because data is received without seeing a segment event.
The gst_adaptive_demux_stream_download_uri function is called 3 times for a fragment: to download the initialization segment, the index segment and the media segment. Adaptive demux will send the segment information only at the beginning, so we cannot deactivate the pad between downloads.
So, because deactivation of internal pad does not work, should we keep sending the flush event?
Looking at the differences between a flush event and gst_adaptive_demux_stream_clear_eos_and_flush_state:
- a flush event will also clear the segment events from the pad (remove_event_by_type (pad, GST_EVENT_SEGMENT);)
- both flush event and gst_adaptive_demux_stream_clear_eos_and_flush_state will clear the EOS flag and EOS events
Also, next time the uri_handler element is activated, it will send a stream start event which will clear the EOS events from all pads, so I don't think it's necessary to clear the EOS events and flag in gst_adaptive_demux_stream_clear_eos_and_flush_state.
From what I can see, the flush event is needed to clear the SEGMENT event and gst_adaptive_demux_stream_clear_eos_and_flush_state needs to clear the FLUSHING flag.
Thiago, Sebastian, do you have any comments on this?
On current master, you may see occasionally in tests errors like these:
"gstcheck.c:79:F:basicTest:testFragmentDownloadError:0: Unexpected critical/warning: gstpad.c:4379:gst_pad_push_data:<bin0:src> Got data flow before segment event
The attached patch seems to fix them.
Only the testFragmentDownloadError test detects this problem.
(In reply to Florin Apostol from comment #21)
> On current master, you may see occasionally in tests errors like these:
> "gstcheck.c:79:F:basicTest:testFragmentDownloadError:0: Unexpected
> critical/warning: gstpad.c:4379:gst_pad_push_data:<bin0:src> Got data flow
> before segment event
> The attached patch seems to fix them.
> Only the testFragmentDownloadError test detects this problem.
Yes, I can confirm this error is caused by the flush start/stop pair. We should fix this.
Did someone run gst-validate with this patch? Does it break anything? Seems like it should work.
The patch seems to work, tried a few scenarios here successfully. Can't run gst-validate atm.
I also investigated why we need to manually cleanup the ghostpad/proxypad is because deactivating it will erase everything, including stream-start and segment that needs to be pushed again.
I'm considering removing the ghostpad and just using 1 floating pad and 1 normal pad on the bin to make sure they are handled separately.
Seems ok. But probably we will loose the ability from tests to identify the http source elements feeding data in dashdemux.
I am currently using in the tests the internal pad for 2 things:
1. to identify the stream when the internal bin element generates a state change message: from the bin pad I get the peer pad (the internal pad) and from here the src->pad which has a name audio_00. Worst case we can assume bin0 corresponds for first stream, bin1 for second stream, etc.
2. To get segment events (which are not forwarded by adaptivedemux) so that I know when the src started to send a new file. This is needed for pattern matching of data in dashdemux tests.
(In reply to Florin Apostol from comment #24)
> Seems ok. But probably we will loose the ability from tests to identify the
> http source elements feeding data in dashdemux.
> I am currently using in the tests the internal pad for 2 things:
> 1. to identify the stream when the internal bin element generates a state
> change message: from the bin pad I get the peer pad (the internal pad) and
> from here the src->pad which has a name audio_00. Worst case we can assume
> bin0 corresponds for first stream, bin1 for second stream, etc.
> 2. To get segment events (which are not forwarded by adaptivedemux) so that
> I know when the src started to send a new file. This is needed for pattern
> matching of data in dashdemux tests.
I'm preparing a patch and noticed that. I'll give meaningful names to the bins that contain the http source and the queue and get their src pad to monitor those same situations.
Created attachment 318970 [details] [review]
adaptivedemux: replace ghostpad with a standard pad
Patch to replace the ghostpad with a standard pad. Seems to keep
stuff working. Please test.
gst-validate has no complaints on this.
Author: Thiago Santos <email@example.com>
Date: Wed Jan 13 09:51:20 2016 -0300
adaptivedemux: replace ghostpad with a standard pad
Handling the ghostpad and its internal pad was causing more issues
than helping because of their coupled activation/deactivation
As we have to install custom chain,event and query functions it is
better to use a floating sink pad internally in the demuxer and just
use those pad functions to push through a standard pad in the demuxer
Author: Florin Apostol <firstname.lastname@example.org>
Date: Wed Nov 11 17:24:33 2015 +0000
adaptivedemux: do not flush the input pad
gst_adaptive_demux_stream_clear_eos_and_flush_state() function will do
all the necessary cleaning.