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

370 diff mbox series

Add class definition and methods to associate a Camera with specific data
 in the pipeline_handler base class.

Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
---
 src/libcamera/include/pipeline_handler.h | 24 +++++++-
 src/libcamera/pipeline_handler.cpp       | 73 ++++++++++++++++++++++++
 2 files changed, 96 insertions(+), 1 deletion(-)

Hi Jacopo,

Nice work!

On 2019-01-24 12:30:19 +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@jmondi.org>

Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>

> ---
>  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;
> +};
> +
>  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_;
>  };
>  
>  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>
> +
>  /**
>   * \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.
> + *
> + * 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
> + *
> + * Release the cameraData_ map, causing all data there referenced to be
> + * deleted, as they are stored as unique_ptr<CameraData>
> + */
> +PipelineHandler::~PipelineHandler()
> +{
> +	cameraData_.clear();
> +};
> +
> +/**
> + * \brief Retrieve the pipeline-specific data associated with a Camera
> + * \param camera The camera data is associate with
> + *
> + * \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.
> + */
> +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
> + * \param camera The camera to associate data to
> + * \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.
> + *
> + * 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.
> + */
> +void PipelineHandler::setCameraData(const Camera *camera,
> +				    std::unique_ptr<CameraData> data)
> +{
> +	if (cameraData_.count(camera))
> +		LOG(Pipeline, Debug)
> +			<< "Replacing data associated with "
> +			<< camera->name();
> +
> +	cameraData_[camera] = std::move(data);
> +}
> +
>  /**
>   * \class PipelineHandlerFactory
>   * \brief Registration of PipelineHandler classes and creation of instances
> -- 
> 2.20.1
> 
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel

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

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

Hi Jacopo,

On Fri, Jan 25, 2019 at 05:34:34PM +0100, Jacopo Mondi wrote:
> On Fri, Jan 25, 2019 at 05:24:25PM +0200, Laurent Pinchart wrote:
> > 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@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?

I'm not sure, it's the const there that seems weird to me, but I have no
idea why.

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

Maybe it's not, maybe my version isn't better, I don't write perfect
documentation :-)

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

I'm fine with that, but I don't expect callers to check the return
value. If callers are careful enough to add error checks (and handle
errors correctly), then I think they'll call the function correctly :-)

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