[libcamera-devel] [PATCH v2 3/5] libcamera: camera_manager: Introduce signals when a camera is added/removed

[Umang Jain]

Hi Kieran,

On Mon, 2020-05-18 at 16:44 +0100, Kieran Bingham wrote:
> Hi Umang,
> 
> On 13/05/2020 18:29, Umang Jain wrote:
> > Emit 'cameraAdded' and 'cameraRemoved' from CameraManager to enable
> > hotplug and hot-unplug support in appplications like QCam.
> 
> s/appplications/applications/
> 
> 
> Hrm ... seeing cameraAdded and cameraRemoved, makes me think back to
> my
> earlier signal name bikeshedding ;-) Perhaps that one should be
> deviceAdded and deviceRemoved (was newDevicesFound?)
> 
> Anyway, let the bikeshedding happen on that patch instead ;-)

The DeviceEnumerator::newDevicesFound is somewhat internal to libcamera
(i.e. not meant to be used by applications - only by CameraManager).
'cameraAdded' and 'cameraRemoved' are application facing signal-probes
that denote hotplugging. Maybe using 'newCameraAdded'
and 'cameraRemoved' for CameraManager will make more things clear? 

As far as DeviceEnumerator:newDevicesFound is concerned, I think:
s/newDeviceFound/deviceAdded/ should do too.

> 
> > Signed-off-by: Umang Jain <email at uajain.com>
> > ---
> >  include/libcamera/camera_manager.h |  6 +++++-
> >  src/libcamera/camera_manager.cpp   | 34
> > +++++++++++++++++++++++++++---
> >  src/libcamera/pipeline_handler.cpp |  2 +-
> >  3 files changed, 37 insertions(+), 5 deletions(-)
> > 
> > diff --git a/include/libcamera/camera_manager.h
> > b/include/libcamera/camera_manager.h
> > index 079f848..366fa07 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 {
> >  
> > @@ -35,13 +36,16 @@ public:
> >  	std::shared_ptr<Camera> get(dev_t devnum);
> >  
> >  	void addCamera(std::shared_ptr<Camera> camera, dev_t devnum);
> > -	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 a13cfe1..b9d2496 100644
> > --- a/src/libcamera/camera_manager.cpp
> > +++ b/src/libcamera/camera_manager.cpp
> > @@ -186,7 +186,7 @@ void
> > CameraManager::Private::addCamera(std::shared_ptr<Camera> &camera,
> >  		}
> >  	}
> >  
> > -	cameras_.push_back(std::move(camera));
> > +	cameras_.push_back(camera);
> >  
> >  	if (devnum) {
> >  		unsigned int index = cameras_.size() - 1;
> > @@ -376,6 +376,32 @@ std::shared_ptr<Camera>
> > CameraManager::get(dev_t devnum)
> >  	return iter->second.lock();
> >  }
> >  
> > +/**
> > + * \brief Notify of a new camera added to the system
> 
> Missing the \var to reference the signal.
> 
> > + *
> > + * 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.
> > + */
> > +
> > +/**
> 
> And missing here too.
> 
> > + * \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 +421,7 @@ void
> > CameraManager::addCamera(std::shared_ptr<Camera> camera, dev_t
> > devnum)
> >  	ASSERT(Thread::current() == p_.get());
> >  
> >  	p_->addCamera(camera, devnum);
> > +	cameraAdded.emit(camera);
> >  }
> >  
> >  /**
> > @@ -407,11 +434,12 @@ void
> > CameraManager::addCamera(std::shared_ptr<Camera> camera, dev_t
> > devnum)
> >   *
> >   * \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);
> 
> Is there any race here, or potential problem emitting the signal with
> the camera, after it's been removed from the CameraManager? (I
> hope/expect not as it's using a shared_ptr ... But I haven't checked
> to
> see if there are any implications caused by the removeCamera call)
> 
> 
> >  }
> >  
> >  /**
> > diff --git a/src/libcamera/pipeline_handler.cpp
> > b/src/libcamera/pipeline_handler.cpp
> > index 254d341..a9187e1 100644
> > --- a/src/libcamera/pipeline_handler.cpp
> > +++ b/src/libcamera/pipeline_handler.cpp
> > @@ -555,7 +555,7 @@ void PipelineHandler::disconnect()
> >  			continue;
> >  
> >  		camera->disconnect();
> > -		manager_->removeCamera(camera.get());
> > +		manager_->removeCamera(camera);
> >  	}
> >  
> >  	cameras_.clear();
> >