From: Marcus Glocker Subject: Re: sys/uvideo: bypass unknown pixelformat to consumer To: "Kirill A. Korinsky" Cc: tech@openbsd.org Date: Sat, 11 Jan 2025 13:56:47 +0100 On Sat, Jan 11, 2025 at 10:33:03AM GMT, Kirill A. Korinsky wrote: > On Sat, 11 Jan 2025 09:38:21 +0100, > Marcus Glocker wrote: > > > > On Fri, Jan 10, 2025 at 11:43:40PM GMT, Kirill A. Korinsky wrote: > > > > > + 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; > > > + break; > > > + } > > > } > > > > What I find odd here, is that we first copy the pixel format, and then > > we only check if we need to map it, and if yes, we need to overwrite > > the pixel format again. > > > > At the end of the loop you could just check if a mapping was required > > by > > > > if (j == ent) /* no mapping was done */ > > > > and only execute the default copy in that 'if' branch if no mapping was > > done. > > > > Something like: > > > > /* but some GUID should be mapped to other pixelformats */ > > for (j = 0, nent = nitems(uvideo_map_fmts); 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; > > break; > > } > > } > > if (j == nent) { > > /* GUID is started by null-terminated pixelformat, usually */ > > memcpy(&sc->sc_fmtgrp[i].pixelformat, > > sc->sc_fmtgrp[i].format->u.uc.guidFormat, sizeof(uint32_t)); > > } > > > > And adapt the comments a bit to the new sequence. > > > > sure, I not sure which way is cleaner but it makes sense. > > Here an updated diff. I've tried to keep comments short and clear. Thanks, looks good to me now. I have regression tested this patch quickly with two of my cams. ok mglocker@ > 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 11 Jan 2025 09:22:18 -0000 > @@ -383,6 +383,18 @@ 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 }, > +}; > + > int > uvideo_open(void *addr, int flags, int *size, uint8_t *buffer, > void (*intr)(void *), void *arg) > @@ -1048,8 +1060,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,19 +1085,23 @@ 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; > + > + /* map GUID to pixel format if a matching entry is found */ > + for (j = 0, nent = nitems(uvideo_map_fmts); 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; > + break; > + } > } > + /* default to using GUID start as the pixel format */ > + if (j == nent) > + memcpy(&sc->sc_fmtgrp[i].pixelformat, > + sc->sc_fmtgrp[i].format->u.uc.guidFormat, > + sizeof(uint32_t)); > > if (sc->sc_fmtgrp_cur == NULL) > /* set UNCOMPRESSED format */ > @@ -2992,26 +3007,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 11 Jan 2025 08:25:58 -0000 > @@ -297,15 +297,23 @@ struct usb_video_probe_commit { > */ > /* 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_NV12 { \ > - 0x4e, 0x56, 0x31, 0x32, 0x00, 0x00, 0x10, 0x00, \ > +#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_UYVY { \ > - 0x55, 0x59, 0x56, 0x59, 0x00, 0x00, 0x10, 0x00, \ > +#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_Y800 { \ > + 'Y', '8', '0', '0', '\0', 0x00, 0x10, 0x00, \ > + 0x80, 0x00, 0x00, 0xaa, 0x00, 0x38, 0x9b, 0x71 } > + > +#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 >