Date:   Wed, 21 Dec 2022 13:23:24 +0200
From:   Laurent Pinchart <laurent.pinchart@...asonboard.com>
To:     Ricardo Ribalda <ribalda@...omium.org>
Cc:     Yunke Cao <yunkec@...omium.org>,
        Mauro Carvalho Chehab <mchehab@...nel.org>,
        Sergey Senozhatsky <senozhatsky@...omium.org>,
        linux-media@...r.kernel.org,
        Mauro Carvalho Chehab <mchehab+huawei@...nel.org>,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v4 3/3] media: uvcvideo: Add a unique suffix to camera
 names

Hi Ricardo,

On Wed, Dec 21, 2022 at 11:57:48AM +0100, Ricardo Ribalda wrote:
> On Wed, 21 Dec 2022 at 11:55, Laurent Pinchart wrote:
> > On Wed, Dec 21, 2022 at 12:00:58AM +0100, Ricardo Ribalda wrote:
> > > On Tue, 6 Dec 2022 at 00:02, Ricardo Ribalda wrote:
> > > > On Mon, 5 Dec 2022 at 23:16, Laurent Pinchart wrote:
> > > > > On Fri, Dec 02, 2022 at 06:08:19PM +0100, Ricardo Ribalda wrote:
> > > > > > Some cameras have multiple data inputs (i.e. IR sensor and RGB sensor),
> >
> > Did you mean "data outputs" by the way ?
> >
> > > > > > append a unique number to the device name.
> > > > > >
> > > > > > Fixes v4l2-compliance:
> > > > > >     Media Controller ioctls:
> > > > > >          fail: v4l2-test-media.cpp(205): v2_entity_names_set.find(key) != v2_entity_names_set.end()
> > > > > >        test MEDIA_IOC_G_TOPOLOGY: FAIL
> > > > > >          fail: v4l2-test-media.cpp(394): num_data_links != num_links
> > > > > >        test MEDIA_IOC_ENUM_ENTITIES/LINKS: FAIL
> > > > > >
> > > > > > Signed-off-by: Ricardo Ribalda <ribalda@...omium.org>
> > > > > > ---
> > > > > >  drivers/media/usb/uvc/uvc_driver.c | 3 ++-
> > > > > >  1 file changed, 2 insertions(+), 1 deletion(-)
> > > > > >
> > > > > > diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
> > > > > > index 215fb483efb0..f4032ebb3689 100644
> > > > > > --- a/drivers/media/usb/uvc/uvc_driver.c
> > > > > > +++ b/drivers/media/usb/uvc/uvc_driver.c
> > > > > > @@ -1963,7 +1963,8 @@ int uvc_register_video_device(struct uvc_device *dev,
> > > > > >               break;
> > > > > >       }
> > > > > >
> > > > > > -     strscpy(vdev->name, dev->name, sizeof(vdev->name));
> > > > > > +     snprintf(vdev->name, sizeof(vdev->name), "%s %u", dev->name,
> > > > > > +              stream->header.bTerminalLink);
> > > > >
> > > > > This won't be perfect as the string is not guaranteed to fit in
> > > > > vdev->name, but I suppose it will help as a quick fix for some devices.
> > > > > How about the other devices ? Won't they still exhibit the above
> > > > > v4l2-compliance failure ? Isn't that something that will still affect
> > > > > Chrome OS devices ?
> > > >
> > > > We could place the id first... but that will look bad: Eg:
> > > >
> > > > 1- My favorite camera
> > > >
> > > > Another option is to remove the last chars to fit the id. Eg:
> > > >
> > > > My favorite came-1
> > > >
> > > > If you prefer any of those options or have a better idea I can implement that.
> > >
> > > @Laurent
> > >
> > > Any preference here?
> >
> > I think the latter is better. Could we do so only when there are
> > multiple video capture devices (excluding the metadata device) though ?
> > That way we won't have a weird "-n" suffix in the majority of use cases.
> >
> > > > > The change should not cause any regression as big as in patch 1/3.
> > > > > However, unless I'm mistaken users will notice a device name change,
> > > > > especially when selecting a device in their web browser. Could that be a
> > > > > problem ?
> > > >
> > > > I think the only side effect is that the first time that the kernel
> > > > changes the naming convention, if there are more than one camera on
> > > > the system, the video conference might pick a different camera.
> > > > The good news is that the user will be presented with cameras with
> > > > different names. Now some cameras show very confusing names:
> > > >
> > > > ribalda@...o:~/work/linux$ for a in /dev/video* ; do yavta -l $a| grep
> > > > "Dell Webcam"; done
> > > > Device `Dell Webcam WB7022' on `usb-0000:2d:00.0-1.2.3.1' (driver
> > > > 'uvcvideo') supports video, capture, without mplanes.
> > > > Device `Dell Webcam WB7022' on `usb-0000:2d:00.0-1.2.3.1' (driver
> > > > 'uvcvideo') supports meta-data, capture, without mplanes.
> > > > Device `Dell Webcam WB7022' on `usb-0000:2d:00.0-1.2.3.1' (driver
> > > > 'uvcvideo') supports video, capture, without mplanes.
> > > > Device `Dell Webcam WB7022' on `usb-0000:2d:00.0-1.2.3.1' (driver
> > > > 'uvcvideo') supports meta-data, capture, without mplanes.
> >
> > I'm tempted to add a new model read-only string control to overcome the
> > length limitation. It could then be combined with other information
> > (such as the location and supported pixel formats) to create a
> > user-friendly camera name by applications.
> 
> Adding the vid:pid would be really useful! Mapping a /dev/videoX to
> vid:pid is kind of complicated now.

libcamera can help there ;-) We already extract the vendor and product
ID. They are only used to create the camera ID at the moment, they are
not exposed to applications independently. That would be a good
addition.

Overall, device naming is something that we have decided *not* to handle
in libcamera. We provide information to applications to help them
construct a meaningful name, but don't create the name ourselves. This
was decided because naming schemes are dependent on application use
cases, and in many cases should be localized (e.g. "Front camera" and
"Back camera" vs. "Etukamera" and "Takakamera").

> > > > > >       /*
> > > > > >        * Set the driver data before calling video_register_device, otherwise
> > > > > >

-- 
Regards,

Laurent Pinchart

Powered by blists - more mailing lists