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

12577 diff mbox series

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@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(-)

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@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,