[libcamera-devel] [RFC PATCH v4] gstreamer: Added virtual functions needed to support request pads

[Nicolas Dufresne]

Thanks for this RFC,

this is going in the right direction. Note that I'm skipping the commit message,
it looks more like notes then a commit message, but I know you will update that
by the time to drop the RFC tag.

Le samedi 12 juin 2021 à 00:49 +0530, Vedant Paranjape a écrit :
> Using request pads needs virtual functions to create new request pads and
> release the allocated pads. Added way to generate unique stream_id in
> gst_libcamera_src_task_enter(). It failed to execute when more than one
> pad was added.
> 
> This patch defines the functions gst_libcamera_src_request_new_pad() and
> gst_libcamera_src_release_pad() which handle, creating and releasing
> request pads. Also assigns these functions to their callback
> handlers in GstLibcameraSrcClass.
> 
> gst_pad_create_stream_id() failed as this gstreamer element has more than 2
> source pads, and a nullptr was passed to it's stream_id argument.
> This function fails to auto generate a stream id for scenario of more
> than one source pad present.
> 
> Used a gint which increments everytime new stream_id is requested and
> group_id to generate an unique stream_id by appending them togther as
> %i%i.
> 
> This doesn't enable request pad support in gstreamer element. This is
> one of the series of changes needed to make it work.
> 
> Signed-off-by: Vedant Paranjape <vedantparanjape160201 at gmail.com>
> ---
> In _request_new_pad and _release_pad, a GLibLock is taken on
> self->state. But since self->state is a C++ object,
> GST_OBJECT(self->state) fails and returns null, so GLibLocker throws a
> warning "invalid cast from '(null)' to 'GstObject'. This means that it
> actually fails to get lock the resource that needs to be protected,
> i.e., the srcpads_ vector.
> 
> The following changes were tested using the following test code: https://pastebin.com/gNDF1cyG
> 
>  src/gstreamer/gstlibcamerasrc.cpp | 57 ++++++++++++++++++++++++++++++-
>  1 file changed, 56 insertions(+), 1 deletion(-)
> 
> diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/gstlibcamerasrc.cpp
> index ccc61590..5bb8c46c 100644
> --- a/src/gstreamer/gstlibcamerasrc.cpp
> +++ b/src/gstreamer/gstlibcamerasrc.cpp
> @@ -361,10 +361,11 @@ gst_libcamera_src_task_enter(GstTask *task, [[maybe_unused]] GThread *thread,
>  	GST_DEBUG_OBJECT(self, "Streaming thread has started");
>  
>  	guint group_id = gst_util_group_id_next();
> +	gint stream_id_num = 0;
>  	StreamRoles roles;
>  	for (GstPad *srcpad : state->srcpads_) {
>  		/* Create stream-id and push stream-start. */
> -		g_autofree gchar *stream_id = gst_pad_create_stream_id(srcpad, GST_ELEMENT(self), nullptr);
> +		g_autofree gchar *stream_id = gst_pad_create_stream_id(srcpad, GST_ELEMENT(self), g_strdup_printf("%i%i", group_id, stream_id_num++));

This would leak the string, produced by g_strdup_printf(). I'd suggest to add a
g_autofree to store this one, it will also make your line of code shorter, which
will look nicer imho.

>  		GstEvent *event = gst_event_new_stream_start(stream_id);
>  		gst_event_set_group_id(event, group_id);
>  		gst_pad_push_event(srcpad, event);
> @@ -640,6 +641,58 @@ gst_libcamera_src_init(GstLibcameraSrc *self)
>  	self->state = state;
>  }
>  
> +static GstPad*
> +gst_libcamera_src_request_new_pad(GstElement *element, GstPadTemplate *templ,
> +					const gchar *name, [[maybe_unused]] const GstCaps *caps)
> +{
> +	GstLibcameraSrc *self = GST_LIBCAMERA_SRC(element);
> +	g_autoptr(GstPad) pad = NULL;
> +
> +	GST_DEBUG_OBJECT(self, "new request pad created");
> +
> +	pad = gst_pad_new_from_template(templ, name);
> +	g_object_ref_sink(pad);
> +	gboolean result = gst_element_add_pad(element, pad);
> +
> +	if (result)

nit, move the call to gst_element_add_pad() in the if (), and then you can drop
the local variable result (up to your taste)

> +	{
> +		GLibLocker lock(GST_OBJECT(self));
> +		self->state->srcpads_.push_back(reinterpret_cast<GstPad*>(g_object_ref(pad)));
> +	}
> +	else
> +	{
> +		GST_ELEMENT_ERROR (element, STREAM, FAILED,
> +					("Internal data stream error."),
> +					("Could not add pad to element"));
> +		return NULL;		
> +	}
> +	
> +	return reinterpret_cast<GstPad*>(g_steal_pointer(&pad));
> +}
> +
> +static void
> +gst_libcamera_src_release_pad(GstElement *element, GstPad *pad)
> +{
> +	GstLibcameraSrc *self = GST_LIBCAMERA_SRC(element);
> +	
> +	GST_DEBUG_OBJECT (self, "Pad %" GST_PTR_FORMAT " being released", pad);
> +
> +	{
> +		GLibLocker lock(GST_OBJECT(self));
> +		std::vector<GstPad*> &pads = self->state->srcpads_;
> +		auto begin_iterator = pads.begin();
> +		auto end_iterator = pads.end();
> +		auto pad_iterator = std::find(begin_iterator, end_iterator, pad);
> +
> +		if (pad_iterator != end_iterator)
> +		{
> +			g_object_unref(*pad_iterator);
> +			pads.erase(pad_iterator);
> +		}
> +	}
> +	gst_element_remove_pad(element, pad);
> +}	
> +
>  static void
>  gst_libcamera_src_class_init(GstLibcameraSrcClass *klass)
>  {
> @@ -650,6 +703,8 @@ gst_libcamera_src_class_init(GstLibcameraSrcClass *klass)
>  	object_class->get_property = gst_libcamera_src_get_property;
>  	object_class->finalize = gst_libcamera_src_finalize;
>  
> +	element_class->request_new_pad = gst_libcamera_src_request_new_pad;
> +	element_class->release_pad = gst_libcamera_src_release_pad;
>  	element_class->change_state = gst_libcamera_src_change_state;
>  
>  	gst_element_class_set_metadata(element_class,