From: Marcus Glocker Subject: Re: sys/uvideo: bypass unknown pixelformat to consumer To: "Kirill A. Korinsky" Cc: tech@openbsd.org Date: Fri, 10 Jan 2025 22:58:27 +0100 On Thu, Jan 09, 2025 at 06:38:29PM GMT, Kirill A. Korinsky wrote: > tech@, > > The GUID format read from a camera usually begins with a null-terminated > pixel format. Certain formats should be mapped, such as converting YUY2 to > YUYV and I420 to YVU420, among others. > > Instead of rejecting unknown driver formats like M420, I bypass them to the > consumer, which likely knows how to handle these formats. > > Tested without any regression on: > - Azurewave, USB camera > - LG UltraFine Display Camera > - Jabra PanaCast 20 > - Shine-Optics, FHD Camera > > Feedback? Tests? Ok? Some feedback, and clarification request in-line. > Index: sys/dev/usb/uvideo.c > =================================================================== > RCS file: /home/cvs/src/sys/dev/usb/uvideo.c,v > diff -u -p -u -p -r1.233 uvideo.c > --- sys/dev/usb/uvideo.c 1 Jan 2025 11:42:07 -0000 1.233 > +++ sys/dev/usb/uvideo.c 9 Jan 2025 16:48:15 -0000 > @@ -383,6 +383,24 @@ const struct uvideo_devs { > #define uvideo_lookup(v, p) \ > ((const struct uvideo_devs *)usb_lookup(uvideo_devs, v, p)) > > + > +const struct uvideo_map_fmts { > + uint8_t guidFormat[16]; > + uint32_t pixelformat; > +} uvideo_map_fmts[] = { > + { UVIDEO_FORMAT_GUID_YUY2, V4L2_PIX_FMT_YUYV }, > + > + { UVIDEO_FORMAT_GUID_YV12, V4L2_PIX_FMT_YVU420 }, > + > + { UVIDEO_FORMAT_GUID_I420, V4L2_PIX_FMT_YUV420 }, > + > + { UVIDEO_FORMAT_GUID_Y800, V4L2_PIX_FMT_GREY }, > + > + { UVIDEO_FORMAT_GUID_Y8, V4L2_PIX_FMT_GREY }, > + > + { UVIDEO_FORMAT_GUID_KSMEDIA_L8_IR, V4L2_PIX_FMT_GREY }, > +}; > + I agree with sthen@. No extra spaces required here. > int > uvideo_open(void *addr, int flags, int *size, uint8_t *buffer, > void (*intr)(void *), void *arg) > @@ -1048,8 +1066,7 @@ uvideo_vs_parse_desc_format_uncompressed > const usb_descriptor_t *desc) > { > struct usb_video_format_uncompressed_desc *d; > - uint8_t guid_8bit_ir[16] = UVIDEO_FORMAT_GUID_KSMEDIA_L8_IR; > - int i; > + int i, j, nent; > > d = (struct usb_video_format_uncompressed_desc *)(uint8_t *)desc; > > @@ -1074,18 +1091,21 @@ uvideo_vs_parse_desc_format_uncompressed > sc->sc_fmtgrp[sc->sc_fmtgrp_idx].format_dfidx = > d->bDefaultFrameIndex; > } > + > i = sc->sc_fmtgrp_idx; > - if (!strcmp(sc->sc_fmtgrp[i].format->u.uc.guidFormat, "YUY2")) { > - sc->sc_fmtgrp[i].pixelformat = V4L2_PIX_FMT_YUYV; > - } else if (!strcmp(sc->sc_fmtgrp[i].format->u.uc.guidFormat, "NV12")) { > - sc->sc_fmtgrp[i].pixelformat = V4L2_PIX_FMT_NV12; > - } else if (!strcmp(sc->sc_fmtgrp[i].format->u.uc.guidFormat, "UYVY")) { > - sc->sc_fmtgrp[i].pixelformat = V4L2_PIX_FMT_UYVY; > - } else if (!memcmp(sc->sc_fmtgrp[i].format->u.uc.guidFormat, > - guid_8bit_ir, 16)) { > - sc->sc_fmtgrp[i].pixelformat = V4L2_PIX_FMT_GREY; > - } else { > - sc->sc_fmtgrp[i].pixelformat = 0; > + > + /* GUID is started by null-terminated pixelformat, usually */ > + memcpy(&sc->sc_fmtgrp[i].pixelformat, > + sc->sc_fmtgrp[i].format->u.uc.guidFormat, sizeof(u_int32_t)); Lets stay consistent throughout the code, and use uint32_t here. > + > + /* but some GUID should be mapped to other pixelformats */ OK, maybe I'm a bit rusty on the pixel formats. Where is this mapping table coming from? Why exactly should we re-map them? > + nent = nitems(uvideo_map_fmts); > + for (j = 0; j < nent; j++) { > + if (!memcmp(sc->sc_fmtgrp[i].format->u.uc.guidFormat, > + uvideo_map_fmts[j].guidFormat, 16)) { > + sc->sc_fmtgrp[i].pixelformat = uvideo_map_fmts[j].pixelformat; Line break ... > + break; > + } > } > if (sc->sc_fmtgrp_cur == NULL) > @@ -2992,26 +3012,10 @@ uvideo_enum_fmt(void *v, struct v4l2_fmt > break; > case UDESCSUB_VS_FORMAT_UNCOMPRESSED: > fmtdesc->flags = 0; > - if (sc->sc_fmtgrp[idx].pixelformat == > - V4L2_PIX_FMT_YUYV) { > - (void)strlcpy(fmtdesc->description, "YUYV", > - sizeof(fmtdesc->description)); > - fmtdesc->pixelformat = V4L2_PIX_FMT_YUYV; > - } else if (sc->sc_fmtgrp[idx].pixelformat == > - V4L2_PIX_FMT_NV12) { > - (void)strlcpy(fmtdesc->description, "NV12", > - sizeof(fmtdesc->description)); > - fmtdesc->pixelformat = V4L2_PIX_FMT_NV12; > - } else if (sc->sc_fmtgrp[idx].pixelformat == > - V4L2_PIX_FMT_UYVY) { > - (void)strlcpy(fmtdesc->description, "UYVY", > - sizeof(fmtdesc->description)); > - fmtdesc->pixelformat = V4L2_PIX_FMT_UYVY; > - } else { > - (void)strlcpy(fmtdesc->description, "Unknown UC Format", > - sizeof(fmtdesc->description)); > - fmtdesc->pixelformat = 0; > - } > + fmtdesc->pixelformat = sc->sc_fmtgrp[idx].pixelformat; > + (void)strlcpy(fmtdesc->description, > + (char *) &fmtdesc->pixelformat, > + sizeof(fmtdesc->description)); > bzero(fmtdesc->reserved, sizeof(fmtdesc->reserved)); > break; > default: > Index: sys/dev/usb/uvideo.h > =================================================================== > RCS file: /home/cvs/src/sys/dev/usb/uvideo.h,v > diff -u -p -u -p -r1.61 uvideo.h > --- sys/dev/usb/uvideo.h 22 Dec 2024 20:30:04 -0000 1.61 > +++ sys/dev/usb/uvideo.h 9 Jan 2025 16:45:22 -0000 > @@ -295,17 +295,25 @@ struct usb_video_probe_commit { > /* > * USB Video Payload Uncompressed > */ > -/* Table 2-1: Compression Formats */ > + > #define UVIDEO_FORMAT_GUID_YUY2 { \ > - 0x59, 0x55, 0x59, 0x32, 0x00, 0x00, 0x10, 0x00, \ > + 'Y', 'U', 'Y', '2', '\0', 0x00, 0x10, 0x00, \ > + 0x80, 0x00, 0x00, 0xaa, 0x00, 0x38, 0x9b, 0x71 } > + > +#define UVIDEO_FORMAT_GUID_YV12 { \ > + 'Y', 'V', '1', '2', '\0', 0x00, 0x10, 0x00, \ > + 0x80, 0x00, 0x00, 0xaa, 0x00, 0x38, 0x9b, 0x71 } > + > +#define UVIDEO_FORMAT_GUID_I420 { \ > + 'I', '4', '2', '0', '\0', 0x00, 0x10, 0x00, \ > 0x80, 0x00, 0x00, 0xaa, 0x00, 0x38, 0x9b, 0x71 } > > -#define UVIDEO_FORMAT_GUID_NV12 { \ > - 0x4e, 0x56, 0x31, 0x32, 0x00, 0x00, 0x10, 0x00, \ > +#define UVIDEO_FORMAT_GUID_Y800 { \ > + 'Y', '8', '0', '0', '\0', 0x00, 0x10, 0x00, \ > 0x80, 0x00, 0x00, 0xaa, 0x00, 0x38, 0x9b, 0x71 } > > -#define UVIDEO_FORMAT_GUID_UYVY { \ > - 0x55, 0x59, 0x56, 0x59, 0x00, 0x00, 0x10, 0x00, \ > +#define UVIDEO_FORMAT_GUID_Y8 { \ > + 'Y', '8', ' ', ' ', '\0', 0x00, 0x10, 0x00, \ > 0x80, 0x00, 0x00, 0xaa, 0x00, 0x38, 0x9b, 0x71 } > > #define UVIDEO_FORMAT_GUID_KSMEDIA_L8_IR { \ > > > -- > wbr, Kirill >