[libcamera-devel] [RFC PATCH v2 1/5] libcamera: pipeline: uvcvideo: Add UVC metadata node
Vedant Paranjape
vedantparanjape160201 at gmail.com
Tue Aug 29 19:59:23 CEST 2023
Hello Jacopo,
This is new, thanks for the info. Need to read the fineprint :)
Regards,
Vedant Paranjape
On Mon, Aug 28, 2023 at 1:46 PM Jacopo Mondi <jacopo.mondi at ideasonboard.com>
wrote:
> 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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20230829/600f761b/attachment.htm>
More information about the libcamera-devel
mailing list