[libcamera-devel] [RFC PATCH v2 1/5] libcamera: pipeline: uvcvideo: Add UVC metadata node
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Mon Aug 28 22:33:37 CEST 2023
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 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;
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!
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list