[libcamera-devel] [RFC 1/2] libcamera: camera: Add CameraData

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Tue, Jan 22, 2019 at 07:12:24PM +0100, Jacopo Mondi wrote:
> Add abstract base class CameraData for pipeline handlers to store
> pipeline specific data in Camera class instances.
> 
> Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> ---
>  include/libcamera/camera.h | 13 ++++++++++
>  src/libcamera/camera.cpp   | 50 ++++++++++++++++++++++++++++++++++++++
>  2 files changed, 63 insertions(+)
> 
> diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h
> index 2ea1a68..50041f1 100644
> --- a/include/libcamera/camera.h
> +++ b/include/libcamera/camera.h
> @@ -12,6 +12,15 @@
>  
>  namespace libcamera {
>  
> +class CameraData
> +{
> +public:
> +	virtual ~CameraData() { }

Missing blank line.

> +protected:
> +	CameraData() { }
> +	CameraData(CameraData &) = delete;

Note that the copy constructor is CameraData(const CameraData &). I
think you also want to delete the operator=(const CameraData &).

> +};
> +
>  class Camera final
>  {
>  public:
> @@ -22,11 +31,15 @@ public:
>  
>  	const std::string &name() const;
>  
> +	CameraData *cameraData() const { return data_.get(); }
> +	void setCameraData(CameraData *data);

You should define this as

	void setCameraData(std::unique_ptr<CameraData> data);

to show that the function takes ownership of the object.

Bikeshedding, this is pipeline-specific data tied to a camera instance,
should the function be called setCameraData or setPipelineData ? I think
the "camera" part of the name is redundant as the method is in the
Camera class, so I'd go for setPipelineData(), or even setData().

The next bikeshedding question is whether the class should be called
CameraData or PipelineData, and I'm not sure about which option is best.
Maybe CameraPipelineData to make sure everybody is happy ? :-)

We should make those two functions available to pipeline handlers only.
One option is to make them private and add the PipelineHandler class as
a friend. Classes deriving from PipelineHandler won't be able to access
them directly though, so you will need a method in PipelineHandler to do
so:

	static CameraData *cameraData(Camera *camera)
	{
		return camera->cameraData();
	}

	static void setCameraData(Camera *camera, CameraData *data)
	{
		camera->setCameraData(data);
	}

Feel free to propose a different option.

> +
>  private:
>  	explicit Camera(const std::string &name);
>  	~Camera();
>  
>  	std::string name_;
> +	std::unique_ptr<CameraData> data_;
>  };
>  
>  } /* namespace libcamera */
> diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp
> index acf912b..1e2c858 100644
> --- a/src/libcamera/camera.cpp
> +++ b/src/libcamera/camera.cpp
> @@ -33,6 +33,26 @@
>  
>  namespace libcamera {
>  
> +/**
> + * \class CameraData
> + * \brief Base class for platform specific data associated with a camera
> + *
> + * The CameraData base abstract class represents platform specific data
> + * a pipeline handler might want to associate with a Camera to access them
> + * at a later time.
> + *
> + * Pipeline handlers are expected to extend this base class with platform
> + * specific implementation, associate instances of the derived classes
> + * using the Camera::setCameraData() method, and access them at a later time
> + * with Camera::cameraData().
> + *
> + * Once associated with a camera, lifetime of derived classes instances will
> + * be tied to the one of the Camera instance itself.
> + *
> + * \sa Camera::setCameraData()
> + * \sa Camera::cameraData()
> + */
> +
>  /**
>   * \class Camera
>   * \brief Camera device
> @@ -83,6 +103,36 @@ const std::string &Camera::name() const
>  	return name_;
>  }
>  
> +/**
> + * \fn CameraData *Camera::cameraData()
> + * \brief Retrieve the pipeline specific data

s/pipeline specific/pipeline-specific/

> + *
> + * Borrow a reference to the platform specific data, associated to a camera
> + * by pipeline handlers using the setCameraData() method.

How about

 * \return A pointer to the pipeline-specific data set by
 * setCameraData(). The returned pointer is guaranteed to be valid until the
 * reference to the Camera object is released.

> + */
> +
> +/**
> + * \brief Set pipeline specific data in the camera

s/pipeline specific/pipeline-specific/

> + *
> + * Pipeline handlers might need to associate platform-specific informations to

s/platform-specific/pipeline-specific/
s/informations to/information with/

> + * camera instances, this method allows them to do so while also transferring

s/, this/. This/

> + * ownership of the \a cameraData to the Camera instance.

"allows them to do so" sounds a bit heavy. How about

 * This method allows pipeline handlers to associate pipeline-specific
 * information with the camera. Ownership of the \a data is transferred to the
 * Camera, and the data will be deleted when the camera is destroyed.
 *
 * If pipeline-specific data has already been associated with the camera by a
 * previous call to this method, is it replaced by \a data and the previous data
 * are deleted, rendering all references to them invalid.
 *
 * The data can be retrieved by pipeline handlers using the cameraData() method.

> + *
> + * Pipeline specific data are stored as unique_ptr<> to guarantee its
> + * destruction at Camera deletion time.
> + *
> + * Pipeline specific data can be accessed again by pipeline handlers by
> + * borrowing a (mutable) reference using the cameraData() method.
> + *
> + * \sa Camera::cameraData()
> + * \sa CameraData

Do we need these given that both the CameraData class and the
cameraData() function are referenced from the text, and should thus
generate html links already ?

> + */
> +void Camera::setCameraData(CameraData *data)
> +{
> +	data_ = std::unique_ptr<CameraData>(data);

	data_ = std::move(data);

after changing the function prototype to pass an std::unique_ptr<>.

> +

Unneeded blank line.

> +}
> +
>  Camera::Camera(const std::string &name)
>  	: name_(name)
>  {

-- 
Regards,

Laurent Pinchart