[libcamera-devel] [PATCH v4 4/6] libcamera: camera_manager: Introduce signals when a camera is added/removed

[Laurent Pinchart]

Hi Umang,

Thank you for the patch.

On Thu, Jun 11, 2020 at 05:16:07PM +0000, Umang Jain wrote:
> Emit 'cameraAdded' and 'cameraRemoved' from CameraManager to enable
> hotplug and hot-unplug support in application 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.
> 
> Also, until now, CameraManager::Private::addCamera() transfers the
> entire ownership of camera shared_ptr to CameraManager using std::move().
> This patch changes the signature of Private::addCamera to accept pass-by-value
> camera parameter. It is done to make it clear from the caller point of view
> that the pointer within the caller will still be valid after this function
> returns. With this change in, we can emit the camera pointer via 'cameraAdded'
> signal without hitting a segfault.

Nice description, thanks. If you reflowed it to 72 columns as per the
normal git commit message policy, it would be perfect ;-)

> Signed-off-by: Umang Jain <email at uajain.com>
> Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> ---
>  include/libcamera/camera_manager.h |  6 ++++-
>  src/libcamera/camera_manager.cpp   | 38 ++++++++++++++++++++++++++----
>  src/libcamera/pipeline_handler.cpp |  2 +-
>  3 files changed, 40 insertions(+), 6 deletions(-)
> 
> diff --git a/include/libcamera/camera_manager.h b/include/libcamera/camera_manager.h
> index 95dc636..9eb2b6f 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 {
>  
> @@ -36,13 +37,16 @@ public:
>  
>  	void addCamera(std::shared_ptr<Camera> camera,
>  		       const std::vector<dev_t> &devnums);
> -	void removeCamera(Camera *camera);
> +	void removeCamera(std::shared_ptr<Camera> camera);
>  
>  	static const std::string &version() { return version_; }
>  
>  	void setEventDispatcher(std::unique_ptr<EventDispatcher> dispatcher);
>  	EventDispatcher *eventDispatcher();
>  
> +	Signal<std::shared_ptr<Camera>> 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 17a4afb..7f846c0 100644
> --- a/src/libcamera/camera_manager.cpp
> +++ b/src/libcamera/camera_manager.cpp
> @@ -36,7 +36,7 @@ public:
>  	Private(CameraManager *cm);
>  
>  	int start();
> -	void addCamera(std::shared_ptr<Camera> &camera,
> +	void addCamera(std::shared_ptr<Camera> camera,
>  		       const std::vector<dev_t> &devnums);
>  	void removeCamera(Camera *camera);
>  
> @@ -172,7 +172,7 @@ void CameraManager::Private::cleanup()
>  	enumerator_.reset(nullptr);
>  }
>  
> -void CameraManager::Private::addCamera(std::shared_ptr<Camera> &camera,
> +void CameraManager::Private::addCamera(std::shared_ptr<Camera> camera,
>  				       const std::vector<dev_t> &devnums)
>  {
>  	MutexLocker locker(mutex_);
> @@ -375,6 +375,34 @@ std::shared_ptr<Camera> CameraManager::get(dev_t devnum)
>  	return iter->second.lock();
>  }
>  
> +/**
> + * \var CameraManager::cameraAdded
> + * \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
> @@ -395,6 +423,7 @@ void CameraManager::addCamera(std::shared_ptr<Camera> camera,
>  	ASSERT(Thread::current() == p_.get());
>  
>  	p_->addCamera(camera, devnums);
> +	cameraAdded.emit(camera);
>  }
>  
>  /**
> @@ -407,11 +436,12 @@ void CameraManager::addCamera(std::shared_ptr<Camera> camera,
>   *
>   * \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 a0f6b0f..bad79dc 100644
> --- a/src/libcamera/pipeline_handler.cpp
> +++ b/src/libcamera/pipeline_handler.cpp
> @@ -565,7 +565,7 @@ void PipelineHandler::disconnect()
>  			continue;
>  
>  		camera->disconnect();
> -		manager_->removeCamera(camera.get());
> +		manager_->removeCamera(camera);
>  	}
>  
>  	cameras_.clear();

-- 
Regards,

Laurent Pinchart