fill device description with product name in uvideo(4)

classic Classic list List threaded Threaded
6 messages Options
Reply | Threaded
Open this post in threaded view
|

fill device description with product name in uvideo(4)

Landry Breuil-5
Hi,

here's a diff to reuse usbd_devinfo_vp() (exposed in usbdivar.h) in
VIDIOC_QUERYCAP ioctl callback, this way we can fill v4l2_capability
card struct member with the actual usb product name instead of a dummy
"Generic USB video class device".

Firefox uses that ioctl to get the user-facing device name in
https://dxr.mozilla.org/mozilla-central/source/media/webrtc/trunk/webrtc/modules/video_capture/linux/device_info_linux.cc#313
so that's helpful when you have several devices
and the webrtc doorhanger asks you which camera you want to
share.. cf http://i.imgur.com/uLnWw6u.png

USB_MAX_STRING_LEN is 127 and card is 32 in v4l2_capability, so strlcpy
should take care of null-terminating/truncating.

Thx mpi@ for the directions..

Landry

uvideo.diff (2K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: fill device description with product name in uvideo(4)

Stuart Henderson
On 2018/04/24 16:34, Landry Breuil wrote:
> Hi,
>
> here's a diff to reuse usbd_devinfo_vp() (exposed in usbdivar.h) in
> VIDIOC_QUERYCAP ioctl callback, this way we can fill v4l2_capability
> card struct member with the actual usb product name instead of a dummy
> "Generic USB video class device".

Generally I think this is a good idea, but some devices have crap in
v/p, is that likely to cause any problems?


> Firefox uses that ioctl to get the user-facing device name in
> https://dxr.mozilla.org/mozilla-central/source/media/webrtc/trunk/webrtc/modules/video_capture/linux/device_info_linux.cc#313
> so that's helpful when you have several devices
> and the webrtc doorhanger asks you which camera you want to
> share.. cf http://i.imgur.com/uLnWw6u.png
>
> USB_MAX_STRING_LEN is 127 and card is 32 in v4l2_capability, so strlcpy
> should take care of null-terminating/truncating.
>
> Thx mpi@ for the directions..
>
> Landry

> Index: usb_subr.c
> ===================================================================
> RCS file: /cvs/src/sys/dev/usb/usb_subr.c,v
> retrieving revision 1.134
> diff -u -r1.134 usb_subr.c
> --- usb_subr.c 8 Apr 2017 02:57:25 -0000 1.134
> +++ usb_subr.c 24 Apr 2018 14:20:00 -0000
> @@ -61,8 +61,6 @@
>  
>  usbd_status usbd_set_config(struct usbd_device *, int);
>  void usbd_devinfo(struct usbd_device *, int, char *, size_t);
> -void usbd_devinfo_vp(struct usbd_device *, char *, size_t,
> -    char *, size_t, int);
>  char *usbd_get_device_string(struct usbd_device *, uByte);
>  char *usbd_get_string(struct usbd_device *, int, char *, size_t);
>  int usbd_getnewaddr(struct usbd_bus *);
> Index: usbdivar.h
> ===================================================================
> RCS file: /cvs/src/sys/dev/usb/usbdivar.h,v
> retrieving revision 1.73
> diff -u -r1.73 usbdivar.h
> --- usbdivar.h 3 Feb 2018 13:37:37 -0000 1.73
> +++ usbdivar.h 24 Apr 2018 14:20:00 -0000
> @@ -257,6 +257,8 @@
>  usbd_status usb_insert_transfer(struct usbd_xfer *);
>  void usb_transfer_complete(struct usbd_xfer *);
>  int usbd_detach(struct usbd_device *, struct device *);
> +void usbd_devinfo_vp(struct usbd_device *, char *, size_t,
> +    char *, size_t, int);
>  
>  /* Routines from usb.c */
>  void usb_needs_explore(struct usbd_device *, int);
> Index: uvideo.c
> ===================================================================
> RCS file: /cvs/src/sys/dev/usb/uvideo.c,v
> retrieving revision 1.196
> diff -u -r1.196 uvideo.c
> --- uvideo.c 30 Dec 2017 23:08:29 -0000 1.196
> +++ uvideo.c 24 Apr 2018 14:20:00 -0000
> @@ -2781,11 +2781,14 @@
>  uvideo_querycap(void *v, struct v4l2_capability *caps)
>  {
>   struct uvideo_softc *sc = v;
> + char vendor[USB_MAX_STRING_LEN];
> + char product[USB_MAX_STRING_LEN];
>  
>   bzero(caps, sizeof(*caps));
>   strlcpy(caps->driver, DEVNAME(sc), sizeof(caps->driver));
> - strlcpy(caps->card, "Generic USB video class device",
> -    sizeof(caps->card));
> + usbd_devinfo_vp(sc->sc_udev, vendor, sizeof (vendor), product,
> +    sizeof (product), 1);
> + strlcpy(caps->card, product, sizeof(caps->card));
>   strlcpy(caps->bus_info, "usb", sizeof(caps->bus_info));
>  
>   caps->version = 1;

Reply | Threaded
Open this post in threaded view
|

Re: fill device description with product name in uvideo(4)

Landry Breuil-5
On Tue, Apr 24, 2018 at 04:04:34PM +0100, Stuart Henderson wrote:

> On 2018/04/24 16:34, Landry Breuil wrote:
> > Hi,
> >
> > here's a diff to reuse usbd_devinfo_vp() (exposed in usbdivar.h) in
> > VIDIOC_QUERYCAP ioctl callback, this way we can fill v4l2_capability
> > card struct member with the actual usb product name instead of a dummy
> > "Generic USB video class device".
>
> Generally I think this is a good idea, but some devices have crap in
> v/p, is that likely to cause any problems?

Well, the exact same function is called in usbd_devinfo() during boot to
generate the dmesg lines for the devices.. so i dont really see how it makes
things worse.

uvideo0 at uhub1 port 1 configuration 1 interface 0 "Logitech QuickCam Orbit AF" rev 2.00/0.08 addr 3
uvideo1 at uhub1 port 8 configuration 1 interface 0 "Chicony Electronics Co.,Ltd. Integrated Camera" rev 2.00/0.29 addr 5

Landry

Reply | Threaded
Open this post in threaded view
|

Re: fill device description with product name in uvideo(4)

Stuart Henderson
On 2018/04/24 17:15, Landry Breuil wrote:

> On Tue, Apr 24, 2018 at 04:04:34PM +0100, Stuart Henderson wrote:
> > On 2018/04/24 16:34, Landry Breuil wrote:
> > > Hi,
> > >
> > > here's a diff to reuse usbd_devinfo_vp() (exposed in usbdivar.h) in
> > > VIDIOC_QUERYCAP ioctl callback, this way we can fill v4l2_capability
> > > card struct member with the actual usb product name instead of a dummy
> > > "Generic USB video class device".
> >
> > Generally I think this is a good idea, but some devices have crap in
> > v/p, is that likely to cause any problems?
>
> Well, the exact same function is called in usbd_devinfo() during boot to
> generate the dmesg lines for the devices.. so i dont really see how it makes
> things worse.
>
> uvideo0 at uhub1 port 1 configuration 1 interface 0 "Logitech QuickCam Orbit AF" rev 2.00/0.08 addr 3
> uvideo1 at uhub1 port 8 configuration 1 interface 0 "Chicony Electronics Co.,Ltd. Integrated Camera" rev 2.00/0.29 addr 5
>
> Landry
>

I've looked through dmesglog for strings in uvideo lines and they all look
pretty sane. I'm surprised, I've seen random-looking binary stuff on some
devices and was unsure how programs might handle that.

Reply | Threaded
Open this post in threaded view
|

Re: fill device description with product name in uvideo(4)

Martin Pieuchot
In reply to this post by Landry Breuil-5
On 24/04/18(Tue) 17:15, Landry Breuil wrote:

> On Tue, Apr 24, 2018 at 04:04:34PM +0100, Stuart Henderson wrote:
> > On 2018/04/24 16:34, Landry Breuil wrote:
> > > Hi,
> > >
> > > here's a diff to reuse usbd_devinfo_vp() (exposed in usbdivar.h) in
> > > VIDIOC_QUERYCAP ioctl callback, this way we can fill v4l2_capability
> > > card struct member with the actual usb product name instead of a dummy
> > > "Generic USB video class device".
> >
> > Generally I think this is a good idea, but some devices have crap in
> > v/p, is that likely to cause any problems?
>
> Well, the exact same function is called in usbd_devinfo() during boot to
> generate the dmesg lines for the devices.. so i dont really see how it makes
> things worse.

And by usbdevs(8) and the libusb.

I'm ok with the diff if you change the last argument to be 0.  Setting
`usedev' to 1 makes the function generate I/O.  We want to avoid that
as much as possible.

I would be very happy if you could reduce the number of calls to
usbd_devinfo_vp() in the stack and used the cached the result instead.

Reply | Threaded
Open this post in threaded view
|

Re: fill device description with product name in uvideo(4)

Mark Kettenis
In reply to this post by Stuart Henderson
> Date: Tue, 24 Apr 2018 16:04:34 +0100
> From: Stuart Henderson <[hidden email]>
>
> On 2018/04/24 16:34, Landry Breuil wrote:
> > Hi,
> >
> > here's a diff to reuse usbd_devinfo_vp() (exposed in usbdivar.h) in
> > VIDIOC_QUERYCAP ioctl callback, this way we can fill v4l2_capability
> > card struct member with the actual usb product name instead of a dummy
> > "Generic USB video class device".
>
> Generally I think this is a good idea, but some devices have crap in
> v/p, is that likely to cause any problems?

Spaces are trimmed, but the strings are not otherwise sanitized.  The
mozilla code seems to interpret the string as UTF8 which opens up
additional ways to misinterpret the string.  Maybe some further
sanitization similar to what we do for scsi devices (scsi_strvis()) is
desirable.

An alternative would be to pass 0 instead of 1 for the last argument,
but then the string doesn't necessarily match what's printed in dmesg.

> > Firefox uses that ioctl to get the user-facing device name in
> > https://dxr.mozilla.org/mozilla-central/source/media/webrtc/trunk/webrtc/modules/video_capture/linux/device_info_linux.cc#313
> > so that's helpful when you have several devices
> > and the webrtc doorhanger asks you which camera you want to
> > share.. cf http://i.imgur.com/uLnWw6u.png
> >
> > USB_MAX_STRING_LEN is 127 and card is 32 in v4l2_capability, so strlcpy
> > should take care of null-terminating/truncating.

I'm almost tempted to suggest putting the device name (uvideo0,
uvideo1, etc.) there to have something that is consistent.

>
> > Index: usb_subr.c
> > ===================================================================
> > RCS file: /cvs/src/sys/dev/usb/usb_subr.c,v
> > retrieving revision 1.134
> > diff -u -r1.134 usb_subr.c
> > --- usb_subr.c 8 Apr 2017 02:57:25 -0000 1.134
> > +++ usb_subr.c 24 Apr 2018 14:20:00 -0000
> > @@ -61,8 +61,6 @@
> >  
> >  usbd_status usbd_set_config(struct usbd_device *, int);
> >  void usbd_devinfo(struct usbd_device *, int, char *, size_t);
> > -void usbd_devinfo_vp(struct usbd_device *, char *, size_t,
> > -    char *, size_t, int);
> >  char *usbd_get_device_string(struct usbd_device *, uByte);
> >  char *usbd_get_string(struct usbd_device *, int, char *, size_t);
> >  int usbd_getnewaddr(struct usbd_bus *);
> > Index: usbdivar.h
> > ===================================================================
> > RCS file: /cvs/src/sys/dev/usb/usbdivar.h,v
> > retrieving revision 1.73
> > diff -u -r1.73 usbdivar.h
> > --- usbdivar.h 3 Feb 2018 13:37:37 -0000 1.73
> > +++ usbdivar.h 24 Apr 2018 14:20:00 -0000
> > @@ -257,6 +257,8 @@
> >  usbd_status usb_insert_transfer(struct usbd_xfer *);
> >  void usb_transfer_complete(struct usbd_xfer *);
> >  int usbd_detach(struct usbd_device *, struct device *);
> > +void usbd_devinfo_vp(struct usbd_device *, char *, size_t,
> > +    char *, size_t, int);
> >  
> >  /* Routines from usb.c */
> >  void usb_needs_explore(struct usbd_device *, int);
> > Index: uvideo.c
> > ===================================================================
> > RCS file: /cvs/src/sys/dev/usb/uvideo.c,v
> > retrieving revision 1.196
> > diff -u -r1.196 uvideo.c
> > --- uvideo.c 30 Dec 2017 23:08:29 -0000 1.196
> > +++ uvideo.c 24 Apr 2018 14:20:00 -0000
> > @@ -2781,11 +2781,14 @@
> >  uvideo_querycap(void *v, struct v4l2_capability *caps)
> >  {
> >   struct uvideo_softc *sc = v;
> > + char vendor[USB_MAX_STRING_LEN];
> > + char product[USB_MAX_STRING_LEN];
> >  
> >   bzero(caps, sizeof(*caps));
> >   strlcpy(caps->driver, DEVNAME(sc), sizeof(caps->driver));
> > - strlcpy(caps->card, "Generic USB video class device",
> > -    sizeof(caps->card));
> > + usbd_devinfo_vp(sc->sc_udev, vendor, sizeof (vendor), product,
> > +    sizeof (product), 1);
> > + strlcpy(caps->card, product, sizeof(caps->card));
> >   strlcpy(caps->bus_info, "usb", sizeof(caps->bus_info));
> >  
> >   caps->version = 1;
>
>