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 757951 - adaptivedemux: set src element to ready before flushing the input pad
adaptivedemux: set src element to ready before flushing the input pad
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-bad
git master
Other Linux
: Normal normal
: 1.7.2
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2015-11-11 14:42 UTC by Florin Apostol
Modified: 2016-01-15 15:05 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
proposed patch (1.96 KB, patch)
2015-11-11 14:43 UTC, Florin Apostol
needs-work Details | Review
proposed patch (1.73 KB, patch)
2015-11-11 17:28 UTC, Florin Apostol
committed Details | Review
adaptivedemux: replace ghostpad with a standard pad (14.37 KB, patch)
2016-01-13 14:29 UTC, Thiago Sousa Santos
committed Details | Review

Description Florin Apostol 2015-11-11 14:42:46 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.
Comment 1 Florin Apostol 2015-11-11 14:43:30 UTC
Created attachment 315269 [details] [review]
proposed patch
Comment 2 Sebastian Dröge (slomo) 2015-11-11 14:57:48 UTC
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.
Comment 3 Florin Apostol 2015-11-11 15:43:42 UTC
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"
Comment 4 Sebastian Dröge (slomo) 2015-11-11 15:50:30 UTC
Just don't send a flush-stop event then ;) Not sure why we send flush events at all here.
Comment 5 Florin Apostol 2015-11-11 16:28:02 UTC
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.
Comment 6 Sebastian Dröge (slomo) 2015-11-11 16:39:12 UTC
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.
Comment 7 Florin Apostol 2015-11-11 17:28:19 UTC
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.
Comment 8 Florin Apostol 2015-11-11 17:28:47 UTC
Created attachment 315290 [details] [review]
proposed patch

removed flushing on input pad
Comment 9 Sebastian Dröge (slomo) 2015-11-11 17:45:17 UTC
Comment on attachment 315290 [details] [review]
proposed patch

Makes sense to me, let's see if Thiago remembers why the flushing is here
Comment 10 Thiago Sousa Santos 2015-11-12 16:19:17 UTC
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?
Comment 11 Sebastian Dröge (slomo) 2015-11-12 16:59:17 UTC
Instead of flushing, deactivating and activating the ghostpad should have the same effect for this.
Comment 12 Thiago Sousa Santos 2015-11-12 18:27:29 UTC
If it all still works, feel free to push it. I might give it some testing myself later today.
Comment 13 Thiago Sousa Santos 2015-11-12 18:32:27 UTC
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.
Comment 14 Sebastian Dröge (slomo) 2015-11-13 08:40:14 UTC
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?
Comment 15 Florin Apostol 2015-11-13 18:05:25 UTC
yes, seek operations work fine and the tests are passing. I haven't seen any problems.
Comment 16 Florin Apostol 2015-11-23 17:09:10 UTC
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.
Comment 17 Sebastian Dröge (slomo) 2015-11-24 13:54:02 UTC
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.
Comment 18 Florin Apostol 2015-11-25 18:48:51 UTC
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.
Comment 19 Florin Apostol 2015-11-26 17:59:25 UTC
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.
Comment 20 Florin Apostol 2015-12-08 11:09:46 UTC
Thiago, Sebastian, do you have any comments on this?
Comment 21 Florin Apostol 2016-01-07 13:24:20 UTC
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.
Comment 22 Thiago Sousa Santos 2016-01-07 19:07:21 UTC
(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.
Comment 23 Thiago Sousa Santos 2016-01-08 14:59:07 UTC
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.

Any opinions?
Comment 24 Florin Apostol 2016-01-08 16:38:48 UTC
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.
Comment 25 Thiago Sousa Santos 2016-01-12 19:00:36 UTC
(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.
Comment 26 Thiago Sousa Santos 2016-01-13 14:29:50 UTC
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.
Comment 27 Thiago Sousa Santos 2016-01-15 15:03:07 UTC
gst-validate has no complaints on this.

commit b7a0be23c6865286674ecac127bc4a1603437ee9
Author: Thiago Santos <thiagoss@osg.samsung.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
    actions.
    
    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
    
    https://bugzilla.gnome.org/show_bug.cgi?id=757951

commit d92f11b8196cf731881204a473f0dafd39df87c9
Author: Florin Apostol <florin.apostol@oregan.net>
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.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=757951