[libcamera-devel] [RFC PATCH v2 1/5] libcamera: pipeline: uvcvideo: Add UVC metadata node
Vedant Paranjape
vedantparanjape160201 at gmail.com
Sun Aug 27 20:58:49 CEST 2023
Hello Gabby,
On Sun, Aug 27, 2023 at 9:59 AM Gabrielle George <gabbymg94 at gmail.com> wrote:
>
>
>
> 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?
https://godbolt.org/z/rhx886abY
I tried doing something similar, check the godbolt link, assigning
null has no effect on the allocated memory. (cc: @Laurent Pinchart and
@Kieran Bingham can you guys have a look once)
>>
>> <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
>> >
Regards,
Vedant Paranjape
More information about the libcamera-devel
mailing list