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

[Laurent Pinchart]

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;

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

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

>  /**
>   * \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.

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

> +};
> +
> +/**
> + * \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."

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

> + *
> + * 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 ?

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