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

[Gabrielle George]

On Mon, Aug 21, 2023 at 6:49 AM Vedant Paranjape <
vedantparanjape160201 at gmail.com> wrote:

> 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 ?
>

Good call! I think we should.

>
> > +       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.
>

I'll make sure it's correct in the next patch.

>
> > +               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
>

I thought assigning null to smart pointers initiates the normal cleanup
routine of the objects they point to.  Is this the leak you're talking
about, or should I clean up something else too?

> <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
>

Thanks for the suggestions!

>
> 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
> >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20230826/fc25df69/attachment.htm>