GNOME Bugzilla – Bug 681198
[renegotiation] Reconfigure events on complex pipelines (multiple capsfilters/converters)
Last modified: 2012-08-10 17:43:25 UTC
In camerabin with wrappercamerabinsrc, before a capture is started multiple capsfilters have its caps property set. This causes multiple reconfigures to reach the source. This is racy, if the source only starts the renegotiation after the last reconfigure, all should work. In case it starts the renegotiation before the last reconfigure (and caps are still changing), a not-negotiated might happen downstream. There are multiple issues involved here: 1) basetransform now posts not-negotiated by itself and upstream won't have a chance to work around the problem if it could. 2) There is no way of skipping the reconfigure events in case multiple parts of the pipeline need to be updated before the final and consistent state is reached. As said above, renegotiations might happen while all caps aren't set and the pipeline can stop 3) basesrc could be improved to retry when a not-negotiated is returned and there's a reconfigure flag on its pad fixing 1 and 3 are enough for my camerabin tests, but maybe 2 would be good to have for applications that know what they are doing with their pipelines.
Created attachment 220315 [details] [review] pad: add gst_pad_peek_reconfigure Add an alternative version of gst_pad_check_reconfigure that doesn't clear the reconfigure flag. Useful for increasing error resilience without duplicating the reconfigure code in pad task functions.
Created attachment 220316 [details] [review] basetransform: do not error on not-negotiated Don't error out too early and let upstream decide if it can workaround a not-negotiated problem
Created attachment 220317 [details] [review] basesrc: retry on not-negotiate if a reconfigure is pending Before erroring out on not-negotiated returns, check if the pad has the reconfigure flag set and retry.
The patches look simple and clean to me - nicely done. I haven't tested them yet.
Makes sense to me This is loosely related to another issue with reconfiguration and the base classes though. Currently the base classes look at the flag and just reconfigure themselves without the subclass ever noticing anything about it before the new caps are known.
Sounds like a good idea too me too. Wouldn't have expected _check_reconfigure() to clear the flag as well, bit counterintuitive naming imho. <bikeshed> perhaps gst_pad_needs_reconfigure() would be better than _peek_reconfigure(), to match the flag? </bikeshed> (don't really mind though :)). I also wonder if basetransform should check if a reconfiguration is pending if it got a not-negotiated from downstream, and try to reconfigure itself and re-transform the input buffer where possible (e.g. videoconvert/audioconvert). That way the 'most downstream' converter would take care of fixing things until the whole branch reconfigures later. Though this is separate from this bug I guess.
I'm a bit concerned about basesrc in effect dropping a buffer (which got a not-negotiated error). Might be a problem for pre-encoded content, etc
I'm thinking it's probably downstream elements that should keep on using the pre-reconfigure state until the source has re-configured...
> I'm thinking it's probably downstream elements that should keep on using the > pre-reconfigure state until the source has re-configured... Why ? It seems to me that continuing to use the pre-reconfigured state in a downstream element(I'm thinking converters for example) if one *could* reconfigure instead means: a) data is *guaranteed* to get dropped, because we know we'll get a not-negotiated, and we don't know how much data there is between the most upstream element and us (could be queues with seconds worth of data). b) we don't know if upstream will be able to renegotiate successfully at all, which means we might actually drop all remaining data c) we might not even ever error out then d) we don't know when upstream fails to reconfigure, so downstream doesn't know that it should try itself now But perhaps I'm missing something somewhere.
If you reconfigure, it means that you previously had a working configuration. So in that case, elements should do their best to never return a not-negotiated error. But I agree that if a not-negotiated error is unavoidable, I guess it should be returned asap, but if we want the source to auto-restart, then we should probably let the subclass know that we've dropped something so it can behave appropriately.
Those patches should go in, IMO. Now we're making the not-negotiated error non-fatal (ie. the element generating the return value does not post an error message, making it possible for upstream to retry with a different format).
Comment on attachment 220315 [details] [review] pad: add gst_pad_peek_reconfigure Pushed, changing the name of the function to gst_pad_needs_reconfigure
9285ee9184113ef9050c80f9e2648837e57f47a3 5c0e02c79c88f8dddcf9b93bd9f30d75c24ac82e 2a5afba1a1ca86c1bd476be2eec5eae23a036bb8 Pushed and fixed. Thanks for the reviews!