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

[Umang Jain]

Hi Laurent,

Thanks for the feedback.

On Wed, May 6, 2020 at 23:53, Laurent Pinchart 
<laurent.pinchart at ideasonboard.com> wrote:
> 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 
>> <mailto: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?

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

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20200508/7a4fb49d/attachment-0001.htm>