[libcamera-devel] [RFC PATCH 1/5] libcamera: pipeline: uvcvideo: Add UVC metadata node

[Kieran Bingham]

Hi Gabby,

Quoting Gabby George (2023-08-14 12:28:45)
> Identify and open the UVC metadata's video node. This will give us
> access to metadata associated with the video stream of the uvc camera.
> The user will not have access to this video node and will not need to
> manage its data in any way.
> 
> Signed-off-by: Gabby George <gabbymg94 at gmail.com>
> ---
>  src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 36 +++++++++++++++++++-
>  1 file changed, 35 insertions(+), 1 deletion(-)
> 
> diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> index 38f48a5d..4470d8a2 100644
> --- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> +++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> @@ -49,10 +49,13 @@ public:
>         const std::string &id() const { return id_; }
>  
>         std::unique_ptr<V4L2VideoDevice> video_;
> +       std::unique_ptr<V4L2VideoDevice> metadata_;
>         Stream stream_;
>         std::map<PixelFormat, std::vector<SizeRange>> formats_;
>  
>  private:
> +       int initMetadata(MediaDevice *media);
> +
>         bool generateId();
>  
>         std::string id_;
> @@ -411,6 +414,37 @@ bool PipelineHandlerUVC::match(DeviceEnumerator *enumerator)
>         return true;
>  }
>  
> +int UVCCameraData::initMetadata(MediaDevice *media)
> +{
> +       int ret;
> +
> +       const std::vector<MediaEntity *> &entities = media->entities();
> +       std::string dev_node_name = video_->deviceNode();
> +       auto metadata = std::find_if(entities.begin(), entities.end(),
> +                                    [&dev_node_name](MediaEntity *e) {
> +                                            return e->type() == MediaEntity::Type::V4L2VideoDevice && !(e->flags() & MEDIA_ENT_FL_DEFAULT);
> +                                    });
> +
> +       if (metadata == entities.end()) {
> +               LOG(UVC, Error) << "Could not find a metadata video device.";
> +               return -ENODEV;
> +       }
> +
> +       /* configure the metadata node */
> +       metadata_ = std::make_unique<V4L2VideoDevice>(*metadata);
> +       ret = metadata_->open();
> +       if (ret)
> +               return ret;
> +
> +       if (!(metadata_->caps().isMeta())) {
> +               /* if the caps do not have the metadata attribute
> +                * (shouldn't happen) */

Keep an eye on the coding styles, multiline comments should be formatted
differently. Try not to be too succint with comments, we like to keep to
full sentences with valid grammar in the code base. Any comments should
explain reasoning and the situation.

Perhaps:

	/*
	 * UVC Devices are usually only anticipated to expose two
	 * devices, so we assume the non-default device is the metadata
	 * device node.
	 */

I do wonder if there are more complex devices out there that might break
this assumption, though I haven't personally come across any.

Laurent - any feedback here? Can we continue with this assumption? or do
we have to do a full search here, opening every entity to check each
caps?

> +               metadata_ = NULL;
> +               return -EINVAL;
> +       }
> +       return 0;
> +}
> +
>  int UVCCameraData::init(MediaDevice *media)
>  {
>         int ret;
> @@ -512,7 +546,7 @@ int UVCCameraData::init(MediaDevice *media)
>  
>         controlInfo_ = ControlInfoMap(std::move(ctrls), controls::controls);
>  
> -       return 0;
> +       return initMetadata(media);

Hrm .... if this fails - we can no longer open the camera at all.

Maybe we should just report an error and continue without the metadata
node just as we already do?

--
Kieran


>  }
>  
>  bool UVCCameraData::generateId()
> -- 
> 2.34.1
>