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

[Laurent Pinchart]

Hi Umang,

On Fri, May 08, 2020 at 01:47:15PM +0000, Umang Jain wrote:
> On Wed, May 6, 2020 at 23:53, Laurent Pinchart wrote:
> > 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.
> 
> Yes, camera is a shared pointer here and is std::move in p_->addCamera(camera, devnum);
> But I noticed that camera becomes nullptr after p_->addCamera() call and that's
> I would expect. I double-checked with running the codepath with your change,
> and plugging in a camera crashes with a `null` segfault backtrace. Am I missing something?

You were absolutely right, I had missed the fact that p_->addCamera()
takes a non-const reference and uses std::move internally.

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

-- 
Regards,

Laurent Pinchart