Download raw body.
sys/uvideo: bypass unknown pixelformat to consumer
On Fri, Jan 10, 2025 at 11:43:40PM GMT, Kirill A. Korinsky wrote:
> Marcus,
>
> thanks for review.
>
> On Fri, 10 Jan 2025 22:58:27 +0100,
> Marcus Glocker <marcus@nazgul.ch> wrote:
> >
> > On Thu, Jan 09, 2025 at 06:38:29PM GMT, Kirill A. Korinsky wrote:
> >
> > > +
> > > + /* but some GUID sh uld 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?
> >
>
> Here a Linux mapping from GUID to pixel format:
> https://github.com/torvalds/linux/blob/master/drivers/media/common/uvc.c
>
> As you may see majority is mapped as is with some exception.
>
> Thus, current code maps YUY2 into YUYV, and KSMEDIA_L8_IR into GREY.
Got it. Thanks for the reference. Fine for me.
A few more comments in-line.
> All other remarks are incorporated into this diff:
>
> 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 10 Jan 2025 22:40:47 -0000
> @@ -383,6 +383,19 @@ const struct uvideo_devs {
> #define uvideo_lookup(v, p) \
> ((const struct uvideo_devs *)usb_lookup(uvideo_devs, v, p))
>
> +
We don't need that extra line here.
> +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 +1061,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 +1086,22 @@ 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(uint32_t));
> +
> + /* but some GUID should be mapped to other pixelformats */
> + nent = nitems(uvideo_map_fmts);
Can you please plug the 'nent' initialization in to the for loop? It's
part of the loop, and it fits nicely on the same line.
> + 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.
> if (sc->sc_fmtgrp_cur == NULL)
> @@ -2992,26 +3008,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 */
To sthen@s point. I see that there are only two compression formats
listed in the specs, but I find the table still helpful in there which
shows the GUID referencing to the pixel format. I would keep it there
for now. The confusion seems to be not too much IMO :-)
> +
> #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
>
sys/uvideo: bypass unknown pixelformat to consumer