On Sun, Feb 23, 2025 at 11:58:41AM GMT, Kirill A. Korinsky wrote:

> On Thu, 20 Feb 2025 01:17:38 +0100,
> Kirill A. Korinsky <kirill@korins.ky> wrote:
> > 
> > On Mon, 17 Feb 2025 01:51:30 +0100,
> > Kirill A. Korinsky <kirill@korins.ky> wrote:
> > > 
> > > tech@,
> > > 
> > > I'd like to add support for the color profile of the webcam when it
> > > announces it. My Jabra PanaCast 20 does this for mjpeg with non-default
> > > luma and chroma values profile: Rec 609 vs 709, and this diff improves
> > > the quality of the picture, and I look alive :), and not a bit green or
> > > blue.
> > > 
> > 
> > Here an improved version of this diff which appliy colorformat to all
> > previous pixelformats, if it the same, instead of the last one.
> >
> 
> Here a version which never overwrite colorformats when a device sends
> descripts similar to that flow:
> - some supported format
> - some frames
> - colorformat
> - some unsupported format and frames
> - colorformat
> 
> It also forward to consumer colorformat only when device sent it, if it
> doesn't send anything, nothing forwarded.

I have no cams supporting color formats here, so I can't test the diff
myself, but you've tested it, and the code doesn't look like too much
regression could be caused.

Some small comments inline.
 
> Index: sys/dev/usb/uvideo.c
> ===================================================================
> RCS file: /home/cvs/src/sys/dev/usb/uvideo.c,v
> diff -u -p -r1.239 uvideo.c
> --- sys/dev/usb/uvideo.c	23 Feb 2025 08:28:57 -0000	1.239
> +++ sys/dev/usb/uvideo.c	23 Feb 2025 10:50:33 -0000
> @@ -131,6 +131,8 @@ usbd_status	uvideo_vs_parse_desc(struct 
>  usbd_status	uvideo_vs_parse_desc_input_header(struct uvideo_softc *,
>  		    const usb_descriptor_t *);
>  usbd_status	uvideo_vs_parse_desc_format(struct uvideo_softc *);
> +usbd_status	uvideo_vs_parse_desc_colorformat(struct uvideo_softc *,
> +		    const usb_descriptor_t *);
>  usbd_status	uvideo_vs_parse_desc_format_mjpeg(struct uvideo_softc *,
>  		    const usb_descriptor_t *);
>  usbd_status	uvideo_vs_parse_desc_format_uncompressed(struct uvideo_softc *,
> @@ -420,6 +422,35 @@ const struct uvideo_map_fmts {
>  	{ UVIDEO_FORMAT_GUID_INVI, V4L2_PIX_FMT_Y10 },
>  };
>  
> +const enum v4l2_colorspace uvideo_color_primaries[] = {
> +	V4L2_COLORSPACE_SRGB,		/* Unspecified */
> +	V4L2_COLORSPACE_SRGB,
> +	V4L2_COLORSPACE_470_SYSTEM_M,
> +	V4L2_COLORSPACE_470_SYSTEM_BG,
> +	V4L2_COLORSPACE_SMPTE170M,
> +	V4L2_COLORSPACE_SMPTE240M,
> +};
> +
> +const enum v4l2_xfer_func uvideo_transfer_characteristics[] = {

Can this be renamed to 'uvideo_xfer_characteristics'?

> +	V4L2_XFER_FUNC_DEFAULT,		/* Unspecified */
> +	V4L2_XFER_FUNC_709,
> +	V4L2_XFER_FUNC_709,		/* Substitution for BT.470-2 M */
> +	V4L2_XFER_FUNC_709,		/* Substitution for BT.470-2 B, G */
> +	V4L2_XFER_FUNC_709,		/* Substitution for SMPTE 170M */
> +	V4L2_XFER_FUNC_SMPTE240M,
> +	V4L2_XFER_FUNC_NONE,
> +	V4L2_XFER_FUNC_SRGB,
> +};
> +
> +const enum v4l2_ycbcr_encoding uvideo_matrix_coefficients[] = {
> +	V4L2_YCBCR_ENC_DEFAULT,		/* Unspecified */
> +	V4L2_YCBCR_ENC_709,
> +	V4L2_YCBCR_ENC_601,		/* Substitution for FCC */
> +	V4L2_YCBCR_ENC_601,		/* Substitution for BT.470-2 B, G */
> +	V4L2_YCBCR_ENC_601,
> +	V4L2_YCBCR_ENC_SMPTE240M,
> +};
> +
>  int
>  uvideo_open(void *addr, int flags, int *size, uint8_t *buffer,
>      void (*intr)(void *), void *arg)
> @@ -1010,6 +1041,12 @@ uvideo_vs_parse_desc_format(struct uvide
>  		}
>  
>  		switch (desc->bDescriptorSubtype) {
> +		case UDESCSUB_VS_COLORFORMAT:
> +			if (desc->bLength == 6) {
> +				(void)uvideo_vs_parse_desc_colorformat(
> +				    sc, desc);
> +			}
> +			break;
>  		case UDESCSUB_VS_FORMAT_MJPEG:
>  			if (desc->bLength == 11) {
>  				(void)uvideo_vs_parse_desc_format_mjpeg(
> @@ -1040,6 +1077,42 @@ uvideo_vs_parse_desc_format(struct uvide
>  }
>  
>  usbd_status
> +uvideo_vs_parse_desc_colorformat(struct uvideo_softc *sc,
> +    const usb_descriptor_t *desc)
> +{
> +	int fmtidx;
> +	struct usb_video_colorformat_desc *d;
> +
> +	d = (struct usb_video_colorformat_desc *)(uint8_t *)desc;
> +
> +	fmtidx = sc->sc_fmtgrp_idx - 1;
> +	if (fmtidx < 0 || sc->sc_fmtgrp[fmtidx].has_colorformat)
> +		return (USBD_INVAL);
> +
> +	if (d->bColorPrimaries < nitems(uvideo_color_primaries))
> +		sc->sc_fmtgrp[fmtidx].colorspace =
> +		    uvideo_color_primaries[d->bColorPrimaries];
> +	else
> +		sc->sc_fmtgrp[fmtidx].colorspace = V4L2_COLORSPACE_SRGB;
> +
> +	if (d->bTransferCharacteristics < nitems(uvideo_transfer_characteristics))

Line break.  Would be fixed by the above renaming suggestion.

> +		sc->sc_fmtgrp[fmtidx].xfer_func =
> +		    uvideo_transfer_characteristics[d->bTransferCharacteristics];

Line break.  Would be fixed by the above renaming suggestion.

> +	else
> +		sc->sc_fmtgrp[fmtidx].xfer_func = V4L2_XFER_FUNC_DEFAULT;
> +
> +	if (d->bMatrixCoefficients < nitems(uvideo_matrix_coefficients))
> +		sc->sc_fmtgrp[fmtidx].ycbcr_enc =
> +		    uvideo_matrix_coefficients[d->bMatrixCoefficients];
> +	else
> +		sc->sc_fmtgrp[fmtidx].ycbcr_enc = V4L2_YCBCR_ENC_DEFAULT;
> +
> +	sc->sc_fmtgrp[fmtidx].has_colorformat = 1;
> +
> +	return (USBD_NORMAL_COMPLETION);
> +}
> +
> +usbd_status
>  uvideo_vs_parse_desc_format_mjpeg(struct uvideo_softc *sc,
>      const usb_descriptor_t *desc)
>  {
> @@ -3251,6 +3324,17 @@ uvideo_g_fmt(void *v, struct v4l2_format
>  	fmt->fmt.pix.width = UGETW(sc->sc_fmtgrp_cur->frame_cur->wWidth);
>  	fmt->fmt.pix.height = UGETW(sc->sc_fmtgrp_cur->frame_cur->wHeight);
>  	fmt->fmt.pix.sizeimage = UGETDW(sc->sc_desc_probe.dwMaxVideoFrameSize);
> +
> +	if (sc->sc_fmtgrp_cur->has_colorformat) {
> +		fmt->fmt.pix.colorspace = sc->sc_fmtgrp_cur->colorspace;
> +		fmt->fmt.pix.xfer_func = sc->sc_fmtgrp_cur->xfer_func;
> +		fmt->fmt.pix.ycbcr_enc = sc->sc_fmtgrp_cur->ycbcr_enc;
> +
> +		DPRINTF(1, "%s: %s: use color format"
> +		    " colorspace=%d, xfer_func=%d, ycbcr_enc=%d\n",
> +		    DEVNAME(sc), __func__, fmt->fmt.pix.colorspace,
> +		    fmt->fmt.pix.xfer_func, fmt->fmt.pix.ycbcr_enc);
> +	}
>  
>  	DPRINTF(1, "%s: %s: current width=%d, height=%d\n",
>  	    DEVNAME(sc), __func__, fmt->fmt.pix.width, fmt->fmt.pix.height);
> Index: sys/dev/usb/uvideo.h
> ===================================================================
> RCS file: /home/cvs/src/sys/dev/usb/uvideo.h,v
> diff -u -p -r1.63 uvideo.h
> --- sys/dev/usb/uvideo.h	16 Jan 2025 22:58:19 -0000	1.63
> +++ sys/dev/usb/uvideo.h	23 Feb 2025 10:51:10 -0000
> @@ -412,6 +412,16 @@ struct usb_video_stream_header {
>  	/* TODO complete struct */
>  } __packed;
>  
> +/* Table 3-19: Color Matching Descriptor */
> +struct usb_video_colorformat_desc {
> +	uByte	bLength;
> +	uByte	bDescriptorType;
> +	uByte	bDescriptorSubtype;
> +	uByte	bColorPrimaries;
> +	uByte	bTransferCharacteristics;
> +	uByte	bMatrixCoefficients;
> +} __packed;
> +
>  /* Table 3-1: Motion-JPEG Video Format Descriptor */
>  struct usb_video_format_mjpeg_desc {
>  	uByte	bLength;
> @@ -551,6 +561,10 @@ typedef SIMPLEQ_HEAD(, uvideo_mmap) q_mm
>  
>  struct uvideo_format_group {
>  	uint32_t				 pixelformat;
> +	int					 has_colorformat;
> +	uint32_t				 colorspace;
> +	uint32_t				 xfer_func;
> +	uint32_t				 ycbcr_enc;
>  	uint8_t					 format_dfidx;
>  	struct uvideo_format_desc		*format;
>  	/* frame descriptors for mjpeg and uncompressed are identical */
>