[libcamera-devel] [RFC PATCH v5] gstreamer: Added virtual functions needed to support request pads
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Fri Jun 18 19:54:33 CEST 2021
On Fri, Jun 18, 2021 at 01:40:15PM -0400, Nicolas Dufresne wrote:
> Le samedi 12 juin 2021 à 13:52 +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.
>
> I would suggest to rework the commit message, and focus on what feature this
> adds and how to use it.
>
> > Signed-off-by: Vedant Paranjape <vedantparanjape160201 at gmail.com>
>
> The code looks fine, I'll let you send the final one before adding by R-b. Note
> that when importing this patch:
>
> .git/rebase-apply/patch:58: trailing whitespace.
> return NULL;
> .git/rebase-apply/patch:60: trailing whitespace.
>
> .git/rebase-apply/patch:68: trailing whitespace.
>
> .git/rebase-apply/patch:85: trailing whitespace.
> }
>
> And also, running checkstyle.py shows some more issues:
This can be automated by using the pre- or post-commit hooks:
cp utils/hooks/post-commit .git/hooks/
or
cp utils/hooks/pre-commit .git/hooks/
Note that checkstyle.py can only provide a best effort, it's fine to
ignore some of its suggestions if there's a valid reason to do so.
> ----------------------------------------------------------------------------------------------------------
> 0e4fbbad9ffaed512593848212facffed0f4620f gstreamer: Added virtual functions needed to support request pads
> ----------------------------------------------------------------------------------------------------------
> --- src/gstreamer/gstlibcamerasrc.cpp
> +++ src/gstreamer/gstlibcamerasrc.cpp
> @@ -642,9 +641,9 @@
> self->state = state;
> }
>
> -static GstPad*
> +static GstPad *
> gst_libcamera_src_request_new_pad(GstElement *element, GstPadTemplate *templ,
> - const gchar *name, [[maybe_unused]] const GstCaps *caps)
> + const gchar *name, [[maybe_unused]] const GstCaps *caps)
> {
> GstLibcameraSrc *self = GST_LIBCAMERA_SRC(element);
> g_autoptr(GstPad) pad = NULL;
> @@ -654,44 +653,40 @@
> pad = gst_pad_new_from_template(templ, name);
> g_object_ref_sink(pad);
>
> - if (gst_element_add_pad(element, pad))
> + if (gst_element_add_pad(element, pad)) {
> + 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));
> - 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_;
> + 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)
> - {
> + 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)
> ---
> 2 potential issues detected, please review
>
>
> > ---
> > 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..1614971d 100644
> > --- a/src/gstreamer/gstlibcamerasrc.cpp
> > +++ b/src/gstreamer/gstlibcamerasrc.cpp
> > @@ -361,10 +361,12 @@ 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_intermediate = g_strdup_printf("%i%i", group_id, stream_id_num++);
> > + g_autofree gchar *stream_id = gst_pad_create_stream_id(srcpad, GST_ELEMENT(self), stream_id_intermediate);
> > 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 +642,57 @@ 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);
> > +
> > + if (gst_element_add_pad(element, pad))
> > + {
> > + 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,
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list