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
>