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