GNOME Bugzilla – Bug 704918
gst_base_src_negotiate() might not be called
Last modified: 2013-07-26 10:03:20 UTC
Created attachment 250183 [details] [review] Patch gst_base_src_negotiate() might not be called before pushing the first buffer in gstbasesrc.c. In gst_base_src_loop(), the buffer pool allocation is done when calling gst_base_src_negotiate(). Using GST_PAD_FLAG_NEED_RECONFIGURE as a marker to know that buffer pool allocation should be done seems not enough. Indeed, in the case of an element with fixed caps (e.g.: v4l2src), the element will unset GST_PAD_FLAG_NEED_RECONFIGURE flag calling gst_pad_check_reconfigure() when receiving a reconfigure event. This means that if a reconfigure event is received between the call of gst_pad_mark_reconfigure() in gst_base_src_start_complete() and the start of the task that will call gst_base_src_loop(), the flag might be unset and the buffer pool will not be allocated, leading to a segmentation fault. I think another condition should be added in gst_base_src_loop() to prevent such a case from happening. Checking that src->priv->pool is a not NULL pointer should make it as done in the patch in attachment.
This was fixed in git master, I'll import the patches to the 1.0 branch. *** This bug has been marked as a duplicate of bug 695981 ***
Indeed this is duplicate, but is this patch not relevant in the sense that gst_base_src_negotiate() should be called if the buffer pool is not allocated to avoid segmentation fault. There is maybe no other cases where it might happen but that's maybe safer.
Do you have other cases than v4l2 in mind ? No one else than this call to gst_pad_check_reconfigure() should be clearing the flag. We may want a g_assert(priv->pool != NULL); after this block instead. I'm afraid your proposed patch would just hide other bugs.
Yes indeed your proposition g_assert(priv->pool != NULL); is better that would directly point the problem if something is badly implemented.