[libcamera-devel] [PATCH 03/10] libcamera: camera: Associate cameras with their pipeline handler
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Fri Jan 25 00:33:59 CET 2019
Hi Jacopo,
On Thu, Jan 24, 2019 at 11:40:56PM +0100, Jacopo Mondi wrote:
> On Thu, Jan 24, 2019 at 12:16:44PM +0200, Laurent Pinchart wrote:
> > From: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> >
> > The PipelineHandler which creates a Camera is responsible for serving
> > any operation requested by the user. In order forward the public API
> > calls, the camera needs to store a reference to its pipeline handler.
> >
> > Signed-off-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> > Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > ---
> > Changes since v1:
> >
> > - Create pipeline handlers is shared pointers, make them inherit from
> > std::enable_shared_from_this<> and stored them in shared pointers.
> > ---
> > include/libcamera/camera.h | 8 ++++++--
> > include/libcamera/camera_manager.h | 2 +-
> > src/libcamera/camera.cpp | 16 ++++++++++------
> > src/libcamera/camera_manager.cpp | 17 +++++++++--------
> > src/libcamera/include/pipeline_handler.h | 9 +++++----
> > src/libcamera/pipeline/ipu3/ipu3.cpp | 2 +-
> > src/libcamera/pipeline/uvcvideo.cpp | 2 +-
> > src/libcamera/pipeline/vimc.cpp | 9 +--------
> > src/libcamera/pipeline_handler.cpp | 10 ++++++++++
> > 9 files changed, 44 insertions(+), 31 deletions(-)
> >
> > diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h
> > index 2ea1a6883311..efafb9e28c56 100644
> > --- a/include/libcamera/camera.h
> > +++ b/include/libcamera/camera.h
> > @@ -12,10 +12,13 @@
> >
> > namespace libcamera {
> >
> > +class PipelineHandler;
> > +
> > class Camera final
> > {
> > public:
> > - static std::shared_ptr<Camera> create(const std::string &name);
> > + static std::shared_ptr<Camera> create(PipelineHandler *pipe,
> > + const std::string &name);
> >
> > Camera(const Camera &) = delete;
> > void operator=(const Camera &) = delete;
> > @@ -23,9 +26,10 @@ public:
> > const std::string &name() const;
> >
> > private:
> > - explicit Camera(const std::string &name);
> > + Camera(PipelineHandler *pipe, const std::string &name);
> > ~Camera();
> >
> > + std::shared_ptr<PipelineHandler> pipe_;
> > std::string name_;
> > };
> >
> > diff --git a/include/libcamera/camera_manager.h b/include/libcamera/camera_manager.h
> > index 9ade29d76692..45e72df0ef65 100644
> > --- a/include/libcamera/camera_manager.h
> > +++ b/include/libcamera/camera_manager.h
> > @@ -41,7 +41,7 @@ private:
> > ~CameraManager();
> >
> > std::unique_ptr<DeviceEnumerator> enumerator_;
> > - std::vector<PipelineHandler *> pipes_;
> > + std::vector<std::shared_ptr<PipelineHandler>> pipes_;
> > std::vector<std::shared_ptr<Camera>> cameras_;
> >
> > std::unique_ptr<EventDispatcher> dispatcher_;
> > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp
> > index acf912bee95c..3a531c7e4d8f 100644
> > --- a/src/libcamera/camera.cpp
> > +++ b/src/libcamera/camera.cpp
> > @@ -8,6 +8,7 @@
> > #include <libcamera/camera.h>
> >
> > #include "log.h"
> > +#include "pipeline_handler.h"
> >
> > /**
> > * \file camera.h
> > @@ -52,17 +53,20 @@ namespace libcamera {
> > /**
> > * \brief Create a camera instance
> > * \param[in] name The name of the camera device
> > + * \param[in] pipe The pipeline handler responsible for the camera device
> > *
> > * The caller is responsible for guaranteeing unicity of the camera name.
> > *
> > * \return A shared pointer to the newly created camera object
> > */
> > -std::shared_ptr<Camera> Camera::create(const std::string &name)
> > +std::shared_ptr<Camera> Camera::create(PipelineHandler *pipe,
> > + const std::string &name)
> > {
> > struct Allocator : std::allocator<Camera> {
> > - void construct(void *p, const std::string &name)
> > + void construct(void *p, PipelineHandler *pipe,
> > + const std::string &name)
> > {
> > - ::new(p) Camera(name);
> > + ::new(p) Camera(pipe, name);
> > }
> > void destroy(Camera *p)
> > {
> > @@ -70,7 +74,7 @@ std::shared_ptr<Camera> Camera::create(const std::string &name)
> > }
> > };
> >
> > - return std::allocate_shared<Camera>(Allocator(), name);
> > + return std::allocate_shared<Camera>(Allocator(), pipe, name);
> > }
> >
> > /**
> > @@ -83,8 +87,8 @@ const std::string &Camera::name() const
> > return name_;
> > }
> >
> > -Camera::Camera(const std::string &name)
> > - : name_(name)
> > +Camera::Camera(PipelineHandler *pipe, const std::string &name)
> > + : pipe_(pipe->shared_from_this()), name_(name)
>
> Very clever design. Am I wrong or it only works if the pipeline
> handler is already managed by a shared_ptr<> ? (but that should be the
> case, right?).
That's correct, shared_from_this can only be used if the object instance
was created with make_shared or allocate_shared. Otherwise the results
are undefined in C++11, and in more recent standards an exception is
raised. This shouldn't be a problem as all pipeline handler instances
are created by their respective factory, using make_shared.
> Also, are we copying it? Doesn't it increase the reference counting?
We create a new reference, and that's by design, as Camera holds a
reference to the pipeline handler.
> > {
> > }
> >
> > diff --git a/src/libcamera/camera_manager.cpp b/src/libcamera/camera_manager.cpp
> > index 14410d4dcda7..3eccf20c4ce9 100644
> > --- a/src/libcamera/camera_manager.cpp
> > +++ b/src/libcamera/camera_manager.cpp
> > @@ -98,16 +98,14 @@ int CameraManager::start()
> > * all pipelines it can provide.
> > */
> > while (1) {
> > - PipelineHandler *pipe = factory->create(this);
> > - if (!pipe->match(enumerator_.get())) {
> > - delete pipe;
> > + std::shared_ptr<PipelineHandler> pipe = factory->create(this);
> > + if (!pipe->match(enumerator_.get()))
> > break;
> > - }
> >
> > LOG(Camera, Debug)
> > << "Pipeline handler \"" << factory->name()
> > << "\" matched";
> > - pipes_.push_back(pipe);
> > + pipes_.push_back(std::move(pipe));
> > }
> > }
> >
> > @@ -130,10 +128,13 @@ void CameraManager::stop()
> > {
> > /* TODO: unregister hot-plug callback here */
> >
> > - for (PipelineHandler *pipe : pipes_)
> > - delete pipe;
> > -
> > + /*
> > + * Release all references to cameras and pipeline handlers to ensure
> > + * they all get destroyed before the device enumerator deletes the
> > + * media devices.
> > + */
> > pipes_.clear();
> > + cameras_.clear();
> >
> > enumerator_.reset(nullptr);
> > }
> > diff --git a/src/libcamera/include/pipeline_handler.h b/src/libcamera/include/pipeline_handler.h
> > index 1da6dc758ca6..e1d6369eb0c4 100644
> > --- a/src/libcamera/include/pipeline_handler.h
> > +++ b/src/libcamera/include/pipeline_handler.h
> > @@ -8,6 +8,7 @@
> > #define __LIBCAMERA_PIPELINE_HANDLER_H__
> >
> > #include <map>
> > +#include <memory>
> > #include <string>
> > #include <vector>
> >
> > @@ -16,7 +17,7 @@ namespace libcamera {
> > class CameraManager;
> > class DeviceEnumerator;
> >
> > -class PipelineHandler
> > +class PipelineHandler : public std::enable_shared_from_this<PipelineHandler>
> > {
> > public:
> > PipelineHandler(CameraManager *manager);
> > @@ -34,7 +35,7 @@ public:
> > PipelineHandlerFactory(const char *name);
> > virtual ~PipelineHandlerFactory() { };
> >
> > - virtual PipelineHandler *create(CameraManager *manager) = 0;
> > + virtual std::shared_ptr<PipelineHandler> create(CameraManager *manager) = 0;
> >
> > const std::string &name() const { return name_; }
> >
> > @@ -50,9 +51,9 @@ class handler##Factory final : public PipelineHandlerFactory \
> > { \
> > public: \
> > handler##Factory() : PipelineHandlerFactory(#handler) {} \
> > - PipelineHandler *create(CameraManager *manager) \
> > + std::shared_ptr<PipelineHandler> create(CameraManager *manager) \
> > { \
> > - return new handler(manager); \
> > + return std::make_shared<handler>(manager); \
> > } \
> > }; \
> > static handler##Factory global_##handler##Factory;
> > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > index 589b3078f301..13ff7da4c99e 100644
> > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > @@ -171,7 +171,7 @@ void PipelineHandlerIPU3::registerCameras()
> > continue;
> >
> > std::string cameraName = sensor->name() + " " + std::to_string(id);
> > - std::shared_ptr<Camera> camera = Camera::create(cameraName);
> > + std::shared_ptr<Camera> camera = Camera::create(this, cameraName);
> > manager_->addCamera(std::move(camera));
> >
> > LOG(IPU3, Info)
> > diff --git a/src/libcamera/pipeline/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo.cpp
> > index 92b23da73901..3ebc074093ab 100644
> > --- a/src/libcamera/pipeline/uvcvideo.cpp
> > +++ b/src/libcamera/pipeline/uvcvideo.cpp
> > @@ -48,7 +48,7 @@ bool PipelineHandlerUVC::match(DeviceEnumerator *enumerator)
> >
> > dev_->acquire();
> >
> > - std::shared_ptr<Camera> camera = Camera::create(dev_->model());
> > + std::shared_ptr<Camera> camera = Camera::create(this, dev_->model());
> > manager_->addCamera(std::move(camera));
> >
> > return true;
> > diff --git a/src/libcamera/pipeline/vimc.cpp b/src/libcamera/pipeline/vimc.cpp
> > index f12d007cd956..68bfe9b12ab6 100644
> > --- a/src/libcamera/pipeline/vimc.cpp
> > +++ b/src/libcamera/pipeline/vimc.cpp
> > @@ -57,14 +57,7 @@ bool PipeHandlerVimc::match(DeviceEnumerator *enumerator)
> >
> > dev_->acquire();
> >
> > - /*
> > - * NOTE: A more complete Camera implementation could
> > - * be passed the MediaDevice(s) it controls here or
> > - * a reference to the PipelineHandler. Which method
> > - * will be chosen depends on how the Camera
> > - * object is modeled.
> > - */
> > - std::shared_ptr<Camera> camera = Camera::create("Dummy VIMC Camera");
> > + std::shared_ptr<Camera> camera = Camera::create(this, "Dummy VIMC Camera");
> > manager_->addCamera(std::move(camera));
> >
> > return true;
> > diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp
> > index 45788487b5c0..3850ea8fadb5 100644
> > --- a/src/libcamera/pipeline_handler.cpp
> > +++ b/src/libcamera/pipeline_handler.cpp
> > @@ -32,11 +32,21 @@ LOG_DEFINE_CATEGORY(Pipeline)
> > *
> > * The PipelineHandler matches the media devices provided by a DeviceEnumerator
> > * with the pipelines it supports and creates corresponding Camera devices.
> > + *
> > + * Pipeline handler instances are reference-counted through std::shared_ptr<>.
> > + * They implement std::enable_shared_from_this<> in order to create new
> > + * std::shared_ptr<> in code paths originating from member functions of the
> > + * PipelineHandler class where only the 'this' pointer is available.
> > */
> >
> > /**
> > * \brief Construct a PipelineHandler instance
> > * \param[in] manager The camera manager
> > + *
> > + * In order to honour the std::enable_shared_from_this<> contract,
> > + * PipelineHandler instances shall never be constructed manually, but always
> > + * through the PipelineHandlerFactory::create() method implemented by the
> > + * respective factories.
> > */
> > PipelineHandler::PipelineHandler(CameraManager *manager)
> > : manager_(manager)
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list