[libcamera-devel,1/4] libcamera: device_enumerator: Emit a signal when a new device is hotplugged

3689 diff mbox series

Emit a signal whenever is a new MediaDevice is added to the
DeviceEnumerator. This will allow CameraManager to get notified
about the new devices that have been hot-plugged.

Signed-off-by: Umang Jain <email@uajain.com>
---
 src/libcamera/camera_manager.cpp          | 27 +++++++++++++++++++++--
 src/libcamera/device_enumerator.cpp       | 10 +++++++++
 src/libcamera/include/device_enumerator.h |  3 +++
 3 files changed, 38 insertions(+), 2 deletions(-)

Hi Umang,

Thank you for the patch.

On Wed, May 06, 2020 at 10:33:52AM +0000, Umang Jain wrote:
> Emit a signal whenever is a new MediaDevice is added to the
> DeviceEnumerator. This will allow CameraManager to get notified
> about the new devices that have been hot-plugged.
> 
> Signed-off-by: Umang Jain <email@uajain.com>
> ---
>  src/libcamera/camera_manager.cpp          | 27 +++++++++++++++++++++--
>  src/libcamera/device_enumerator.cpp       | 10 +++++++++
>  src/libcamera/include/device_enumerator.h |  3 +++
>  3 files changed, 38 insertions(+), 2 deletions(-)
> 
> diff --git a/src/libcamera/camera_manager.cpp b/src/libcamera/camera_manager.cpp
> index fddf734..c75979a 100644
> --- a/src/libcamera/camera_manager.cpp
> +++ b/src/libcamera/camera_manager.cpp
> @@ -54,6 +54,7 @@ protected:
>  private:
>  	int init();
>  	void cleanup();
> +	void enumerateNewDevices(DeviceEnumerator *enumerator);

How about calling this enumerateDevices(), and calling it from
CameraManager::Private::init() ? Please move the TODO comment to this
function (and while at it, s/TODO:/\\todo/

>  
>  	CameraManager *cm_;
>  
> @@ -144,14 +145,36 @@ int CameraManager::Private::init()
>  		}
>  	}
>  
> -	/* TODO: register hot-plug callback here */
> +	enumerator_->newDevicesFound.connect(this, &CameraManager::Private::enumerateNewDevices);

You can shorten this line to

	enumerator_->newDevicesFound.connect(this, &Private::enumerateNewDevices);

>  
>  	return 0;
>  }
>  
> +void CameraManager::Private::enumerateNewDevices(DeviceEnumerator *enumerator)
> +{
> +	std::vector<PipelineHandlerFactory *> &factories = PipelineHandlerFactory::factories();
> +
> +	for (PipelineHandlerFactory *factory : factories) {
> +		/*
> +		 * Try each pipeline handler until it exhaust
> +		 * all pipelines it can provide.
> +		 */
> +		while (1) {
> +			std::shared_ptr<PipelineHandler> pipe = factory->create(cm_);
> +			if (!pipe->match(enumerator_.get()))
> +				break;
> +
> +			LOG(Camera, Debug)
> +				<< "Pipeline handler \"" << factory->name()
> +				<< "\" matched";
> +			pipes_.push_back(std::move(pipe));
> +		}
> +	}
> +}
> +
>  void CameraManager::Private::cleanup()
>  {
> -	/* TODO: unregister hot-plug callback here */
> +	enumerator_->newDevicesFound.disconnect(this, &CameraManager::Private::enumerateNewDevices);

And this one to

	enumerator_->newDevicesFound.disconnect(this, &Private::enumerateNewDevices);

>  
>  	/*
>  	 * Release all references to cameras and pipeline handlers to ensure
> diff --git a/src/libcamera/device_enumerator.cpp b/src/libcamera/device_enumerator.cpp
> index dd17e3e..2721120 100644
> --- a/src/libcamera/device_enumerator.cpp
> +++ b/src/libcamera/device_enumerator.cpp
> @@ -227,6 +227,15 @@ std::unique_ptr<MediaDevice> DeviceEnumerator::createDevice(const std::string &d
>  	return media;
>  }
>  
> +/**
> + * \var DeviceEnumerator::newDevicesFound
> + * \brief Signal emitted when a new MediaDevice is added to the DeviceEnumerator
> + *
> + * CameraManager connects to this signal to know about new MediaDevices being plugged-in,
> + * while it is running. CameraManager will then iterate over the DeviceEnumerator, to match
> + * their respective pipeline-handlers and prepare the newly plugged-in device for use.

Could you please wrap lines at a 80 columns limit ? Up to 120 columns
are accepted when that improves readability, but otherwise we try to
keep lines at most 80 characters long.

I think this text should also be reworked a bit to focus on the
DeviceEnumerator side of it. Signals are used to connected components
that are not necessarily tightly integrated, and thus should not make
too many assumptions (and ideally no assumption at all) regarding what
they will be connected to. We could decide tomorrow to rework how the
DeviceEnumerator is used, without changing how it's implemented, and
connect the signal to something else. How about the following ?

 * \brief Notify of new media devices being found
 *
 * This signal is emitted when the device enumerator finds new media devices in
 * the system. It may be emitted for every newly detected device, or once for
 * multiple of devices, at the discretion of the device enumerator. Not all
 * device enumerator types may support dynamic detection of new devices.

And this brings another comment I wanted to make: As matching pipeline
handlers with media devices is an expensive operation, it could be nice
to emit the signal with a small delay, to batch multiple devices
together. That won't bring much improvement for UVC, but for other types
of cameras that could be exposed as multiple media devices, all handled
by a single pipeline handler instance, it could be beneficial. I don't
think it needs to be implemented as part of this series though, it's
likely overkill for now.

> + */
> +
>  /**
>   * \brief Add a media device to the enumerator
>   * \param[in] media media device instance to add
> @@ -242,6 +251,7 @@ void DeviceEnumerator::addDevice(std::unique_ptr<MediaDevice> &&media)
>  		<< "Added device " << media->deviceNode() << ": " << media->driver();
>  
>  	devices_.push_back(std::move(media));
> +	newDevicesFound.emit(this);
>  }
>  
>  /**
> diff --git a/src/libcamera/include/device_enumerator.h b/src/libcamera/include/device_enumerator.h
> index 433e357..6cc6ec2 100644
> --- a/src/libcamera/include/device_enumerator.h
> +++ b/src/libcamera/include/device_enumerator.h
> @@ -11,6 +11,7 @@
>  #include <string>
>  #include <vector>
>  
> +#include <libcamera/signal.h>
>  #include <linux/media.h>

This should be

#include <linux/media.h>

#include <libcamera/signal.h>

(see http://libcamera.org/coding-style.html#order-of-includes)

>  
>  namespace libcamera {
> @@ -43,6 +44,8 @@ public:
>  
>  	std::shared_ptr<MediaDevice> search(const DeviceMatch &dm);
>  
> +	Signal<DeviceEnumerator *> newDevicesFound;

I think you can drop the DeviceEnumerator argument to the signal.

	Signal<> newDevicesFound;

> +
>  protected:
>  	std::unique_ptr<MediaDevice> createDevice(const std::string &deviceNode);
>  	void addDevice(std::unique_ptr<MediaDevice> &&media);