[libcamera-devel,4/4] libcamera: camera: Handle camera objects through shared pointers

270 diff mbox series

The Camera class is explicitly reference-counted to manage the lifetime
of camera objects. Replace this open-coded implementation with usage of
the std::shared_ptr<> class.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 include/libcamera/camera.h         | 13 +++++----
 include/libcamera/camera_manager.h |  8 +++---
 src/libcamera/camera.cpp           | 46 +++++++++++++++++-------------
 src/libcamera/camera_manager.cpp   | 20 ++++++-------
 src/libcamera/pipeline/vimc.cpp    |  2 +-
 test/list-cameras.cpp              |  2 +-
 6 files changed, 50 insertions(+), 41 deletions(-)

Hi Laurent
   thanks, very nice :)

On Fri, Jan 18, 2019 at 01:59:16AM +0200, Laurent Pinchart wrote:
> The Camera class is explicitly reference-counted to manage the lifetime
> of camera objects. Replace this open-coded implementation with usage of
> the std::shared_ptr<> class.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>  include/libcamera/camera.h         | 13 +++++----
>  include/libcamera/camera_manager.h |  8 +++---
>  src/libcamera/camera.cpp           | 46 +++++++++++++++++-------------
>  src/libcamera/camera_manager.cpp   | 20 ++++++-------
>  src/libcamera/pipeline/vimc.cpp    |  2 +-
>  test/list-cameras.cpp              |  2 +-
>  6 files changed, 50 insertions(+), 41 deletions(-)
>
> diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h
> index 9a7579d61fa3..ef0a794e3a82 100644
> --- a/include/libcamera/camera.h
> +++ b/include/libcamera/camera.h
> @@ -7,6 +7,7 @@
>  #ifndef __LIBCAMERA_CAMERA_H__
>  #define __LIBCAMERA_CAMERA_H__
>
> +#include <memory>
>  #include <string>
>
>  namespace libcamera {
> @@ -14,15 +15,17 @@ namespace libcamera {
>  class Camera

Should we make this 'final', it cannot be extended anyhow, let's make
this a clear design choice.

>  {
>  public:
> -	Camera(const std::string &name);
> +	static std::shared_ptr<Camera> create(const std::string &name);
> +
> +	Camera(const Camera &) = delete;
> +	void operator=(const Camera &) = delete;
>
>  	const std::string &name() const;
> -	void get();
> -	void put();
>
>  private:
> -	virtual ~Camera() { };
> -	int ref_;
> +	Camera(const std::string &name);
> +	~Camera();
> +
>  	std::string name_;
>  };
>
> diff --git a/include/libcamera/camera_manager.h b/include/libcamera/camera_manager.h
> index 4b941fd9183b..738b13f4cfb1 100644
> --- a/include/libcamera/camera_manager.h
> +++ b/include/libcamera/camera_manager.h
> @@ -24,10 +24,10 @@ public:
>  	int start();
>  	void stop();
>
> -	const std::vector<Camera *> &cameras() const { return cameras_; }
> -	Camera *get(const std::string &name);
> +	const std::vector<std::shared_ptr<Camera>> &cameras() const { return cameras_; }
> +	std::shared_ptr<Camera> get(const std::string &name);

This return by value guarantees that even if the manager dies, cameras "got()"
by the applications will stay around as long as application will be alive,
right?

>
> -	void addCamera(Camera *camera);
> +	void addCamera(std::shared_ptr<Camera> &camera);
>
>  	static CameraManager *instance();
>
> @@ -42,7 +42,7 @@ private:
>
>  	std::unique_ptr<DeviceEnumerator> enumerator_;
>  	std::vector<PipelineHandler *> pipes_;
> -	std::vector<Camera *> cameras_;
> +	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 6da2b20137d4..acf912bee95c 100644
> --- a/src/libcamera/camera.cpp
> +++ b/src/libcamera/camera.cpp
> @@ -41,19 +41,36 @@ namespace libcamera {
>   * streams from a single image source. It provides the main interface to
>   * configuring and controlling the device, and capturing image streams. It is
>   * the central object exposed by libcamera.
> + *
> + * To support the central nature of Camera objects, libcamera manages the
> + * lifetime of camera instances with std::shared_ptr<>. Instances shall be
> + * created with the create() function which returns a shared pointer. The
> + * Camera constructors and destructor are private, to prevent instances from
> + * being constructed and destroyed manually.
>   */
>
>  /**
> - * \brief Construct a named camera device
> + * \brief Create a camera instance
> + * \param[in] name The name of the camera device
>   *
> - * \param[in] name The name to set on the camera device
> + * The caller is responsible for guaranteeing unicity of the camera name.
>   *
> - * The caller is responsible for guaranteeing unicity of the camera
> - * device name.
> + * \return A shared pointer to the newly created camera object
>   */
> -Camera::Camera(const std::string &name)
> -	: ref_(1), name_(name)
> +std::shared_ptr<Camera> Camera::create(const std::string &name)
>  {
> +	struct Allocator : std::allocator<Camera> {

out of curiosity, why a struct and not a class?

> +		void construct(void *p, const std::string &name)
> +		{
> +			::new(p) Camera(name);

This "new(p)" scares me :)

> +		}
> +		void destroy(Camera *p)
> +		{
> +			p->~Camera();
> +		}
> +	};
> +
> +	return std::allocate_shared<Camera>(Allocator(), name);
>  }
>
>  /**
> @@ -66,24 +83,13 @@ const std::string &Camera::name() const
>  	return name_;
>  }
>
> -/**
> - * \brief Acquire a reference to the camera
> - */
> -void Camera::get()
> +Camera::Camera(const std::string &name)
> +	: name_(name)
>  {
> -	ref_++;
>  }
>
> -/**
> - * \brief Release a reference to the camera
> - *
> - * When the last reference is released the camera device is deleted. Callers
> - * shall not access the camera device after calling this function.
> - */
> -void Camera::put()
> +Camera::~Camera()
>  {
> -	if (--ref_ == 0)
> -		delete this;
>  }
>
>  } /* namespace libcamera */
> diff --git a/src/libcamera/camera_manager.cpp b/src/libcamera/camera_manager.cpp
> index 852e5ed70fe3..d12bd42001ae 100644
> --- a/src/libcamera/camera_manager.cpp
> +++ b/src/libcamera/camera_manager.cpp
> @@ -39,9 +39,11 @@ namespace libcamera {
>   * This will enumerate all the cameras present in the system, which can then be
>   * listed with list() and retrieved with get().
>   *
> - * Cameras are reference-counted, and shall be returned to the camera manager
> - * with Camera::put() after being used. Once all cameras have been returned to
> - * the manager, it can be stopped with stop().
> + * Cameras are shared through std::shared_ptr<>, ensuring that a camera will
> + * stay valid until the last reference is released without requiring any special
> + * action from the application. Once the application has released all the
> + * references it held to cameras, the camera manager can be stopped with
> + * stop().
>   *
>   * \todo Add ability to add and remove media devices based on hot-(un)plug
>   * events coming from the device enumerator.
> @@ -147,15 +149,13 @@ void CameraManager::stop()
>   * \param[in] name Name of camera to get
>   *
>   * Before calling this function the caller is responsible for ensuring that
> - * the camera manger is running. A camera fetched this way shall be
> - * released by the user with the put() method of the Camera object once
> - * it is done using the camera.
> + * the camera manger is running.
>   *
> - * \return Pointer to Camera object or nullptr if camera not found
> + * \return Shared pointer to Camera object or nullptr if camera not found
>   */
> -Camera *CameraManager::get(const std::string &name)
> +std::shared_ptr<Camera> CameraManager::get(const std::string &name)
>  {
> -	for (Camera *camera : cameras_) {
> +	for (std::shared_ptr<Camera> camera : cameras_) {
>  		if (camera->name() == name)
>  			return camera;
>  	}
> @@ -171,7 +171,7 @@ Camera *CameraManager::get(const std::string &name)
>   * handle with the camera manager. Registered cameras are immediately made
>   * available to the system.
>   */
> -void CameraManager::addCamera(Camera *camera)
> +void CameraManager::addCamera(std::shared_ptr<Camera> &camera)
>  {
>  	cameras_.push_back(camera);
>  }
> diff --git a/src/libcamera/pipeline/vimc.cpp b/src/libcamera/pipeline/vimc.cpp
> index 8742e0bae9a8..306a5b85cd60 100644
> --- a/src/libcamera/pipeline/vimc.cpp
> +++ b/src/libcamera/pipeline/vimc.cpp
> @@ -64,7 +64,7 @@ bool PipeHandlerVimc::match(CameraManager *manager, DeviceEnumerator *enumerator
>  	 * will be chosen depends on how the Camera
>  	 * object is modeled.
>  	 */
> -	Camera *camera = new Camera("Dummy VIMC Camera");
> +	std::shared_ptr<Camera> camera = Camera::create("Dummy VIMC Camera");
>  	manager->addCamera(camera);
>
>  	return true;
> diff --git a/test/list-cameras.cpp b/test/list-cameras.cpp
> index fdbbda0957b2..070cbf2be977 100644
> --- a/test/list-cameras.cpp
> +++ b/test/list-cameras.cpp
> @@ -30,7 +30,7 @@ protected:
>  	{
>  		unsigned int count = 0;
>
> -		for (Camera *camera : cm->cameras()) {
> +		for (const std::shared_ptr<Camera> &camera : cm->cameras()) {
>  			cout << "- " << camera->name() << endl;
>  			count++;
>  		}

These are just minor comments, documentation apart, feel free to add
my tag to the whole series :)
Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>

Thanks
  j

> --
> Regards,
>
> Laurent Pinchart
>
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel

Hi Jacopo,

On Fri, Jan 18, 2019 at 04:39:56PM +0100, Jacopo Mondi wrote:
> On Fri, Jan 18, 2019 at 01:59:16AM +0200, Laurent Pinchart wrote:
> > The Camera class is explicitly reference-counted to manage the lifetime
> > of camera objects. Replace this open-coded implementation with usage of
> > the std::shared_ptr<> class.
> >
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
> >  include/libcamera/camera.h         | 13 +++++----
> >  include/libcamera/camera_manager.h |  8 +++---
> >  src/libcamera/camera.cpp           | 46 +++++++++++++++++-------------
> >  src/libcamera/camera_manager.cpp   | 20 ++++++-------
> >  src/libcamera/pipeline/vimc.cpp    |  2 +-
> >  test/list-cameras.cpp              |  2 +-
> >  6 files changed, 50 insertions(+), 41 deletions(-)
> >
> > diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h
> > index 9a7579d61fa3..ef0a794e3a82 100644
> > --- a/include/libcamera/camera.h
> > +++ b/include/libcamera/camera.h
> > @@ -7,6 +7,7 @@
> >  #ifndef __LIBCAMERA_CAMERA_H__
> >  #define __LIBCAMERA_CAMERA_H__
> >
> > +#include <memory>
> >  #include <string>
> >
> >  namespace libcamera {
> > @@ -14,15 +15,17 @@ namespace libcamera {
> >  class Camera
> 
> Should we make this 'final', it cannot be extended anyhow, let's make
> this a clear design choice.

Good point, I'll do that.

> >  {
> >  public:
> > -	Camera(const std::string &name);
> > +	static std::shared_ptr<Camera> create(const std::string &name);
> > +
> > +	Camera(const Camera &) = delete;
> > +	void operator=(const Camera &) = delete;
> >
> >  	const std::string &name() const;
> > -	void get();
> > -	void put();
> >
> >  private:
> > -	virtual ~Camera() { };
> > -	int ref_;
> > +	Camera(const std::string &name);
> > +	~Camera();
> > +
> >  	std::string name_;
> >  };
> >
> > diff --git a/include/libcamera/camera_manager.h b/include/libcamera/camera_manager.h
> > index 4b941fd9183b..738b13f4cfb1 100644
> > --- a/include/libcamera/camera_manager.h
> > +++ b/include/libcamera/camera_manager.h
> > @@ -24,10 +24,10 @@ public:
> >  	int start();
> >  	void stop();
> >
> > -	const std::vector<Camera *> &cameras() const { return cameras_; }
> > -	Camera *get(const std::string &name);
> > +	const std::vector<std::shared_ptr<Camera>> &cameras() const { return cameras_; }
> > +	std::shared_ptr<Camera> get(const std::string &name);
> 
> This return by value guarantees that even if the manager dies, cameras "got()"
> by the applications will stay around as long as application will be alive,
> right?

Correct, it returns a new shared reference, so the camera instance won't
disappear.

> >
> > -	void addCamera(Camera *camera);
> > +	void addCamera(std::shared_ptr<Camera> &camera);
> >
> >  	static CameraManager *instance();
> >
> > @@ -42,7 +42,7 @@ private:
> >
> >  	std::unique_ptr<DeviceEnumerator> enumerator_;
> >  	std::vector<PipelineHandler *> pipes_;
> > -	std::vector<Camera *> cameras_;
> > +	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 6da2b20137d4..acf912bee95c 100644
> > --- a/src/libcamera/camera.cpp
> > +++ b/src/libcamera/camera.cpp
> > @@ -41,19 +41,36 @@ namespace libcamera {
> >   * streams from a single image source. It provides the main interface to
> >   * configuring and controlling the device, and capturing image streams. It is
> >   * the central object exposed by libcamera.
> > + *
> > + * To support the central nature of Camera objects, libcamera manages the
> > + * lifetime of camera instances with std::shared_ptr<>. Instances shall be
> > + * created with the create() function which returns a shared pointer. The
> > + * Camera constructors and destructor are private, to prevent instances from
> > + * being constructed and destroyed manually.
> >   */
> >
> >  /**
> > - * \brief Construct a named camera device
> > + * \brief Create a camera instance
> > + * \param[in] name The name of the camera device
> >   *
> > - * \param[in] name The name to set on the camera device
> > + * The caller is responsible for guaranteeing unicity of the camera name.
> >   *
> > - * The caller is responsible for guaranteeing unicity of the camera
> > - * device name.
> > + * \return A shared pointer to the newly created camera object
> >   */
> > -Camera::Camera(const std::string &name)
> > -	: ref_(1), name_(name)
> > +std::shared_ptr<Camera> Camera::create(const std::string &name)
> >  {
> > +	struct Allocator : std::allocator<Camera> {
> 
> out of curiosity, why a struct and not a class?

No reason, I think I found this in sample code somewhere. Should I make
it a class ?

> > +		void construct(void *p, const std::string &name)
> > +		{
> > +			::new(p) Camera(name);
> 
> This "new(p)" scares me :)

Shhhhhh... :-)

> > +		}
> > +		void destroy(Camera *p)
> > +		{
> > +			p->~Camera();
> > +		}
> > +	};
> > +
> > +	return std::allocate_shared<Camera>(Allocator(), name);
> >  }
> >
> >  /**
> > @@ -66,24 +83,13 @@ const std::string &Camera::name() const
> >  	return name_;
> >  }
> >
> > -/**
> > - * \brief Acquire a reference to the camera
> > - */
> > -void Camera::get()
> > +Camera::Camera(const std::string &name)
> > +	: name_(name)
> >  {
> > -	ref_++;
> >  }
> >
> > -/**
> > - * \brief Release a reference to the camera
> > - *
> > - * When the last reference is released the camera device is deleted. Callers
> > - * shall not access the camera device after calling this function.
> > - */
> > -void Camera::put()
> > +Camera::~Camera()
> >  {
> > -	if (--ref_ == 0)
> > -		delete this;
> >  }
> >
> >  } /* namespace libcamera */
> > diff --git a/src/libcamera/camera_manager.cpp b/src/libcamera/camera_manager.cpp
> > index 852e5ed70fe3..d12bd42001ae 100644
> > --- a/src/libcamera/camera_manager.cpp
> > +++ b/src/libcamera/camera_manager.cpp
> > @@ -39,9 +39,11 @@ namespace libcamera {
> >   * This will enumerate all the cameras present in the system, which can then be
> >   * listed with list() and retrieved with get().
> >   *
> > - * Cameras are reference-counted, and shall be returned to the camera manager
> > - * with Camera::put() after being used. Once all cameras have been returned to
> > - * the manager, it can be stopped with stop().
> > + * Cameras are shared through std::shared_ptr<>, ensuring that a camera will
> > + * stay valid until the last reference is released without requiring any special
> > + * action from the application. Once the application has released all the
> > + * references it held to cameras, the camera manager can be stopped with
> > + * stop().
> >   *
> >   * \todo Add ability to add and remove media devices based on hot-(un)plug
> >   * events coming from the device enumerator.
> > @@ -147,15 +149,13 @@ void CameraManager::stop()
> >   * \param[in] name Name of camera to get
> >   *
> >   * Before calling this function the caller is responsible for ensuring that
> > - * the camera manger is running. A camera fetched this way shall be
> > - * released by the user with the put() method of the Camera object once
> > - * it is done using the camera.
> > + * the camera manger is running.
> >   *
> > - * \return Pointer to Camera object or nullptr if camera not found
> > + * \return Shared pointer to Camera object or nullptr if camera not found
> >   */
> > -Camera *CameraManager::get(const std::string &name)
> > +std::shared_ptr<Camera> CameraManager::get(const std::string &name)
> >  {
> > -	for (Camera *camera : cameras_) {
> > +	for (std::shared_ptr<Camera> camera : cameras_) {
> >  		if (camera->name() == name)
> >  			return camera;
> >  	}
> > @@ -171,7 +171,7 @@ Camera *CameraManager::get(const std::string &name)
> >   * handle with the camera manager. Registered cameras are immediately made
> >   * available to the system.
> >   */
> > -void CameraManager::addCamera(Camera *camera)
> > +void CameraManager::addCamera(std::shared_ptr<Camera> &camera)
> >  {
> >  	cameras_.push_back(camera);
> >  }
> > diff --git a/src/libcamera/pipeline/vimc.cpp b/src/libcamera/pipeline/vimc.cpp
> > index 8742e0bae9a8..306a5b85cd60 100644
> > --- a/src/libcamera/pipeline/vimc.cpp
> > +++ b/src/libcamera/pipeline/vimc.cpp
> > @@ -64,7 +64,7 @@ bool PipeHandlerVimc::match(CameraManager *manager, DeviceEnumerator *enumerator
> >  	 * will be chosen depends on how the Camera
> >  	 * object is modeled.
> >  	 */
> > -	Camera *camera = new Camera("Dummy VIMC Camera");
> > +	std::shared_ptr<Camera> camera = Camera::create("Dummy VIMC Camera");
> >  	manager->addCamera(camera);
> >
> >  	return true;
> > diff --git a/test/list-cameras.cpp b/test/list-cameras.cpp
> > index fdbbda0957b2..070cbf2be977 100644
> > --- a/test/list-cameras.cpp
> > +++ b/test/list-cameras.cpp
> > @@ -30,7 +30,7 @@ protected:
> >  	{
> >  		unsigned int count = 0;
> >
> > -		for (Camera *camera : cm->cameras()) {
> > +		for (const std::shared_ptr<Camera> &camera : cm->cameras()) {
> >  			cout << "- " << camera->name() << endl;
> >  			count++;
> >  		}
> 
> These are just minor comments, documentation apart, feel free to add
> my tag to the whole series :)
> Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>

Hi Laurent,

Thanks for your work.

On 2019-01-18 01:59:16 +0200, Laurent Pinchart wrote:
> The Camera class is explicitly reference-counted to manage the lifetime
> of camera objects. Replace this open-coded implementation with usage of
> the std::shared_ptr<> class.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

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

> ---
>  include/libcamera/camera.h         | 13 +++++----
>  include/libcamera/camera_manager.h |  8 +++---
>  src/libcamera/camera.cpp           | 46 +++++++++++++++++-------------
>  src/libcamera/camera_manager.cpp   | 20 ++++++-------
>  src/libcamera/pipeline/vimc.cpp    |  2 +-
>  test/list-cameras.cpp              |  2 +-
>  6 files changed, 50 insertions(+), 41 deletions(-)
> 
> diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h
> index 9a7579d61fa3..ef0a794e3a82 100644
> --- a/include/libcamera/camera.h
> +++ b/include/libcamera/camera.h
> @@ -7,6 +7,7 @@
>  #ifndef __LIBCAMERA_CAMERA_H__
>  #define __LIBCAMERA_CAMERA_H__
>  
> +#include <memory>
>  #include <string>
>  
>  namespace libcamera {
> @@ -14,15 +15,17 @@ namespace libcamera {
>  class Camera
>  {
>  public:
> -	Camera(const std::string &name);
> +	static std::shared_ptr<Camera> create(const std::string &name);
> +
> +	Camera(const Camera &) = delete;
> +	void operator=(const Camera &) = delete;
>  
>  	const std::string &name() const;
> -	void get();
> -	void put();
>  
>  private:
> -	virtual ~Camera() { };
> -	int ref_;
> +	Camera(const std::string &name);
> +	~Camera();
> +
>  	std::string name_;
>  };
>  
> diff --git a/include/libcamera/camera_manager.h b/include/libcamera/camera_manager.h
> index 4b941fd9183b..738b13f4cfb1 100644
> --- a/include/libcamera/camera_manager.h
> +++ b/include/libcamera/camera_manager.h
> @@ -24,10 +24,10 @@ public:
>  	int start();
>  	void stop();
>  
> -	const std::vector<Camera *> &cameras() const { return cameras_; }
> -	Camera *get(const std::string &name);
> +	const std::vector<std::shared_ptr<Camera>> &cameras() const { return cameras_; }
> +	std::shared_ptr<Camera> get(const std::string &name);
>  
> -	void addCamera(Camera *camera);
> +	void addCamera(std::shared_ptr<Camera> &camera);
>  
>  	static CameraManager *instance();
>  
> @@ -42,7 +42,7 @@ private:
>  
>  	std::unique_ptr<DeviceEnumerator> enumerator_;
>  	std::vector<PipelineHandler *> pipes_;
> -	std::vector<Camera *> cameras_;
> +	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 6da2b20137d4..acf912bee95c 100644
> --- a/src/libcamera/camera.cpp
> +++ b/src/libcamera/camera.cpp
> @@ -41,19 +41,36 @@ namespace libcamera {
>   * streams from a single image source. It provides the main interface to
>   * configuring and controlling the device, and capturing image streams. It is
>   * the central object exposed by libcamera.
> + *
> + * To support the central nature of Camera objects, libcamera manages the
> + * lifetime of camera instances with std::shared_ptr<>. Instances shall be
> + * created with the create() function which returns a shared pointer. The
> + * Camera constructors and destructor are private, to prevent instances from
> + * being constructed and destroyed manually.
>   */
>  
>  /**
> - * \brief Construct a named camera device
> + * \brief Create a camera instance
> + * \param[in] name The name of the camera device
>   *
> - * \param[in] name The name to set on the camera device
> + * The caller is responsible for guaranteeing unicity of the camera name.
>   *
> - * The caller is responsible for guaranteeing unicity of the camera
> - * device name.
> + * \return A shared pointer to the newly created camera object
>   */
> -Camera::Camera(const std::string &name)
> -	: ref_(1), name_(name)
> +std::shared_ptr<Camera> Camera::create(const std::string &name)
>  {
> +	struct Allocator : std::allocator<Camera> {
> +		void construct(void *p, const std::string &name)
> +		{
> +			::new(p) Camera(name);
> +		}
> +		void destroy(Camera *p)
> +		{
> +			p->~Camera();
> +		}
> +	};
> +
> +	return std::allocate_shared<Camera>(Allocator(), name);
>  }
>  
>  /**
> @@ -66,24 +83,13 @@ const std::string &Camera::name() const
>  	return name_;
>  }
>  
> -/**
> - * \brief Acquire a reference to the camera
> - */
> -void Camera::get()
> +Camera::Camera(const std::string &name)
> +	: name_(name)
>  {
> -	ref_++;
>  }
>  
> -/**
> - * \brief Release a reference to the camera
> - *
> - * When the last reference is released the camera device is deleted. Callers
> - * shall not access the camera device after calling this function.
> - */
> -void Camera::put()
> +Camera::~Camera()
>  {
> -	if (--ref_ == 0)
> -		delete this;
>  }
>  
>  } /* namespace libcamera */
> diff --git a/src/libcamera/camera_manager.cpp b/src/libcamera/camera_manager.cpp
> index 852e5ed70fe3..d12bd42001ae 100644
> --- a/src/libcamera/camera_manager.cpp
> +++ b/src/libcamera/camera_manager.cpp
> @@ -39,9 +39,11 @@ namespace libcamera {
>   * This will enumerate all the cameras present in the system, which can then be
>   * listed with list() and retrieved with get().
>   *
> - * Cameras are reference-counted, and shall be returned to the camera manager
> - * with Camera::put() after being used. Once all cameras have been returned to
> - * the manager, it can be stopped with stop().
> + * Cameras are shared through std::shared_ptr<>, ensuring that a camera will
> + * stay valid until the last reference is released without requiring any special
> + * action from the application. Once the application has released all the
> + * references it held to cameras, the camera manager can be stopped with
> + * stop().
>   *
>   * \todo Add ability to add and remove media devices based on hot-(un)plug
>   * events coming from the device enumerator.
> @@ -147,15 +149,13 @@ void CameraManager::stop()
>   * \param[in] name Name of camera to get
>   *
>   * Before calling this function the caller is responsible for ensuring that
> - * the camera manger is running. A camera fetched this way shall be
> - * released by the user with the put() method of the Camera object once
> - * it is done using the camera.
> + * the camera manger is running.
>   *
> - * \return Pointer to Camera object or nullptr if camera not found
> + * \return Shared pointer to Camera object or nullptr if camera not found
>   */
> -Camera *CameraManager::get(const std::string &name)
> +std::shared_ptr<Camera> CameraManager::get(const std::string &name)
>  {
> -	for (Camera *camera : cameras_) {
> +	for (std::shared_ptr<Camera> camera : cameras_) {
>  		if (camera->name() == name)
>  			return camera;
>  	}
> @@ -171,7 +171,7 @@ Camera *CameraManager::get(const std::string &name)
>   * handle with the camera manager. Registered cameras are immediately made
>   * available to the system.
>   */
> -void CameraManager::addCamera(Camera *camera)
> +void CameraManager::addCamera(std::shared_ptr<Camera> &camera)
>  {
>  	cameras_.push_back(camera);
>  }
> diff --git a/src/libcamera/pipeline/vimc.cpp b/src/libcamera/pipeline/vimc.cpp
> index 8742e0bae9a8..306a5b85cd60 100644
> --- a/src/libcamera/pipeline/vimc.cpp
> +++ b/src/libcamera/pipeline/vimc.cpp
> @@ -64,7 +64,7 @@ bool PipeHandlerVimc::match(CameraManager *manager, DeviceEnumerator *enumerator
>  	 * will be chosen depends on how the Camera
>  	 * object is modeled.
>  	 */
> -	Camera *camera = new Camera("Dummy VIMC Camera");
> +	std::shared_ptr<Camera> camera = Camera::create("Dummy VIMC Camera");
>  	manager->addCamera(camera);
>  
>  	return true;
> diff --git a/test/list-cameras.cpp b/test/list-cameras.cpp
> index fdbbda0957b2..070cbf2be977 100644
> --- a/test/list-cameras.cpp
> +++ b/test/list-cameras.cpp
> @@ -30,7 +30,7 @@ protected:
>  	{
>  		unsigned int count = 0;
>  
> -		for (Camera *camera : cm->cameras()) {
> +		for (const std::shared_ptr<Camera> &camera : cm->cameras()) {
>  			cout << "- " << camera->name() << endl;
>  			count++;
>  		}
> -- 
> Regards,
> 
> Laurent Pinchart
> 
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel

Hi Laurent,

On Fri, Jan 18, 2019 at 06:47:22PM +0200, Laurent Pinchart wrote:
> Hi Jacopo,
>
> On Fri, Jan 18, 2019 at 04:39:56PM +0100, Jacopo Mondi wrote:

[snip]

> > > +	struct Allocator : std::allocator<Camera> {
> >
> > out of curiosity, why a struct and not a class?
>
> No reason, I think I found this in sample code somewhere. Should I make
> it a class ?
>

Not really if there is not requirement to do so.


> > > +		void construct(void *p, const std::string &name)
> > > +		{
> > > +			::new(p) Camera(name);
> >
> > This "new(p)" scares me :)
>
> Shhhhhh... :-)
>

There will be one day where I could say "I finally got C++" and not be
prove wrong a few minutes after.

Thanks
  j

Hi Laurent,

On Fri, Jan 18, 2019 at 7:59 AM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> The Camera class is explicitly reference-counted to manage the lifetime
> of camera objects. Replace this open-coded implementation with usage of
> the std::shared_ptr<> class.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>  include/libcamera/camera.h         | 13 +++++----
>  include/libcamera/camera_manager.h |  8 +++---
>  src/libcamera/camera.cpp           | 46 +++++++++++++++++-------------
>  src/libcamera/camera_manager.cpp   | 20 ++++++-------
>  src/libcamera/pipeline/vimc.cpp    |  2 +-
>  test/list-cameras.cpp              |  2 +-
>  6 files changed, 50 insertions(+), 41 deletions(-)
>
> diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h
> index 9a7579d61fa3..ef0a794e3a82 100644
> --- a/include/libcamera/camera.h
> +++ b/include/libcamera/camera.h
> @@ -7,6 +7,7 @@
>  #ifndef __LIBCAMERA_CAMERA_H__
>  #define __LIBCAMERA_CAMERA_H__
>
> +#include <memory>
>  #include <string>
>
>  namespace libcamera {
> @@ -14,15 +15,17 @@ namespace libcamera {
>  class Camera
>  {
>  public:
> -       Camera(const std::string &name);
> +       static std::shared_ptr<Camera> create(const std::string &name);
> +
> +       Camera(const Camera &) = delete;
> +       void operator=(const Camera &) = delete;

We probably want to delete the implicit constructor as well:

  Camera() = delete;

>
>         const std::string &name() const;
> -       void get();
> -       void put();
>
>  private:
> -       virtual ~Camera() { };
> -       int ref_;
> +       Camera(const std::string &name);

Can we add the 'explicit' specifier for constructor with only one
argument? Not super important for this patch as it's a private
constructor here, but may be a good general guideline.

> +       ~Camera();
> +
>         std::string name_;
>  };
>
> diff --git a/include/libcamera/camera_manager.h b/include/libcamera/camera_manager.h
> index 4b941fd9183b..738b13f4cfb1 100644
> --- a/include/libcamera/camera_manager.h
> +++ b/include/libcamera/camera_manager.h
> @@ -24,10 +24,10 @@ public:
>         int start();
>         void stop();
>
> -       const std::vector<Camera *> &cameras() const { return cameras_; }
> -       Camera *get(const std::string &name);
> +       const std::vector<std::shared_ptr<Camera>> &cameras() const { return cameras_; }
> +       std::shared_ptr<Camera> get(const std::string &name);

Some high level design questions:

1. Who are the callers of these getter methods?
2. What's the threading model that we plan to use for exercising the
Camera objects?

While making Camera objects as shared pointers makes the coding
easier, it may also be hard to track the ownership of the objects once
the code base gets large. In some cases it may lead to memory/fd leaks
that are hard to debug, e.g. a singleton object holding a shared
pointer reference to an object and never releases the reference.

I tend to agree that fundamental objects like Camera has the necessity
to be shared pointers, but would also like to know if it's possible to
design the threading model such that CameraManager is the main owner
of the Camera objects. For example, if we can design it such that all
the access to Camera objects is serialized on a thread, then we
probably don't need to use shared pointers. Or, we can achieve
concurrency by storing Camera objects as std::shared_ptr in
CameraManager and only giving out std::weak_ptr of the Camera objects.

>
> -       void addCamera(Camera *camera);
> +       void addCamera(std::shared_ptr<Camera> &camera);

We can pass by value here so that we provide the caller with the
freedom to pass its own shared ownership to the callee through
std::move().

>
>         static CameraManager *instance();
>
> @@ -42,7 +42,7 @@ private:
>
>         std::unique_ptr<DeviceEnumerator> enumerator_;
>         std::vector<PipelineHandler *> pipes_;
> -       std::vector<Camera *> cameras_;
> +       std::vector<std::shared_ptr<Camera>> cameras_;

If the camera name is required to be unique then perhaps we can store
the cameras as

  std::unordered_map<std::string, std::shared_ptr<Camera>> cameras_;

>
>         std::unique_ptr<EventDispatcher> dispatcher_;
>  };
> diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp
> index 6da2b20137d4..acf912bee95c 100644
> --- a/src/libcamera/camera.cpp
> +++ b/src/libcamera/camera.cpp
> @@ -41,19 +41,36 @@ namespace libcamera {
>   * streams from a single image source. It provides the main interface to
>   * configuring and controlling the device, and capturing image streams. It is
>   * the central object exposed by libcamera.
> + *
> + * To support the central nature of Camera objects, libcamera manages the
> + * lifetime of camera instances with std::shared_ptr<>. Instances shall be
> + * created with the create() function which returns a shared pointer. The
> + * Camera constructors and destructor are private, to prevent instances from
> + * being constructed and destroyed manually.
>   */
>
>  /**
> - * \brief Construct a named camera device
> + * \brief Create a camera instance
> + * \param[in] name The name of the camera device
>   *
> - * \param[in] name The name to set on the camera device
> + * The caller is responsible for guaranteeing unicity of the camera name.
>   *
> - * The caller is responsible for guaranteeing unicity of the camera
> - * device name.
> + * \return A shared pointer to the newly created camera object
>   */
> -Camera::Camera(const std::string &name)
> -       : ref_(1), name_(name)
> +std::shared_ptr<Camera> Camera::create(const std::string &name)
>  {
> +       struct Allocator : std::allocator<Camera> {
> +               void construct(void *p, const std::string &name)
> +               {
> +                       ::new(p) Camera(name);
> +               }
> +               void destroy(Camera *p)
> +               {
> +                       p->~Camera();
> +               }
> +       };
> +
> +       return std::allocate_shared<Camera>(Allocator(), name);
>  }
>
>  /**
> @@ -66,24 +83,13 @@ const std::string &Camera::name() const
>         return name_;
>  }
>
> -/**
> - * \brief Acquire a reference to the camera
> - */
> -void Camera::get()
> +Camera::Camera(const std::string &name)
> +       : name_(name)
>  {
> -       ref_++;
>  }
>
> -/**
> - * \brief Release a reference to the camera
> - *
> - * When the last reference is released the camera device is deleted. Callers
> - * shall not access the camera device after calling this function.
> - */
> -void Camera::put()
> +Camera::~Camera()
>  {
> -       if (--ref_ == 0)
> -               delete this;
>  }
>
>  } /* namespace libcamera */
> diff --git a/src/libcamera/camera_manager.cpp b/src/libcamera/camera_manager.cpp
> index 852e5ed70fe3..d12bd42001ae 100644
> --- a/src/libcamera/camera_manager.cpp
> +++ b/src/libcamera/camera_manager.cpp
> @@ -39,9 +39,11 @@ namespace libcamera {
>   * This will enumerate all the cameras present in the system, which can then be
>   * listed with list() and retrieved with get().
>   *
> - * Cameras are reference-counted, and shall be returned to the camera manager
> - * with Camera::put() after being used. Once all cameras have been returned to
> - * the manager, it can be stopped with stop().
> + * Cameras are shared through std::shared_ptr<>, ensuring that a camera will
> + * stay valid until the last reference is released without requiring any special
> + * action from the application. Once the application has released all the
> + * references it held to cameras, the camera manager can be stopped with
> + * stop().
>   *
>   * \todo Add ability to add and remove media devices based on hot-(un)plug
>   * events coming from the device enumerator.
> @@ -147,15 +149,13 @@ void CameraManager::stop()
>   * \param[in] name Name of camera to get
>   *
>   * Before calling this function the caller is responsible for ensuring that
> - * the camera manger is running. A camera fetched this way shall be
> - * released by the user with the put() method of the Camera object once
> - * it is done using the camera.
> + * the camera manger is running.
>   *
> - * \return Pointer to Camera object or nullptr if camera not found
> + * \return Shared pointer to Camera object or nullptr if camera not found
>   */
> -Camera *CameraManager::get(const std::string &name)
> +std::shared_ptr<Camera> CameraManager::get(const std::string &name)
>  {
> -       for (Camera *camera : cameras_) {
> +       for (std::shared_ptr<Camera> camera : cameras_) {
>                 if (camera->name() == name)
>                         return camera;
>         }
> @@ -171,7 +171,7 @@ Camera *CameraManager::get(const std::string &name)
>   * handle with the camera manager. Registered cameras are immediately made
>   * available to the system.
>   */
> -void CameraManager::addCamera(Camera *camera)
> +void CameraManager::addCamera(std::shared_ptr<Camera> &camera)
>  {
>         cameras_.push_back(camera);
>  }
> diff --git a/src/libcamera/pipeline/vimc.cpp b/src/libcamera/pipeline/vimc.cpp
> index 8742e0bae9a8..306a5b85cd60 100644
> --- a/src/libcamera/pipeline/vimc.cpp
> +++ b/src/libcamera/pipeline/vimc.cpp
> @@ -64,7 +64,7 @@ bool PipeHandlerVimc::match(CameraManager *manager, DeviceEnumerator *enumerator
>          * will be chosen depends on how the Camera
>          * object is modeled.
>          */
> -       Camera *camera = new Camera("Dummy VIMC Camera");
> +       std::shared_ptr<Camera> camera = Camera::create("Dummy VIMC Camera");
>         manager->addCamera(camera);
>
>         return true;
> diff --git a/test/list-cameras.cpp b/test/list-cameras.cpp
> index fdbbda0957b2..070cbf2be977 100644
> --- a/test/list-cameras.cpp
> +++ b/test/list-cameras.cpp
> @@ -30,7 +30,7 @@ protected:
>         {
>                 unsigned int count = 0;
>
> -               for (Camera *camera : cm->cameras()) {
> +               for (const std::shared_ptr<Camera> &camera : cm->cameras()) {
>                         cout << "- " << camera->name() << endl;
>                         count++;
>                 }
> --
> Regards,
>
> Laurent Pinchart
>
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel

Hi Ricky,

On Sat, Jan 19, 2019 at 01:39:35AM +0800, Ricky Liang wrote:
> On Fri, Jan 18, 2019 at 7:59 AM Laurent Pinchart
> <laurent.pinchart@ideasonboard.com> wrote:
> >
> > The Camera class is explicitly reference-counted to manage the lifetime
> > of camera objects. Replace this open-coded implementation with usage of
> > the std::shared_ptr<> class.
> >
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
> >  include/libcamera/camera.h         | 13 +++++----
> >  include/libcamera/camera_manager.h |  8 +++---
> >  src/libcamera/camera.cpp           | 46 +++++++++++++++++-------------
> >  src/libcamera/camera_manager.cpp   | 20 ++++++-------
> >  src/libcamera/pipeline/vimc.cpp    |  2 +-
> >  test/list-cameras.cpp              |  2 +-
> >  6 files changed, 50 insertions(+), 41 deletions(-)
> >
> > diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h
> > index 9a7579d61fa3..ef0a794e3a82 100644
> > --- a/include/libcamera/camera.h
> > +++ b/include/libcamera/camera.h
> > @@ -7,6 +7,7 @@
> >  #ifndef __LIBCAMERA_CAMERA_H__
> >  #define __LIBCAMERA_CAMERA_H__
> >
> > +#include <memory>
> >  #include <string>
> >
> >  namespace libcamera {
> > @@ -14,15 +15,17 @@ namespace libcamera {
> >  class Camera
> >  {
> >  public:
> > -       Camera(const std::string &name);
> > +       static std::shared_ptr<Camera> create(const std::string &name);
> > +
> > +       Camera(const Camera &) = delete;
> > +       void operator=(const Camera &) = delete;
> 
> We probably want to delete the implicit constructor as well:
> 
>   Camera() = delete;

The compiler only creates an implicit constructor when no explicit
constructor is specified. As we define Camera(const std::string &name),
there's no need to delete the implicit constructor.

> >
> >         const std::string &name() const;
> > -       void get();
> > -       void put();
> >
> >  private:
> > -       virtual ~Camera() { };
> > -       int ref_;
> > +       Camera(const std::string &name);
> 
> Can we add the 'explicit' specifier for constructor with only one
> argument? Not super important for this patch as it's a private
> constructor here, but may be a good general guideline.

Good point, I'll do that.

> > +       ~Camera();
> > +
> >         std::string name_;
> >  };
> >
> > diff --git a/include/libcamera/camera_manager.h b/include/libcamera/camera_manager.h
> > index 4b941fd9183b..738b13f4cfb1 100644
> > --- a/include/libcamera/camera_manager.h
> > +++ b/include/libcamera/camera_manager.h
> > @@ -24,10 +24,10 @@ public:
> >         int start();
> >         void stop();
> >
> > -       const std::vector<Camera *> &cameras() const { return cameras_; }
> > -       Camera *get(const std::string &name);
> > +       const std::vector<std::shared_ptr<Camera>> &cameras() const { return cameras_; }
> > +       std::shared_ptr<Camera> get(const std::string &name);
> 
> Some high level design questions:
> 
> 1. Who are the callers of these getter methods?

These methods are meant to be called by applications. I expect the API
to change as we continue developing though, but the general idea will
stay the same.

> 2. What's the threading model that we plan to use for exercising the
> Camera objects?

We don't support multiple threads yet. This will be fixed down the road,
but not all details have been decided yet.

> While making Camera objects as shared pointers makes the coding
> easier, it may also be hard to track the ownership of the objects once
> the code base gets large. In some cases it may lead to memory/fd leaks
> that are hard to debug, e.g. a singleton object holding a shared
> pointer reference to an object and never releases the reference.
> 
> I tend to agree that fundamental objects like Camera has the necessity
> to be shared pointers, but would also like to know if it's possible to
> design the threading model such that CameraManager is the main owner
> of the Camera objects. For example, if we can design it such that all
> the access to Camera objects is serialized on a thread,

I think they will, possibly with the exception of request queuing and
request completion handling. This remains to be decided. In any case, I
don't think we should support camera lookup from multiple threads.

> then we probably don't need to use shared pointers. Or, we can achieve
> concurrency by storing Camera objects as std::shared_ptr in
> CameraManager and only giving out std::weak_ptr of the Camera objects.

It's not just a concurrency issue. Support for hot-pluggable cameras
will be added in the future, and we'll need to track users of camera
objects in order to delete the camera only when no users are left. This
doesn't require std::shared_ptr<>, a manual reference count would work
too (and it exists today, this patch is replacing it), but I think it's
better to use std::shared_ptr<> rather than reinventing (and
open-coding) the wheel.

Giving out weak pointers would I think cause more issues than it would
solve, as any user of a camera instance would need to be prepared for
the pointer becoming null at any time when the camera is unplugged.

> > -       void addCamera(Camera *camera);
> > +       void addCamera(std::shared_ptr<Camera> &camera);
> 
> We can pass by value here so that we provide the caller with the
> freedom to pass its own shared ownership to the callee through
> std::move().

In the typical use case the caller will likely retain a shared pointer
to the camera. I can pass by value and use std::move() in the
addCamera() function.

> >
> >         static CameraManager *instance();
> >
> > @@ -42,7 +42,7 @@ private:
> >
> >         std::unique_ptr<DeviceEnumerator> enumerator_;
> >         std::vector<PipelineHandler *> pipes_;
> > -       std::vector<Camera *> cameras_;
> > +       std::vector<std::shared_ptr<Camera>> cameras_;
> 
> If the camera name is required to be unique then perhaps we can store
> the cameras as
> 
>   std::unordered_map<std::string, std::shared_ptr<Camera>> cameras_;

We used to store them in a std::map<>, but it make it cumbersome to
retrieve the list of all cameras. As I expect the camera lookup API to
change anyway I think we can revisit this later.

> >
> >         std::unique_ptr<EventDispatcher> dispatcher_;
> >  };

Hi Laurent,

On Sat, Jan 19, 2019 at 6:51 AM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi Ricky,
>
> On Sat, Jan 19, 2019 at 01:39:35AM +0800, Ricky Liang wrote:
> > On Fri, Jan 18, 2019 at 7:59 AM Laurent Pinchart
> > <laurent.pinchart@ideasonboard.com> wrote:
> > >
> > > The Camera class is explicitly reference-counted to manage the lifetime
> > > of camera objects. Replace this open-coded implementation with usage of
> > > the std::shared_ptr<> class.
> > >
> > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > ---
> > >  include/libcamera/camera.h         | 13 +++++----
> > >  include/libcamera/camera_manager.h |  8 +++---
> > >  src/libcamera/camera.cpp           | 46 +++++++++++++++++-------------
> > >  src/libcamera/camera_manager.cpp   | 20 ++++++-------
> > >  src/libcamera/pipeline/vimc.cpp    |  2 +-
> > >  test/list-cameras.cpp              |  2 +-
> > >  6 files changed, 50 insertions(+), 41 deletions(-)
> > >
> > > diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h
> > > index 9a7579d61fa3..ef0a794e3a82 100644
> > > --- a/include/libcamera/camera.h
> > > +++ b/include/libcamera/camera.h
> > > @@ -7,6 +7,7 @@
> > >  #ifndef __LIBCAMERA_CAMERA_H__
> > >  #define __LIBCAMERA_CAMERA_H__
> > >
> > > +#include <memory>
> > >  #include <string>
> > >
> > >  namespace libcamera {
> > > @@ -14,15 +15,17 @@ namespace libcamera {
> > >  class Camera
> > >  {
> > >  public:
> > > -       Camera(const std::string &name);
> > > +       static std::shared_ptr<Camera> create(const std::string &name);
> > > +
> > > +       Camera(const Camera &) = delete;
> > > +       void operator=(const Camera &) = delete;
> >
> > We probably want to delete the implicit constructor as well:
> >
> >   Camera() = delete;
>
> The compiler only creates an implicit constructor when no explicit
> constructor is specified. As we define Camera(const std::string &name),
> there's no need to delete the implicit constructor.

Ack.

>
> > >
> > >         const std::string &name() const;
> > > -       void get();
> > > -       void put();
> > >
> > >  private:
> > > -       virtual ~Camera() { };
> > > -       int ref_;
> > > +       Camera(const std::string &name);
> >
> > Can we add the 'explicit' specifier for constructor with only one
> > argument? Not super important for this patch as it's a private
> > constructor here, but may be a good general guideline.
>
> Good point, I'll do that.
>
> > > +       ~Camera();
> > > +
> > >         std::string name_;
> > >  };
> > >
> > > diff --git a/include/libcamera/camera_manager.h b/include/libcamera/camera_manager.h
> > > index 4b941fd9183b..738b13f4cfb1 100644
> > > --- a/include/libcamera/camera_manager.h
> > > +++ b/include/libcamera/camera_manager.h
> > > @@ -24,10 +24,10 @@ public:
> > >         int start();
> > >         void stop();
> > >
> > > -       const std::vector<Camera *> &cameras() const { return cameras_; }
> > > -       Camera *get(const std::string &name);
> > > +       const std::vector<std::shared_ptr<Camera>> &cameras() const { return cameras_; }
> > > +       std::shared_ptr<Camera> get(const std::string &name);
> >
> > Some high level design questions:
> >
> > 1. Who are the callers of these getter methods?
>
> These methods are meant to be called by applications. I expect the API
> to change as we continue developing though, but the general idea will
> stay the same.
>
> > 2. What's the threading model that we plan to use for exercising the
> > Camera objects?
>
> We don't support multiple threads yet. This will be fixed down the road,
> but not all details have been decided yet.
>
> > While making Camera objects as shared pointers makes the coding
> > easier, it may also be hard to track the ownership of the objects once
> > the code base gets large. In some cases it may lead to memory/fd leaks
> > that are hard to debug, e.g. a singleton object holding a shared
> > pointer reference to an object and never releases the reference.
> >
> > I tend to agree that fundamental objects like Camera has the necessity
> > to be shared pointers, but would also like to know if it's possible to
> > design the threading model such that CameraManager is the main owner
> > of the Camera objects. For example, if we can design it such that all
> > the access to Camera objects is serialized on a thread,
>
> I think they will, possibly with the exception of request queuing and
> request completion handling. This remains to be decided. In any case, I
> don't think we should support camera lookup from multiple threads.
>
> > then we probably don't need to use shared pointers. Or, we can achieve
> > concurrency by storing Camera objects as std::shared_ptr in
> > CameraManager and only giving out std::weak_ptr of the Camera objects.
>
> It's not just a concurrency issue. Support for hot-pluggable cameras
> will be added in the future, and we'll need to track users of camera
> objects in order to delete the camera only when no users are left. This
> doesn't require std::shared_ptr<>, a manual reference count would work
> too (and it exists today, this patch is replacing it), but I think it's
> better to use std::shared_ptr<> rather than reinventing (and
> open-coding) the wheel.

I'd imagine in the hot-pluggable camera case, the action of actually
removing the camera (e.g. un-plugging the USB camera) would make the
camera no longer usable and what we need would be to handle the
exception gracefully.

Or, do you mean hot-pluggable in the software sense, like the
bind/unbind in kernel drivers? Most of the camera devices allow only
one active user at a time. Do we plan to design the APIs to allow
concurrent access to a camera?

>
> Giving out weak pointers would I think cause more issues than it would
> solve, as any user of a camera instance would need to be prepared for
> the pointer becoming null at any time when the camera is unplugged.

Ack.

>
> > > -       void addCamera(Camera *camera);
> > > +       void addCamera(std::shared_ptr<Camera> &camera);
> >
> > We can pass by value here so that we provide the caller with the
> > freedom to pass its own shared ownership to the callee through
> > std::move().
>
> In the typical use case the caller will likely retain a shared pointer
> to the camera. I can pass by value and use std::move() in the
> addCamera() function.
>
> > >
> > >         static CameraManager *instance();
> > >
> > > @@ -42,7 +42,7 @@ private:
> > >
> > >         std::unique_ptr<DeviceEnumerator> enumerator_;
> > >         std::vector<PipelineHandler *> pipes_;
> > > -       std::vector<Camera *> cameras_;
> > > +       std::vector<std::shared_ptr<Camera>> cameras_;
> >
> > If the camera name is required to be unique then perhaps we can store
> > the cameras as
> >
> >   std::unordered_map<std::string, std::shared_ptr<Camera>> cameras_;
>
> We used to store them in a std::map<>, but it make it cumbersome to
> retrieve the list of all cameras. As I expect the camera lookup API to
> change anyway I think we can revisit this later.

Ack.

>
> > >
> > >         std::unique_ptr<EventDispatcher> dispatcher_;
> > >  };
>
> --
> Regards,
>
> Laurent Pinchart

Hi Ricky,

On Tue, Jan 22, 2019 at 11:19:01AM +0800, Ricky Liang wrote:
>  On Sat, Jan 19, 2019 at 6:51 AM Laurent Pinchart
>  <laurent.pinchart@ideasonboard.com> wrote:
> > On Sat, Jan 19, 2019 at 01:39:35AM +0800, Ricky Liang wrote:
> >> On Fri, Jan 18, 2019 at 7:59 AM Laurent Pinchart
> >> <laurent.pinchart@ideasonboard.com> wrote:
> >>> 
> >>> The Camera class is explicitly reference-counted to manage the lifetime
> >>> of camera objects. Replace this open-coded implementation with usage of
> >>> the std::shared_ptr<> class.
> >>> 
> >>> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> >>> ---
> >>> include/libcamera/camera.h         | 13 +++++----
> >>> include/libcamera/camera_manager.h |  8 +++---
> >>> src/libcamera/camera.cpp           | 46 +++++++++++++++++-------------
> >>> src/libcamera/camera_manager.cpp   | 20 ++++++-------
> >>> src/libcamera/pipeline/vimc.cpp    |  2 +-
> >>> test/list-cameras.cpp              |  2 +-
> >>> 6 files changed, 50 insertions(+), 41 deletions(-)
> >>> 
> >>> diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h
> >>> index 9a7579d61fa3..ef0a794e3a82 100644
> >>> --- a/include/libcamera/camera.h
> >>> +++ b/include/libcamera/camera.h
> >>> @@ -7,6 +7,7 @@
> >>> #ifndef __LIBCAMERA_CAMERA_H__
> >>> #define __LIBCAMERA_CAMERA_H__
> >>> 
> >>> +#include <memory>
> >>> #include <string>
> >>> 
> >>> namespace libcamera {
> >>> @@ -14,15 +15,17 @@ namespace libcamera {
> >>> class Camera
> >>> {
> >>> public:
> >>> -       Camera(const std::string &name);
> >>> +       static std::shared_ptr<Camera> create(const std::string &name);
> >>> +
> >>> +       Camera(const Camera &) = delete;
> >>> +       void operator=(const Camera &) = delete;
> >> 
> >> We probably want to delete the implicit constructor as well:
> >> 
> >> Camera() = delete;
> > 
> > The compiler only creates an implicit constructor when no explicit
> > constructor is specified. As we define Camera(const std::string &name),
> > there's no need to delete the implicit constructor.
>  
>  Ack.
>  
> >>> 
> >>> const std::string &name() const;
> >>> -       void get();
> >>> -       void put();
> >>> 
> >>> private:
> >>> -       virtual ~Camera() { };
> >>> -       int ref_;
> >>> +       Camera(const std::string &name);
> >> 
> >> Can we add the 'explicit' specifier for constructor with only one
> >> argument? Not super important for this patch as it's a private
> >> constructor here, but may be a good general guideline.
> > 
> > Good point, I'll do that.
> > 
> >>> +       ~Camera();
> >>> +
> >>> std::string name_;
> >>> };
> >>> 
> >>> diff --git a/include/libcamera/camera_manager.h b/include/libcamera/camera_manager.h
> >>> index 4b941fd9183b..738b13f4cfb1 100644
> >>> --- a/include/libcamera/camera_manager.h
> >>> +++ b/include/libcamera/camera_manager.h
> >>> @@ -24,10 +24,10 @@ public:
> >>> int start();
> >>> void stop();
> >>> 
> >>> -       const std::vector<Camera *> &cameras() const { return cameras_; }
> >>> -       Camera *get(const std::string &name);
> >>> +       const std::vector<std::shared_ptr<Camera>> &cameras() const { return cameras_; }
> >>> +       std::shared_ptr<Camera> get(const std::string &name);
> >> 
> >> Some high level design questions:
> >> 
> >> 1. Who are the callers of these getter methods?
> > 
> > These methods are meant to be called by applications. I expect the API
> > to change as we continue developing though, but the general idea will
> > stay the same.
> > 
> >> 2. What's the threading model that we plan to use for exercising the
> >> Camera objects?
> > 
> > We don't support multiple threads yet. This will be fixed down the road,
> > but not all details have been decided yet.
> > 
> >> While making Camera objects as shared pointers makes the coding
> >> easier, it may also be hard to track the ownership of the objects once
> >> the code base gets large. In some cases it may lead to memory/fd leaks
> >> that are hard to debug, e.g. a singleton object holding a shared
> >> pointer reference to an object and never releases the reference.
> >> 
> >> I tend to agree that fundamental objects like Camera has the necessity
> >> to be shared pointers, but would also like to know if it's possible to
> >> design the threading model such that CameraManager is the main owner
> >> of the Camera objects. For example, if we can design it such that all
> >> the access to Camera objects is serialized on a thread,
> > 
> > I think they will, possibly with the exception of request queuing and
> > request completion handling. This remains to be decided. In any case, I
> > don't think we should support camera lookup from multiple threads.
> > 
> >> then we probably don't need to use shared pointers. Or, we can achieve
> >> concurrency by storing Camera objects as std::shared_ptr in
> >> CameraManager and only giving out std::weak_ptr of the Camera objects.
> > 
> > It's not just a concurrency issue. Support for hot-pluggable cameras
> > will be added in the future, and we'll need to track users of camera
> > objects in order to delete the camera only when no users are left. This
> > doesn't require std::shared_ptr<>, a manual reference count would work
> > too (and it exists today, this patch is replacing it), but I think it's
> > better to use std::shared_ptr<> rather than reinventing (and
> > open-coding) the wheel.
>  
>  I'd imagine in the hot-pluggable camera case, the action of actually
>  removing the camera (e.g. un-plugging the USB camera) would make the
>  camera no longer usable and what we need would be to handle the
>  exception gracefully.

Correct, the camera object would be marked as disconnected, API calls
would start failing, and when the camera is released by the application
it will be deleted.

>  Or, do you mean hot-pluggable in the software sense, like the
>  bind/unbind in kernel drivers? Most of the camera devices allow only
>  one active user at a time. Do we plan to design the APIs to allow
>  concurrent access to a camera?

I meant hotplugging a USB webcam for instance, but from a userspace
point of view unbinding a device from its kernel driver should achieve
the same effect. I'd like pipeline handlers to handle that gracefully if
it happens, but it's not a priority.

We don't plan to support concurrent access to a camera by multiple
applications, that would be handled by a layer above libcamera if
needed.

> > Giving out weak pointers would I think cause more issues than it would
> > solve, as any user of a camera instance would need to be prepared for
> > the pointer becoming null at any time when the camera is unplugged.
>  
>  Ack.
>  
> >>> -       void addCamera(Camera *camera);
> >>> +       void addCamera(std::shared_ptr<Camera> &camera);
> >> 
> >> We can pass by value here so that we provide the caller with the
> >> freedom to pass its own shared ownership to the callee through
> >> std::move().
> > 
> > In the typical use case the caller will likely retain a shared pointer
> > to the camera. I can pass by value and use std::move() in the
> > addCamera() function.
> > 
> >>> 
> >>> static CameraManager *instance();
> >>> 
> >>> @@ -42,7 +42,7 @@ private:
> >>> 
> >>> std::unique_ptr<DeviceEnumerator> enumerator_;
> >>> std::vector<PipelineHandler *> pipes_;
> >>> -       std::vector<Camera *> cameras_;
> >>> +       std::vector<std::shared_ptr<Camera>> cameras_;
> >> 
> >> If the camera name is required to be unique then perhaps we can store
> >> the cameras as
> >> 
> >> std::unordered_map<std::string, std::shared_ptr<Camera>> cameras_;
> > 
> > We used to store them in a std::map<>, but it make it cumbersome to
> > retrieve the list of all cameras. As I expect the camera lookup API to
> > change anyway I think we can revisit this later.
>  
>  Ack.
>  
> >>> 
> >>> std::unique_ptr<EventDispatcher> dispatcher_;
> >>> };

On Tue, Jan 22, 2019 at 9:22 PM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi Ricky,
>
> On Tue, Jan 22, 2019 at 11:19:01AM +0800, Ricky Liang wrote:
> >  On Sat, Jan 19, 2019 at 6:51 AM Laurent Pinchart
> >  <laurent.pinchart@ideasonboard.com> wrote:
> > > On Sat, Jan 19, 2019 at 01:39:35AM +0800, Ricky Liang wrote:
> > >> On Fri, Jan 18, 2019 at 7:59 AM Laurent Pinchart
> > >> <laurent.pinchart@ideasonboard.com> wrote:
> > >>>
> > >>> The Camera class is explicitly reference-counted to manage the lifetime
> > >>> of camera objects. Replace this open-coded implementation with usage of
> > >>> the std::shared_ptr<> class.
> > >>>
> > >>> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > >>> ---
> > >>> include/libcamera/camera.h         | 13 +++++----
> > >>> include/libcamera/camera_manager.h |  8 +++---
> > >>> src/libcamera/camera.cpp           | 46 +++++++++++++++++-------------
> > >>> src/libcamera/camera_manager.cpp   | 20 ++++++-------
> > >>> src/libcamera/pipeline/vimc.cpp    |  2 +-
> > >>> test/list-cameras.cpp              |  2 +-
> > >>> 6 files changed, 50 insertions(+), 41 deletions(-)
> > >>>
> > >>> diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h
> > >>> index 9a7579d61fa3..ef0a794e3a82 100644
> > >>> --- a/include/libcamera/camera.h
> > >>> +++ b/include/libcamera/camera.h
> > >>> @@ -7,6 +7,7 @@
> > >>> #ifndef __LIBCAMERA_CAMERA_H__
> > >>> #define __LIBCAMERA_CAMERA_H__
> > >>>
> > >>> +#include <memory>
> > >>> #include <string>
> > >>>
> > >>> namespace libcamera {
> > >>> @@ -14,15 +15,17 @@ namespace libcamera {
> > >>> class Camera
> > >>> {
> > >>> public:
> > >>> -       Camera(const std::string &name);
> > >>> +       static std::shared_ptr<Camera> create(const std::string &name);
> > >>> +
> > >>> +       Camera(const Camera &) = delete;
> > >>> +       void operator=(const Camera &) = delete;
> > >>
> > >> We probably want to delete the implicit constructor as well:
> > >>
> > >> Camera() = delete;
> > >
> > > The compiler only creates an implicit constructor when no explicit
> > > constructor is specified. As we define Camera(const std::string &name),
> > > there's no need to delete the implicit constructor.
> >
> >  Ack.
> >
> > >>>
> > >>> const std::string &name() const;
> > >>> -       void get();
> > >>> -       void put();
> > >>>
> > >>> private:
> > >>> -       virtual ~Camera() { };
> > >>> -       int ref_;
> > >>> +       Camera(const std::string &name);
> > >>
> > >> Can we add the 'explicit' specifier for constructor with only one
> > >> argument? Not super important for this patch as it's a private
> > >> constructor here, but may be a good general guideline.
> > >
> > > Good point, I'll do that.
> > >
> > >>> +       ~Camera();
> > >>> +
> > >>> std::string name_;
> > >>> };
> > >>>
> > >>> diff --git a/include/libcamera/camera_manager.h b/include/libcamera/camera_manager.h
> > >>> index 4b941fd9183b..738b13f4cfb1 100644
> > >>> --- a/include/libcamera/camera_manager.h
> > >>> +++ b/include/libcamera/camera_manager.h
> > >>> @@ -24,10 +24,10 @@ public:
> > >>> int start();
> > >>> void stop();
> > >>>
> > >>> -       const std::vector<Camera *> &cameras() const { return cameras_; }
> > >>> -       Camera *get(const std::string &name);
> > >>> +       const std::vector<std::shared_ptr<Camera>> &cameras() const { return cameras_; }
> > >>> +       std::shared_ptr<Camera> get(const std::string &name);
> > >>
> > >> Some high level design questions:
> > >>
> > >> 1. Who are the callers of these getter methods?
> > >
> > > These methods are meant to be called by applications. I expect the API
> > > to change as we continue developing though, but the general idea will
> > > stay the same.
> > >
> > >> 2. What's the threading model that we plan to use for exercising the
> > >> Camera objects?
> > >
> > > We don't support multiple threads yet. This will be fixed down the road,
> > > but not all details have been decided yet.
> > >
> > >> While making Camera objects as shared pointers makes the coding
> > >> easier, it may also be hard to track the ownership of the objects once
> > >> the code base gets large. In some cases it may lead to memory/fd leaks
> > >> that are hard to debug, e.g. a singleton object holding a shared
> > >> pointer reference to an object and never releases the reference.
> > >>
> > >> I tend to agree that fundamental objects like Camera has the necessity
> > >> to be shared pointers, but would also like to know if it's possible to
> > >> design the threading model such that CameraManager is the main owner
> > >> of the Camera objects. For example, if we can design it such that all
> > >> the access to Camera objects is serialized on a thread,
> > >
> > > I think they will, possibly with the exception of request queuing and
> > > request completion handling. This remains to be decided. In any case, I
> > > don't think we should support camera lookup from multiple threads.
> > >
> > >> then we probably don't need to use shared pointers. Or, we can achieve
> > >> concurrency by storing Camera objects as std::shared_ptr in
> > >> CameraManager and only giving out std::weak_ptr of the Camera objects.
> > >
> > > It's not just a concurrency issue. Support for hot-pluggable cameras
> > > will be added in the future, and we'll need to track users of camera
> > > objects in order to delete the camera only when no users are left. This
> > > doesn't require std::shared_ptr<>, a manual reference count would work
> > > too (and it exists today, this patch is replacing it), but I think it's
> > > better to use std::shared_ptr<> rather than reinventing (and
> > > open-coding) the wheel.
> >
> >  I'd imagine in the hot-pluggable camera case, the action of actually
> >  removing the camera (e.g. un-plugging the USB camera) would make the
> >  camera no longer usable and what we need would be to handle the
> >  exception gracefully.
>
> Correct, the camera object would be marked as disconnected, API calls
> would start failing, and when the camera is released by the application
> it will be deleted.
>
> >  Or, do you mean hot-pluggable in the software sense, like the
> >  bind/unbind in kernel drivers? Most of the camera devices allow only
> >  one active user at a time. Do we plan to design the APIs to allow
> >  concurrent access to a camera?
>
> I meant hotplugging a USB webcam for instance, but from a userspace
> point of view unbinding a device from its kernel driver should achieve
> the same effect. I'd like pipeline handlers to handle that gracefully if
> it happens, but it's not a priority.
>
> We don't plan to support concurrent access to a camera by multiple
> applications, that would be handled by a layer above libcamera if
> needed.

So I'm actually wondering if we're trying to solve a real problem
here. Physical camera hotplug is already handled for us by the kernel.
As long as the userspace has the V4L2 file descriptors open, it can
interact with them and get error codes when the camera goes away. We
could just put our internal objects in some error state, fail any
pending capture requests and have any further calls on this Camera
object just fail, in a way that would explicitly tell the consumer
that the reason for the failure was disconnection. Then it would be up
to the consumer to actually destroy the Camera object.

Best regards,
Tomasz

Hi Tomasz,

On Fri, Jan 25, 2019 at 03:55:12PM +0900, Tomasz Figa wrote:
> On Tue, Jan 22, 2019 at 9:22 PM Laurent Pinchart
> <laurent.pinchart@ideasonboard.com> wrote:
> > On Tue, Jan 22, 2019 at 11:19:01AM +0800, Ricky Liang wrote:
> >> On Sat, Jan 19, 2019 at 6:51 AM Laurent Pinchart
> >> <laurent.pinchart@ideasonboard.com> wrote:
> >>> On Sat, Jan 19, 2019 at 01:39:35AM +0800, Ricky Liang wrote:
> >>>> On Fri, Jan 18, 2019 at 7:59 AM Laurent Pinchart
> >>>> <laurent.pinchart@ideasonboard.com> wrote:
> >>>>>
> >>>>> The Camera class is explicitly reference-counted to manage the lifetime
> >>>>> of camera objects. Replace this open-coded implementation with usage of
> >>>>> the std::shared_ptr<> class.
> >>>>>
> >>>>> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> >>>>> ---
> >>>>> include/libcamera/camera.h         | 13 +++++----
> >>>>> include/libcamera/camera_manager.h |  8 +++---
> >>>>> src/libcamera/camera.cpp           | 46 +++++++++++++++++-------------
> >>>>> src/libcamera/camera_manager.cpp   | 20 ++++++-------
> >>>>> src/libcamera/pipeline/vimc.cpp    |  2 +-
> >>>>> test/list-cameras.cpp              |  2 +-
> >>>>> 6 files changed, 50 insertions(+), 41 deletions(-)
> >>>>>
> >>>>> diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h
> >>>>> index 9a7579d61fa3..ef0a794e3a82 100644
> >>>>> --- a/include/libcamera/camera.h
> >>>>> +++ b/include/libcamera/camera.h
> >>>>> @@ -7,6 +7,7 @@
> >>>>> #ifndef __LIBCAMERA_CAMERA_H__
> >>>>> #define __LIBCAMERA_CAMERA_H__
> >>>>>
> >>>>> +#include <memory>
> >>>>> #include <string>
> >>>>>
> >>>>> namespace libcamera {
> >>>>> @@ -14,15 +15,17 @@ namespace libcamera {
> >>>>> class Camera
> >>>>> {
> >>>>> public:
> >>>>> -       Camera(const std::string &name);
> >>>>> +       static std::shared_ptr<Camera> create(const std::string &name);
> >>>>> +
> >>>>> +       Camera(const Camera &) = delete;
> >>>>> +       void operator=(const Camera &) = delete;
> >>>>
> >>>> We probably want to delete the implicit constructor as well:
> >>>>
> >>>> Camera() = delete;
> >>>
> >>> The compiler only creates an implicit constructor when no explicit
> >>> constructor is specified. As we define Camera(const std::string &name),
> >>> there's no need to delete the implicit constructor.
> >>
> >>  Ack.
> >>
> >>>>>
> >>>>> const std::string &name() const;
> >>>>> -       void get();
> >>>>> -       void put();
> >>>>>
> >>>>> private:
> >>>>> -       virtual ~Camera() { };
> >>>>> -       int ref_;
> >>>>> +       Camera(const std::string &name);
> >>>>
> >>>> Can we add the 'explicit' specifier for constructor with only one
> >>>> argument? Not super important for this patch as it's a private
> >>>> constructor here, but may be a good general guideline.
> >>>
> >>> Good point, I'll do that.
> >>>
> >>>>> +       ~Camera();
> >>>>> +
> >>>>> std::string name_;
> >>>>> };
> >>>>>
> >>>>> diff --git a/include/libcamera/camera_manager.h b/include/libcamera/camera_manager.h
> >>>>> index 4b941fd9183b..738b13f4cfb1 100644
> >>>>> --- a/include/libcamera/camera_manager.h
> >>>>> +++ b/include/libcamera/camera_manager.h
> >>>>> @@ -24,10 +24,10 @@ public:
> >>>>> int start();
> >>>>> void stop();
> >>>>>
> >>>>> -       const std::vector<Camera *> &cameras() const { return cameras_; }
> >>>>> -       Camera *get(const std::string &name);
> >>>>> +       const std::vector<std::shared_ptr<Camera>> &cameras() const { return cameras_; }
> >>>>> +       std::shared_ptr<Camera> get(const std::string &name);
> >>>>
> >>>> Some high level design questions:
> >>>>
> >>>> 1. Who are the callers of these getter methods?
> >>>
> >>> These methods are meant to be called by applications. I expect the API
> >>> to change as we continue developing though, but the general idea will
> >>> stay the same.
> >>>
> >>>> 2. What's the threading model that we plan to use for exercising the
> >>>> Camera objects?
> >>>
> >>> We don't support multiple threads yet. This will be fixed down the road,
> >>> but not all details have been decided yet.
> >>>
> >>>> While making Camera objects as shared pointers makes the coding
> >>>> easier, it may also be hard to track the ownership of the objects once
> >>>> the code base gets large. In some cases it may lead to memory/fd leaks
> >>>> that are hard to debug, e.g. a singleton object holding a shared
> >>>> pointer reference to an object and never releases the reference.
> >>>>
> >>>> I tend to agree that fundamental objects like Camera has the necessity
> >>>> to be shared pointers, but would also like to know if it's possible to
> >>>> design the threading model such that CameraManager is the main owner
> >>>> of the Camera objects. For example, if we can design it such that all
> >>>> the access to Camera objects is serialized on a thread,
> >>>
> >>> I think they will, possibly with the exception of request queuing and
> >>> request completion handling. This remains to be decided. In any case, I
> >>> don't think we should support camera lookup from multiple threads.
> >>>
> >>>> then we probably don't need to use shared pointers. Or, we can achieve
> >>>> concurrency by storing Camera objects as std::shared_ptr in
> >>>> CameraManager and only giving out std::weak_ptr of the Camera objects.
> >>>
> >>> It's not just a concurrency issue. Support for hot-pluggable cameras
> >>> will be added in the future, and we'll need to track users of camera
> >>> objects in order to delete the camera only when no users are left. This
> >>> doesn't require std::shared_ptr<>, a manual reference count would work
> >>> too (and it exists today, this patch is replacing it), but I think it's
> >>> better to use std::shared_ptr<> rather than reinventing (and
> >>> open-coding) the wheel.
> >>
> >>  I'd imagine in the hot-pluggable camera case, the action of actually
> >>  removing the camera (e.g. un-plugging the USB camera) would make the
> >>  camera no longer usable and what we need would be to handle the
> >>  exception gracefully.
> >
> > Correct, the camera object would be marked as disconnected, API calls
> > would start failing, and when the camera is released by the application
> > it will be deleted.
> >
> >>  Or, do you mean hot-pluggable in the software sense, like the
> >>  bind/unbind in kernel drivers? Most of the camera devices allow only
> >>  one active user at a time. Do we plan to design the APIs to allow
> >>  concurrent access to a camera?
> >
> > I meant hotplugging a USB webcam for instance, but from a userspace
> > point of view unbinding a device from its kernel driver should achieve
> > the same effect. I'd like pipeline handlers to handle that gracefully if
> > it happens, but it's not a priority.
> >
> > We don't plan to support concurrent access to a camera by multiple
> > applications, that would be handled by a layer above libcamera if
> > needed.
> 
> So I'm actually wondering if we're trying to solve a real problem
> here. Physical camera hotplug is already handled for us by the kernel.
> As long as the userspace has the V4L2 file descriptors open, it can
> interact with them and get error codes when the camera goes away. We
> could just put our internal objects in some error state, fail any
> pending capture requests and have any further calls on this Camera
> object just fail, in a way that would explicitly tell the consumer
> that the reason for the failure was disconnection. Then it would be up
> to the consumer to actually destroy the Camera object.

One core design principle in libcamera is that the library enumerates
cameras by scanning for media devices (in the MC sense) and associating
them with pipeline handlers (which can be seen as a kind of userspace
counterpart of the kernel drivers, exposing various MC device nodes as
higher level camera objects, the same way a kernel driver exposes
various pieces of hardware as standardized device nodes). The library is
thus responsible for creating the camera objects, which are then
retrieved and used by applications. In the context of hotplug handling
we need to signal hot-unplug to applications, certainly in the form of
completing pending capture requests with an error and failing futher
calls, but I think also as an explicit signal to the application. For
this to work I believe handling camera objects as shared pointers is the
best option, as the consumer has to release the object, but shouldn't
destroy it. Am I biased ? :-)

On Fri, Jan 25, 2019 at 7:34 PM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi Tomasz,
>
> On Fri, Jan 25, 2019 at 03:55:12PM +0900, Tomasz Figa wrote:
> > On Tue, Jan 22, 2019 at 9:22 PM Laurent Pinchart
> > <laurent.pinchart@ideasonboard.com> wrote:
> > > On Tue, Jan 22, 2019 at 11:19:01AM +0800, Ricky Liang wrote:
> > >> On Sat, Jan 19, 2019 at 6:51 AM Laurent Pinchart
> > >> <laurent.pinchart@ideasonboard.com> wrote:
> > >>> On Sat, Jan 19, 2019 at 01:39:35AM +0800, Ricky Liang wrote:
> > >>>> On Fri, Jan 18, 2019 at 7:59 AM Laurent Pinchart
> > >>>> <laurent.pinchart@ideasonboard.com> wrote:
> > >>>>>
> > >>>>> The Camera class is explicitly reference-counted to manage the lifetime
> > >>>>> of camera objects. Replace this open-coded implementation with usage of
> > >>>>> the std::shared_ptr<> class.
> > >>>>>
> > >>>>> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > >>>>> ---
> > >>>>> include/libcamera/camera.h         | 13 +++++----
> > >>>>> include/libcamera/camera_manager.h |  8 +++---
> > >>>>> src/libcamera/camera.cpp           | 46 +++++++++++++++++-------------
> > >>>>> src/libcamera/camera_manager.cpp   | 20 ++++++-------
> > >>>>> src/libcamera/pipeline/vimc.cpp    |  2 +-
> > >>>>> test/list-cameras.cpp              |  2 +-
> > >>>>> 6 files changed, 50 insertions(+), 41 deletions(-)
> > >>>>>
> > >>>>> diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h
> > >>>>> index 9a7579d61fa3..ef0a794e3a82 100644
> > >>>>> --- a/include/libcamera/camera.h
> > >>>>> +++ b/include/libcamera/camera.h
> > >>>>> @@ -7,6 +7,7 @@
> > >>>>> #ifndef __LIBCAMERA_CAMERA_H__
> > >>>>> #define __LIBCAMERA_CAMERA_H__
> > >>>>>
> > >>>>> +#include <memory>
> > >>>>> #include <string>
> > >>>>>
> > >>>>> namespace libcamera {
> > >>>>> @@ -14,15 +15,17 @@ namespace libcamera {
> > >>>>> class Camera
> > >>>>> {
> > >>>>> public:
> > >>>>> -       Camera(const std::string &name);
> > >>>>> +       static std::shared_ptr<Camera> create(const std::string &name);
> > >>>>> +
> > >>>>> +       Camera(const Camera &) = delete;
> > >>>>> +       void operator=(const Camera &) = delete;
> > >>>>
> > >>>> We probably want to delete the implicit constructor as well:
> > >>>>
> > >>>> Camera() = delete;
> > >>>
> > >>> The compiler only creates an implicit constructor when no explicit
> > >>> constructor is specified. As we define Camera(const std::string &name),
> > >>> there's no need to delete the implicit constructor.
> > >>
> > >>  Ack.
> > >>
> > >>>>>
> > >>>>> const std::string &name() const;
> > >>>>> -       void get();
> > >>>>> -       void put();
> > >>>>>
> > >>>>> private:
> > >>>>> -       virtual ~Camera() { };
> > >>>>> -       int ref_;
> > >>>>> +       Camera(const std::string &name);
> > >>>>
> > >>>> Can we add the 'explicit' specifier for constructor with only one
> > >>>> argument? Not super important for this patch as it's a private
> > >>>> constructor here, but may be a good general guideline.
> > >>>
> > >>> Good point, I'll do that.
> > >>>
> > >>>>> +       ~Camera();
> > >>>>> +
> > >>>>> std::string name_;
> > >>>>> };
> > >>>>>
> > >>>>> diff --git a/include/libcamera/camera_manager.h b/include/libcamera/camera_manager.h
> > >>>>> index 4b941fd9183b..738b13f4cfb1 100644
> > >>>>> --- a/include/libcamera/camera_manager.h
> > >>>>> +++ b/include/libcamera/camera_manager.h
> > >>>>> @@ -24,10 +24,10 @@ public:
> > >>>>> int start();
> > >>>>> void stop();
> > >>>>>
> > >>>>> -       const std::vector<Camera *> &cameras() const { return cameras_; }
> > >>>>> -       Camera *get(const std::string &name);
> > >>>>> +       const std::vector<std::shared_ptr<Camera>> &cameras() const { return cameras_; }
> > >>>>> +       std::shared_ptr<Camera> get(const std::string &name);
> > >>>>
> > >>>> Some high level design questions:
> > >>>>
> > >>>> 1. Who are the callers of these getter methods?
> > >>>
> > >>> These methods are meant to be called by applications. I expect the API
> > >>> to change as we continue developing though, but the general idea will
> > >>> stay the same.
> > >>>
> > >>>> 2. What's the threading model that we plan to use for exercising the
> > >>>> Camera objects?
> > >>>
> > >>> We don't support multiple threads yet. This will be fixed down the road,
> > >>> but not all details have been decided yet.
> > >>>
> > >>>> While making Camera objects as shared pointers makes the coding
> > >>>> easier, it may also be hard to track the ownership of the objects once
> > >>>> the code base gets large. In some cases it may lead to memory/fd leaks
> > >>>> that are hard to debug, e.g. a singleton object holding a shared
> > >>>> pointer reference to an object and never releases the reference.
> > >>>>
> > >>>> I tend to agree that fundamental objects like Camera has the necessity
> > >>>> to be shared pointers, but would also like to know if it's possible to
> > >>>> design the threading model such that CameraManager is the main owner
> > >>>> of the Camera objects. For example, if we can design it such that all
> > >>>> the access to Camera objects is serialized on a thread,
> > >>>
> > >>> I think they will, possibly with the exception of request queuing and
> > >>> request completion handling. This remains to be decided. In any case, I
> > >>> don't think we should support camera lookup from multiple threads.
> > >>>
> > >>>> then we probably don't need to use shared pointers. Or, we can achieve
> > >>>> concurrency by storing Camera objects as std::shared_ptr in
> > >>>> CameraManager and only giving out std::weak_ptr of the Camera objects.
> > >>>
> > >>> It's not just a concurrency issue. Support for hot-pluggable cameras
> > >>> will be added in the future, and we'll need to track users of camera
> > >>> objects in order to delete the camera only when no users are left. This
> > >>> doesn't require std::shared_ptr<>, a manual reference count would work
> > >>> too (and it exists today, this patch is replacing it), but I think it's
> > >>> better to use std::shared_ptr<> rather than reinventing (and
> > >>> open-coding) the wheel.
> > >>
> > >>  I'd imagine in the hot-pluggable camera case, the action of actually
> > >>  removing the camera (e.g. un-plugging the USB camera) would make the
> > >>  camera no longer usable and what we need would be to handle the
> > >>  exception gracefully.
> > >
> > > Correct, the camera object would be marked as disconnected, API calls
> > > would start failing, and when the camera is released by the application
> > > it will be deleted.
> > >
> > >>  Or, do you mean hot-pluggable in the software sense, like the
> > >>  bind/unbind in kernel drivers? Most of the camera devices allow only
> > >>  one active user at a time. Do we plan to design the APIs to allow
> > >>  concurrent access to a camera?
> > >
> > > I meant hotplugging a USB webcam for instance, but from a userspace
> > > point of view unbinding a device from its kernel driver should achieve
> > > the same effect. I'd like pipeline handlers to handle that gracefully if
> > > it happens, but it's not a priority.
> > >
> > > We don't plan to support concurrent access to a camera by multiple
> > > applications, that would be handled by a layer above libcamera if
> > > needed.
> >
> > So I'm actually wondering if we're trying to solve a real problem
> > here. Physical camera hotplug is already handled for us by the kernel.
> > As long as the userspace has the V4L2 file descriptors open, it can
> > interact with them and get error codes when the camera goes away. We
> > could just put our internal objects in some error state, fail any
> > pending capture requests and have any further calls on this Camera
> > object just fail, in a way that would explicitly tell the consumer
> > that the reason for the failure was disconnection. Then it would be up
> > to the consumer to actually destroy the Camera object.
>
> One core design principle in libcamera is that the library enumerates
> cameras by scanning for media devices (in the MC sense) and associating
> them with pipeline handlers (which can be seen as a kind of userspace
> counterpart of the kernel drivers, exposing various MC device nodes as
> higher level camera objects, the same way a kernel driver exposes
> various pieces of hardware as standardized device nodes). The library is
> thus responsible for creating the camera objects, which are then
> retrieved and used by applications. In the context of hotplug handling
> we need to signal hot-unplug to applications, certainly in the form of
> completing pending capture requests with an error and failing futher
> calls, but I think also as an explicit signal to the application. For
> this to work I believe handling camera objects as shared pointers is the
> best option, as the consumer has to release the object, but shouldn't
> destroy it. Am I biased ? :-)

I'd give the consumer an object it can destroy, which only points to
the camera object created by the framework. That would eliminate the
shared pointers from the public API, but still give you the ability to
use them internally. For any signals to the application, we need some
event loop on the application side anyway, which would poll the object
for messages, so that should work as well with this kind of "consumer"
object.

Best regards,
Tomasz

Hi Tomasz,

On Mon, Jan 28, 2019 at 04:53:39PM +0900, Tomasz Figa wrote:
> On Fri, Jan 25, 2019 at 7:34 PM Laurent Pinchart
> <laurent.pinchart@ideasonboard.com> wrote:
> > On Fri, Jan 25, 2019 at 03:55:12PM +0900, Tomasz Figa wrote:
> >> On Tue, Jan 22, 2019 at 9:22 PM Laurent Pinchart
> >> <laurent.pinchart@ideasonboard.com> wrote:
> >>> On Tue, Jan 22, 2019 at 11:19:01AM +0800, Ricky Liang wrote:
> >>>> On Sat, Jan 19, 2019 at 6:51 AM Laurent Pinchart
> >>>> <laurent.pinchart@ideasonboard.com> wrote:
> >>>>> On Sat, Jan 19, 2019 at 01:39:35AM +0800, Ricky Liang wrote:
> >>>>>> On Fri, Jan 18, 2019 at 7:59 AM Laurent Pinchart
> >>>>>> <laurent.pinchart@ideasonboard.com> wrote:
> >>>>>>>
> >>>>>>> The Camera class is explicitly reference-counted to manage the lifetime
> >>>>>>> of camera objects. Replace this open-coded implementation with usage of
> >>>>>>> the std::shared_ptr<> class.
> >>>>>>>
> >>>>>>> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> >>>>>>> ---
> >>>>>>> include/libcamera/camera.h         | 13 +++++----
> >>>>>>> include/libcamera/camera_manager.h |  8 +++---
> >>>>>>> src/libcamera/camera.cpp           | 46 +++++++++++++++++-------------
> >>>>>>> src/libcamera/camera_manager.cpp   | 20 ++++++-------
> >>>>>>> src/libcamera/pipeline/vimc.cpp    |  2 +-
> >>>>>>> test/list-cameras.cpp              |  2 +-
> >>>>>>> 6 files changed, 50 insertions(+), 41 deletions(-)
> >>>>>>>
> >>>>>>> diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h
> >>>>>>> index 9a7579d61fa3..ef0a794e3a82 100644
> >>>>>>> --- a/include/libcamera/camera.h
> >>>>>>> +++ b/include/libcamera/camera.h
> >>>>>>> @@ -7,6 +7,7 @@
> >>>>>>> #ifndef __LIBCAMERA_CAMERA_H__
> >>>>>>> #define __LIBCAMERA_CAMERA_H__
> >>>>>>>
> >>>>>>> +#include <memory>
> >>>>>>> #include <string>
> >>>>>>>
> >>>>>>> namespace libcamera {
> >>>>>>> @@ -14,15 +15,17 @@ namespace libcamera {
> >>>>>>> class Camera
> >>>>>>> {
> >>>>>>> public:
> >>>>>>> -       Camera(const std::string &name);
> >>>>>>> +       static std::shared_ptr<Camera> create(const std::string &name);
> >>>>>>> +
> >>>>>>> +       Camera(const Camera &) = delete;
> >>>>>>> +       void operator=(const Camera &) = delete;
> >>>>>>
> >>>>>> We probably want to delete the implicit constructor as well:
> >>>>>>
> >>>>>> Camera() = delete;
> >>>>>
> >>>>> The compiler only creates an implicit constructor when no explicit
> >>>>> constructor is specified. As we define Camera(const std::string &name),
> >>>>> there's no need to delete the implicit constructor.
> >>>>
> >>>>  Ack.
> >>>>
> >>>>>>>
> >>>>>>> const std::string &name() const;
> >>>>>>> -       void get();
> >>>>>>> -       void put();
> >>>>>>>
> >>>>>>> private:
> >>>>>>> -       virtual ~Camera() { };
> >>>>>>> -       int ref_;
> >>>>>>> +       Camera(const std::string &name);
> >>>>>>
> >>>>>> Can we add the 'explicit' specifier for constructor with only one
> >>>>>> argument? Not super important for this patch as it's a private
> >>>>>> constructor here, but may be a good general guideline.
> >>>>>
> >>>>> Good point, I'll do that.
> >>>>>
> >>>>>>> +       ~Camera();
> >>>>>>> +
> >>>>>>> std::string name_;
> >>>>>>> };
> >>>>>>>
> >>>>>>> diff --git a/include/libcamera/camera_manager.h b/include/libcamera/camera_manager.h
> >>>>>>> index 4b941fd9183b..738b13f4cfb1 100644
> >>>>>>> --- a/include/libcamera/camera_manager.h
> >>>>>>> +++ b/include/libcamera/camera_manager.h
> >>>>>>> @@ -24,10 +24,10 @@ public:
> >>>>>>> int start();
> >>>>>>> void stop();
> >>>>>>>
> >>>>>>> -       const std::vector<Camera *> &cameras() const { return cameras_; }
> >>>>>>> -       Camera *get(const std::string &name);
> >>>>>>> +       const std::vector<std::shared_ptr<Camera>> &cameras() const { return cameras_; }
> >>>>>>> +       std::shared_ptr<Camera> get(const std::string &name);
> >>>>>>
> >>>>>> Some high level design questions:
> >>>>>>
> >>>>>> 1. Who are the callers of these getter methods?
> >>>>>
> >>>>> These methods are meant to be called by applications. I expect the API
> >>>>> to change as we continue developing though, but the general idea will
> >>>>> stay the same.
> >>>>>
> >>>>>> 2. What's the threading model that we plan to use for exercising the
> >>>>>> Camera objects?
> >>>>>
> >>>>> We don't support multiple threads yet. This will be fixed down the road,
> >>>>> but not all details have been decided yet.
> >>>>>
> >>>>>> While making Camera objects as shared pointers makes the coding
> >>>>>> easier, it may also be hard to track the ownership of the objects once
> >>>>>> the code base gets large. In some cases it may lead to memory/fd leaks
> >>>>>> that are hard to debug, e.g. a singleton object holding a shared
> >>>>>> pointer reference to an object and never releases the reference.
> >>>>>>
> >>>>>> I tend to agree that fundamental objects like Camera has the necessity
> >>>>>> to be shared pointers, but would also like to know if it's possible to
> >>>>>> design the threading model such that CameraManager is the main owner
> >>>>>> of the Camera objects. For example, if we can design it such that all
> >>>>>> the access to Camera objects is serialized on a thread,
> >>>>>
> >>>>> I think they will, possibly with the exception of request queuing and
> >>>>> request completion handling. This remains to be decided. In any case, I
> >>>>> don't think we should support camera lookup from multiple threads.
> >>>>>
> >>>>>> then we probably don't need to use shared pointers. Or, we can achieve
> >>>>>> concurrency by storing Camera objects as std::shared_ptr in
> >>>>>> CameraManager and only giving out std::weak_ptr of the Camera objects.
> >>>>>
> >>>>> It's not just a concurrency issue. Support for hot-pluggable cameras
> >>>>> will be added in the future, and we'll need to track users of camera
> >>>>> objects in order to delete the camera only when no users are left. This
> >>>>> doesn't require std::shared_ptr<>, a manual reference count would work
> >>>>> too (and it exists today, this patch is replacing it), but I think it's
> >>>>> better to use std::shared_ptr<> rather than reinventing (and
> >>>>> open-coding) the wheel.
> >>>>
> >>>>  I'd imagine in the hot-pluggable camera case, the action of actually
> >>>>  removing the camera (e.g. un-plugging the USB camera) would make the
> >>>>  camera no longer usable and what we need would be to handle the
> >>>>  exception gracefully.
> >>>
> >>> Correct, the camera object would be marked as disconnected, API calls
> >>> would start failing, and when the camera is released by the application
> >>> it will be deleted.
> >>>
> >>>>  Or, do you mean hot-pluggable in the software sense, like the
> >>>>  bind/unbind in kernel drivers? Most of the camera devices allow only
> >>>>  one active user at a time. Do we plan to design the APIs to allow
> >>>>  concurrent access to a camera?
> >>>
> >>> I meant hotplugging a USB webcam for instance, but from a userspace
> >>> point of view unbinding a device from its kernel driver should achieve
> >>> the same effect. I'd like pipeline handlers to handle that gracefully if
> >>> it happens, but it's not a priority.
> >>>
> >>> We don't plan to support concurrent access to a camera by multiple
> >>> applications, that would be handled by a layer above libcamera if
> >>> needed.
> >>
> >> So I'm actually wondering if we're trying to solve a real problem
> >> here. Physical camera hotplug is already handled for us by the kernel.
> >> As long as the userspace has the V4L2 file descriptors open, it can
> >> interact with them and get error codes when the camera goes away. We
> >> could just put our internal objects in some error state, fail any
> >> pending capture requests and have any further calls on this Camera
> >> object just fail, in a way that would explicitly tell the consumer
> >> that the reason for the failure was disconnection. Then it would be up
> >> to the consumer to actually destroy the Camera object.
> >
> > One core design principle in libcamera is that the library enumerates
> > cameras by scanning for media devices (in the MC sense) and associating
> > them with pipeline handlers (which can be seen as a kind of userspace
> > counterpart of the kernel drivers, exposing various MC device nodes as
> > higher level camera objects, the same way a kernel driver exposes
> > various pieces of hardware as standardized device nodes). The library is
> > thus responsible for creating the camera objects, which are then
> > retrieved and used by applications. In the context of hotplug handling
> > we need to signal hot-unplug to applications, certainly in the form of
> > completing pending capture requests with an error and failing futher
> > calls, but I think also as an explicit signal to the application. For
> > this to work I believe handling camera objects as shared pointers is the
> > best option, as the consumer has to release the object, but shouldn't
> > destroy it. Am I biased ? :-)
> 
> I'd give the consumer an object it can destroy, which only points to
> the camera object created by the framework. That would eliminate the
> shared pointers from the public API, but still give you the ability to
> use them internally.

That's an interesting idea. I'll give it a try, even if it means I'll
have to find two class names for the external and internal camera
objects :-)

> For any signals to the application, we need some event loop on the
> application side anyway, which would poll the object for messages, so
> that should work as well with this kind of "consumer" object.

libcamera already contains an event dispatcher API that allows the
library to plug itself in the application event loop (with a default
poll-based event loop provided by libcamera itself to ease application
development), and a signal/slot mechanism inspired by Qt. I think that
will be enough to signal camera disconnection, even though more work
will likely be needed to support multi-threaded applications in this
regard.