[libcamera-devel] [RFC PATCH v2 1/5] libcamera: pipeline: uvcvideo: Add UVC metadata node
Jacopo Mondi
jacopo.mondi at ideasonboard.com
Mon Aug 28 10:16:13 CEST 2023
Hello
On Mon, Aug 28, 2023 at 12:28:49AM +0530, Vedant Paranjape via libcamera-devel wrote:
> 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?
>From https://en.cppreference.com/w/cpp/memory/unique_ptr/operator%3D
(3) unique_ptr& operator=( std::nullptr_t ) noexcept;
3) Effectively the same as calling reset().
https://en.cppreference.com/w/cpp/memory/unique_ptr/reset
If the old pointer was non-empty, deletes the previously managed
object if (old_ptr) get_deleter()(old_ptr).
if metadata_ is a unique_ptr, assigning nullptr to it will delete the object
it previously managed.
>
> 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)
>
Try to use unique_ptr, shared_ptr is harder to follow as it has to do
reference counting. With unique_ptr<> I see
callq operator delete(void*)@PLT
Thanks
j
> >>
> >> <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