[libcamera-devel,v2,12/27] gst: libcamerasrc: Store the srcpad in a vector

2894 diff mbox series

This will allow implementing generic algorithm even if we cannot
request pads yet.

Signed-off-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 src/gstreamer/gstlibcamerasrc.cpp | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

Hi Nicolas,

Thank you for the patch.

On Thu, Feb 27, 2020 at 03:03:52PM -0500, Nicolas Dufresne wrote:
> This will allow implementing generic algorithm even if we cannot
> request pads yet.
> 
> Signed-off-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>  src/gstreamer/gstlibcamerasrc.cpp | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/gstlibcamerasrc.cpp
> index 53ece26..5a86a6d 100644
> --- a/src/gstreamer/gstlibcamerasrc.cpp
> +++ b/src/gstreamer/gstlibcamerasrc.cpp
> @@ -12,6 +12,7 @@
>  
>  #include <libcamera/camera.h>
>  #include <libcamera/camera_manager.h>
> +#include <vector>

As documented in coding-style.rst:

 # The header declaring the API being implemented (if any)
 # The C and C++ system and standard library headers
 # Other libraries' headers, with one group per library
 # Other project's headers

this should be

#include <vector>

#include <libcamera/camera.h>
#include <libcamera/camera_manager.h>

and looking at gstlibcamerasrc.cpp as a whole at the end of the series,
I would write:

#include "gstlibcamerasrc.h"

#include <queue>
#include <vector>

#include <gst/base/base.h>

#include <libcamera/camera.h>
#include <libcamera/camera_manager.h>

#include "gstlibcamera-utils.h"
#include "gstlibcameraallocator.h"
#include "gstlibcamerapad.h"
#include "gstlibcamerapool.h"

Includeing gstlibcamerasrc.h first is especially important to test that
the header is self-contained and avoid future issues. Would you be able
to give it a go through this series ?

>  using namespace libcamera;
>  
> @@ -22,6 +23,7 @@ GST_DEBUG_CATEGORY_STATIC(source_debug);
>  struct GstLibcameraSrcState {
>  	std::unique_ptr<CameraManager> cm;
>  	std::shared_ptr<Camera> cam;
> +	std::vector<GstPad *> srcpads;
>  };
>  
>  struct _GstLibcameraSrc {
> @@ -29,7 +31,6 @@ struct _GstLibcameraSrc {
>  
>  	GRecMutex stream_lock;
>  	GstTask *task;
> -	GstPad *srcpad;
>  
>  	gchar *camera_name;
>  
> @@ -262,8 +263,8 @@ gst_libcamera_src_init(GstLibcameraSrc *self)
>  	gst_task_set_leave_callback(self->task, gst_libcamera_src_task_leave, self, nullptr);
>  	gst_task_set_lock(self->task, &self->stream_lock);
>  
> -	self->srcpad = gst_pad_new_from_template(templ, "src");
> -	gst_element_add_pad(GST_ELEMENT(self), self->srcpad);
> +	state->srcpads.push_back(gst_pad_new_from_template(templ, "src"));
> +	gst_element_add_pad(GST_ELEMENT(self), state->srcpads[0]);
>  	self->state = state;
>  }
>

Le samedi 29 février 2020 à 16:02 +0200, Laurent Pinchart a écrit :
> Hi Nicolas,
> 
> Thank you for the patch.
> 
> On Thu, Feb 27, 2020 at 03:03:52PM -0500, Nicolas Dufresne wrote:
> > This will allow implementing generic algorithm even if we cannot
> > request pads yet.
> > 
> > Signed-off-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
> > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
> >  src/gstreamer/gstlibcamerasrc.cpp | 7 ++++---
> >  1 file changed, 4 insertions(+), 3 deletions(-)
> > 
> > diff --git a/src/gstreamer/gstlibcamerasrc.cpp
> > b/src/gstreamer/gstlibcamerasrc.cpp
> > index 53ece26..5a86a6d 100644
> > --- a/src/gstreamer/gstlibcamerasrc.cpp
> > +++ b/src/gstreamer/gstlibcamerasrc.cpp
> > @@ -12,6 +12,7 @@
> >  
> >  #include <libcamera/camera.h>
> >  #include <libcamera/camera_manager.h>
> > +#include <vector>
> 
> As documented in coding-style.rst:
> 
>  # The header declaring the API being implemented (if any)
>  # The C and C++ system and standard library headers
>  # Other libraries' headers, with one group per library
>  # Other project's headers
> 
> this should be
> 
> #include <vector>
> 
> #include <libcamera/camera.h>
> #include <libcamera/camera_manager.h>
> 
> and looking at gstlibcamerasrc.cpp as a whole at the end of the series,
> I would write:
> 
> #include "gstlibcamerasrc.h"
> 
> #include <queue>
> #include <vector>
> 
> #include <gst/base/base.h>
> 
> #include <libcamera/camera.h>
> #include <libcamera/camera_manager.h>
> 
> #include "gstlibcamera-utils.h"
> #include "gstlibcameraallocator.h"
> #include "gstlibcamerapad.h"
> #include "gstlibcamerapool.h"
> 
> Includeing gstlibcamerasrc.h first is especially important to test that
> the header is self-contained and avoid future issues. Would you be able
> to give it a go through this series ?

Yep, I also fixed other GstObject to follow this.

> 
> >  using namespace libcamera;
> >  
> > @@ -22,6 +23,7 @@ GST_DEBUG_CATEGORY_STATIC(source_debug);
> >  struct GstLibcameraSrcState {
> >  	std::unique_ptr<CameraManager> cm;
> >  	std::shared_ptr<Camera> cam;
> > +	std::vector<GstPad *> srcpads;
> >  };
> >  
> >  struct _GstLibcameraSrc {
> > @@ -29,7 +31,6 @@ struct _GstLibcameraSrc {
> >  
> >  	GRecMutex stream_lock;
> >  	GstTask *task;
> > -	GstPad *srcpad;
> >  
> >  	gchar *camera_name;
> >  
> > @@ -262,8 +263,8 @@ gst_libcamera_src_init(GstLibcameraSrc *self)
> >  	gst_task_set_leave_callback(self->task, gst_libcamera_src_task_leave,
> > self, nullptr);
> >  	gst_task_set_lock(self->task, &self->stream_lock);
> >  
> > -	self->srcpad = gst_pad_new_from_template(templ, "src");
> > -	gst_element_add_pad(GST_ELEMENT(self), self->srcpad);
> > +	state->srcpads.push_back(gst_pad_new_from_template(templ, "src"));
> > +	gst_element_add_pad(GST_ELEMENT(self), state->srcpads[0]);
> >  	self->state = state;
> >  }
> >