[libcamera-devel] [PATCH 1/2] v4l2: Use SystemDevices properties to identify cameras

[Kieran Bingham]

Quoting Laurent Pinchart (2023-06-16 15:09:16)
> Hi Kieran,
> 
> Thank you for the patch.
> 
> On Thu, Jun 15, 2023 at 11:51:32PM +0100, Kieran Bingham via libcamera-devel wrote:
> > The CameraManager->get(dev_t) helper was implemented only to support the
> > V4L2 Adaptation layer, and has been deprecated now that a new camera
> > property - SystemDevices has been introduced.
> > 
> > Rework the implementation of getCameraIndex() to use the SystemDevices
> > property and remove reliance on the now deprecated call.
> > 
> > Signed-off-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> > ---
> >  src/v4l2/v4l2_compat_manager.cpp | 28 +++++++++++++++++++++++-----
> >  1 file changed, 23 insertions(+), 5 deletions(-)
> > 
> > diff --git a/src/v4l2/v4l2_compat_manager.cpp b/src/v4l2/v4l2_compat_manager.cpp
> > index 0f7575c54b22..c49124290b42 100644
> > --- a/src/v4l2/v4l2_compat_manager.cpp
> > +++ b/src/v4l2/v4l2_compat_manager.cpp
> > @@ -24,6 +24,7 @@
> >  
> >  #include <libcamera/camera.h>
> >  #include <libcamera/camera_manager.h>
> > +#include <libcamera/property_ids.h>
> >  
> >  #include "v4l2_camera_file.h"
> >  
> > @@ -113,14 +114,31 @@ int V4L2CompatManager::getCameraIndex(int fd)
> >       if (ret < 0)
> >               return -1;
> >  
> > -     std::shared_ptr<Camera> target = cm_->get(statbuf.st_rdev);
> > -     if (!target)
> > -             return -1;
> > +     const dev_t devnum = statbuf.st_rdev;
> 
>         const int64_t devnum = static_cast<int64_t>(statbuf.st_rdev);
> 
> will save you a static_cast<> in the loop below. Up to you.
> 
> >  
> > +     /*
> > +      * Iterate each known camera and identify if it reports this nodes
> > +      * device number in its list of SystemDevices.
> > +      */
> >       auto cameras = cm_->cameras();
> >       for (auto [index, camera] : utils::enumerate(cameras)) {
> > -             if (camera == target)
> > -                     return index;
> > +             const auto devices = camera->properties()
> > +                                          .get(properties::SystemDevices)
> > +                                          .value_or(std::vector<int64_t>{});
> 
> Can we be explicit ?
> 
>                 const std::vector<int64_t> devices = camera->properties()
>                                         .get(properties::SystemDevices)
>                                         .value_or(std::vector<int64_t>{});
> 
> > +
> > +             /*
> > +              * While there may be multiple Cameras that could reference the
> > +              * same device node, we take a first match as a best effort for
> > +              * now.
> > +              *
> > +              * \todo Consider reworking the V4L2 adaptation layer to stop
> > +              * trying to map video nodes directly to a camera and instead
> > +              * hide all devices that may be used by libcamera and expose a
> > +              * consistent 1:1 mapping with each Camera instance.
> 
> I'm not sure what you mean here, could you elaborate ?

What I mean is I think this could all be improved. In particular to stop
the issue where if mulitple cameras reference the same video node
(unicam for instance) they would only ever work for the first one.

If there were time on this I would investigate making this layer
provides a better mapping where each camera is /dev/videoN directly.



> > +              */
> > +             for (const int64_t dev : devices)
> > +                     if (dev == static_cast<int64_t>(devnum))
> > +                             return index;
> 
>                 for (const int64_t dev : devices) {
>                         if (dev == static_cast<int64_t>(devnum))
>                                 return index;
>                 }
> 
> >       }
> >  
> >       return -1;
> 
> -- 
> Regards,
> 
> Laurent Pinchart