GNOME Bugzilla – Bug 770657
basetransform: Don't disable pool in set_allocation if it is not replaced
Last modified: 2016-10-20 14:48:45 UTC
Created attachment 334542 [details] [review] basetransform: Don't disable pool in set_allocation if it is not replaced If the pool and allocator don't change, skip disabling the pool. If the "videotestsrc ! v4l2transform ! tee ! queue ! fakesink" pipeline is running and a new "queue ! fakesink" is added to the tee, caps negotiation in reconfigure results in the same caps, but setcaps still calls do_bufferpool, which calls set_allocation with the same pool and subsequently stops the v4l2 capture pool, issuing a VIDIOC_STREAMOFF to the kernel driver. This unnecessary stream stop can be avoided if disabling an unchanged pool is skipped in set_allocation.
Comment on attachment 334542 [details] [review] basetransform: Don't disable pool in set_allocation if it is not replaced Looks good, but please do (in more commits) the same change in basesink, basesrc, videoencoder/decoder, audioencoder/decoder and probably other places that have exactly the same code
Actually, I failed to unref oldquery. Let me update the patch.
Created attachment 334604 [details] [review] basetransform: Don't disable pool in set_allocation if it is not replaced The new patch disables the old pool if it changed. It also disables an unchanged pool if the allocator or allocation params changed. Transfer of ownership into set_allocation for the pool, allocator, and query parameters is maintained.
Comment on attachment 334604 [details] [review] basetransform: Don't disable pool in set_allocation if it is not replaced Ok :) Will you make patches for the other places with the same code?
GstBaseSrc already deactivates oldpool only if the pool changed. I can prepare a patch to also deactivate oldpool if allocator or allocation params changed in gst_base_src_set_allocation. GstAudioVisualizer, GstDeinterlace, and GstGLBaseMixer have similar code to GstBaseTransform. I could add patches for those. I'm not sure if gst_dvdec_negotiate_pool in GstDVDec and gst_gdk_pixbuf_dec_setup_pool in GstGdkPixbufDec deserve the same treatment. GstBaseSink, GstVideoEncoder, GstAudioEncoder and GstAudioDecoder don't call gst_buffer_pool_set_active at all. GstVideoDecoder calls gst_buffer_pool_set_active(...,FALSE) only in gst_video_decoder_reset (called on init and state changes between READY and PAUSED).
Comment on attachment 334604 [details] [review] basetransform: Don't disable pool in set_allocation if it is not replaced Can you add patches for the other cases?
Review of attachment 334604 [details] [review]: Sorry, but this case only exist because of a bug in v4l2transform. V4L2 elements still send the same pool over multiple allocation query. This is known to break certain negotiation. Additionally, ignoring the renegotiation may lead to buffer stall as the new query would require more buffers. What should happen: ** v4l2 element should send a new buffer pool instance for every allocation query ** For this to happen, we need to remove the queue handling (streamon/off) from the buffer pool. It's really the wrong place, you want the element to keep control over that. It will also simplify the thread handling a bit. Then, a v4l2 pool becomes a simple element that wrap the allocator. At activation, the pool will ask the allocator for new requirement, and the allocator will figure-out what to do to reply. If the number of buffers is the same, it won't need to streamoff.
Let's close this then