From: Kirill A. Korinsky Subject: Re: sys/uvideo: bypass unknown pixelformat to consumer To: Marcus Glocker Cc: tech@openbsd.org Date: Sat, 11 Jan 2025 10:33:03 +0100 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. 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