[libcamera-devel] [PATCH 1/2] libcamera: pipeline_handler: Add CameraData
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Fri Jan 25 18:01:55 CET 2019
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 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?
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
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list