GNOME Bugzilla – Bug 748344
bufferpool: in renegotation, bufferpool can be given to element while another one still uses it
Last modified: 2018-11-03 12:27:16 UTC
Suppose the following pipeline: source ! filter ! sink Initially it works in passthrough mode so source gets a bufferpool from sink and uses it. At some point when filter gets a buffer it decides it can't do passthrough anymore (the user might have set a property to it, for example). Filter will request a bufferpool from sink. If the caps in the request are the same as source have been using the sink might return the same bufferpool. It sets the bufferpool to active (it already was active) and will use it to push a buffer. In this process filter also sent a reconfigure upstream to make source realize it isn't in passthrough anymore and that it should request a new bufferpool. At the next source loop it will do the reconfigure and as part of the acquisition of a new bufferpool, set the old one to inactive. Now filter receives the new buffer from source and tries to get a buffer from the pool to use. This same pool has been stopped by source in its renegotiation and filter will fail because its pool is now unexpectedly flushing. Adding queues to the pipeline can make this racy as well. I have an unfinished work that triggers this issue and I'll try to make it into an application so others can easily reproduce it. (Likely uploading it tomorrow)
Created attachment 302449 [details] test application Took a little longer but here is a sample code to reproduce the failure. It is very simple: videotestsrc ! videocrop ! videoscale ! capsfilter ! autovideosink, set a pad probe for caps event after videocrop src pad and from that set the same caps to capsfilter and also set some cropping in videocrop. This way the same caps will first reach videoscale and it will go on passthrough but on the next iteration videocrop will push a different caps and videoscale will need to switch to non-passthrough.
With glimagesink from git master, this isn't an issue, since it does not do pool caching any-more. This can already be solved by removing pool cache in x(v)imagesink elements. I think this was introduced to limit the amount of shared memory being allocated at the same time. Though, it has the side effect that the second element cannot negotiate the right size, and may risk a pipeline stall. I think it's safer to just drop that cache from these. Will solve most use cases. The remaining will be with element that can only have one pool (v4l2sink, v4l2videoconvert, future v4l2videoencoder and possibly OMX stuff). In this case, the most straightforward solution seems to not offer an already in use pool. This mean that the element moving from passthrough to no-passthrough, will be using a generic pool. To make this situation transitory, a mechanism could be added so the element offering the pool can detect when a pool is no longer in use upstream and suggest a reconfigure upstream. It was also propose to add an extra API on top of the pool to be able to transform ownship. This is complex and would require a lot of porting. It may break ABI also. Adding a way to transfer the ownership does not solve the issue that the newly negotiated pool may be too small for the new situation, leading to pipeline stalls.
Created attachment 304668 [details] [review] ximagesink: Don't share internal pool Sharing the internal pool results in situation where the pool may have two upstream owners. This create a race upon deactivation. Instead, always offer a new pool, and keep the internal pool internal in case we absolutely need it.
Created attachment 304669 [details] [review] xvimagesink: Don't share internal pool Sharing the internal pool results in situation where the pool may have two upstream owners. This creates a race upon deactivation. Instead, always offer a new pool, and keep the internal pool internal in case we absolutely need it.
The patches are incomplete, sorry about that. Adding the missing part now.
Created attachment 304802 [details] [review] ximagesink: Don't share internal pool Sharing the internal pool results in situation where the pool may have two upstream owners. This create a race upon deactivation. Instead, always offer a new pool, and keep the internal pool internal in case we absolutely need it.
Created attachment 304803 [details] [review] xvimagesink: Don't share internal pool Sharing the internal pool results in situation where the pool may have two upstream owners. This creates a race upon deactivation. Instead, always offer a new pool, and keep the internal pool internal in case we absolutely need it.
These are the correct patches, and works with the example.
Review of attachment 304803 [details] [review]: ::: sys/xvimage/xvimagesink.c @@ +670,3 @@ + + config = gst_buffer_pool_get_config (pool); + gst_buffer_pool_config_set_params (config, caps, size, 2, 0); Should be min instead of 2 here.
Created attachment 304806 [details] [review] xvimagesink: Don't share internal pool Sharing the internal pool results in situation where the pool may have two upstream owners. This creates a race upon deactivation. Instead, always offer a new pool, and keep the internal pool internal in case we absolutely need it.
Sor merging this since it fixes probably 90% of the cases. The remaining is when doing so with element that are bound to a single pool while active. I've looks at how the API could be change in many ways, and still unhappy with solutions. I'm starting to think the way forward is to split internal pool from external pool. So an internal pool is never directly shared, but maybe we could have some proxy pool, or in fact better allocators. Attachment 304802 [details] pushed as ae20ba7 - ximagesink: Don't share internal pool Attachment 304806 [details] pushed as df313a6 - xvimagesink: Don't share internal pool
Created attachment 305186 [details] [review] glimagesink: Don't do pool caching We now know that pool caching can cause renegotiation issues when an element in the pipeline change from passthrough to not passthrough. As it's not needed, don't cache existing pools.
Attachment 305186 [details] pushed as c72213f - glimagesink: Don't do pool caching
Another way this issue can happen is with GstBaseTransform. The base transform element never relay the allocation query directly. Instead it will cache the query upon negoaite() and return this on propose allocation. This may lead to two elements getting the same pool in the above case.
-- 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/109.