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

[Jacopo Mondi]

Hi Laurent,

On Wed, Jan 23, 2019 at 12:29:15PM +0200, Laurent Pinchart wrote:
> 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 &).

Yes, thanks

>
> > +};
> > +
> >  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.

we're then going through 2 moves... Is it worth it? I agree that
forcing a move in the caller clarifies the reference is not valid
afterwards, but it also forces the caller to a very specific pattern:

        unique_ptr u = make_unique<T>()
        camera->setData(move(u))

while the caller might have the data to set not stored as unique_ptr.
Is this good in your opinion?

>
> 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().
>

setData() is good

> 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 ? :-)
>

I sincerely do not find disturbing having a member field of a Camera
class of type CameraData, seems quite natural to me.

True, those are pipeline specific data, as platform data are embedded
in a struct device... let's go with PipelineData

> 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.
>

I think this is good and I will take this in

> > +
> >  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.

Thanks
  j

>
> > +}
> > +
> >  Camera::Camera(const std::string &name)
> >  	: name_(name)
> >  {
>
> --
> Regards,
>
> Laurent Pinchart
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20190123/0ea28471/attachment.sig>