[libcamera-devel,v1] gstreamer: Factor out _pad_push_stream_start from _task_enter
diff mbox series

Message ID 20210718064850.837873-1-vedantparanjape160201@gmail.com
State Superseded
Headers show
Series
  • [libcamera-devel,v1] gstreamer: Factor out _pad_push_stream_start from _task_enter
Related show

Commit Message

Vedant Paranjape July 18, 2021, 6:48 a.m. UTC 
This patch creates gst_libcamera_pad_push_stream_start function to
create stream id and to push the stream start. This is a non functional
change.

Signed-off-by: Vedant Paranjape <vedantparanjape160201@gmail.com>
---
 src/gstreamer/gstlibcamerapad.cpp | 13 +++++++++++++
 src/gstreamer/gstlibcamerapad.h   |  2 ++
 src/gstreamer/gstlibcamerasrc.cpp |  7 +------
 3 files changed, 16 insertions(+), 6 deletions(-)

Comments

Paul Elder July 19, 2021, 7:24 a.m. UTC | #1 
Hi Vedant,

On Sun, Jul 18, 2021 at 12:18:50PM +0530, Vedant Paranjape wrote:
> This patch creates gst_libcamera_pad_push_stream_start function to
> create stream id and to push the stream start. This is a non functional
> change.
> 
> Signed-off-by: Vedant Paranjape <vedantparanjape160201@gmail.com>
> ---
>  src/gstreamer/gstlibcamerapad.cpp | 13 +++++++++++++
>  src/gstreamer/gstlibcamerapad.h   |  2 ++
>  src/gstreamer/gstlibcamerasrc.cpp |  7 +------
>  3 files changed, 16 insertions(+), 6 deletions(-)
> 
> diff --git a/src/gstreamer/gstlibcamerapad.cpp b/src/gstreamer/gstlibcamerapad.cpp
> index c00e81c8..6f889472 100644
> --- a/src/gstreamer/gstlibcamerapad.cpp
> +++ b/src/gstreamer/gstlibcamerapad.cpp
> @@ -155,6 +155,19 @@ gst_libcamera_pad_get_stream(GstPad *pad)
>  	return nullptr;
>  }
>  
> +void
> +gst_libcamera_pad_push_stream_start(GstPad *pad, const guint group_id)
> +{
> +	GstElement *element = gst_pad_get_parent_element(pad);
> +	static gint stream_id_num = 0;

If gst_libcamera_src_task_enter() is called more than once then this won't
be reset to zero, as the original behavior was.


Paul

> +
> +	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(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 79b0d867..f9ac50ef 100644
> --- a/src/gstreamer/gstlibcamerasrc.cpp
> +++ b/src/gstreamer/gstlibcamerasrc.cpp
> @@ -364,15 +364,10 @@ gst_libcamera_src_task_enter(GstTask *task, [[maybe_unused]] GThread *thread,
>  	if (state->group_id_ == 0) {
>  		state->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_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
>
Vedant Paranjape July 19, 2021, 7:26 a.m. UTC | #2 
Hi Paul,
Yes, it is expected behaviour. group_id should stay same throughout
lifetime of application.

I will remove the non-functional part from commit message.

Regards
Vedant

On Mon, 19 Jul, 2021, 12:54 , <paul.elder@ideasonboard.com> wrote:

> Hi Vedant,
>
> On Sun, Jul 18, 2021 at 12:18:50PM +0530, Vedant Paranjape wrote:
> > This patch creates gst_libcamera_pad_push_stream_start function to
> > create stream id and to push the stream start. This is a non functional
> > change.
> >
> > Signed-off-by: Vedant Paranjape <vedantparanjape160201@gmail.com>
> > ---
> >  src/gstreamer/gstlibcamerapad.cpp | 13 +++++++++++++
> >  src/gstreamer/gstlibcamerapad.h   |  2 ++
> >  src/gstreamer/gstlibcamerasrc.cpp |  7 +------
> >  3 files changed, 16 insertions(+), 6 deletions(-)
> >
> > diff --git a/src/gstreamer/gstlibcamerapad.cpp
> b/src/gstreamer/gstlibcamerapad.cpp
> > index c00e81c8..6f889472 100644
> > --- a/src/gstreamer/gstlibcamerapad.cpp
> > +++ b/src/gstreamer/gstlibcamerapad.cpp
> > @@ -155,6 +155,19 @@ gst_libcamera_pad_get_stream(GstPad *pad)
> >       return nullptr;
> >  }
> >
> > +void
> > +gst_libcamera_pad_push_stream_start(GstPad *pad, const guint group_id)
> > +{
> > +     GstElement *element = gst_pad_get_parent_element(pad);
> > +     static gint stream_id_num = 0;
>
> If gst_libcamera_src_task_enter() is called more than once then this won't
> be reset to zero, as the original behavior was.
>
>
> Paul
>
> > +
> > +     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(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 79b0d867..f9ac50ef 100644
> > --- a/src/gstreamer/gstlibcamerasrc.cpp
> > +++ b/src/gstreamer/gstlibcamerasrc.cpp
> > @@ -364,15 +364,10 @@ gst_libcamera_src_task_enter(GstTask *task,
> [[maybe_unused]] GThread *thread,
> >       if (state->group_id_ == 0) {
> >               state->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_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
> >
>
Paul Elder July 19, 2021, 9:41 a.m. UTC | #3 
Hi Vedant,

On Mon, Jul 19, 2021 at 12:56:55PM +0530, Vedant Paranjape wrote:
> Hi Paul,
> Yes, it is expected behaviour. group_id should stay same throughout lifetime of
> application.

I'm confused. I'm talking about stream_id_num. The previous behavior is
that every gst_libcamera_src_task_enter() would assign the stream_id
from 0 to state->srcpads_.size(). The new behavior is that the stream_id
would be assigned from (0 * call_count) to (state->srcpads_.size() *
call_count), where call_count is the number of times
gst_libcamera_src_task_enter() has been called. That change is
intentional?


Paul

> 
> I will remove the non-functional part from commit message.
> 
> On Mon, 19 Jul, 2021, 12:54 , <paul.elder@ideasonboard.com> wrote:
> 
>     Hi Vedant,
> 
>     On Sun, Jul 18, 2021 at 12:18:50PM +0530, Vedant Paranjape wrote:
>     > This patch creates gst_libcamera_pad_push_stream_start function to
>     > create stream id and to push the stream start. This is a non functional
>     > change.
>     >
>     > Signed-off-by: Vedant Paranjape <vedantparanjape160201@gmail.com>
>     > ---
>     >  src/gstreamer/gstlibcamerapad.cpp | 13 +++++++++++++
>     >  src/gstreamer/gstlibcamerapad.h   |  2 ++
>     >  src/gstreamer/gstlibcamerasrc.cpp |  7 +------
>     >  3 files changed, 16 insertions(+), 6 deletions(-)
>     >
>     > diff --git a/src/gstreamer/gstlibcamerapad.cpp b/src/gstreamer/
>     gstlibcamerapad.cpp
>     > index c00e81c8..6f889472 100644
>     > --- a/src/gstreamer/gstlibcamerapad.cpp
>     > +++ b/src/gstreamer/gstlibcamerapad.cpp
>     > @@ -155,6 +155,19 @@ gst_libcamera_pad_get_stream(GstPad *pad)
>     >       return nullptr;
>     >  }
>     > 
>     > +void
>     > +gst_libcamera_pad_push_stream_start(GstPad *pad, const guint group_id)
>     > +{
>     > +     GstElement *element = gst_pad_get_parent_element(pad);
>     > +     static gint stream_id_num = 0;
> 
>     If gst_libcamera_src_task_enter() is called more than once then this won't
>     be reset to zero, as the original behavior was.
> 
> 
>     Paul
> 
>     > +
>     > +     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(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 79b0d867..f9ac50ef 100644
>     > --- a/src/gstreamer/gstlibcamerasrc.cpp
>     > +++ b/src/gstreamer/gstlibcamerasrc.cpp
>     > @@ -364,15 +364,10 @@ gst_libcamera_src_task_enter(GstTask *task,
>     [[maybe_unused]] GThread *thread,
>     >       if (state->group_id_ == 0) {
>     >               state->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_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
>     >
>
Nicolas Dufresne July 19, 2021, 1:28 p.m. UTC | #4 
Le dimanche 18 juillet 2021 à 12:18 +0530, Vedant Paranjape a écrit :
> This patch creates gst_libcamera_pad_push_stream_start function to
> create stream id and to push the stream start. This is a non functional
> change.

None functional changes should not be sent separately but with other patches
making use of it.

> 
> Signed-off-by: Vedant Paranjape <vedantparanjape160201@gmail.com>
> ---
>  src/gstreamer/gstlibcamerapad.cpp | 13 +++++++++++++
>  src/gstreamer/gstlibcamerapad.h   |  2 ++
>  src/gstreamer/gstlibcamerasrc.cpp |  7 +------
>  3 files changed, 16 insertions(+), 6 deletions(-)
> 
> diff --git a/src/gstreamer/gstlibcamerapad.cpp b/src/gstreamer/gstlibcamerapad.cpp
> index c00e81c8..6f889472 100644
> --- a/src/gstreamer/gstlibcamerapad.cpp
> +++ b/src/gstreamer/gstlibcamerapad.cpp
> @@ -155,6 +155,19 @@ gst_libcamera_pad_get_stream(GstPad *pad)
>  	return nullptr;
>  }
>  
> +void
> +gst_libcamera_pad_push_stream_start(GstPad *pad, const guint group_id)
> +{
> +	GstElement *element = gst_pad_get_parent_element(pad);
> +	static gint stream_id_num = 0;
> +
> +	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(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 79b0d867..f9ac50ef 100644
> --- a/src/gstreamer/gstlibcamerasrc.cpp
> +++ b/src/gstreamer/gstlibcamerasrc.cpp
> @@ -364,15 +364,10 @@ gst_libcamera_src_task_enter(GstTask *task, [[maybe_unused]] GThread *thread,
>  	if (state->group_id_ == 0) {
>  		state->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_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));
Laurent Pinchart July 19, 2021, 3:06 p.m. UTC | #5 
On Mon, Jul 19, 2021 at 12:56:55PM +0530, Vedant Paranjape wrote:
> Hi Paul,
> Yes, it is expected behaviour. group_id should stay same throughout
> lifetime of application.
> 
> I will remove the non-functional part from commit message.

Better, please split the patch in two, with the non-functional
refactoring and the functional change in two separate patches.

> On Mon, 19 Jul, 2021, 12:54 , <paul.elder@ideasonboard.com> wrote:
> > On Sun, Jul 18, 2021 at 12:18:50PM +0530, Vedant Paranjape wrote:
> > > This patch creates gst_libcamera_pad_push_stream_start function to
> > > create stream id and to push the stream start. This is a non functional
> > > change.
> > >
> > > Signed-off-by: Vedant Paranjape <vedantparanjape160201@gmail.com>
> > > ---
> > >  src/gstreamer/gstlibcamerapad.cpp | 13 +++++++++++++
> > >  src/gstreamer/gstlibcamerapad.h   |  2 ++
> > >  src/gstreamer/gstlibcamerasrc.cpp |  7 +------
> > >  3 files changed, 16 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/src/gstreamer/gstlibcamerapad.cpp b/src/gstreamer/gstlibcamerapad.cpp
> > > index c00e81c8..6f889472 100644
> > > --- a/src/gstreamer/gstlibcamerapad.cpp
> > > +++ b/src/gstreamer/gstlibcamerapad.cpp
> > > @@ -155,6 +155,19 @@ gst_libcamera_pad_get_stream(GstPad *pad)
> > >       return nullptr;
> > >  }
> > >
> > > +void
> > > +gst_libcamera_pad_push_stream_start(GstPad *pad, const guint group_id)
> > > +{
> > > +     GstElement *element = gst_pad_get_parent_element(pad);
> > > +     static gint stream_id_num = 0;
> >
> > If gst_libcamera_src_task_enter() is called more than once then this won't
> > be reset to zero, as the original behavior was.
> >
> > > +
> > > +     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(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 79b0d867..f9ac50ef 100644
> > > --- a/src/gstreamer/gstlibcamerasrc.cpp
> > > +++ b/src/gstreamer/gstlibcamerasrc.cpp
> > > @@ -364,15 +364,10 @@ gst_libcamera_src_task_enter(GstTask *task, [[maybe_unused]] GThread *thread,
> > >       if (state->group_id_ == 0) {
> > >               state->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_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));
Vedant Paranjape July 20, 2021, 11:40 a.m. UTC | #6 
Hi Laurent,
Sure, I will divide it into 2 patches.

Regards,
Vedant Paranjape

On Mon, Jul 19, 2021 at 8:36 PM Laurent Pinchart <
laurent.pinchart@ideasonboard.com> wrote:

> On Mon, Jul 19, 2021 at 12:56:55PM +0530, Vedant Paranjape wrote:
> > Hi Paul,
> > Yes, it is expected behaviour. group_id should stay same throughout
> > lifetime of application.
> >
> > I will remove the non-functional part from commit message.
>
> Better, please split the patch in two, with the non-functional
> refactoring and the functional change in two separate patches.
>
> > On Mon, 19 Jul, 2021, 12:54 , <paul.elder@ideasonboard.com> wrote:
> > > On Sun, Jul 18, 2021 at 12:18:50PM +0530, Vedant Paranjape wrote:
> > > > This patch creates gst_libcamera_pad_push_stream_start function to
> > > > create stream id and to push the stream start. This is a non
> functional
> > > > change.
> > > >
> > > > Signed-off-by: Vedant Paranjape <vedantparanjape160201@gmail.com>
> > > > ---
> > > >  src/gstreamer/gstlibcamerapad.cpp | 13 +++++++++++++
> > > >  src/gstreamer/gstlibcamerapad.h   |  2 ++
> > > >  src/gstreamer/gstlibcamerasrc.cpp |  7 +------
> > > >  3 files changed, 16 insertions(+), 6 deletions(-)
> > > >
> > > > diff --git a/src/gstreamer/gstlibcamerapad.cpp
> b/src/gstreamer/gstlibcamerapad.cpp
> > > > index c00e81c8..6f889472 100644
> > > > --- a/src/gstreamer/gstlibcamerapad.cpp
> > > > +++ b/src/gstreamer/gstlibcamerapad.cpp
> > > > @@ -155,6 +155,19 @@ gst_libcamera_pad_get_stream(GstPad *pad)
> > > >       return nullptr;
> > > >  }
> > > >
> > > > +void
> > > > +gst_libcamera_pad_push_stream_start(GstPad *pad, const guint
> group_id)
> > > > +{
> > > > +     GstElement *element = gst_pad_get_parent_element(pad);
> > > > +     static gint stream_id_num = 0;
> > >
> > > If gst_libcamera_src_task_enter() is called more than once then this
> won't
> > > be reset to zero, as the original behavior was.
> > >
> > > > +
> > > > +     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(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 79b0d867..f9ac50ef 100644
> > > > --- a/src/gstreamer/gstlibcamerasrc.cpp
> > > > +++ b/src/gstreamer/gstlibcamerasrc.cpp
> > > > @@ -364,15 +364,10 @@ gst_libcamera_src_task_enter(GstTask *task,
> [[maybe_unused]] GThread *thread,
> > > >       if (state->group_id_ == 0) {
> > > >               state->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_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));
>
> --
> Regards,
>
> Laurent Pinchart
>

Patch
diff mbox series 

diff --git a/src/gstreamer/gstlibcamerapad.cpp b/src/gstreamer/gstlibcamerapad.cpp
index c00e81c8..6f889472 100644
--- a/src/gstreamer/gstlibcamerapad.cpp
+++ b/src/gstreamer/gstlibcamerapad.cpp
@@ -155,6 +155,19 @@  gst_libcamera_pad_get_stream(GstPad *pad)
 	return nullptr;
 }
 
+void
+gst_libcamera_pad_push_stream_start(GstPad *pad, const guint group_id)
+{
+	GstElement *element = gst_pad_get_parent_element(pad);
+	static gint stream_id_num = 0;
+
+	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(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 79b0d867..f9ac50ef 100644
--- a/src/gstreamer/gstlibcamerasrc.cpp
+++ b/src/gstreamer/gstlibcamerasrc.cpp
@@ -364,15 +364,10 @@  gst_libcamera_src_task_enter(GstTask *task, [[maybe_unused]] GThread *thread,
 	if (state->group_id_ == 0) {
 		state->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_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));