GNOME Bugzilla – Bug 733320
baseparse: Return FLOW_FLUSHING when pushing a frame on a pad that is flushing
Last modified: 2018-11-03 12:21:28 UTC
Another solution would be to set a dedictaed flag in PAUSED_TO_READY before linking up so we could detect that precise case, but that patch solves the issue and sounds correct to me.
Created attachment 281002 [details] [review] baseparse: Return FLOW_FLUSHING when pushing a frame on a pad that has been flushed When going to READY, it is possible that we are still pusing a frame but that our srcpad has already been set to flushing. In that case we should not post any error on the bus but instead cleanly return FLOW_FLUSHING.
Both that if (gst_pad_has_current_caps (pad)) check and this flushing check seem slightly weird imho.
Why do you say that? I do not think there is a check in GstPad to see if it has CAPS in its sticky event list does it?
I say that because it seems weird to me from a logic point of view, I'm not bothered about the specific method used to determine whether caps are set or not. Perhaps I don't understand the exact scenario yet where this happens. Could you provide more information? It looks to me like this has_current_caps check is basically a check for programming errors, if it is triggered the sub-class is buggy, no?
The scenario is: t1. gst_base_push_frame is called t2. the parser state is set to READY, flushing the parser src pad, thus resetting its current caps t1. gst_pad_has_current_caps is called, returning FALSE t1. BaseParse sends an ERROR message on the bus.
The test reproducing that issue is: https://jenkins.arracacha.collabora.co.uk/view/pitivi/job/gst-validate-tests/lastCompletedBuild/testReport/validate.file.playback/change_state_intensive/op1a_pal_mpeg2_mxf/
This sounds like what the negotiate methods in the audio/video decoder base classes also try to prevent.
@Sebastian You mean here: http://cgit.freedesktop.org/gstreamer/gst-plugins-base/tree/gst-libs/gst/video/gstvideodecoder.c#n2648 ? Does that solution sound correct to you or do you have any other possible approach in mind?
I think the patch is ok, at least I have no better idea right now.
Attachment 281002 [details] pushed as 4c38895 - baseparse: Return FLOW_FLUSHING when pushing a frame on a pad that has been flushed
Right, I think that's ok then too, but I'm not entirely convinced what we do in GstPad is 100% right here, clearing the caps while streaming still takes place just doesn't seem entirely right.
Yes, this does not look completely right. The caps should be removed while holding the stream lock, so that nothing is doing data flow anymore while the caps go away.
The events are removed in post_activate() in gstpad.c btw.
... while holding the stream *and* object lock. However that's the srcpad stream lock, while inside baseparse we're only having the sinkpad stream lock at that point.
Should we open another bug or reopen this one to do a deeper rework so we can totally avoid that situation to exist?
-- GitLab Migration Automatic Message -- This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity. You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/gstreamer/gstreamer/issues/65.