[libcamera-devel,03/10] libcamera: camera: Associate cameras with their pipeline handler
diff mbox series

Message ID 20190124101651.9993-4-laurent.pinchart@ideasonboard.com
State Accepted
Headers show
Series
  • Hotplug support and object lifetime management
Related show

Commit Message

Laurent Pinchart Jan. 24, 2019, 10:16 a.m. UTC 
From: Niklas Söderlund <niklas.soderlund@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@ragnatech.se>
Signed-off-by: Laurent Pinchart <laurent.pinchart@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(-)

Comments

Jacopo Mondi Jan. 24, 2019, 10:40 p.m. UTC | #1 
Hi Laurent,

On Thu, Jan 24, 2019 at 12:16:44PM +0200, Laurent Pinchart wrote:
> From: Niklas Söderlund <niklas.soderlund@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@ragnatech.se>
> Signed-off-by: Laurent Pinchart <laurent.pinchart@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@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
Laurent Pinchart Jan. 24, 2019, 11:33 p.m. UTC | #2 
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@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@ragnatech.se>
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@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)

Patch
diff mbox series 

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)
 {
 }
 
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)