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

[Laurent Pinchart]

Hi Umang,

Thank you for the patch.

On Wed, May 06, 2020 at 10:33:53AM +0000, Umang Jain wrote:
> Emit 'cameraAdded' and 'cameraRemoved' from CameraManager to enable
> hotplug and hot-unplug support in appplications like QCam.
> 
> Signed-off-by: Umang Jain <email at uajain.com>
> ---
>  include/libcamera/camera_manager.h |  4 ++++
>  src/libcamera/camera_manager.cpp   | 20 ++++++++++++++++++++
>  2 files changed, 24 insertions(+)
> 
> diff --git a/include/libcamera/camera_manager.h b/include/libcamera/camera_manager.h
> index 079f848..558bb96 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 {
>  
> @@ -42,6 +43,9 @@ public:
>  	void setEventDispatcher(std::unique_ptr<EventDispatcher> dispatcher);
>  	EventDispatcher *eventDispatcher();
>  
> +	Signal<Camera *> cameraAdded;
> +	Signal<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 c75979a..6438f87 100644
> --- a/src/libcamera/camera_manager.cpp
> +++ b/src/libcamera/camera_manager.cpp
> @@ -391,6 +391,23 @@ std::shared_ptr<Camera> CameraManager::get(dev_t devnum)
>  	return iter->second.lock();
>  }
>  
> +/**
> + * \var CameraManager::cameraAdded
> + * \brief Signal emitted when a new camera is added in CameraManager
> + *
> + * This signal is emitted when a new camera is added by the CameraManager
> + * in the list of cameras it manages. A pointer to the newly-added camera
> + * is passed as a parameter.

I think we should detail here the relationship with
CameraManager::start() and CameraManager::cameras(), and note that the
signal handler should be fast (otherwise it would block operation of
other cameras). How about the following ?

 * \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 Signal emitted when a camera is removed in CameraManager
> + *
> + * This signal is emitted when a camera is removed from the CameraManager.
> + * A pointer to the removed camera is passed as a parameter.

And something similar here ?

 * \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
> @@ -409,7 +426,9 @@ void CameraManager::addCamera(std::shared_ptr<Camera> camera, dev_t devnum)
>  {
>  	ASSERT(Thread::current() == p_.get());
>  
> +	Camera *cam = camera.get();
>  	p_->addCamera(camera, devnum);
> +	cameraAdded.emit(cam);

You don't need a local cam variable as camera is a shared pointer, not a
unique pointer (and if it was a unique pointer and was given to
addCamera() with std::move, it would be potentially unsafe to use it
afterwards).

	cameraAdded.emit(camera.get());

is fine.

>  }
>  
>  /**
> @@ -427,6 +446,7 @@ void CameraManager::removeCamera(Camera *camera)
>  	ASSERT(Thread::current() == p_.get());
>  
>  	p_->removeCamera(camera);
> +	cameraRemoved.emit(camera);
>  }
>  
>  /**

-- 
Regards,

Laurent Pinchart