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

4029 diff mbox series

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.

Signed-off-by: Umang Jain <email@uajain.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@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(-)

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@uajain.com>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@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();