<div dir="ltr"><div>Hello Jacopo,</div><div><br></div><div>This is new, thanks for the info. Need to read the fineprint :)<br></div><div><br></div><div>Regards,</div><div>Vedant Paranjape<br></div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Mon, Aug 28, 2023 at 1:46 PM Jacopo Mondi <<a href="mailto:jacopo.mondi@ideasonboard.com">jacopo.mondi@ideasonboard.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Hello<br>
<br>
On Mon, Aug 28, 2023 at 12:28:49AM +0530, Vedant Paranjape via libcamera-devel wrote:<br>
> Hello Gabby,<br>
><br>
> On Sun, Aug 27, 2023 at 9:59 AM Gabrielle George <<a href="mailto:gabbymg94@gmail.com" target="_blank">gabbymg94@gmail.com</a>> wrote:<br>
> ><br>
> ><br>
> ><br>
> > On Mon, Aug 21, 2023 at 6:49 AM Vedant Paranjape <<a href="mailto:vedantparanjape160201@gmail.com" target="_blank">vedantparanjape160201@gmail.com</a>> wrote:<br>
> >><br>
> >> Hello Gabby,<br>
> >><br>
> >> Thanks for your patch.<br>
> >><br>
> >> On Mon, Aug 21, 2023 at 6:40 PM Gabby George <<a href="mailto:gabbymg94@gmail.com" target="_blank">gabbymg94@gmail.com</a>> wrote:<br>
> >> ><br>
> >> > Identify and open the UVC metadata's video node. This will give us<br>
> >> > access to metadata associated with the video stream of the uvc camera.<br>
> >> > The user will not have access to this video node and will not need to<br>
> >> > manage its data in any way.<br>
> >> ><br>
> >> > Signed-off-by: Gabby George <<a href="mailto:gabbymg94@gmail.com" target="_blank">gabbymg94@gmail.com</a>><br>
> >> > ---<br>
> >> > src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 42 ++++++++++++++++++++<br>
> >> > 1 file changed, 42 insertions(+)<br>
> >> ><br>
> >> > diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp<br>
> >> > index 38f48a5d..dbe0cc8c 100644<br>
> >> > --- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp<br>
> >> > +++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp<br>
> >> > @@ -49,10 +49,13 @@ public:<br>
> >> > const std::string &id() const { return id_; }<br>
> >> ><br>
> >> > std::unique_ptr<V4L2VideoDevice> video_;<br>
> >> > + std::unique_ptr<V4L2VideoDevice> metadata_;<br>
> >> > Stream stream_;<br>
> >> > std::map<PixelFormat, std::vector<SizeRange>> formats_;<br>
> >> ><br>
> >> > private:<br>
> >> > + int initMetadata(MediaDevice *media);<br>
> >> > +<br>
> >> > bool generateId();<br>
> >> ><br>
> >> > std::string id_;<br>
> >> > @@ -411,6 +414,39 @@ bool PipelineHandlerUVC::match(DeviceEnumerator *enumerator)<br>
> >> > return true;<br>
> >> > }<br>
> >> ><br>
> >> > +int UVCCameraData::initMetadata(MediaDevice *media)<br>
> >> > +{<br>
> >> > + int ret;<br>
> >> > +<br>
> >> > + const std::vector<MediaEntity *> &entities = media->entities();<br>
> >> > + std::string dev_node_name = video_->deviceNode();<br>
> >> > + auto metadata = std::find_if(entities.begin(), entities.end(),<br>
> >> > + [&dev_node_name](MediaEntity *e) {<br>
> >> > + return e->type() == MediaEntity::Type::V4L2VideoDevice<br>
> >> > + && !(e->flags() & MEDIA_ENT_FL_DEFAULT);<br>
> >> > + });<br>
> >> > +<br>
> >> > + if (metadata == entities.end()) {<br>
> >> > + return -ENODEV;<br>
> >> > + }<br>
> >> > +<br>
> >> > + /* configure the metadata node */<br>
> >> > + metadata_ = std::make_unique<V4L2VideoDevice>(*metadata);<br>
> >> > + ret = metadata_->open();<br>
> >><br>
> >> Should we check for validity of metadata_ with a simple if (metadata_)<br>
> >> ? or is it overengineering ?<br>
> ><br>
> ><br>
> > Good call! I think we should.<br>
> >><br>
> >><br>
> >> > + if (ret)<br>
> >> > + return ret;<br>
> >> > +<br>
> >> > + if (!(metadata_->caps().isMeta())) {<br>
> >> > + /*<br>
> >> > + * UVC Devices are usually only anticipated to expose two<br>
> >> > + * devices, so we assume the non-default device is the metadata<br>
> >> > + * device node<br>
> >> > + */<br>
> >><br>
> >> The comment seems misaligned in indentation, please fix it.<br>
> ><br>
> ><br>
> > I'll make sure it's correct in the next patch.<br>
> >><br>
> >><br>
> >> > + return -EINVAL;<br>
> >> > + }<br>
> >> > + return 0;<br>
> >> > +}<br>
> >> > +<br>
> >> > int UVCCameraData::init(MediaDevice *media)<br>
> >> > {<br>
> >> > int ret;<br>
> >> > @@ -512,6 +548,12 @@ int UVCCameraData::init(MediaDevice *media)<br>
> >> ><br>
> >> > controlInfo_ = ControlInfoMap(std::move(ctrls), controls::controls);<br>
> >> ><br>
> >> > + ret = initMetadata(media);<br>
> >> > + if (ret) {<br>
> >> > + metadata_ = nullptr;<br>
> >><br>
> >> I think, you are leaking metadata_ if the initMetadata function<br>
> >> returns from failure of<br>
> ><br>
> ><br>
> > 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?<br>
<br>
>From <a href="https://en.cppreference.com/w/cpp/memory/unique_ptr/operator%3D" rel="noreferrer" target="_blank">https://en.cppreference.com/w/cpp/memory/unique_ptr/operator%3D</a><br>
<br>
(3) unique_ptr& operator=( std::nullptr_t ) noexcept;<br>
3) Effectively the same as calling reset().<br>
<br>
<a href="https://en.cppreference.com/w/cpp/memory/unique_ptr/reset" rel="noreferrer" target="_blank">https://en.cppreference.com/w/cpp/memory/unique_ptr/reset</a><br>
If the old pointer was non-empty, deletes the previously managed<br>
object if (old_ptr) get_deleter()(old_ptr).<br>
<br>
if metadata_ is a unique_ptr, assigning nullptr to it will delete the object<br>
it previously managed.<br>
<br>
><br>
> <a href="https://godbolt.org/z/rhx886abY" rel="noreferrer" target="_blank">https://godbolt.org/z/rhx886abY</a><br>
><br>
> I tried doing something similar, check the godbolt link, assigning<br>
> null has no effect on the allocated memory. (cc: @Laurent Pinchart and<br>
> @Kieran Bingham can you guys have a look once)<br>
><br>
<br>
Try to use unique_ptr, shared_ptr is harder to follow as it has to do<br>
reference counting. With unique_ptr<> I see<br>
<br>
callq operator delete(void*)@PLT<br>
<br>
Thanks<br>
j<br>
<br>
<br>
> >><br>
> >> <snip><br>
> >> > + ret = metadata_->open();<br>
> >> > + if (ret)<br>
> >> > + return ret;<br>
> >> </snip><br>
> >><br>
> >> > + LOG(UVC, Error) << "Could not find a metadata video device.";<br>
> >> > + }<br>
> >> > +<br>
> >> > return 0;<br>
> >> > }<br>
> >> ><br>
> >> > --<br>
> >> > 2.34.1<br>
> >> ><br>
> >><br>
> >> Regards,<br>
> >> Vedant Paranjape<br>
> ><br>
> ><br>
> > Thanks for the suggestions!<br>
> >><br>
> >><br>
> >> On Mon, Aug 21, 2023 at 6:40 PM Gabby George <<a href="mailto:gabbymg94@gmail.com" target="_blank">gabbymg94@gmail.com</a>> wrote:<br>
> >> ><br>
> >> > Identify and open the UVC metadata's video node. This will give us<br>
> >> > access to metadata associated with the video stream of the uvc camera.<br>
> >> > The user will not have access to this video node and will not need to<br>
> >> > manage its data in any way.<br>
> >> ><br>
> >> > Signed-off-by: Gabby George <<a href="mailto:gabbymg94@gmail.com" target="_blank">gabbymg94@gmail.com</a>><br>
> >> > ---<br>
> >> > src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 42 ++++++++++++++++++++<br>
> >> > 1 file changed, 42 insertions(+)<br>
> >> ><br>
> >> > diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp<br>
> >> > index 38f48a5d..dbe0cc8c 100644<br>
> >> > --- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp<br>
> >> > +++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp<br>
> >> > @@ -49,10 +49,13 @@ public:<br>
> >> > const std::string &id() const { return id_; }<br>
> >> ><br>
> >> > std::unique_ptr<V4L2VideoDevice> video_;<br>
> >> > + std::unique_ptr<V4L2VideoDevice> metadata_;<br>
> >> > Stream stream_;<br>
> >> > std::map<PixelFormat, std::vector<SizeRange>> formats_;<br>
> >> ><br>
> >> > private:<br>
> >> > + int initMetadata(MediaDevice *media);<br>
> >> > +<br>
> >> > bool generateId();<br>
> >> ><br>
> >> > std::string id_;<br>
> >> > @@ -411,6 +414,39 @@ bool PipelineHandlerUVC::match(DeviceEnumerator *enumerator)<br>
> >> > return true;<br>
> >> > }<br>
> >> ><br>
> >> > +int UVCCameraData::initMetadata(MediaDevice *media)<br>
> >> > +{<br>
> >> > + int ret;<br>
> >> > +<br>
> >> > + const std::vector<MediaEntity *> &entities = media->entities();<br>
> >> > + std::string dev_node_name = video_->deviceNode();<br>
> >> > + auto metadata = std::find_if(entities.begin(), entities.end(),<br>
> >> > + [&dev_node_name](MediaEntity *e) {<br>
> >> > + return e->type() == MediaEntity::Type::V4L2VideoDevice<br>
> >> > + && !(e->flags() & MEDIA_ENT_FL_DEFAULT);<br>
> >> > + });<br>
> >> > +<br>
> >> > + if (metadata == entities.end()) {<br>
> >> > + return -ENODEV;<br>
> >> > + }<br>
> >> > +<br>
> >> > + /* configure the metadata node */<br>
> >> > + metadata_ = std::make_unique<V4L2VideoDevice>(*metadata);<br>
> >> > + ret = metadata_->open();<br>
> >> > + if (ret)<br>
> >> > + return ret;<br>
> >> > +<br>
> >> > + if (!(metadata_->caps().isMeta())) {<br>
> >> > + /*<br>
> >> > + * UVC Devices are usually only anticipated to expose two<br>
> >> > + * devices, so we assume the non-default device is the metadata<br>
> >> > + * device node<br>
> >> > + */<br>
> >> > + return -EINVAL;<br>
> >> > + }<br>
> >> > + return 0;<br>
> >> > +}<br>
> >> > +<br>
> >> > int UVCCameraData::init(MediaDevice *media)<br>
> >> > {<br>
> >> > int ret;<br>
> >> > @@ -512,6 +548,12 @@ int UVCCameraData::init(MediaDevice *media)<br>
> >> ><br>
> >> > controlInfo_ = ControlInfoMap(std::move(ctrls), controls::controls);<br>
> >> ><br>
> >> > + ret = initMetadata(media);<br>
> >> > + if (ret) {<br>
> >> > + metadata_ = nullptr;<br>
> >> > + LOG(UVC, Error) << "Could not find a metadata video device.";<br>
> >> > + }<br>
> >> > +<br>
> >> > return 0;<br>
> >> > }<br>
> >> ><br>
> >> > --<br>
> >> > 2.34.1<br>
> >> ><br>
><br>
> Regards,<br>
> Vedant Paranjape<br>
</blockquote></div>