<div dir="ltr"><div dir="ltr"><br></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Mon, Aug 21, 2023 at 6:49 AM Vedant Paranjape <<a href="mailto:vedantparanjape160201@gmail.com">vedantparanjape160201@gmail.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 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></blockquote><div><br></div><div>Good call! I think we should.<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<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></blockquote><div><br></div><div>I'll make sure it's correct in the next patch. <br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<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></blockquote><div><br></div><div>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></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<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></blockquote><div><br></div><div>Thanks for the suggestions! <br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<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>
</blockquote></div></div>