GNOME Bugzilla – Bug 618644
gst_pad_get_caps() Return pad template if parent element is in GST_STATE_NULL
Last modified: 2010-06-15 10:15:39 UTC
Right now gst_pad_get_caps() always calculates the full caps even if the element to which it belongs is in NULL, which is before any actual dataflow or negotiation needs to take place (which will then call again get_caps() through this pad). The following patch return the pad templates (if any) instead of calling the getcapsfunc when the parent is in GST_STATE_NULL. This speeds up creating pipelines (since we avoid a lot of useless caps negotiation everytime we link elements) and passes make check on core.
Created attachment 161069 [details] [review] gstpad: Return template caps in get_caps() if parent is in NULL state.
This patch would cause a pipeline like this one to fail when it starts playing instead of failing when it links making it harder to debug: audiotestsrc ! capsfilter caps="audio/x-raw-float, rate=1000" ! audiorate ! capsfilter caps="audio/x-raw-int, rate=2000" ! fakesink
agreed, when you build pipelines that fail, you won't get the error anymore at link time but at preroll. The problem is that we're talking about hundreds of milliseconds (if not whole seconds in some setups) of speed gain setting up pipelines with many elements. So it comes down to "do we prefer to have faster preroll for virtually all pipelines, or do we prefer to have early abort" ? BTW, running the pipeline you mention does come up with a pretty good hint (last line): ERROR: Pipeline doesn't want to pause. ERROR: from element /GstPipeline:pipeline0/GstAudioTestSrc:audiotestsrc0: Could not negotiate format Additional debug info: gstbasesrc.c(2757): gst_base_src_start (): /GstPipeline:pipeline0/GstAudioTestSrc:audiotestsrc0: Check your filtered caps, if any
Created attachment 161223 [details] [review] GstPad: Add GST_PAD_AWAKE GstPadFlag A pad is 'awake' when its container element is in a state greater than GST_STATE_READY
Created attachment 161224 [details] [review] gstpad: Return pad template in get_caps if pad is not awake
Looks good IMHO but the name "awake" doesn't sound right :) If this goes in the patches in bug #618853 need to be updated too to do the same in the get_caps_iterator function.
I'm open to suggestions for a better designation.
I think we need better documentation of the states a pad can be in, we already have: active, blocked, blocking and linked. "awake" could be named "negotiatable", if the effect of turning awake of is the same to get_caps, like setting blocked to push/pull.
Created attachment 162002 [details] [review] GstPad: Add GST_PAD_NEGOTIABLE GstPadFlag A pad is 'negotiable' when its container element is in a state greater than GST_STATE_READY
Created attachment 162003 [details] [review] gstpad: Return pad template in get_caps if pad is not negotiable
(In reply to comment #8) > I think we need better documentation of the states a pad can be in, we already > have: active, blocked, blocking and linked. Seems to be a bit outside of the scope of this bug though, no ? > > "awake" could be named "negotiatable", if the effect of turning awake of is the > same to get_caps, like setting blocked to push/pull. Indeed negotiable is better. Updated the patches accordingly. Any more comments ?
(In reply to comment #11) > (In reply to comment #8) > > I think we need better documentation of the states a pad can be in, we already > > have: active, blocked, blocking and linked. > > Seems to be a bit outside of the scope of this bug though, no ? > Yes, one could call it a pre-requisite. I am just a bit worried about the independent state flags for pads. It is certainly not blocking these patches. > > > > "awake" could be named "negotiatable", if the effect of turning awake of is the > > same to get_caps, like setting blocked to push/pull. > > Indeed negotiable is better. Updated the patches accordingly. > > Any more comments ?
Ok, any chance for getting this in before the next freeze in ~2 days? :)
> Ok, any chance for getting this in before the next freeze in ~2 days? :) Call me overly cautious, but this looks very much like something that shouldn't be committed 2 days before a freeze ;) (I think it will be more than 2 days before we freeze though)
Ok, I pushed it. I understand being cautious... but I've been using these patches on my 3 systems for the past month installed system-wide and with many apps and having seen any regressions/issues. If something *really* screwed up comes up ... well.. that's what freezes are for, we can always fix it before the final release. commit 7460321a600438966d7152ab2b4318be48eadce0 Author: Edward Hervey <bilboed@bilboed.com> Date: Mon May 17 15:06:37 2010 +0200 gstpad: Return pad template in get_caps if pad is not negotiable https://bugzilla.gnome.org/show_bug.cgi?id=618644 commit dc38e75d88bd8921895821f7afed01cab30e46c9 Author: Edward Hervey <bilboed@bilboed.com> Date: Mon May 17 15:04:48 2010 +0200 GstPad: Add GST_PAD_NEGOTIABLE GstPadFlag A pad is 'negotiable' when its container element is in a state greater than GST_STATE_READY API:gst_pad_is_negotiable API:gst_pad_set_negotiable API:GST_PAD_NEGOTIABLE https://bugzilla.gnome.org/show_bug.cgi?id=618644
This causes a regression for me in buzztard: In the revision before your patch (4a5552bf15924d959fe9faa0292208e98a139a07) things worked fine. I have no other local changes that could cause this. GStreamer-WARNING **: Pad list returned error on element Filter3_Reverb:tee aborting... Program received signal SIGABRT, Aborted. 0xffffe424 in __kernel_vsyscall () (gdb) bt
+ Trace 222407
There indeed seems to be an issue regarding GstGhostPad/GstProxyPad. I'm looking into this.
I can reproduce this via the camerabin test in -bad. Will investigate further.
commit 4a11063768e904c3c026e2f4fcd0fb156321ad05 Author: Edward Hervey <bilboed@bilboed.com> Date: Tue Jun 15 11:48:26 2010 +0200 Revert "GstPad: Add GST_PAD_NEGOTIABLE GstPadFlag" This reverts commit dc38e75d88bd8921895821f7afed01cab30e46c9. boom commit 3df54c45bff7f206dbf833b5c4e4aaaaac0b2f9a Author: Edward Hervey <bilboed@bilboed.com> Date: Tue Jun 15 11:48:17 2010 +0200 Revert "gstpad: Return pad template in get_caps if pad is not negotiable" This reverts commit 7460321a600438966d7152ab2b4318be48eadce0. crack commit 19334dbb85f968dd085fa8059ee7c6d7ad50b294 Author: Edward Hervey <bilboed@bilboed.com> Date: Tue Jun 15 11:48:07 2010 +0200 Revert "pad: fix comment" This reverts commit 8e92cb4a7d56cdfa4674315c64b58c1b1b9d8208. whatever... commit 27a58ed70ded862d3d8823f9f50ed7b45a6b064e Author: Edward Hervey <bilboed@bilboed.com> Date: Tue Jun 15 11:47:57 2010 +0200 Revert "element: only clear negotiable when going to NULL" This reverts commit 8f5ec1f737c3b37538b2307aef160d9d21f1c422. bleeeeh
What's exactly the problem? That ghostpads have as parent their proxypad but are still negotiatable? IMHO instead of reverting all this you should make a pad negotiable by default if it has no parent or no GstElement parent.
The problem in the end wasn't really because of ghostpads (making them negotiable by default did indeed fix that). The real problem was regarding linking. If the template of a pad is ANY (like for capsfilter)... then you could link together streams that were guaranteed never to negotiate (videotestsrc ! capsfilter which you'd link to alsasink for example). One alternative idea to solve the performance issue would be to have a gst_pad_link that doesn't do the expensive checks. Modifying the existing gst_pad_link to do that doesn't help though (we end up in the same situation as above).