[libcamera-devel] [PATCH 1/2] libcamera: pipeline_handler: Add CameraData

[Jacopo Mondi]

Hi Laurent,

On Fri, Jan 25, 2019 at 05:24:25PM +0200, Laurent Pinchart wrote:
> Hi Jacopo,
>
> Thank you for the patch.
>
> On Thu, Jan 24, 2019 at 12:30:19PM +0100, Jacopo Mondi wrote:
> > Add class definition and methods to associate a Camera with specific data
> >  in the pipeline_handler base class.
> >
> > Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> > ---
> >  src/libcamera/include/pipeline_handler.h | 24 +++++++-
> >  src/libcamera/pipeline_handler.cpp       | 73 ++++++++++++++++++++++++
> >  2 files changed, 96 insertions(+), 1 deletion(-)
> >
> > diff --git a/src/libcamera/include/pipeline_handler.h b/src/libcamera/include/pipeline_handler.h
> > index b03217d..41699a5 100644
> > --- a/src/libcamera/include/pipeline_handler.h
> > +++ b/src/libcamera/include/pipeline_handler.h
> > @@ -11,17 +11,39 @@
> >  #include <string>
> >  #include <vector>
> >
> > +#include <libcamera/camera.h>
> > +
> >  namespace libcamera {
> >
> >  class CameraManager;
> >  class DeviceEnumerator;
> >
> > +class CameraData
> > +{
> > +public:
> > +	virtual ~CameraData() {}
> > +
> > +protected:
> > +	CameraData() {}
> > +
> > +private:
> > +	CameraData(const CameraData &) = delete;
> > +	void operator=(const CameraData &) = delete;
>
> I think you meant
>
> 	CameraData &operator=(const CameraData &) = delete;
>

Thanks, fixed

> > +};
> > +
> >  class PipelineHandler
> >  {
> >  public:
> > -	virtual ~PipelineHandler() { };
> > +	virtual ~PipelineHandler();
> >
> >  	virtual bool match(CameraManager *manager, DeviceEnumerator *enumerator) = 0;
> > +
> > +protected:
> > +	CameraData *cameraData(const Camera *camera);
> > +	void setCameraData(const Camera *camera, std::unique_ptr<CameraData> data);
> > +
> > +private:
> > +	std::map<const Camera *, std::unique_ptr<CameraData>> cameraData_;
>
> I don't know why indexing on a const pointer seems a bit weird, but I
> don't see why it wouldn't work.
>

why does this bother you?

> >  };
> >
> >  class PipelineHandlerFactory
> > diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp
> > index c24feea..fb49fde 100644
> > --- a/src/libcamera/pipeline_handler.cpp
> > +++ b/src/libcamera/pipeline_handler.cpp
> > @@ -8,6 +8,8 @@
> >  #include "log.h"
> >  #include "pipeline_handler.h"
> >
> > +#include <libcamera/camera.h>
> > +
>
> Please move this above the private headers.
>

It should be dropped, it was there already.

> >  /**
> >   * \file pipeline_handler.h
> >   * \brief Create pipelines and cameras from a set of media devices
> > @@ -26,6 +28,20 @@ namespace libcamera {
> >
> >  LOG_DEFINE_CATEGORY(Pipeline)
> >
> > +/**
> > + * \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.
>
> How about starting by explaining the usage instead of what the class
> represent ?
>
> The CameraData abstract class is part of a mechanism that allows
> pipeline handlers to associate pipeline-specific data to a camera for
> their own usage.
>

I don't see any more precise indication on how the class should be
used more than in the original comment, but ok.

> > + *
> > + * Pipeline handlers are expected to extend this base class with platform
> > + * specific implementation, associate instances of the derived classes
> > + * using the setCameraData() method, and access them at a later time
> > + * with cameraData().
> > + */
> > +
> >  /**
> >   * \class PipelineHandler
> >   * \brief Create and manage cameras based on a set of media devices
> > @@ -66,6 +82,63 @@ LOG_DEFINE_CATEGORY(Pipeline)
> >   * created, or false otherwise
> >   */
> >
> > +/**
> > + * \brief Delete the pipeline handler
>
> s/Delete/Destroy/
>
> The destructor doesn't delete, it's called by the delete operation.
>
> > + *
> > + * Release the cameraData_ map, causing all data there referenced to be
> > + * deleted, as they are stored as unique_ptr<CameraData>
>
> s/$/./
>
> I think you can drop that comment, it doesn't seem very necessary.
>
> > + */
> > +PipelineHandler::~PipelineHandler()
> > +{
> > +	cameraData_.clear();
>
> This isn't needed, the cameraData_ vector is deleted, so there's no need
> to clear it first.
>

I've dropped it completely.


> > +};
> > +
> > +/**
> > + * \brief Retrieve the pipeline-specific data associated with a Camera
> > + * \param camera The camera data is associate with
>
> s/associate/associated/
>
> or
>
> "The camera whose data to retrieve"
>
> > + *
> > + * \return A pointer to the pipeline-specific data set with setCameraData().
> > + * The returned pointer lifetime is associated with the one of the pipeline
> > + * handler, and caller of this function shall never release it manually.
>
> "The returned pointer is a borrowed reference and is guaranteed to remain
> valid until the pipeline handler is destroyed. It shall not be deleted
> manually by the caller."
>
> It would be really nice if we could make the class friend of
> std::unique_ptr<CameraData> and make the destructor private :-S
>
> > + */
> > +CameraData *PipelineHandler::cameraData(const Camera *camera)
> > +{
> > +	if (!cameraData_.count(camera)) {
> > +		LOG(Pipeline, Error)
> > +			<< "Cannot get data associated with camera "
> > +			<< camera->name();
> > +		return nullptr;
> > +	}
> > +
> > +	return cameraData_[camera].get();
> > +}
> > +
> > +/**
> > + * \brief Set pipeline-specific data in the camera
>
> Maybe s/in the/for the/ ?
>
> > + * \param camera The camera to associate data to
>
> s/to$/with/
>
> > + * \param data The pipeline-specific data
> > + *
> > + * This method allows pipeline handlers to associate pipeline-specific
> > + * information with \a camera. The \a data lifetime gets associated with
> > + * the pipeline handler one, and gets released at deletion time.
>
> I would write the second sentence as just
>
> "Ownership of \a data is transferred to the PipelineHandler."
>

Taken in.


> > + *
> > + * 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.
>
> I wonder whether we should disallow this and return an error instead, as
> I don't think it's a valid use case. It would avoid potential invalid
> references problems caused by
>
> 	setCameraData(camera, std::move(data_ptr));
> 	data_ = cameraData(camera);
> 	...
>
> 	setCameraData(camera, std::move(new_data_ptr));
> 	...
> 	data_->foo = bar;	/* CRASH */
>
> I would also violate the cameraData() documentation's promise that the
> pointer stays valid as long as the pipeline handler exists.

I see. I would like to add a return value to the function, to make
sure we return success or failure to the caller.

>
> > + *
> > + * The data can be retrieved by pipeline handlers using the cameraData() method.
> > + */
> > +void PipelineHandler::setCameraData(const Camera *camera,
> > +				    std::unique_ptr<CameraData> data)
> > +{
> > +	if (cameraData_.count(camera))
>
> I'd use find() instead of count().
>
> > +		LOG(Pipeline, Debug)
>
> Maybe error instead of debug ?

I'll change this, thanks.


>
> > +			<< "Replacing data associated with "
> > +			<< camera->name();
> > +
> > +	cameraData_[camera] = std::move(data);
> > +}
> > +
> >  /**
> >   * \class PipelineHandlerFactory
> >   * \brief Registration of PipelineHandler classes and creation of instances
>
> --
> 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/20190125/568b469c/attachment.sig>