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

[Laurent Pinchart]

Hi Niklas,

Thank you for the patch.

On Wed, Jan 23, 2019 at 10:10:44AM +0100, Niklas Söderlund wrote:
> On 2019-01-23 09:56:35 +0100, Jacopo Mondi wrote:
> > 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!

I would have said the other way around, I think that the camera is first
qualified by its name, then by its pipeline handler, but it's not a big
deal :-)

> >>  	~Camera();
> >>
> >>  	std::string name_;
> >> +	class PipelineHandler *pipe_;

This screams of lifetime management issues. How about storing it as a
std::shared_ptr<>, given that it will potentially be shared by multiple
cameras, and should not be destroyed as long as the camera object keeps
a reference to the pipeline handler (otherwise the pipe->*() call
fowarding will bring painful surprises) ?

> >>  };
> >>
> >>  } /* 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;

-- 
Regards,

Laurent Pinchart