[libcamera-devel] [PATCH 03/10] libcamera: camera: Associate cameras with their pipeline handler
Jacopo Mondi
jacopo at jmondi.org
Thu Jan 24 23:40:56 CET 2019
Hi Laurent,
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?).
Also, are we copying it? Doesn't it increase the reference counting?
> {
> }
>
> 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
>
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
-------------- 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/20190124/89baa652/attachment.sig>
More information about the libcamera-devel
mailing list