[libcamera-devel] [PATCH 1/3] libcamera: camera: create a association with the responsible pipeline handler

[Niklas Söderlund]

Hi Jacopo,

Thanks for your feedback.

On 2019-01-23 09:56:35 +0100, Jacopo Mondi wrote:
> Hi Niklas,
>    thanks for this, very welcome
> 
> On Wed, Jan 23, 2019 at 12:29:53AM +0100, Niklas Söderlund wrote:
> > The PipelineHandler which creates a Camera is responsible for serving
> > any operation requested by the user. In order to do so the Camera needs
> > to store a reference to its creator.
> >
> > Signed-off-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> > ---
> >  include/libcamera/camera.h           |  8 ++++++--
> >  src/libcamera/camera.cpp             | 15 +++++++++------
> >  src/libcamera/pipeline/ipu3/ipu3.cpp |  3 ++-
> >  src/libcamera/pipeline/uvcvideo.cpp  |  2 +-
> >  src/libcamera/pipeline/vimc.cpp      | 10 ++--------
> >  5 files changed, 20 insertions(+), 18 deletions(-)
> >
> > diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h
> > index 2ea1a6883311cf9f..d3bae4cbee1e0cea 100644
> > --- a/include/libcamera/camera.h
> > +++ b/include/libcamera/camera.h
> > @@ -12,10 +12,13 @@
> >
> >  namespace libcamera {
> >
> > +class PipelineHandler;
> > +
> >  class Camera final
> >  {
> >  public:
> > -	static std::shared_ptr<Camera> create(const std::string &name);
> > +	static std::shared_ptr<Camera> create(const std::string &name,
> > +					      class PipelineHandler *pipe);
> >
> >  	Camera(const Camera &) = delete;
> >  	void operator=(const Camera &) = delete;
> > @@ -23,10 +26,11 @@ public:
> >  	const std::string &name() const;
> >
> >  private:
> > -	explicit Camera(const std::string &name);
> > +	explicit Camera(const std::string &name, class PipelineHandler *pipe);
> 
> You can drop the 'explicit' here, as it avoid copy-construction and
> conversion-contruction, which can't happen with 2 parameters (if I got
> this right :)

Thanks, did not know that will fix.

> 
> I'm fine with rest, with the minor thing that I feel more natural to
> have 'this' passed as first parameter of the Camera constructor.

I agree, I will swap the arguments around!

> 
> Thanks
>   j
> 
> >  	~Camera();
> >
> >  	std::string name_;
> > +	class PipelineHandler *pipe_;
> >  };
> >
> >  } /* namespace libcamera */
> > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp
> > index acf912bee95cbec4..f198eb4978b12239 100644
> > --- a/src/libcamera/camera.cpp
> > +++ b/src/libcamera/camera.cpp
> > @@ -52,17 +52,20 @@ namespace libcamera {
> >  /**
> >   * \brief Create a camera instance
> >   * \param[in] name The name of the camera device
> > + * \param[in] pipe The pipeline handler responsible for the camera device
> >   *
> >   * The caller is responsible for guaranteeing unicity of the camera name.
> >   *
> >   * \return A shared pointer to the newly created camera object
> >   */
> > -std::shared_ptr<Camera> Camera::create(const std::string &name)
> > +std::shared_ptr<Camera> Camera::create(const std::string &name,
> > +				       class PipelineHandler *pipe)
> >  {
> >  	struct Allocator : std::allocator<Camera> {
> > -		void construct(void *p, const std::string &name)
> > +		void construct(void *p, const std::string &name,
> > +			       class PipelineHandler *pipe)
> >  		{
> > -			::new(p) Camera(name);
> > +			::new(p) Camera(name, pipe);
> >  		}
> >  		void destroy(Camera *p)
> >  		{
> > @@ -70,7 +73,7 @@ std::shared_ptr<Camera> Camera::create(const std::string &name)
> >  		}
> >  	};
> >
> > -	return std::allocate_shared<Camera>(Allocator(), name);
> > +	return std::allocate_shared<Camera>(Allocator(), name, pipe);
> >  }
> >
> >  /**
> > @@ -83,8 +86,8 @@ const std::string &Camera::name() const
> >  	return name_;
> >  }
> >
> > -Camera::Camera(const std::string &name)
> > -	: name_(name)
> > +Camera::Camera(const std::string &name, class PipelineHandler *pipe)
> > +	: name_(name), pipe_(pipe)
> >  {
> >  }
> >
> > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > index 8cbc10acfbb571fd..48d028f7e6cd9b4d 100644
> > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > @@ -171,7 +171,8 @@ void PipelineHandlerIPU3::registerCameras(CameraManager *manager)
> >  			continue;
> >
> >  		std::string cameraName = sensor->name() + " " + std::to_string(id);
> > -		std::shared_ptr<Camera> camera = Camera::create(cameraName);
> > +		std::shared_ptr<Camera> camera = Camera::create(cameraName,
> > +								this);
> >  		manager->addCamera(std::move(camera));
> >
> >  		LOG(IPU3, Info)
> > diff --git a/src/libcamera/pipeline/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo.cpp
> > index 3ba69da8b77586e3..3651250b683e7810 100644
> > --- a/src/libcamera/pipeline/uvcvideo.cpp
> > +++ b/src/libcamera/pipeline/uvcvideo.cpp
> > @@ -48,7 +48,7 @@ bool PipelineHandlerUVC::match(CameraManager *manager, DeviceEnumerator *enumera
> >
> >  	dev_->acquire();
> >
> > -	std::shared_ptr<Camera> camera = Camera::create(dev_->model());
> > +	std::shared_ptr<Camera> camera = Camera::create(dev_->model(), this);
> >  	manager->addCamera(std::move(camera));
> >
> >  	return true;
> > diff --git a/src/libcamera/pipeline/vimc.cpp b/src/libcamera/pipeline/vimc.cpp
> > index 82b9237a3d7d93e5..81d8319eb88e06d2 100644
> > --- a/src/libcamera/pipeline/vimc.cpp
> > +++ b/src/libcamera/pipeline/vimc.cpp
> > @@ -57,14 +57,8 @@ bool PipeHandlerVimc::match(CameraManager *manager, DeviceEnumerator *enumerator
> >
> >  	dev_->acquire();
> >
> > -	/*
> > -	 * NOTE: A more complete Camera implementation could
> > -	 * be passed the MediaDevice(s) it controls here or
> > -	 * a reference to the PipelineHandler. Which method
> > -	 * will be chosen depends on how the Camera
> > -	 * object is modeled.
> > -	 */
> > -	std::shared_ptr<Camera> camera = Camera::create("Dummy VIMC Camera");
> > +	std::shared_ptr<Camera> camera = Camera::create("Dummy VIMC Camera",
> > +							this);
> >  	manager->addCamera(std::move(camera));
> >
> >  	return true;
> > --
> > 2.20.1
> >
> > _______________________________________________
> > libcamera-devel mailing list
> > libcamera-devel at lists.libcamera.org
> > https://lists.libcamera.org/listinfo/libcamera-devel



-- 
Regards,
Niklas Söderlund