From: Marcus Glocker Subject: Re: sys/uvideo: support colorformat from device To: "Kirill A. Korinsky" Cc: tech@openbsd.org Date: Wed, 26 Feb 2025 20:54:33 +0100 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 wrote: > > > > On Mon, 17 Feb 2025 01:51:30 +0100, > > Kirill A. Korinsky 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 */ >