| Message ID | 20230821131039.127370-2-gabbymg94@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series |
|
| Related | show |
Hello Gabby, Thanks for your patch. On Mon, Aug 21, 2023 at 6:40 PM Gabby George <gabbymg94@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@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 ? > + 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. > + 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 <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 On Mon, Aug 21, 2023 at 6:40 PM Gabby George <gabbymg94@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@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 >
On Mon, Aug 21, 2023 at 6:49 AM Vedant Paranjape < vedantparanjape160201@gmail.com> wrote: > Hello Gabby, > > Thanks for your patch. > > On Mon, Aug 21, 2023 at 6:40 PM Gabby George <gabbymg94@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@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? > <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@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@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 > > >
Hello Gabby, On Sun, Aug 27, 2023 at 9:59 AM Gabrielle George <gabbymg94@gmail.com> wrote: > > > > On Mon, Aug 21, 2023 at 6:49 AM Vedant Paranjape <vedantparanjape160201@gmail.com> wrote: >> >> Hello Gabby, >> >> Thanks for your patch. >> >> On Mon, Aug 21, 2023 at 6:40 PM Gabby George <gabbymg94@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@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@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@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
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@gmail.com> wrote: > > > > > > > > On Mon, Aug 21, 2023 at 6:49 AM Vedant Paranjape <vedantparanjape160201@gmail.com> wrote: > >> > >> Hello Gabby, > >> > >> Thanks for your patch. > >> > >> On Mon, Aug 21, 2023 at 6:40 PM Gabby George <gabbymg94@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@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@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@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
Hello, On Mon, Aug 28, 2023 at 10:16:13AM +0200, Jacopo Mondi wrote: > On Mon, Aug 28, 2023 at 12:28:49AM +0530, Vedant Paranjape via libcamera-devel wrote: > > On Sun, Aug 27, 2023 at 9:59 AM Gabrielle George wrote: > > > On Mon, Aug 21, 2023 at 6:49 AM Vedant Paranjape wrote: > > >> On Mon, Aug 21, 2023 at 6:40 PM Gabby George 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. s/uvc/UVC/ (as UVC is an acronym) > > >> > 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@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; You can declare ret on first use. > > >> > + > > >> > + const std::vector<MediaEntity *> &entities = media->entities(); > > >> > + std::string dev_node_name = video_->deviceNode(); devNodeName. We use camelCase. And this should be a const reference to avoid a copy. > > >> > + auto metadata = std::find_if(entities.begin(), entities.end(), > > >> > + [&dev_node_name](MediaEntity *e) { You don't use dev_node_name anywhere. > > >> > + return e->type() == MediaEntity::Type::V4L2VideoDevice > > >> > + && !(e->flags() & MEDIA_ENT_FL_DEFAULT); > > >> > + }); That won't work correctly if the UVC device has multiple image capture device nodes. Only one of the image capture device nodes will be marked as default, the other one may get picked by this code. There's nothing in the kernel uvcvideo driver that currently exposes the relationship between the image capture device node and the metadata device node :-S This should be fixed, but that's out of scope for your current work. Let's thus record this in a comment here: /* * \todo The kernel doesn't expose the relationship between image * capture video devices and metadata capture video devices. Until this * gets fixed on the kernel side, do our best by picking one metadata * capture video device. */ As for how to find a metadata capture device node, you can simply check the number of pads. The entities corresponding to metadata devices currently have no pad. > > >> > + > > >> > + if (metadata == entities.end()) { > > >> > + return -ENODEV; > > >> > + } No need for curly braces. > > >> > + > > >> > + /* 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. As far as I can tell, std::make_unique<>() never returns a null std::unique_ptr<>. > > >> > + if (ret) > > >> > + return ret; > > >> > + > > >> > + if (!(metadata_->caps().isMeta())) { No need for the outer parentheses. > > >> > + /* > > >> > + * 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. This should have been caught by checkstyle.py. Are you using it through the git post-commit hook ? > > >> > + 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. Correct. I would however move this to the initMetadata() function to make it self-contained, it's not a good practice to make the caller aware of the internal implementation of the callee. > > 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 > > > >> <snip> > > >> > + ret = metadata_->open(); > > >> > + if (ret) > > >> > + return ret; > > >> </snip> > > >> > > >> > + LOG(UVC, Error) << "Could not find a metadata video device."; > > >> > + } > > >> > + > > >> > return 0; > > >> > } > > >> > > > > > > > Thanks for the suggestions!
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@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@gmail.com> > wrote: > > > > > > > > > > > > On Mon, Aug 21, 2023 at 6:49 AM Vedant Paranjape < > vedantparanjape160201@gmail.com> wrote: > > >> > > >> Hello Gabby, > > >> > > >> Thanks for your patch. > > >> > > >> On Mon, Aug 21, 2023 at 6:40 PM Gabby George <gabbymg94@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@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@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@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 >
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; }
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@gmail.com> --- src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 42 ++++++++++++++++++++ 1 file changed, 42 insertions(+)