[libcamera-devel] [PATCH 1/2] v4l2: Use SystemDevices properties to identify cameras

[Laurent Pinchart]

Hi Kieran,

Thank you for the patch.

On Thu, Jun 15, 2023 at 11:51:32PM +0100, Kieran Bingham via libcamera-devel wrote:
> The CameraManager->get(dev_t) helper was implemented only to support the
> V4L2 Adaptation layer, and has been deprecated now that a new camera
> property - SystemDevices has been introduced.
> 
> Rework the implementation of getCameraIndex() to use the SystemDevices
> property and remove reliance on the now deprecated call.
> 
> Signed-off-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> ---
>  src/v4l2/v4l2_compat_manager.cpp | 28 +++++++++++++++++++++++-----
>  1 file changed, 23 insertions(+), 5 deletions(-)
> 
> diff --git a/src/v4l2/v4l2_compat_manager.cpp b/src/v4l2/v4l2_compat_manager.cpp
> index 0f7575c54b22..c49124290b42 100644
> --- a/src/v4l2/v4l2_compat_manager.cpp
> +++ b/src/v4l2/v4l2_compat_manager.cpp
> @@ -24,6 +24,7 @@
>  
>  #include <libcamera/camera.h>
>  #include <libcamera/camera_manager.h>
> +#include <libcamera/property_ids.h>
>  
>  #include "v4l2_camera_file.h"
>  
> @@ -113,14 +114,31 @@ int V4L2CompatManager::getCameraIndex(int fd)
>  	if (ret < 0)
>  		return -1;
>  
> -	std::shared_ptr<Camera> target = cm_->get(statbuf.st_rdev);
> -	if (!target)
> -		return -1;
> +	const dev_t devnum = statbuf.st_rdev;

	const int64_t devnum = static_cast<int64_t>(statbuf.st_rdev);

will save you a static_cast<> in the loop below. Up to you.

>  
> +	/*
> +	 * Iterate each known camera and identify if it reports this nodes
> +	 * device number in its list of SystemDevices.
> +	 */
>  	auto cameras = cm_->cameras();
>  	for (auto [index, camera] : utils::enumerate(cameras)) {
> -		if (camera == target)
> -			return index;
> +		const auto devices = camera->properties()
> +					     .get(properties::SystemDevices)
> +					     .value_or(std::vector<int64_t>{});

Can we be explicit ?

		const std::vector<int64_t> devices = camera->properties()
					.get(properties::SystemDevices)
					.value_or(std::vector<int64_t>{});

> +
> +		/*
> +		 * While there may be multiple Cameras that could reference the
> +		 * same device node, we take a first match as a best effort for
> +		 * now.
> +		 *
> +		 * \todo Consider reworking the V4L2 adaptation layer to stop
> +		 * trying to map video nodes directly to a camera and instead
> +		 * hide all devices that may be used by libcamera and expose a
> +		 * consistent 1:1 mapping with each Camera instance.

I'm not sure what you mean here, could you elaborate ?

> +		 */
> +		for (const int64_t dev : devices)
> +			if (dev == static_cast<int64_t>(devnum))
> +				return index;

		for (const int64_t dev : devices) {
			if (dev == static_cast<int64_t>(devnum))
				return index;
		}

>  	}
>  
>  	return -1;

-- 
Regards,

Laurent Pinchart