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

[Vedant Paranjape]

Hello Gabby,

Thanks for your patch.

On Mon, Aug 21, 2023 at 6:40 PM Gabby George <gabbymg94 at gmail.com> wrote:
>
> 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 | 42 ++++++++++++++++++++
>  1 file changed, 42 insertions(+)
>
> diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> index 38f48a5d..dbe0cc8c 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,39 @@ 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()) {
> +               return -ENODEV;
> +       }
> +
> +       /* configure the metadata node */
> +       metadata_ = std::make_unique<V4L2VideoDevice>(*metadata);
> +       ret = metadata_->open();

Should we check for validity of metadata_ with a simple if (metadata_)
? or is it overengineering ?

> +       if (ret)
> +               return ret;
> +
> +       if (!(metadata_->caps().isMeta())) {
> +               /*
> +                * UVC Devices are usually only anticipated to expose two
> +         * devices, so we assume the non-default device is the metadata
> +         * device node
> +                */

The comment seems misaligned in indentation, please fix it.

> +               return -EINVAL;
> +       }
> +       return 0;
> +}
> +
>  int UVCCameraData::init(MediaDevice *media)
>  {
>         int ret;
> @@ -512,6 +548,12 @@ int UVCCameraData::init(MediaDevice *media)
>
>         controlInfo_ = ControlInfoMap(std::move(ctrls), controls::controls);
>
> +       ret = initMetadata(media);
> +       if (ret) {
> +               metadata_ = nullptr;

I think, you are leaking metadata_ if the initMetadata function
returns from failure of
<snip>
> +       ret = metadata_->open();
> +       if (ret)
> +               return ret;
</snip>

> +               LOG(UVC, Error) << "Could not find a metadata video device.";
> +       }
> +
>         return 0;
>  }
>
> --
> 2.34.1
>

Regards,
Vedant Paranjape

On Mon, Aug 21, 2023 at 6:40 PM Gabby George <gabbymg94 at gmail.com> wrote:
>
> 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 | 42 ++++++++++++++++++++
>  1 file changed, 42 insertions(+)
>
> diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> index 38f48a5d..dbe0cc8c 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,39 @@ 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()) {
> +               return -ENODEV;
> +       }
> +
> +       /* configure the metadata node */
> +       metadata_ = std::make_unique<V4L2VideoDevice>(*metadata);
> +       ret = metadata_->open();
> +       if (ret)
> +               return ret;
> +
> +       if (!(metadata_->caps().isMeta())) {
> +               /*
> +                * UVC Devices are usually only anticipated to expose two
> +         * devices, so we assume the non-default device is the metadata
> +         * device node
> +                */
> +               return -EINVAL;
> +       }
> +       return 0;
> +}
> +
>  int UVCCameraData::init(MediaDevice *media)
>  {
>         int ret;
> @@ -512,6 +548,12 @@ int UVCCameraData::init(MediaDevice *media)
>
>         controlInfo_ = ControlInfoMap(std::move(ctrls), controls::controls);
>
> +       ret = initMetadata(media);
> +       if (ret) {
> +               metadata_ = nullptr;
> +               LOG(UVC, Error) << "Could not find a metadata video device.";
> +       }
> +
>         return 0;
>  }
>
> --
> 2.34.1
>