[libcamera-devel] [RFC PATCH v2] gstreamer: Factor out _pad_push_stream_start from _task_enter
Vedant Paranjape
vedantparanjape160201 at gmail.com
Mon Aug 2 09:15:50 CEST 2021
Hi Paul
On Mon, 2 Aug, 2021, 12:26 , <paul.elder at ideasonboard.com> wrote:
> Hi Vedant,
>
> On Mon, Aug 02, 2021 at 11:26:56AM +0530, Vedant Paranjape wrote:
> > Hi Paul,
> > This change is needed as the specific code is pad specific and pretty
> generic.
>
> "pad-specific" and "generic"? :D
Oops, I meant the "the pad-specific code" is used in several places and not
libcamera specific.
> As in, the code is specific to pads (so it belongs in pad and not src),
> and generic as a function for pads?
>
Yes, it should belong to the pads. Generic as in "typical gstreamer code
for pads", I meant to say in the sense that it can be reused in lot of
places again.
> > Also it might be used again in reconfig function. So to reduce code
>
> s/might/will ?
>
> > duplication, this is needed. ood?
> >
> > I will add this explaination in commit message, does it sound good to
> you?
>
> iirc you also said there was some significance in changing the stream
> id. Explaining that in the commit message would be good too.
>
Yup, the one where I said, "stream id is something unique and should stay
different for all streams during lifetime of a application", right ?
Regards
Vedant
Paul
>
> >
> > On Mon, 2 Aug, 2021, 10:16 , <paul.elder at ideasonboard.com> wrote:
> >
> > Hi Vedant,
> >
> > On Mon, Aug 02, 2021 at 12:21:44AM +0530, Vedant Paranjape wrote:
> > > This patch creates gst_libcamera_pad_push_stream_start function to
> > > create stream id and to push the stream start.
> > >
> > > Update functional code in gst_libcamera_src_task_enter(), which
> creates
> > > stream id and pushes the stream start with the refactored
> > > function gst_libcamera_pad_push_stream_start().
> >
> > The code looks better than before, but I'm not quite seeing the
> > rationale. Why is this change important? Perhaps you told me before,
> but
> > the "why" is what should be in the commit message, so that reviewers
> can
> > see. To some extent, the "what" isn't as important, since the
> reviewer
> > can simply read the patch (though it is useful when the patch is long
> > and complex).
> >
> >
> > Paul
> >
> > >
> > > Signed-off-by: Vedant Paranjape <vedantparanjape160201 at gmail.com>
> > > ---
> > > src/gstreamer/gstlibcamerapad.cpp | 19 +++++++++++++++++++
> > > src/gstreamer/gstlibcamerapad.h | 2 ++
> > > src/gstreamer/gstlibcamerasrc.cpp | 7 +------
> > > 3 files changed, 22 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/src/gstreamer/gstlibcamerapad.cpp b/src/gstreamer/
> > gstlibcamerapad.cpp
> > > index c00e81c8..d4211050 100644
> > > --- a/src/gstreamer/gstlibcamerapad.cpp
> > > +++ b/src/gstreamer/gstlibcamerapad.cpp
> > > @@ -20,6 +20,7 @@ struct _GstLibcameraPad {
> > > GstLibcameraPool *pool;
> > > GQueue pending_buffers;
> > > GstClockTime latency;
> > > + gint stream_id_num;
> > > };
> > >
> > > enum {
> > > @@ -81,6 +82,7 @@ static void
> > > gst_libcamera_pad_init(GstLibcameraPad *self)
> > > {
> > > GST_PAD_QUERYFUNC(self) = gst_libcamera_pad_query;
> > > + self->stream_id_num = 0;
> > > }
> > >
> > > static GType
> > > @@ -155,6 +157,23 @@ gst_libcamera_pad_get_stream(GstPad *pad)
> > > return nullptr;
> > > }
> > >
> > > +void
> > > +gst_libcamera_pad_push_stream_start(GstPad *pad, const guint
> group_id)
> > > +{
> > > + auto *self = GST_LIBCAMERA_PAD(pad);
> > > + {
> > > + GLibLocker lock(GST_OBJECT(self));
> > > + self->stream_id_num++;
> > > + }
> > > +
> > > + g_autoptr(GstElement) element =
> gst_pad_get_parent_element(pad);
> > > + g_autofree gchar *stream_id_intermediate =
> g_strdup_printf("%i%i",
> > group_id, self->stream_id_num);
> > > + g_autofree gchar *stream_id = gst_pad_create_stream_id(pad,
> > element, stream_id_intermediate);
> > > + GstEvent *event = gst_event_new_stream_start(stream_id);
> > > + gst_event_set_group_id(event, group_id);
> > > + gst_pad_push_event(pad, event);
> > > +}
> > > +
> > > void
> > > gst_libcamera_pad_queue_buffer(GstPad *pad, GstBuffer *buffer)
> > > {
> > > diff --git a/src/gstreamer/gstlibcamerapad.h b/src/gstreamer/
> > gstlibcamerapad.h
> > > index 779f2d13..7693374f 100644
> > > --- a/src/gstreamer/gstlibcamerapad.h
> > > +++ b/src/gstreamer/gstlibcamerapad.h
> > > @@ -26,6 +26,8 @@ void gst_libcamera_pad_set_pool(GstPad *pad,
> > GstLibcameraPool *pool);
> > >
> > > libcamera::Stream *gst_libcamera_pad_get_stream(GstPad *pad);
> > >
> > > +void gst_libcamera_pad_push_stream_start(GstPad *pad, const guint
> > group_id);
> > > +
> > > void gst_libcamera_pad_queue_buffer(GstPad *pad, GstBuffer
> *buffer);
> > >
> > > GstFlowReturn gst_libcamera_pad_push_pending(GstPad *pad);
> > > diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/
> > gstlibcamerasrc.cpp
> > > index b18f1efb..bb8ea07a 100644
> > > --- a/src/gstreamer/gstlibcamerasrc.cpp
> > > +++ b/src/gstreamer/gstlibcamerasrc.cpp
> > > @@ -361,15 +361,10 @@ gst_libcamera_src_task_enter(GstTask *task,
> > [[maybe_unused]] GThread *thread,
> > >
> > > GST_DEBUG_OBJECT(self, "Streaming thread has started");
> > >
> > > - gint stream_id_num = 0;
> > > StreamRoles roles;
> > > for (GstPad *srcpad : state->srcpads_) {
> > > /* Create stream-id and push stream-start. */
> > > - g_autofree gchar *stream_id_intermediate =
> g_strdup_printf
> > ("%i%i", state->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, state->group_id_);
> > > - gst_pad_push_event(srcpad, event);
> > > + gst_libcamera_pad_push_stream_start(srcpad, state->
> > group_id_);
> > >
> > > /* Collect the streams roles for the next iteration.
> */
> > > roles.push_back(gst_libcamera_pad_get_role(srcpad));
> > > --
> > > 2.25.1
> > >
> >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20210802/5a4911c3/attachment-0001.htm>
More information about the libcamera-devel
mailing list