[libcamera-devel] [PATCH v3 3/5] libcamera: camera_manager: Introduce signals when a camera is added/removed

[Laurent Pinchart]

Hi Umang,

Thank you for the patch.

On Thu, May 21, 2020 at 01:54:25PM +0000, Umang Jain wrote:
> Emit 'newCameraAdded' and 'cameraRemoved' from CameraManager to enable
> hotplug and hot-unplug support in appplication like QCam.
> 
> To avoid use-after-free race between the CameraManager and the
> application, emit the 'cameraRemoved' with the shared_ptr version
> of <Camera *>. This requires to change the function signature of
> CameraManager::removeCamera() API.
> 
> Signed-off-by: Umang Jain <email at uajain.com>
> ---
>  include/libcamera/camera_manager.h |  6 ++++-
>  src/libcamera/camera_manager.cpp   | 36 +++++++++++++++++++++++++++---
>  src/libcamera/pipeline_handler.cpp |  2 +-
>  3 files changed, 39 insertions(+), 5 deletions(-)
> 
> diff --git a/include/libcamera/camera_manager.h b/include/libcamera/camera_manager.h
> index 079f848..365e286 100644
> --- a/include/libcamera/camera_manager.h
> +++ b/include/libcamera/camera_manager.h
> @@ -13,6 +13,7 @@
>  #include <vector>
>  
>  #include <libcamera/object.h>
> +#include <libcamera/signal.h>
>  
>  namespace libcamera {
>  
> @@ -35,13 +36,16 @@ public:
>  	std::shared_ptr<Camera> get(dev_t devnum);
>  
>  	void addCamera(std::shared_ptr<Camera> camera, dev_t devnum);
> -	void removeCamera(Camera *camera);
> +	void removeCamera(std::shared_ptr<Camera> camera);

This may not be strictly required, as we could use
camera->shared_from_this() in removeCamera(), but I think it's cleaner
to pass the shared_ptr.

>  
>  	static const std::string &version() { return version_; }
>  
>  	void setEventDispatcher(std::unique_ptr<EventDispatcher> dispatcher);
>  	EventDispatcher *eventDispatcher();
>  
> +	Signal<std::shared_ptr<Camera>> newCameraAdded;

For symmetry, should this be called cameraAdded ?

> +	Signal<std::shared_ptr<Camera>> cameraRemoved;
> +
>  private:
>  	static const std::string version_;
>  	static CameraManager *self_;
> diff --git a/src/libcamera/camera_manager.cpp b/src/libcamera/camera_manager.cpp
> index 27064d2..bdd78f5 100644
> --- a/src/libcamera/camera_manager.cpp
> +++ b/src/libcamera/camera_manager.cpp
> @@ -185,7 +185,7 @@ void CameraManager::Private::addCamera(std::shared_ptr<Camera> &camera,
>  		}
>  	}
>  
> -	cameras_.push_back(std::move(camera));
> +	cameras_.push_back(camera);

Could we keep the std::move here, and turn the &camera argument into
camera ? Passing a non-const reference and having the caller rely on
this function not modifying the reference is a bit fragile. If we pass a
copy of the shared_ptr to the function, it's clear from the caller point
of view that the pointer within the caller will still be valid after
this function returns.

With those two small issues addressed,

Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>

>  
>  	if (devnum) {
>  		unsigned int index = cameras_.size() - 1;
> @@ -375,6 +375,34 @@ std::shared_ptr<Camera> CameraManager::get(dev_t devnum)
>  	return iter->second.lock();
>  }
>  
> +/**
> + * \var CameraManager::newCameraAdded
> + * \brief Notify of a new camera added to the system
> + *
> + * This signal is emitted when a new camera is detected and successfully handled
> + * by the camera manager. The notification occurs alike for cameras detected
> + * when the manager is started with start() or when new cameras are later
> + * connected to the system. When the signal is emitted the new camera is already
> + * available from the list of cameras().
> + *
> + * The signal is emitted from the CameraManager thread. Applications shall
> + * minimize the time spent in the signal handler and shall in particular not
> + * perform any blocking operation.
> + */
> +
> +/**
> + * \var CameraManager::cameraRemoved
> + * \brief Notify of a new camera removed from the system
> + *
> + * This signal is emitted when a camera is removed from the system. When the
> + * signal is emitted the camera is not available from the list of cameras()
> + * anymore.
> + *
> + * The signal is emitted from the CameraManager thread. Applications shall
> + * minimize the time spent in the signal handler and shall in particular not
> + * perform any blocking operation.
> + */
> +
>  /**
>   * \brief Add a camera to the camera manager
>   * \param[in] camera The camera to be added
> @@ -394,6 +422,7 @@ void CameraManager::addCamera(std::shared_ptr<Camera> camera, dev_t devnum)
>  	ASSERT(Thread::current() == p_.get());
>  
>  	p_->addCamera(camera, devnum);
> +	newCameraAdded.emit(camera);
>  }
>  
>  /**
> @@ -406,11 +435,12 @@ void CameraManager::addCamera(std::shared_ptr<Camera> camera, dev_t devnum)
>   *
>   * \context This function shall be called from the CameraManager thread.
>   */
> -void CameraManager::removeCamera(Camera *camera)
> +void CameraManager::removeCamera(std::shared_ptr<Camera> camera)
>  {
>  	ASSERT(Thread::current() == p_.get());
>  
> -	p_->removeCamera(camera);
> +	p_->removeCamera(camera.get());
> +	cameraRemoved.emit(camera);
>  }
>  
>  /**
> diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp
> index 53aeebd..9a1e6ff 100644
> --- a/src/libcamera/pipeline_handler.cpp
> +++ b/src/libcamera/pipeline_handler.cpp
> @@ -555,7 +555,7 @@ void PipelineHandler::disconnect()
>  			continue;
>  
>  		camera->disconnect();
> -		manager_->removeCamera(camera.get());
> +		manager_->removeCamera(camera);
>  	}
>  
>  	cameras_.clear();

-- 
Regards,

Laurent Pinchart