[PATCH 1/2] media: uvcvideo: Ensure all probed info is returned to v4l2

[PATCH 1/2] media: uvcvideo: Ensure all probed info is returned to v4l2

[Adam Goode]

bFrameIndex and bFormatIndex can be negotiated by the camera during
probing, resulting in the camera choosing a different format than
expected. v4l2 can already accommodate such changes, but the code was
not updating the proper fields.

Without such a change, v4l2 would potentially interpret the payload
incorrectly, causing corrupted output. This was happening on the
Elgato HD60 S+, which currently always renegotiates to format 1.

As an aside, the Elgato firmware is buggy and should not be renegotating,
but it is still a valid thing for the camera to do. Both macOS and Windows
will properly probe and read uncorrupted images from this camera.

With this change, both qv4l2 and chromium can now read uncorrupted video
from the Elgato HD60 S+.

Signed-off-by: Adam Goode <agoode@google.com>
---
 drivers/media/usb/uvc/uvc_v4l2.c | 26 ++++++++++++++++++++++++++
 1 file changed, 26 insertions(+)

diff --git a/drivers/media/usb/uvc/uvc_v4l2.c b/drivers/media/usb/uvc/uvc_v4l2.c
index 0335e69b70ab..7f14096cb44d 100644
--- a/drivers/media/usb/uvc/uvc_v4l2.c
+++ b/drivers/media/usb/uvc/uvc_v4l2.c
@@ -247,11 +247,37 @@ static int uvc_v4l2_try_format(struct uvc_streaming *stream,
 	if (ret < 0)
 		goto done;
 
+	/* After the probe, update fmt with the values returned from
+	 * negotiation with the device.
+	 */
+	for (i = 0; i < stream->nformats; ++i) {
+		if (probe->bFormatIndex == stream->format[i].index) {
+			format = &stream->format[i];
+			break;
+		}
+	}
+	if (i == stream->nformats) {
+		uvc_trace(UVC_TRACE_FORMAT, "Unknown bFormatIndex %u.\n",
+			  probe->bFormatIndex);
+		return -EINVAL;
+	}
+	for (i = 0; i < format->nframes; ++i) {
+		if (probe->bFrameIndex == format->frame[i].bFrameIndex) {
+			frame = &format->frame[i];
+			break;
+		}
+	}
+	if (i == format->nframes) {
+		uvc_trace(UVC_TRACE_FORMAT, "Unknown bFrameIndex %u.\n",
+			  probe->bFrameIndex);
+		return -EINVAL;
+	}
 	fmt->fmt.pix.width = frame->wWidth;
 	fmt->fmt.pix.height = frame->wHeight;
 	fmt->fmt.pix.field = V4L2_FIELD_NONE;
 	fmt->fmt.pix.bytesperline = uvc_v4l2_get_bytesperline(format, frame);
 	fmt->fmt.pix.sizeimage = probe->dwMaxVideoFrameSize;
+	fmt->fmt.pix.pixelformat = format->fcc;
 	fmt->fmt.pix.colorspace = format->colorspace;
 
 	if (uvc_format != NULL)
-- 
2.28.0.297.g1956fa8f8d-goog

[PATCH 2/2] media: uvcvideo: Convey full ycbcr colorspace information to v4l2

[Adam Goode]

The Color Matching Descriptor has been present in USB cameras since
the original version of UVC, but it has never been fully used
in Linux.

This change informs V4L2 of all of the critical colorspace parameters:
colorspace (called "color primaries" in UVC), transfer function
(called "transfer characteristics" in UVC), ycbcr encoding (called
"matrix coefficients" in UVC), and quantization, which is always
LIMITED for UVC, see section 2.26 in USB_Video_FAQ_1.5.pdf.

The quantization is the most important improvement for this patch,
because V4L2 will otherwise interpret MJPEG as FULL range. Web browsers
such as Chrome and Firefox already ignore V4L2's quantization for USB
devices and use the correct LIMITED value, but other programs such
as qv4l2 will incorrectly interpret the output of MJPEG from USB
cameras without this change.

Signed-off-by: Adam Goode <agoode@google.com>
---
 drivers/media/usb/uvc/uvc_driver.c | 52 +++++++++++++++++++++++++++---
 drivers/media/usb/uvc/uvc_v4l2.c   |  6 ++++
 drivers/media/usb/uvc/uvcvideo.h   |  5 ++-
 3 files changed, 58 insertions(+), 5 deletions(-)

diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
index 431d86e1c94b..c0c81b089b7d 100644
--- a/drivers/media/usb/uvc/uvc_driver.c
+++ b/drivers/media/usb/uvc/uvc_driver.c
@@ -248,10 +248,10 @@ static struct uvc_format_desc *uvc_format_by_guid(const u8 guid[16])
 	return NULL;
 }
 
-static u32 uvc_colorspace(const u8 primaries)
+static enum v4l2_colorspace uvc_colorspace(const u8 primaries)
 {
-	static const u8 colorprimaries[] = {
-		0,
+	static const enum v4l2_colorspace colorprimaries[] = {
+		V4L2_COLORSPACE_DEFAULT,  /* Unspecified */
 		V4L2_COLORSPACE_SRGB,
 		V4L2_COLORSPACE_470_SYSTEM_M,
 		V4L2_COLORSPACE_470_SYSTEM_BG,
@@ -262,7 +262,43 @@ static u32 uvc_colorspace(const u8 primaries)
 	if (primaries < ARRAY_SIZE(colorprimaries))
 		return colorprimaries[primaries];
 
-	return 0;
+	return V4L2_COLORSPACE_DEFAULT;  /* Reserved */
+}
+
+static enum v4l2_xfer_func uvc_xfer_func(const u8 transfer_characteristics)
+{
+	static const enum v4l2_xfer_func xfer_funcs[] = {
+		V4L2_XFER_FUNC_DEFAULT,  /* Unspecified */
+		V4L2_XFER_FUNC_709,
+		V4L2_XFER_FUNC_709,      /* BT.470-2 M */
+		V4L2_XFER_FUNC_709,      /* BT.470-2 B, G */
+		V4L2_XFER_FUNC_709,      /* SMPTE 170M */
+		V4L2_XFER_FUNC_SMPTE240M,
+		V4L2_XFER_FUNC_NONE,     /* Linear (V = Lc) */
+		V4L2_XFER_FUNC_SRGB,
+	};
+
+	if (transfer_characteristics < ARRAY_SIZE(xfer_funcs))
+		return xfer_funcs[transfer_characteristics];
+
+	return V4L2_XFER_FUNC_DEFAULT;  /* Reserved */
+}
+
+static enum v4l2_ycbcr_encoding uvc_ycbcr_enc(const u8 matrix_coefficients)
+{
+	static const enum v4l2_ycbcr_encoding ycbcr_encs[] = {
+		V4L2_YCBCR_ENC_DEFAULT,  /* Unspecified */
+		V4L2_YCBCR_ENC_709,
+		V4L2_YCBCR_ENC_601,      /* FCC */
+		V4L2_YCBCR_ENC_601,      /* BT.470-2 B, G */
+		V4L2_YCBCR_ENC_601,      /* SMPTE 170M */
+		V4L2_YCBCR_ENC_SMPTE240M,
+	};
+
+	if (matrix_coefficients < ARRAY_SIZE(ycbcr_encs))
+		return ycbcr_encs[matrix_coefficients];
+
+	return V4L2_YCBCR_ENC_DEFAULT;  /* Reserved */
 }
 
 /* Simplify a fraction using a simple continued fraction decomposition. The
@@ -704,6 +740,14 @@ static int uvc_parse_format(struct uvc_device *dev,
 		}
 
 		format->colorspace = uvc_colorspace(buffer[3]);
+		format->xfer_func = uvc_xfer_func(buffer[4]);
+		format->ycbcr_enc = uvc_ycbcr_enc(buffer[5]);
+		/* All USB YCbCr encodings use LIMITED range as of UVC 1.5.
+		 * This is true even for MJPEG, which V4L2 otherwise assumes to
+		 * be FULL.
+		 * See section 2.26 in USB_Video_FAQ_1.5.pdf.
+		 */
+		format->quantization = V4L2_QUANTIZATION_LIM_RANGE;
 
 		buflen -= buffer[0];
 		buffer += buffer[0];
diff --git a/drivers/media/usb/uvc/uvc_v4l2.c b/drivers/media/usb/uvc/uvc_v4l2.c
index 7f14096cb44d..79daf46b3dcd 100644
--- a/drivers/media/usb/uvc/uvc_v4l2.c
+++ b/drivers/media/usb/uvc/uvc_v4l2.c
@@ -279,6 +279,9 @@ static int uvc_v4l2_try_format(struct uvc_streaming *stream,
 	fmt->fmt.pix.sizeimage = probe->dwMaxVideoFrameSize;
 	fmt->fmt.pix.pixelformat = format->fcc;
 	fmt->fmt.pix.colorspace = format->colorspace;
+	fmt->fmt.pix.xfer_func = format->xfer_func;
+	fmt->fmt.pix.ycbcr_enc = format->ycbcr_enc;
+	fmt->fmt.pix.quantization = format->quantization;
 
 	if (uvc_format != NULL)
 		*uvc_format = format;
@@ -315,6 +318,9 @@ static int uvc_v4l2_get_format(struct uvc_streaming *stream,
 	fmt->fmt.pix.bytesperline = uvc_v4l2_get_bytesperline(format, frame);
 	fmt->fmt.pix.sizeimage = stream->ctrl.dwMaxVideoFrameSize;
 	fmt->fmt.pix.colorspace = format->colorspace;
+	fmt->fmt.pix.xfer_func = format->xfer_func;
+	fmt->fmt.pix.ycbcr_enc = format->ycbcr_enc;
+	fmt->fmt.pix.quantization = format->quantization;
 
 done:
 	mutex_unlock(&stream->mutex);
diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
index 6ab972c643e3..6508192173dd 100644
--- a/drivers/media/usb/uvc/uvcvideo.h
+++ b/drivers/media/usb/uvc/uvcvideo.h
@@ -370,7 +370,10 @@ struct uvc_format {
 	u8 type;
 	u8 index;
 	u8 bpp;
-	u8 colorspace;
+	enum v4l2_colorspace colorspace;
+	enum v4l2_xfer_func xfer_func;
+	enum v4l2_ycbcr_encoding ycbcr_enc;
+	enum v4l2_quantization quantization;
 	u32 fcc;
 	u32 flags;
 
-- 
2.28.0.297.g1956fa8f8d-goog

Re: [PATCH 2/2] media: uvcvideo: Convey full ycbcr colorspace information to v4l2

[Laurent Pinchart]

Hi Adam,

Thank you for the patch.

On Sat, Aug 22, 2020 at 09:21:34PM -0400, Adam Goode wrote:
> The Color Matching Descriptor has been present in USB cameras since
> the original version of UVC, but it has never been fully used
> in Linux.
> 
> This change informs V4L2 of all of the critical colorspace parameters:
> colorspace (called "color primaries" in UVC), transfer function
> (called "transfer characteristics" in UVC), ycbcr encoding (called
> "matrix coefficients" in UVC), and quantization, which is always
> LIMITED for UVC, see section 2.26 in USB_Video_FAQ_1.5.pdf.

Isn't this valid for MJPEG only though ? There's not much else we can do
though, as UVC cameras don't report quantization information. Shouldn't
we however reflect this in the commit message, and in the comment below,
to state that UVC requires limited quantization range for MJPEG, and
while it doesn't explicitly specify the quantization range for other
formats, we can only assume it should be limited as well ?

The code otherwise looks good to me.

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Please let me know if you'd like to address the above issue.

> The quantization is the most important improvement for this patch,
> because V4L2 will otherwise interpret MJPEG as FULL range. Web browsers
> such as Chrome and Firefox already ignore V4L2's quantization for USB
> devices and use the correct LIMITED value, but other programs such
> as qv4l2 will incorrectly interpret the output of MJPEG from USB
> cameras without this change.
> 
> Signed-off-by: Adam Goode <agoode@google.com>
> ---
>  drivers/media/usb/uvc/uvc_driver.c | 52 +++++++++++++++++++++++++++---
>  drivers/media/usb/uvc/uvc_v4l2.c   |  6 ++++
>  drivers/media/usb/uvc/uvcvideo.h   |  5 ++-
>  3 files changed, 58 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
> index 431d86e1c94b..c0c81b089b7d 100644
> --- a/drivers/media/usb/uvc/uvc_driver.c
> +++ b/drivers/media/usb/uvc/uvc_driver.c
> @@ -248,10 +248,10 @@ static struct uvc_format_desc *uvc_format_by_guid(const u8 guid[16])
>  	return NULL;
>  }
>  
> -static u32 uvc_colorspace(const u8 primaries)
> +static enum v4l2_colorspace uvc_colorspace(const u8 primaries)
>  {
> -	static const u8 colorprimaries[] = {
> -		0,
> +	static const enum v4l2_colorspace colorprimaries[] = {
> +		V4L2_COLORSPACE_DEFAULT,  /* Unspecified */
>  		V4L2_COLORSPACE_SRGB,
>  		V4L2_COLORSPACE_470_SYSTEM_M,
>  		V4L2_COLORSPACE_470_SYSTEM_BG,
> @@ -262,7 +262,43 @@ static u32 uvc_colorspace(const u8 primaries)
>  	if (primaries < ARRAY_SIZE(colorprimaries))
>  		return colorprimaries[primaries];
>  
> -	return 0;
> +	return V4L2_COLORSPACE_DEFAULT;  /* Reserved */
> +}
> +
> +static enum v4l2_xfer_func uvc_xfer_func(const u8 transfer_characteristics)
> +{
> +	static const enum v4l2_xfer_func xfer_funcs[] = {
> +		V4L2_XFER_FUNC_DEFAULT,  /* Unspecified */
> +		V4L2_XFER_FUNC_709,
> +		V4L2_XFER_FUNC_709,      /* BT.470-2 M */
> +		V4L2_XFER_FUNC_709,      /* BT.470-2 B, G */
> +		V4L2_XFER_FUNC_709,      /* SMPTE 170M */
> +		V4L2_XFER_FUNC_SMPTE240M,
> +		V4L2_XFER_FUNC_NONE,     /* Linear (V = Lc) */
> +		V4L2_XFER_FUNC_SRGB,
> +	};
> +
> +	if (transfer_characteristics < ARRAY_SIZE(xfer_funcs))
> +		return xfer_funcs[transfer_characteristics];
> +
> +	return V4L2_XFER_FUNC_DEFAULT;  /* Reserved */
> +}
> +
> +static enum v4l2_ycbcr_encoding uvc_ycbcr_enc(const u8 matrix_coefficients)
> +{
> +	static const enum v4l2_ycbcr_encoding ycbcr_encs[] = {
> +		V4L2_YCBCR_ENC_DEFAULT,  /* Unspecified */
> +		V4L2_YCBCR_ENC_709,
> +		V4L2_YCBCR_ENC_601,      /* FCC */
> +		V4L2_YCBCR_ENC_601,      /* BT.470-2 B, G */
> +		V4L2_YCBCR_ENC_601,      /* SMPTE 170M */
> +		V4L2_YCBCR_ENC_SMPTE240M,
> +	};
> +
> +	if (matrix_coefficients < ARRAY_SIZE(ycbcr_encs))
> +		return ycbcr_encs[matrix_coefficients];
> +
> +	return V4L2_YCBCR_ENC_DEFAULT;  /* Reserved */
>  }
>  
>  /* Simplify a fraction using a simple continued fraction decomposition. The
> @@ -704,6 +740,14 @@ static int uvc_parse_format(struct uvc_device *dev,
>  		}
>  
>  		format->colorspace = uvc_colorspace(buffer[3]);
> +		format->xfer_func = uvc_xfer_func(buffer[4]);
> +		format->ycbcr_enc = uvc_ycbcr_enc(buffer[5]);
> +		/* All USB YCbCr encodings use LIMITED range as of UVC 1.5.
> +		 * This is true even for MJPEG, which V4L2 otherwise assumes to
> +		 * be FULL.
> +		 * See section 2.26 in USB_Video_FAQ_1.5.pdf.
> +		 */
> +		format->quantization = V4L2_QUANTIZATION_LIM_RANGE;
>  
>  		buflen -= buffer[0];
>  		buffer += buffer[0];
> diff --git a/drivers/media/usb/uvc/uvc_v4l2.c b/drivers/media/usb/uvc/uvc_v4l2.c
> index 7f14096cb44d..79daf46b3dcd 100644
> --- a/drivers/media/usb/uvc/uvc_v4l2.c
> +++ b/drivers/media/usb/uvc/uvc_v4l2.c
> @@ -279,6 +279,9 @@ static int uvc_v4l2_try_format(struct uvc_streaming *stream,
>  	fmt->fmt.pix.sizeimage = probe->dwMaxVideoFrameSize;
>  	fmt->fmt.pix.pixelformat = format->fcc;
>  	fmt->fmt.pix.colorspace = format->colorspace;
> +	fmt->fmt.pix.xfer_func = format->xfer_func;
> +	fmt->fmt.pix.ycbcr_enc = format->ycbcr_enc;
> +	fmt->fmt.pix.quantization = format->quantization;
>  
>  	if (uvc_format != NULL)
>  		*uvc_format = format;
> @@ -315,6 +318,9 @@ static int uvc_v4l2_get_format(struct uvc_streaming *stream,
>  	fmt->fmt.pix.bytesperline = uvc_v4l2_get_bytesperline(format, frame);
>  	fmt->fmt.pix.sizeimage = stream->ctrl.dwMaxVideoFrameSize;
>  	fmt->fmt.pix.colorspace = format->colorspace;
> +	fmt->fmt.pix.xfer_func = format->xfer_func;
> +	fmt->fmt.pix.ycbcr_enc = format->ycbcr_enc;
> +	fmt->fmt.pix.quantization = format->quantization;
>  
>  done:
>  	mutex_unlock(&stream->mutex);
> diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
> index 6ab972c643e3..6508192173dd 100644
> --- a/drivers/media/usb/uvc/uvcvideo.h
> +++ b/drivers/media/usb/uvc/uvcvideo.h
> @@ -370,7 +370,10 @@ struct uvc_format {
>  	u8 type;
>  	u8 index;
>  	u8 bpp;
> -	u8 colorspace;
> +	enum v4l2_colorspace colorspace;
> +	enum v4l2_xfer_func xfer_func;
> +	enum v4l2_ycbcr_encoding ycbcr_enc;
> +	enum v4l2_quantization quantization;
>  	u32 fcc;
>  	u32 flags;
>  

-- 
Regards,

Laurent Pinchart

Re: [PATCH 2/2] media: uvcvideo: Convey full ycbcr colorspace information to v4l2

[Laurent Pinchart]

Hi Adam,

(CC'ing Hans Verkuil)

On Sun, Aug 23, 2020 at 05:54:24PM +0300, Laurent Pinchart wrote:
> Hi Adam,
> 
> Thank you for the patch.
> 
> On Sat, Aug 22, 2020 at 09:21:34PM -0400, Adam Goode wrote:
> > The Color Matching Descriptor has been present in USB cameras since
> > the original version of UVC, but it has never been fully used
> > in Linux.
> > 
> > This change informs V4L2 of all of the critical colorspace parameters:
> > colorspace (called "color primaries" in UVC), transfer function
> > (called "transfer characteristics" in UVC), ycbcr encoding (called
> > "matrix coefficients" in UVC), and quantization, which is always
> > LIMITED for UVC, see section 2.26 in USB_Video_FAQ_1.5.pdf.
> 
> Isn't this valid for MJPEG only though ? There's not much else we can do
> though, as UVC cameras don't report quantization information. Shouldn't
> we however reflect this in the commit message, and in the comment below,
> to state that UVC requires limited quantization range for MJPEG, and
> while it doesn't explicitly specify the quantization range for other
> formats, we can only assume it should be limited as well ?
> 
> The code otherwise looks good to me.
> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> 
> Please let me know if you'd like to address the above issue.
> 
> > The quantization is the most important improvement for this patch,
> > because V4L2 will otherwise interpret MJPEG as FULL range. Web browsers
> > such as Chrome and Firefox already ignore V4L2's quantization for USB
> > devices and use the correct LIMITED value, but other programs such
> > as qv4l2 will incorrectly interpret the output of MJPEG from USB
> > cameras without this change.
> > 
> > Signed-off-by: Adam Goode <agoode@google.com>
> > ---
> >  drivers/media/usb/uvc/uvc_driver.c | 52 +++++++++++++++++++++++++++---
> >  drivers/media/usb/uvc/uvc_v4l2.c   |  6 ++++
> >  drivers/media/usb/uvc/uvcvideo.h   |  5 ++-
> >  3 files changed, 58 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
> > index 431d86e1c94b..c0c81b089b7d 100644
> > --- a/drivers/media/usb/uvc/uvc_driver.c
> > +++ b/drivers/media/usb/uvc/uvc_driver.c
> > @@ -248,10 +248,10 @@ static struct uvc_format_desc *uvc_format_by_guid(const u8 guid[16])
> >  	return NULL;
> >  }
> >  
> > -static u32 uvc_colorspace(const u8 primaries)
> > +static enum v4l2_colorspace uvc_colorspace(const u8 primaries)
> >  {
> > -	static const u8 colorprimaries[] = {
> > -		0,
> > +	static const enum v4l2_colorspace colorprimaries[] = {
> > +		V4L2_COLORSPACE_DEFAULT,  /* Unspecified */
> >  		V4L2_COLORSPACE_SRGB,
> >  		V4L2_COLORSPACE_470_SYSTEM_M,
> >  		V4L2_COLORSPACE_470_SYSTEM_BG,
> > @@ -262,7 +262,43 @@ static u32 uvc_colorspace(const u8 primaries)
> >  	if (primaries < ARRAY_SIZE(colorprimaries))
> >  		return colorprimaries[primaries];
> >  
> > -	return 0;
> > +	return V4L2_COLORSPACE_DEFAULT;  /* Reserved */
> > +}
> > +
> > +static enum v4l2_xfer_func uvc_xfer_func(const u8 transfer_characteristics)
> > +{
> > +	static const enum v4l2_xfer_func xfer_funcs[] = {
> > +		V4L2_XFER_FUNC_DEFAULT,  /* Unspecified */
> > +		V4L2_XFER_FUNC_709,
> > +		V4L2_XFER_FUNC_709,      /* BT.470-2 M */
> > +		V4L2_XFER_FUNC_709,      /* BT.470-2 B, G */
> > +		V4L2_XFER_FUNC_709,      /* SMPTE 170M */
> > +		V4L2_XFER_FUNC_SMPTE240M,
> > +		V4L2_XFER_FUNC_NONE,     /* Linear (V = Lc) */
> > +		V4L2_XFER_FUNC_SRGB,
> > +	};
> > +
> > +	if (transfer_characteristics < ARRAY_SIZE(xfer_funcs))
> > +		return xfer_funcs[transfer_characteristics];
> > +
> > +	return V4L2_XFER_FUNC_DEFAULT;  /* Reserved */
> > +}
> > +
> > +static enum v4l2_ycbcr_encoding uvc_ycbcr_enc(const u8 matrix_coefficients)
> > +{
> > +	static const enum v4l2_ycbcr_encoding ycbcr_encs[] = {
> > +		V4L2_YCBCR_ENC_DEFAULT,  /* Unspecified */
> > +		V4L2_YCBCR_ENC_709,
> > +		V4L2_YCBCR_ENC_601,      /* FCC */

I may have spoken a bit too fast. Doesn't FCC differ from BT.601 ?
According to https://en.wikipedia.org/wiki/Talk%3AYCbCr, the former uses

 E'Y = 0.59 E'G + 0.11 E'B + 0.30 E'R
 E'PB = – 0.331 E'G + 0.500 E'B – 0.169 E'R
 E'PR = – 0.421 E'G – 0.079 E'B + 0.500 E'R

while the latter uses

 E'Y = 0.587 E'G + 0.114 E'B + 0.299 E'R
 E'PB = – 0.331 E'G + 0.500 E'B – 0.169 E'R
 E'PR = – 0.419 E'G – 0.081 E'B + 0.500 E'R

We seems to be missing FCC in the V4L2 color space definitions.

> > +		V4L2_YCBCR_ENC_601,      /* BT.470-2 B, G */
> > +		V4L2_YCBCR_ENC_601,      /* SMPTE 170M */
> > +		V4L2_YCBCR_ENC_SMPTE240M,
> > +	};
> > +
> > +	if (matrix_coefficients < ARRAY_SIZE(ycbcr_encs))
> > +		return ycbcr_encs[matrix_coefficients];
> > +
> > +	return V4L2_YCBCR_ENC_DEFAULT;  /* Reserved */
> >  }
> >  
> >  /* Simplify a fraction using a simple continued fraction decomposition. The
> > @@ -704,6 +740,14 @@ static int uvc_parse_format(struct uvc_device *dev,
> >  		}
> >  
> >  		format->colorspace = uvc_colorspace(buffer[3]);
> > +		format->xfer_func = uvc_xfer_func(buffer[4]);
> > +		format->ycbcr_enc = uvc_ycbcr_enc(buffer[5]);
> > +		/* All USB YCbCr encodings use LIMITED range as of UVC 1.5.
> > +		 * This is true even for MJPEG, which V4L2 otherwise assumes to
> > +		 * be FULL.
> > +		 * See section 2.26 in USB_Video_FAQ_1.5.pdf.
> > +		 */
> > +		format->quantization = V4L2_QUANTIZATION_LIM_RANGE;
> >  
> >  		buflen -= buffer[0];
> >  		buffer += buffer[0];
> > diff --git a/drivers/media/usb/uvc/uvc_v4l2.c b/drivers/media/usb/uvc/uvc_v4l2.c
> > index 7f14096cb44d..79daf46b3dcd 100644
> > --- a/drivers/media/usb/uvc/uvc_v4l2.c
> > +++ b/drivers/media/usb/uvc/uvc_v4l2.c
> > @@ -279,6 +279,9 @@ static int uvc_v4l2_try_format(struct uvc_streaming *stream,
> >  	fmt->fmt.pix.sizeimage = probe->dwMaxVideoFrameSize;
> >  	fmt->fmt.pix.pixelformat = format->fcc;
> >  	fmt->fmt.pix.colorspace = format->colorspace;
> > +	fmt->fmt.pix.xfer_func = format->xfer_func;
> > +	fmt->fmt.pix.ycbcr_enc = format->ycbcr_enc;
> > +	fmt->fmt.pix.quantization = format->quantization;
> >  
> >  	if (uvc_format != NULL)
> >  		*uvc_format = format;
> > @@ -315,6 +318,9 @@ static int uvc_v4l2_get_format(struct uvc_streaming *stream,
> >  	fmt->fmt.pix.bytesperline = uvc_v4l2_get_bytesperline(format, frame);
> >  	fmt->fmt.pix.sizeimage = stream->ctrl.dwMaxVideoFrameSize;
> >  	fmt->fmt.pix.colorspace = format->colorspace;
> > +	fmt->fmt.pix.xfer_func = format->xfer_func;
> > +	fmt->fmt.pix.ycbcr_enc = format->ycbcr_enc;
> > +	fmt->fmt.pix.quantization = format->quantization;
> >  
> >  done:
> >  	mutex_unlock(&stream->mutex);
> > diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
> > index 6ab972c643e3..6508192173dd 100644
> > --- a/drivers/media/usb/uvc/uvcvideo.h
> > +++ b/drivers/media/usb/uvc/uvcvideo.h
> > @@ -370,7 +370,10 @@ struct uvc_format {
> >  	u8 type;
> >  	u8 index;
> >  	u8 bpp;
> > -	u8 colorspace;
> > +	enum v4l2_colorspace colorspace;
> > +	enum v4l2_xfer_func xfer_func;
> > +	enum v4l2_ycbcr_encoding ycbcr_enc;
> > +	enum v4l2_quantization quantization;
> >  	u32 fcc;
> >  	u32 flags;
> >  

-- 
Regards,

Laurent Pinchart

Re: [PATCH 2/2] media: uvcvideo: Convey full ycbcr colorspace information to v4l2

[Hans Verkuil]

On 23/08/2020 17:08, Laurent Pinchart wrote:
> Hi Adam,
> 
> (CC'ing Hans Verkuil)
> 
> On Sun, Aug 23, 2020 at 05:54:24PM +0300, Laurent Pinchart wrote:
>> Hi Adam,
>>
>> Thank you for the patch.
>>
>> On Sat, Aug 22, 2020 at 09:21:34PM -0400, Adam Goode wrote:
>>> The Color Matching Descriptor has been present in USB cameras since
>>> the original version of UVC, but it has never been fully used
>>> in Linux.
>>>
>>> This change informs V4L2 of all of the critical colorspace parameters:
>>> colorspace (called "color primaries" in UVC), transfer function
>>> (called "transfer characteristics" in UVC), ycbcr encoding (called
>>> "matrix coefficients" in UVC), and quantization, which is always
>>> LIMITED for UVC, see section 2.26 in USB_Video_FAQ_1.5.pdf.
>>
>> Isn't this valid for MJPEG only though ? There's not much else we can do
>> though, as UVC cameras don't report quantization information. Shouldn't
>> we however reflect this in the commit message, and in the comment below,
>> to state that UVC requires limited quantization range for MJPEG, and
>> while it doesn't explicitly specify the quantization range for other
>> formats, we can only assume it should be limited as well ?
>>
>> The code otherwise looks good to me.
>>
>> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>>
>> Please let me know if you'd like to address the above issue.
>>
>>> The quantization is the most important improvement for this patch,
>>> because V4L2 will otherwise interpret MJPEG as FULL range. Web browsers
>>> such as Chrome and Firefox already ignore V4L2's quantization for USB
>>> devices and use the correct LIMITED value, but other programs such
>>> as qv4l2 will incorrectly interpret the output of MJPEG from USB
>>> cameras without this change.
>>>
>>> Signed-off-by: Adam Goode <agoode@google.com>
>>> ---
>>>  drivers/media/usb/uvc/uvc_driver.c | 52 +++++++++++++++++++++++++++---
>>>  drivers/media/usb/uvc/uvc_v4l2.c   |  6 ++++
>>>  drivers/media/usb/uvc/uvcvideo.h   |  5 ++-
>>>  3 files changed, 58 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
>>> index 431d86e1c94b..c0c81b089b7d 100644
>>> --- a/drivers/media/usb/uvc/uvc_driver.c
>>> +++ b/drivers/media/usb/uvc/uvc_driver.c
>>> @@ -248,10 +248,10 @@ static struct uvc_format_desc *uvc_format_by_guid(const u8 guid[16])
>>>  	return NULL;
>>>  }
>>>  
>>> -static u32 uvc_colorspace(const u8 primaries)
>>> +static enum v4l2_colorspace uvc_colorspace(const u8 primaries)
>>>  {
>>> -	static const u8 colorprimaries[] = {
>>> -		0,
>>> +	static const enum v4l2_colorspace colorprimaries[] = {
>>> +		V4L2_COLORSPACE_DEFAULT,  /* Unspecified */
>>>  		V4L2_COLORSPACE_SRGB,
>>>  		V4L2_COLORSPACE_470_SYSTEM_M,
>>>  		V4L2_COLORSPACE_470_SYSTEM_BG,
>>> @@ -262,7 +262,43 @@ static u32 uvc_colorspace(const u8 primaries)
>>>  	if (primaries < ARRAY_SIZE(colorprimaries))
>>>  		return colorprimaries[primaries];
>>>  
>>> -	return 0;
>>> +	return V4L2_COLORSPACE_DEFAULT;  /* Reserved */
>>> +}
>>> +
>>> +static enum v4l2_xfer_func uvc_xfer_func(const u8 transfer_characteristics)
>>> +{
>>> +	static const enum v4l2_xfer_func xfer_funcs[] = {
>>> +		V4L2_XFER_FUNC_DEFAULT,  /* Unspecified */
>>> +		V4L2_XFER_FUNC_709,
>>> +		V4L2_XFER_FUNC_709,      /* BT.470-2 M */
>>> +		V4L2_XFER_FUNC_709,      /* BT.470-2 B, G */
>>> +		V4L2_XFER_FUNC_709,      /* SMPTE 170M */
>>> +		V4L2_XFER_FUNC_SMPTE240M,
>>> +		V4L2_XFER_FUNC_NONE,     /* Linear (V = Lc) */
>>> +		V4L2_XFER_FUNC_SRGB,
>>> +	};
>>> +
>>> +	if (transfer_characteristics < ARRAY_SIZE(xfer_funcs))
>>> +		return xfer_funcs[transfer_characteristics];
>>> +
>>> +	return V4L2_XFER_FUNC_DEFAULT;  /* Reserved */
>>> +}
>>> +
>>> +static enum v4l2_ycbcr_encoding uvc_ycbcr_enc(const u8 matrix_coefficients)
>>> +{
>>> +	static const enum v4l2_ycbcr_encoding ycbcr_encs[] = {
>>> +		V4L2_YCBCR_ENC_DEFAULT,  /* Unspecified */
>>> +		V4L2_YCBCR_ENC_709,
>>> +		V4L2_YCBCR_ENC_601,      /* FCC */
> 
> I may have spoken a bit too fast. Doesn't FCC differ from BT.601 ?
> According to https://en.wikipedia.org/wiki/Talk%3AYCbCr, the former uses
> 
>  E'Y = 0.59 E'G + 0.11 E'B + 0.30 E'R
>  E'PB = – 0.331 E'G + 0.500 E'B – 0.169 E'R
>  E'PR = – 0.421 E'G – 0.079 E'B + 0.500 E'R
> 
> while the latter uses
> 
>  E'Y = 0.587 E'G + 0.114 E'B + 0.299 E'R
>  E'PB = – 0.331 E'G + 0.500 E'B – 0.169 E'R
>  E'PR = – 0.419 E'G – 0.081 E'B + 0.500 E'R
> 
> We seems to be missing FCC in the V4L2 color space definitions.

The differences between the two are minuscule. Add to that that NTSC 1953
hasn't been in use for many decades. So I have no plans to add another YCC
encoding for this. I'll double check this in a few weeks time when I have
access to a better book on colorimetry.

> 
>>> +		V4L2_YCBCR_ENC_601,      /* BT.470-2 B, G */
>>> +		V4L2_YCBCR_ENC_601,      /* SMPTE 170M */
>>> +		V4L2_YCBCR_ENC_SMPTE240M,
>>> +	};
>>> +
>>> +	if (matrix_coefficients < ARRAY_SIZE(ycbcr_encs))
>>> +		return ycbcr_encs[matrix_coefficients];
>>> +
>>> +	return V4L2_YCBCR_ENC_DEFAULT;  /* Reserved */
>>>  }
>>>  
>>>  /* Simplify a fraction using a simple continued fraction decomposition. The
>>> @@ -704,6 +740,14 @@ static int uvc_parse_format(struct uvc_device *dev,
>>>  		}
>>>  
>>>  		format->colorspace = uvc_colorspace(buffer[3]);
>>> +		format->xfer_func = uvc_xfer_func(buffer[4]);
>>> +		format->ycbcr_enc = uvc_ycbcr_enc(buffer[5]);
>>> +		/* All USB YCbCr encodings use LIMITED range as of UVC 1.5.
>>> +		 * This is true even for MJPEG, which V4L2 otherwise assumes to
>>> +		 * be FULL.
>>> +		 * See section 2.26 in USB_Video_FAQ_1.5.pdf.

Not true. I checked the FAQ: the FAQ describes what happens when a video renderer
incorrectly interprets the decoded JPEG color components as limited range instead
of full range (which they are to be JPEG compliant). JPEG always encodes YCbCr as
full range.

>>> +		 */
>>> +		format->quantization = V4L2_QUANTIZATION_LIM_RANGE;

What about sRGB? That uses full range.

Regards,

	Hans

>>>  
>>>  		buflen -= buffer[0];
>>>  		buffer += buffer[0];
>>> diff --git a/drivers/media/usb/uvc/uvc_v4l2.c b/drivers/media/usb/uvc/uvc_v4l2.c
>>> index 7f14096cb44d..79daf46b3dcd 100644
>>> --- a/drivers/media/usb/uvc/uvc_v4l2.c
>>> +++ b/drivers/media/usb/uvc/uvc_v4l2.c
>>> @@ -279,6 +279,9 @@ static int uvc_v4l2_try_format(struct uvc_streaming *stream,
>>>  	fmt->fmt.pix.sizeimage = probe->dwMaxVideoFrameSize;
>>>  	fmt->fmt.pix.pixelformat = format->fcc;
>>>  	fmt->fmt.pix.colorspace = format->colorspace;
>>> +	fmt->fmt.pix.xfer_func = format->xfer_func;
>>> +	fmt->fmt.pix.ycbcr_enc = format->ycbcr_enc;
>>> +	fmt->fmt.pix.quantization = format->quantization;
>>>  
>>>  	if (uvc_format != NULL)
>>>  		*uvc_format = format;
>>> @@ -315,6 +318,9 @@ static int uvc_v4l2_get_format(struct uvc_streaming *stream,
>>>  	fmt->fmt.pix.bytesperline = uvc_v4l2_get_bytesperline(format, frame);
>>>  	fmt->fmt.pix.sizeimage = stream->ctrl.dwMaxVideoFrameSize;
>>>  	fmt->fmt.pix.colorspace = format->colorspace;
>>> +	fmt->fmt.pix.xfer_func = format->xfer_func;
>>> +	fmt->fmt.pix.ycbcr_enc = format->ycbcr_enc;
>>> +	fmt->fmt.pix.quantization = format->quantization;
>>>  
>>>  done:
>>>  	mutex_unlock(&stream->mutex);
>>> diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
>>> index 6ab972c643e3..6508192173dd 100644
>>> --- a/drivers/media/usb/uvc/uvcvideo.h
>>> +++ b/drivers/media/usb/uvc/uvcvideo.h
>>> @@ -370,7 +370,10 @@ struct uvc_format {
>>>  	u8 type;
>>>  	u8 index;
>>>  	u8 bpp;
>>> -	u8 colorspace;
>>> +	enum v4l2_colorspace colorspace;
>>> +	enum v4l2_xfer_func xfer_func;
>>> +	enum v4l2_ycbcr_encoding ycbcr_enc;
>>> +	enum v4l2_quantization quantization;
>>>  	u32 fcc;
>>>  	u32 flags;
>>>  
> 

Re: [PATCH 2/2] media: uvcvideo: Convey full ycbcr colorspace information to v4l2

[Adam Goode]

On Mon, Aug 24, 2020 at 4:48 AM Hans Verkuil <hverkuil-cisco@xs4all.nl> wrote:
>
> On 23/08/2020 17:08, Laurent Pinchart wrote:
> > Hi Adam,
> >
> > (CC'ing Hans Verkuil)
> >
> > On Sun, Aug 23, 2020 at 05:54:24PM +0300, Laurent Pinchart wrote:
> >> Hi Adam,
> >>
> >> Thank you for the patch.
> >>
> >> On Sat, Aug 22, 2020 at 09:21:34PM -0400, Adam Goode wrote:
> >>> The Color Matching Descriptor has been present in USB cameras since
> >>> the original version of UVC, but it has never been fully used
> >>> in Linux.
> >>>
> >>> This change informs V4L2 of all of the critical colorspace parameters:
> >>> colorspace (called "color primaries" in UVC), transfer function
> >>> (called "transfer characteristics" in UVC), ycbcr encoding (called
> >>> "matrix coefficients" in UVC), and quantization, which is always
> >>> LIMITED for UVC, see section 2.26 in USB_Video_FAQ_1.5.pdf.
> >>
> >> Isn't this valid for MJPEG only though ? There's not much else we can do
> >> though, as UVC cameras don't report quantization information. Shouldn't
> >> we however reflect this in the commit message, and in the comment below,
> >> to state that UVC requires limited quantization range for MJPEG, and
> >> while it doesn't explicitly specify the quantization range for other
> >> formats, we can only assume it should be limited as well ?
> >>

Yes, I am happy to improve the comment to be clearer.


> >> The code otherwise looks good to me.
> >>
> >> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> >>
> >> Please let me know if you'd like to address the above issue.
> >>
> >>> The quantization is the most important improvement for this patch,
> >>> because V4L2 will otherwise interpret MJPEG as FULL range. Web browsers
> >>> such as Chrome and Firefox already ignore V4L2's quantization for USB
> >>> devices and use the correct LIMITED value, but other programs such
> >>> as qv4l2 will incorrectly interpret the output of MJPEG from USB
> >>> cameras without this change.
> >>>
> >>> Signed-off-by: Adam Goode <agoode@google.com>
> >>> ---
> >>>  drivers/media/usb/uvc/uvc_driver.c | 52 +++++++++++++++++++++++++++---
> >>>  drivers/media/usb/uvc/uvc_v4l2.c   |  6 ++++
> >>>  drivers/media/usb/uvc/uvcvideo.h   |  5 ++-
> >>>  3 files changed, 58 insertions(+), 5 deletions(-)
> >>>
> >>> diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
> >>> index 431d86e1c94b..c0c81b089b7d 100644
> >>> --- a/drivers/media/usb/uvc/uvc_driver.c
> >>> +++ b/drivers/media/usb/uvc/uvc_driver.c
> >>> @@ -248,10 +248,10 @@ static struct uvc_format_desc *uvc_format_by_guid(const u8 guid[16])
> >>>     return NULL;
> >>>  }
> >>>
> >>> -static u32 uvc_colorspace(const u8 primaries)
> >>> +static enum v4l2_colorspace uvc_colorspace(const u8 primaries)
> >>>  {
> >>> -   static const u8 colorprimaries[] = {
> >>> -           0,
> >>> +   static const enum v4l2_colorspace colorprimaries[] = {
> >>> +           V4L2_COLORSPACE_DEFAULT,  /* Unspecified */
> >>>             V4L2_COLORSPACE_SRGB,
> >>>             V4L2_COLORSPACE_470_SYSTEM_M,
> >>>             V4L2_COLORSPACE_470_SYSTEM_BG,
> >>> @@ -262,7 +262,43 @@ static u32 uvc_colorspace(const u8 primaries)
> >>>     if (primaries < ARRAY_SIZE(colorprimaries))
> >>>             return colorprimaries[primaries];
> >>>
> >>> -   return 0;
> >>> +   return V4L2_COLORSPACE_DEFAULT;  /* Reserved */
> >>> +}
> >>> +
> >>> +static enum v4l2_xfer_func uvc_xfer_func(const u8 transfer_characteristics)
> >>> +{
> >>> +   static const enum v4l2_xfer_func xfer_funcs[] = {
> >>> +           V4L2_XFER_FUNC_DEFAULT,  /* Unspecified */
> >>> +           V4L2_XFER_FUNC_709,
> >>> +           V4L2_XFER_FUNC_709,      /* BT.470-2 M */
> >>> +           V4L2_XFER_FUNC_709,      /* BT.470-2 B, G */
> >>> +           V4L2_XFER_FUNC_709,      /* SMPTE 170M */
> >>> +           V4L2_XFER_FUNC_SMPTE240M,
> >>> +           V4L2_XFER_FUNC_NONE,     /* Linear (V = Lc) */
> >>> +           V4L2_XFER_FUNC_SRGB,
> >>> +   };
> >>> +
> >>> +   if (transfer_characteristics < ARRAY_SIZE(xfer_funcs))
> >>> +           return xfer_funcs[transfer_characteristics];
> >>> +
> >>> +   return V4L2_XFER_FUNC_DEFAULT;  /* Reserved */
> >>> +}
> >>> +
> >>> +static enum v4l2_ycbcr_encoding uvc_ycbcr_enc(const u8 matrix_coefficients)
> >>> +{
> >>> +   static const enum v4l2_ycbcr_encoding ycbcr_encs[] = {
> >>> +           V4L2_YCBCR_ENC_DEFAULT,  /* Unspecified */
> >>> +           V4L2_YCBCR_ENC_709,
> >>> +           V4L2_YCBCR_ENC_601,      /* FCC */
> >
> > I may have spoken a bit too fast. Doesn't FCC differ from BT.601 ?
> > According to https://en.wikipedia.org/wiki/Talk%3AYCbCr, the former uses
> >
> >  E'Y = 0.59 E'G + 0.11 E'B + 0.30 E'R
> >  E'PB = – 0.331 E'G + 0.500 E'B – 0.169 E'R
> >  E'PR = – 0.421 E'G – 0.079 E'B + 0.500 E'R
> >
> > while the latter uses
> >
> >  E'Y = 0.587 E'G + 0.114 E'B + 0.299 E'R
> >  E'PB = – 0.331 E'G + 0.500 E'B – 0.169 E'R
> >  E'PR = – 0.419 E'G – 0.081 E'B + 0.500 E'R
> >
> > We seems to be missing FCC in the V4L2 color space definitions.
>
> The differences between the two are minuscule. Add to that that NTSC 1953
> hasn't been in use for many decades. So I have no plans to add another YCC
> encoding for this. I'll double check this in a few weeks time when I have
> access to a better book on colorimetry.
>

I can add a comment directly to clarify, but I am following the
mappings described in videodev2.h (with the assumption that "FCC" is
close enough to 601):

/*
* Mapping of V4L2_XFER_FUNC_DEFAULT to actual transfer functions
* for the various colorspaces:
*
* V4L2_COLORSPACE_SMPTE170M, V4L2_COLORSPACE_470_SYSTEM_M,
* V4L2_COLORSPACE_470_SYSTEM_BG, V4L2_COLORSPACE_REC709 and
* V4L2_COLORSPACE_BT2020: V4L2_XFER_FUNC_709
*
* V4L2_COLORSPACE_SRGB, V4L2_COLORSPACE_JPEG: V4L2_XFER_FUNC_SRGB
*
* V4L2_COLORSPACE_OPRGB: V4L2_XFER_FUNC_OPRGB
*
* V4L2_COLORSPACE_SMPTE240M: V4L2_XFER_FUNC_SMPTE240M
*
* V4L2_COLORSPACE_RAW: V4L2_XFER_FUNC_NONE
*
* V4L2_COLORSPACE_DCI_P3: V4L2_XFER_FUNC_DCI_P3
*/

/*
* Mapping of V4L2_YCBCR_ENC_DEFAULT to actual encodings for the
* various colorspaces:
*
* V4L2_COLORSPACE_SMPTE170M, V4L2_COLORSPACE_470_SYSTEM_M,
* V4L2_COLORSPACE_470_SYSTEM_BG, V4L2_COLORSPACE_SRGB,
* V4L2_COLORSPACE_OPRGB and V4L2_COLORSPACE_JPEG: V4L2_YCBCR_ENC_601
*
* V4L2_COLORSPACE_REC709 and V4L2_COLORSPACE_DCI_P3: V4L2_YCBCR_ENC_709
*
* V4L2_COLORSPACE_BT2020: V4L2_YCBCR_ENC_BT2020
*
* V4L2_COLORSPACE_SMPTE240M: V4L2_YCBCR_ENC_SMPTE240M
*/

We could potentially do with some more xfer functions, though.

> >
> >>> +           V4L2_YCBCR_ENC_601,      /* BT.470-2 B, G */
> >>> +           V4L2_YCBCR_ENC_601,      /* SMPTE 170M */
> >>> +           V4L2_YCBCR_ENC_SMPTE240M,
> >>> +   };
> >>> +
> >>> +   if (matrix_coefficients < ARRAY_SIZE(ycbcr_encs))
> >>> +           return ycbcr_encs[matrix_coefficients];
> >>> +
> >>> +   return V4L2_YCBCR_ENC_DEFAULT;  /* Reserved */
> >>>  }
> >>>
> >>>  /* Simplify a fraction using a simple continued fraction decomposition. The
> >>> @@ -704,6 +740,14 @@ static int uvc_parse_format(struct uvc_device *dev,
> >>>             }
> >>>
> >>>             format->colorspace = uvc_colorspace(buffer[3]);
> >>> +           format->xfer_func = uvc_xfer_func(buffer[4]);
> >>> +           format->ycbcr_enc = uvc_ycbcr_enc(buffer[5]);
> >>> +           /* All USB YCbCr encodings use LIMITED range as of UVC 1.5.
> >>> +            * This is true even for MJPEG, which V4L2 otherwise assumes to
> >>> +            * be FULL.
> >>> +            * See section 2.26 in USB_Video_FAQ_1.5.pdf.
>
> Not true. I checked the FAQ: the FAQ describes what happens when a video renderer
> incorrectly interprets the decoded JPEG color components as limited range instead
> of full range (which they are to be JPEG compliant). JPEG always encodes YCbCr as
> full range.
>

Here is what the FAQ says:

"If the images are encoded with the luma and chroma units in the 0-255
range that is used
for typical JPEG still images, then the saturation and contrast will
look artificially boosted
when the video is rendered under the assumption that the levels were
in the YCbCr color
space. BT601 specifies eight-bit coding where Y is in the range of 16
(black) to 235 (white)
inclusive."

I read this as saying "if you encode MJPEG the same as typical JPEG
still images, it is wrong because Y must be in the range 16-235". Am I
reading this incorrectly?

> >>> +            */
> >>> +           format->quantization = V4L2_QUANTIZATION_LIM_RANGE;
>
> What about sRGB? That uses full range.
>

It is a little confusing in the code, but I only set the quantization
explicitly when we get a Color Matching descriptor from the device. My
reading of the spec says that this descriptor isn't present for RGB
formats, only YCrCb. When the spec mentions sRGB in Color Matching, it
is referring only to primaries or gamma.

> Regards,
>
>         Hans
>
> >>>
> >>>             buflen -= buffer[0];
> >>>             buffer += buffer[0];
> >>> diff --git a/drivers/media/usb/uvc/uvc_v4l2.c b/drivers/media/usb/uvc/uvc_v4l2.c
> >>> index 7f14096cb44d..79daf46b3dcd 100644
> >>> --- a/drivers/media/usb/uvc/uvc_v4l2.c
> >>> +++ b/drivers/media/usb/uvc/uvc_v4l2.c
> >>> @@ -279,6 +279,9 @@ static int uvc_v4l2_try_format(struct uvc_streaming *stream,
> >>>     fmt->fmt.pix.sizeimage = probe->dwMaxVideoFrameSize;
> >>>     fmt->fmt.pix.pixelformat = format->fcc;
> >>>     fmt->fmt.pix.colorspace = format->colorspace;
> >>> +   fmt->fmt.pix.xfer_func = format->xfer_func;
> >>> +   fmt->fmt.pix.ycbcr_enc = format->ycbcr_enc;
> >>> +   fmt->fmt.pix.quantization = format->quantization;
> >>>
> >>>     if (uvc_format != NULL)
> >>>             *uvc_format = format;
> >>> @@ -315,6 +318,9 @@ static int uvc_v4l2_get_format(struct uvc_streaming *stream,
> >>>     fmt->fmt.pix.bytesperline = uvc_v4l2_get_bytesperline(format, frame);
> >>>     fmt->fmt.pix.sizeimage = stream->ctrl.dwMaxVideoFrameSize;
> >>>     fmt->fmt.pix.colorspace = format->colorspace;
> >>> +   fmt->fmt.pix.xfer_func = format->xfer_func;
> >>> +   fmt->fmt.pix.ycbcr_enc = format->ycbcr_enc;
> >>> +   fmt->fmt.pix.quantization = format->quantization;
> >>>
> >>>  done:
> >>>     mutex_unlock(&stream->mutex);
> >>> diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
> >>> index 6ab972c643e3..6508192173dd 100644
> >>> --- a/drivers/media/usb/uvc/uvcvideo.h
> >>> +++ b/drivers/media/usb/uvc/uvcvideo.h
> >>> @@ -370,7 +370,10 @@ struct uvc_format {
> >>>     u8 type;
> >>>     u8 index;
> >>>     u8 bpp;
> >>> -   u8 colorspace;
> >>> +   enum v4l2_colorspace colorspace;
> >>> +   enum v4l2_xfer_func xfer_func;
> >>> +   enum v4l2_ycbcr_encoding ycbcr_enc;
> >>> +   enum v4l2_quantization quantization;
> >>>     u32 fcc;
> >>>     u32 flags;
> >>>
> >
>

Re: [PATCH 2/2] media: uvcvideo: Convey full ycbcr colorspace information to v4l2

[Hans Verkuil]

On 24/08/2020 15:56, Adam Goode wrote:
> On Mon, Aug 24, 2020 at 4:48 AM Hans Verkuil <hverkuil-cisco@xs4all.nl> wrote:
>>
>> On 23/08/2020 17:08, Laurent Pinchart wrote:
>>> Hi Adam,
>>>
>>> (CC'ing Hans Verkuil)
>>>
>>> On Sun, Aug 23, 2020 at 05:54:24PM +0300, Laurent Pinchart wrote:
>>>> Hi Adam,
>>>>
>>>> Thank you for the patch.
>>>>
>>>> On Sat, Aug 22, 2020 at 09:21:34PM -0400, Adam Goode wrote:
>>>>> The Color Matching Descriptor has been present in USB cameras since
>>>>> the original version of UVC, but it has never been fully used
>>>>> in Linux.
>>>>>
>>>>> This change informs V4L2 of all of the critical colorspace parameters:
>>>>> colorspace (called "color primaries" in UVC), transfer function
>>>>> (called "transfer characteristics" in UVC), ycbcr encoding (called
>>>>> "matrix coefficients" in UVC), and quantization, which is always
>>>>> LIMITED for UVC, see section 2.26 in USB_Video_FAQ_1.5.pdf.
>>>>
>>>> Isn't this valid for MJPEG only though ? There's not much else we can do
>>>> though, as UVC cameras don't report quantization information. Shouldn't
>>>> we however reflect this in the commit message, and in the comment below,
>>>> to state that UVC requires limited quantization range for MJPEG, and
>>>> while it doesn't explicitly specify the quantization range for other
>>>> formats, we can only assume it should be limited as well ?
>>>>
> 
> Yes, I am happy to improve the comment to be clearer.
> 
> 
>>>> The code otherwise looks good to me.
>>>>
>>>> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>>>>
>>>> Please let me know if you'd like to address the above issue.
>>>>
>>>>> The quantization is the most important improvement for this patch,
>>>>> because V4L2 will otherwise interpret MJPEG as FULL range. Web browsers
>>>>> such as Chrome and Firefox already ignore V4L2's quantization for USB
>>>>> devices and use the correct LIMITED value, but other programs such
>>>>> as qv4l2 will incorrectly interpret the output of MJPEG from USB
>>>>> cameras without this change.
>>>>>
>>>>> Signed-off-by: Adam Goode <agoode@google.com>
>>>>> ---
>>>>>  drivers/media/usb/uvc/uvc_driver.c | 52 +++++++++++++++++++++++++++---
>>>>>  drivers/media/usb/uvc/uvc_v4l2.c   |  6 ++++
>>>>>  drivers/media/usb/uvc/uvcvideo.h   |  5 ++-
>>>>>  3 files changed, 58 insertions(+), 5 deletions(-)
>>>>>
>>>>> diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
>>>>> index 431d86e1c94b..c0c81b089b7d 100644
>>>>> --- a/drivers/media/usb/uvc/uvc_driver.c
>>>>> +++ b/drivers/media/usb/uvc/uvc_driver.c
>>>>> @@ -248,10 +248,10 @@ static struct uvc_format_desc *uvc_format_by_guid(const u8 guid[16])
>>>>>     return NULL;
>>>>>  }
>>>>>
>>>>> -static u32 uvc_colorspace(const u8 primaries)
>>>>> +static enum v4l2_colorspace uvc_colorspace(const u8 primaries)
>>>>>  {
>>>>> -   static const u8 colorprimaries[] = {
>>>>> -           0,
>>>>> +   static const enum v4l2_colorspace colorprimaries[] = {
>>>>> +           V4L2_COLORSPACE_DEFAULT,  /* Unspecified */
>>>>>             V4L2_COLORSPACE_SRGB,
>>>>>             V4L2_COLORSPACE_470_SYSTEM_M,
>>>>>             V4L2_COLORSPACE_470_SYSTEM_BG,
>>>>> @@ -262,7 +262,43 @@ static u32 uvc_colorspace(const u8 primaries)
>>>>>     if (primaries < ARRAY_SIZE(colorprimaries))
>>>>>             return colorprimaries[primaries];
>>>>>
>>>>> -   return 0;
>>>>> +   return V4L2_COLORSPACE_DEFAULT;  /* Reserved */
>>>>> +}
>>>>> +
>>>>> +static enum v4l2_xfer_func uvc_xfer_func(const u8 transfer_characteristics)
>>>>> +{
>>>>> +   static const enum v4l2_xfer_func xfer_funcs[] = {
>>>>> +           V4L2_XFER_FUNC_DEFAULT,  /* Unspecified */
>>>>> +           V4L2_XFER_FUNC_709,
>>>>> +           V4L2_XFER_FUNC_709,      /* BT.470-2 M */
>>>>> +           V4L2_XFER_FUNC_709,      /* BT.470-2 B, G */
>>>>> +           V4L2_XFER_FUNC_709,      /* SMPTE 170M */
>>>>> +           V4L2_XFER_FUNC_SMPTE240M,
>>>>> +           V4L2_XFER_FUNC_NONE,     /* Linear (V = Lc) */
>>>>> +           V4L2_XFER_FUNC_SRGB,
>>>>> +   };
>>>>> +
>>>>> +   if (transfer_characteristics < ARRAY_SIZE(xfer_funcs))
>>>>> +           return xfer_funcs[transfer_characteristics];
>>>>> +
>>>>> +   return V4L2_XFER_FUNC_DEFAULT;  /* Reserved */
>>>>> +}
>>>>> +
>>>>> +static enum v4l2_ycbcr_encoding uvc_ycbcr_enc(const u8 matrix_coefficients)
>>>>> +{
>>>>> +   static const enum v4l2_ycbcr_encoding ycbcr_encs[] = {
>>>>> +           V4L2_YCBCR_ENC_DEFAULT,  /* Unspecified */
>>>>> +           V4L2_YCBCR_ENC_709,
>>>>> +           V4L2_YCBCR_ENC_601,      /* FCC */
>>>
>>> I may have spoken a bit too fast. Doesn't FCC differ from BT.601 ?
>>> According to https://en.wikipedia.org/wiki/Talk%3AYCbCr, the former uses
>>>
>>>  E'Y = 0.59 E'G + 0.11 E'B + 0.30 E'R
>>>  E'PB = – 0.331 E'G + 0.500 E'B – 0.169 E'R
>>>  E'PR = – 0.421 E'G – 0.079 E'B + 0.500 E'R
>>>
>>> while the latter uses
>>>
>>>  E'Y = 0.587 E'G + 0.114 E'B + 0.299 E'R
>>>  E'PB = – 0.331 E'G + 0.500 E'B – 0.169 E'R
>>>  E'PR = – 0.419 E'G – 0.081 E'B + 0.500 E'R
>>>
>>> We seems to be missing FCC in the V4L2 color space definitions.
>>
>> The differences between the two are minuscule. Add to that that NTSC 1953
>> hasn't been in use for many decades. So I have no plans to add another YCC
>> encoding for this. I'll double check this in a few weeks time when I have
>> access to a better book on colorimetry.
>>
> 
> I can add a comment directly to clarify, but I am following the
> mappings described in videodev2.h (with the assumption that "FCC" is
> close enough to 601):
> 
> /*
> * Mapping of V4L2_XFER_FUNC_DEFAULT to actual transfer functions
> * for the various colorspaces:
> *
> * V4L2_COLORSPACE_SMPTE170M, V4L2_COLORSPACE_470_SYSTEM_M,
> * V4L2_COLORSPACE_470_SYSTEM_BG, V4L2_COLORSPACE_REC709 and
> * V4L2_COLORSPACE_BT2020: V4L2_XFER_FUNC_709
> *
> * V4L2_COLORSPACE_SRGB, V4L2_COLORSPACE_JPEG: V4L2_XFER_FUNC_SRGB
> *
> * V4L2_COLORSPACE_OPRGB: V4L2_XFER_FUNC_OPRGB
> *
> * V4L2_COLORSPACE_SMPTE240M: V4L2_XFER_FUNC_SMPTE240M
> *
> * V4L2_COLORSPACE_RAW: V4L2_XFER_FUNC_NONE
> *
> * V4L2_COLORSPACE_DCI_P3: V4L2_XFER_FUNC_DCI_P3
> */
> 
> /*
> * Mapping of V4L2_YCBCR_ENC_DEFAULT to actual encodings for the
> * various colorspaces:
> *
> * V4L2_COLORSPACE_SMPTE170M, V4L2_COLORSPACE_470_SYSTEM_M,
> * V4L2_COLORSPACE_470_SYSTEM_BG, V4L2_COLORSPACE_SRGB,
> * V4L2_COLORSPACE_OPRGB and V4L2_COLORSPACE_JPEG: V4L2_YCBCR_ENC_601
> *
> * V4L2_COLORSPACE_REC709 and V4L2_COLORSPACE_DCI_P3: V4L2_YCBCR_ENC_709
> *
> * V4L2_COLORSPACE_BT2020: V4L2_YCBCR_ENC_BT2020
> *
> * V4L2_COLORSPACE_SMPTE240M: V4L2_YCBCR_ENC_SMPTE240M
> */
> 
> We could potentially do with some more xfer functions, though.
> 
>>>
>>>>> +           V4L2_YCBCR_ENC_601,      /* BT.470-2 B, G */
>>>>> +           V4L2_YCBCR_ENC_601,      /* SMPTE 170M */
>>>>> +           V4L2_YCBCR_ENC_SMPTE240M,
>>>>> +   };
>>>>> +
>>>>> +   if (matrix_coefficients < ARRAY_SIZE(ycbcr_encs))
>>>>> +           return ycbcr_encs[matrix_coefficients];
>>>>> +
>>>>> +   return V4L2_YCBCR_ENC_DEFAULT;  /* Reserved */
>>>>>  }
>>>>>
>>>>>  /* Simplify a fraction using a simple continued fraction decomposition. The
>>>>> @@ -704,6 +740,14 @@ static int uvc_parse_format(struct uvc_device *dev,
>>>>>             }
>>>>>
>>>>>             format->colorspace = uvc_colorspace(buffer[3]);
>>>>> +           format->xfer_func = uvc_xfer_func(buffer[4]);
>>>>> +           format->ycbcr_enc = uvc_ycbcr_enc(buffer[5]);
>>>>> +           /* All USB YCbCr encodings use LIMITED range as of UVC 1.5.
>>>>> +            * This is true even for MJPEG, which V4L2 otherwise assumes to
>>>>> +            * be FULL.
>>>>> +            * See section 2.26 in USB_Video_FAQ_1.5.pdf.
>>
>> Not true. I checked the FAQ: the FAQ describes what happens when a video renderer
>> incorrectly interprets the decoded JPEG color components as limited range instead
>> of full range (which they are to be JPEG compliant). JPEG always encodes YCbCr as
>> full range.
>>
> 
> Here is what the FAQ says:
> 
> "If the images are encoded with the luma and chroma units in the 0-255
> range that is used
> for typical JPEG still images, then the saturation and contrast will
> look artificially boosted
> when the video is rendered under the assumption that the levels were
> in the YCbCr color
> space. BT601 specifies eight-bit coding where Y is in the range of 16
> (black) to 235 (white)
> inclusive."
> 
> I read this as saying "if you encode MJPEG the same as typical JPEG
> still images, it is wrong because Y must be in the range 16-235". Am I
> reading this incorrectly?

I think so. It's the 'when the video is rendered under the assumption that
the levels were in the YCbCr color space.' that is the reason why the colors
are boosted. Normally a JPEG is either decoded to full range RGB or limited
range YCbCr. If it is decoded to full range YCbCr, then the renderer should
take that into account, otherwise the colors will be wrong ('boosted').

The text is a bit ambiguous, but JPEG always uses full range YCbCr, that's
just part of the JPEG standard.

Regards,

	Hans

> 
>>>>> +            */
>>>>> +           format->quantization = V4L2_QUANTIZATION_LIM_RANGE;
>>
>> What about sRGB? That uses full range.
>>
> 
> It is a little confusing in the code, but I only set the quantization
> explicitly when we get a Color Matching descriptor from the device. My
> reading of the spec says that this descriptor isn't present for RGB
> formats, only YCrCb. When the spec mentions sRGB in Color Matching, it
> is referring only to primaries or gamma.
> 
>> Regards,
>>
>>         Hans
>>
>>>>>
>>>>>             buflen -= buffer[0];
>>>>>             buffer += buffer[0];
>>>>> diff --git a/drivers/media/usb/uvc/uvc_v4l2.c b/drivers/media/usb/uvc/uvc_v4l2.c
>>>>> index 7f14096cb44d..79daf46b3dcd 100644
>>>>> --- a/drivers/media/usb/uvc/uvc_v4l2.c
>>>>> +++ b/drivers/media/usb/uvc/uvc_v4l2.c
>>>>> @@ -279,6 +279,9 @@ static int uvc_v4l2_try_format(struct uvc_streaming *stream,
>>>>>     fmt->fmt.pix.sizeimage = probe->dwMaxVideoFrameSize;
>>>>>     fmt->fmt.pix.pixelformat = format->fcc;
>>>>>     fmt->fmt.pix.colorspace = format->colorspace;
>>>>> +   fmt->fmt.pix.xfer_func = format->xfer_func;
>>>>> +   fmt->fmt.pix.ycbcr_enc = format->ycbcr_enc;
>>>>> +   fmt->fmt.pix.quantization = format->quantization;
>>>>>
>>>>>     if (uvc_format != NULL)
>>>>>             *uvc_format = format;
>>>>> @@ -315,6 +318,9 @@ static int uvc_v4l2_get_format(struct uvc_streaming *stream,
>>>>>     fmt->fmt.pix.bytesperline = uvc_v4l2_get_bytesperline(format, frame);
>>>>>     fmt->fmt.pix.sizeimage = stream->ctrl.dwMaxVideoFrameSize;
>>>>>     fmt->fmt.pix.colorspace = format->colorspace;
>>>>> +   fmt->fmt.pix.xfer_func = format->xfer_func;
>>>>> +   fmt->fmt.pix.ycbcr_enc = format->ycbcr_enc;
>>>>> +   fmt->fmt.pix.quantization = format->quantization;
>>>>>
>>>>>  done:
>>>>>     mutex_unlock(&stream->mutex);
>>>>> diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
>>>>> index 6ab972c643e3..6508192173dd 100644
>>>>> --- a/drivers/media/usb/uvc/uvcvideo.h
>>>>> +++ b/drivers/media/usb/uvc/uvcvideo.h
>>>>> @@ -370,7 +370,10 @@ struct uvc_format {
>>>>>     u8 type;
>>>>>     u8 index;
>>>>>     u8 bpp;
>>>>> -   u8 colorspace;
>>>>> +   enum v4l2_colorspace colorspace;
>>>>> +   enum v4l2_xfer_func xfer_func;
>>>>> +   enum v4l2_ycbcr_encoding ycbcr_enc;
>>>>> +   enum v4l2_quantization quantization;
>>>>>     u32 fcc;
>>>>>     u32 flags;
>>>>>
>>>
>>

Re: [PATCH 2/2] media: uvcvideo: Convey full ycbcr colorspace information to v4l2

[Adam Goode]

On Mon, Aug 24, 2020 at 10:38 AM Hans Verkuil <hverkuil-cisco@xs4all.nl> wrote:
>
> On 24/08/2020 15:56, Adam Goode wrote:
> > On Mon, Aug 24, 2020 at 4:48 AM Hans Verkuil <hverkuil-cisco@xs4all.nl> wrote:
> >>
> >> On 23/08/2020 17:08, Laurent Pinchart wrote:
> >>> Hi Adam,
> >>>
> >>> (CC'ing Hans Verkuil)
> >>>
> >>> On Sun, Aug 23, 2020 at 05:54:24PM +0300, Laurent Pinchart wrote:
> >>>> Hi Adam,
> >>>>
> >>>> Thank you for the patch.
> >>>>
> >>>> On Sat, Aug 22, 2020 at 09:21:34PM -0400, Adam Goode wrote:
> >>>>> The Color Matching Descriptor has been present in USB cameras since
> >>>>> the original version of UVC, but it has never been fully used
> >>>>> in Linux.
> >>>>>
> >>>>> This change informs V4L2 of all of the critical colorspace parameters:
> >>>>> colorspace (called "color primaries" in UVC), transfer function
> >>>>> (called "transfer characteristics" in UVC), ycbcr encoding (called
> >>>>> "matrix coefficients" in UVC), and quantization, which is always
> >>>>> LIMITED for UVC, see section 2.26 in USB_Video_FAQ_1.5.pdf.
> >>>>
> >>>> Isn't this valid for MJPEG only though ? There's not much else we can do
> >>>> though, as UVC cameras don't report quantization information. Shouldn't
> >>>> we however reflect this in the commit message, and in the comment below,
> >>>> to state that UVC requires limited quantization range for MJPEG, and
> >>>> while it doesn't explicitly specify the quantization range for other
> >>>> formats, we can only assume it should be limited as well ?
> >>>>
> >
> > Yes, I am happy to improve the comment to be clearer.
> >
> >
> >>>> The code otherwise looks good to me.
> >>>>
> >>>> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> >>>>
> >>>> Please let me know if you'd like to address the above issue.
> >>>>
> >>>>> The quantization is the most important improvement for this patch,
> >>>>> because V4L2 will otherwise interpret MJPEG as FULL range. Web browsers
> >>>>> such as Chrome and Firefox already ignore V4L2's quantization for USB
> >>>>> devices and use the correct LIMITED value, but other programs such
> >>>>> as qv4l2 will incorrectly interpret the output of MJPEG from USB
> >>>>> cameras without this change.
> >>>>>
> >>>>> Signed-off-by: Adam Goode <agoode@google.com>
> >>>>> ---
> >>>>>  drivers/media/usb/uvc/uvc_driver.c | 52 +++++++++++++++++++++++++++---
> >>>>>  drivers/media/usb/uvc/uvc_v4l2.c   |  6 ++++
> >>>>>  drivers/media/usb/uvc/uvcvideo.h   |  5 ++-
> >>>>>  3 files changed, 58 insertions(+), 5 deletions(-)
> >>>>>
> >>>>> diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
> >>>>> index 431d86e1c94b..c0c81b089b7d 100644
> >>>>> --- a/drivers/media/usb/uvc/uvc_driver.c
> >>>>> +++ b/drivers/media/usb/uvc/uvc_driver.c
> >>>>> @@ -248,10 +248,10 @@ static struct uvc_format_desc *uvc_format_by_guid(const u8 guid[16])
> >>>>>     return NULL;
> >>>>>  }
> >>>>>
> >>>>> -static u32 uvc_colorspace(const u8 primaries)
> >>>>> +static enum v4l2_colorspace uvc_colorspace(const u8 primaries)
> >>>>>  {
> >>>>> -   static const u8 colorprimaries[] = {
> >>>>> -           0,
> >>>>> +   static const enum v4l2_colorspace colorprimaries[] = {
> >>>>> +           V4L2_COLORSPACE_DEFAULT,  /* Unspecified */
> >>>>>             V4L2_COLORSPACE_SRGB,
> >>>>>             V4L2_COLORSPACE_470_SYSTEM_M,
> >>>>>             V4L2_COLORSPACE_470_SYSTEM_BG,
> >>>>> @@ -262,7 +262,43 @@ static u32 uvc_colorspace(const u8 primaries)
> >>>>>     if (primaries < ARRAY_SIZE(colorprimaries))
> >>>>>             return colorprimaries[primaries];
> >>>>>
> >>>>> -   return 0;
> >>>>> +   return V4L2_COLORSPACE_DEFAULT;  /* Reserved */
> >>>>> +}
> >>>>> +
> >>>>> +static enum v4l2_xfer_func uvc_xfer_func(const u8 transfer_characteristics)
> >>>>> +{
> >>>>> +   static const enum v4l2_xfer_func xfer_funcs[] = {
> >>>>> +           V4L2_XFER_FUNC_DEFAULT,  /* Unspecified */
> >>>>> +           V4L2_XFER_FUNC_709,
> >>>>> +           V4L2_XFER_FUNC_709,      /* BT.470-2 M */
> >>>>> +           V4L2_XFER_FUNC_709,      /* BT.470-2 B, G */
> >>>>> +           V4L2_XFER_FUNC_709,      /* SMPTE 170M */
> >>>>> +           V4L2_XFER_FUNC_SMPTE240M,
> >>>>> +           V4L2_XFER_FUNC_NONE,     /* Linear (V = Lc) */
> >>>>> +           V4L2_XFER_FUNC_SRGB,
> >>>>> +   };
> >>>>> +
> >>>>> +   if (transfer_characteristics < ARRAY_SIZE(xfer_funcs))
> >>>>> +           return xfer_funcs[transfer_characteristics];
> >>>>> +
> >>>>> +   return V4L2_XFER_FUNC_DEFAULT;  /* Reserved */
> >>>>> +}
> >>>>> +
> >>>>> +static enum v4l2_ycbcr_encoding uvc_ycbcr_enc(const u8 matrix_coefficients)
> >>>>> +{
> >>>>> +   static const enum v4l2_ycbcr_encoding ycbcr_encs[] = {
> >>>>> +           V4L2_YCBCR_ENC_DEFAULT,  /* Unspecified */
> >>>>> +           V4L2_YCBCR_ENC_709,
> >>>>> +           V4L2_YCBCR_ENC_601,      /* FCC */
> >>>
> >>> I may have spoken a bit too fast. Doesn't FCC differ from BT.601 ?
> >>> According to https://en.wikipedia.org/wiki/Talk%3AYCbCr, the former uses
> >>>
> >>>  E'Y = 0.59 E'G + 0.11 E'B + 0.30 E'R
> >>>  E'PB = – 0.331 E'G + 0.500 E'B – 0.169 E'R
> >>>  E'PR = – 0.421 E'G – 0.079 E'B + 0.500 E'R
> >>>
> >>> while the latter uses
> >>>
> >>>  E'Y = 0.587 E'G + 0.114 E'B + 0.299 E'R
> >>>  E'PB = – 0.331 E'G + 0.500 E'B – 0.169 E'R
> >>>  E'PR = – 0.419 E'G – 0.081 E'B + 0.500 E'R
> >>>
> >>> We seems to be missing FCC in the V4L2 color space definitions.
> >>
> >> The differences between the two are minuscule. Add to that that NTSC 1953
> >> hasn't been in use for many decades. So I have no plans to add another YCC
> >> encoding for this. I'll double check this in a few weeks time when I have
> >> access to a better book on colorimetry.
> >>
> >
> > I can add a comment directly to clarify, but I am following the
> > mappings described in videodev2.h (with the assumption that "FCC" is
> > close enough to 601):
> >
> > /*
> > * Mapping of V4L2_XFER_FUNC_DEFAULT to actual transfer functions
> > * for the various colorspaces:
> > *
> > * V4L2_COLORSPACE_SMPTE170M, V4L2_COLORSPACE_470_SYSTEM_M,
> > * V4L2_COLORSPACE_470_SYSTEM_BG, V4L2_COLORSPACE_REC709 and
> > * V4L2_COLORSPACE_BT2020: V4L2_XFER_FUNC_709
> > *
> > * V4L2_COLORSPACE_SRGB, V4L2_COLORSPACE_JPEG: V4L2_XFER_FUNC_SRGB
> > *
> > * V4L2_COLORSPACE_OPRGB: V4L2_XFER_FUNC_OPRGB
> > *
> > * V4L2_COLORSPACE_SMPTE240M: V4L2_XFER_FUNC_SMPTE240M
> > *
> > * V4L2_COLORSPACE_RAW: V4L2_XFER_FUNC_NONE
> > *
> > * V4L2_COLORSPACE_DCI_P3: V4L2_XFER_FUNC_DCI_P3
> > */
> >
> > /*
> > * Mapping of V4L2_YCBCR_ENC_DEFAULT to actual encodings for the
> > * various colorspaces:
> > *
> > * V4L2_COLORSPACE_SMPTE170M, V4L2_COLORSPACE_470_SYSTEM_M,
> > * V4L2_COLORSPACE_470_SYSTEM_BG, V4L2_COLORSPACE_SRGB,
> > * V4L2_COLORSPACE_OPRGB and V4L2_COLORSPACE_JPEG: V4L2_YCBCR_ENC_601
> > *
> > * V4L2_COLORSPACE_REC709 and V4L2_COLORSPACE_DCI_P3: V4L2_YCBCR_ENC_709
> > *
> > * V4L2_COLORSPACE_BT2020: V4L2_YCBCR_ENC_BT2020
> > *
> > * V4L2_COLORSPACE_SMPTE240M: V4L2_YCBCR_ENC_SMPTE240M
> > */
> >
> > We could potentially do with some more xfer functions, though.
> >
> >>>
> >>>>> +           V4L2_YCBCR_ENC_601,      /* BT.470-2 B, G */
> >>>>> +           V4L2_YCBCR_ENC_601,      /* SMPTE 170M */
> >>>>> +           V4L2_YCBCR_ENC_SMPTE240M,
> >>>>> +   };
> >>>>> +
> >>>>> +   if (matrix_coefficients < ARRAY_SIZE(ycbcr_encs))
> >>>>> +           return ycbcr_encs[matrix_coefficients];
> >>>>> +
> >>>>> +   return V4L2_YCBCR_ENC_DEFAULT;  /* Reserved */
> >>>>>  }
> >>>>>
> >>>>>  /* Simplify a fraction using a simple continued fraction decomposition. The
> >>>>> @@ -704,6 +740,14 @@ static int uvc_parse_format(struct uvc_device *dev,
> >>>>>             }
> >>>>>
> >>>>>             format->colorspace = uvc_colorspace(buffer[3]);
> >>>>> +           format->xfer_func = uvc_xfer_func(buffer[4]);
> >>>>> +           format->ycbcr_enc = uvc_ycbcr_enc(buffer[5]);
> >>>>> +           /* All USB YCbCr encodings use LIMITED range as of UVC 1.5.
> >>>>> +            * This is true even for MJPEG, which V4L2 otherwise assumes to
> >>>>> +            * be FULL.
> >>>>> +            * See section 2.26 in USB_Video_FAQ_1.5.pdf.
> >>
> >> Not true. I checked the FAQ: the FAQ describes what happens when a video renderer
> >> incorrectly interprets the decoded JPEG color components as limited range instead
> >> of full range (which they are to be JPEG compliant). JPEG always encodes YCbCr as
> >> full range.
> >>
> >
> > Here is what the FAQ says:
> >
> > "If the images are encoded with the luma and chroma units in the 0-255
> > range that is used
> > for typical JPEG still images, then the saturation and contrast will
> > look artificially boosted
> > when the video is rendered under the assumption that the levels were
> > in the YCbCr color
> > space. BT601 specifies eight-bit coding where Y is in the range of 16
> > (black) to 235 (white)
> > inclusive."
> >
> > I read this as saying "if you encode MJPEG the same as typical JPEG
> > still images, it is wrong because Y must be in the range 16-235". Am I
> > reading this incorrectly?
>
> I think so. It's the 'when the video is rendered under the assumption that
> the levels were in the YCbCr color space.' that is the reason why the colors
> are boosted. Normally a JPEG is either decoded to full range RGB or limited
> range YCbCr. If it is decoded to full range YCbCr, then the renderer should
> take that into account, otherwise the colors will be wrong ('boosted').
>
> The text is a bit ambiguous, but JPEG always uses full range YCbCr, that's
> just part of the JPEG standard.
>
> Regards,
>
>         Hans
>

Hmm. The text is definitely confusing. Here are some facts I know:

About JPEG and JFIF:
- JPEG itself doesn't mandate the YCbCr encoding (this is specified in
JFIF). [https://www.w3.org/Graphics/JPEG/jfif3.pdf]
- JFIF specifies YCbCr encoding and range as 601 but with an explicit
change: "Y, Cb, and Cr are converted from R, G, and B as defined in
CCIR Recommendation 601 but are normalized so as to occupy the full
256 levels of a 8-bit binary encoding".
- A JPEG file isn't JFIF unless it has an APP0 with 'JFIF'.

About MJPEG:
- MJPEG doesn't specify explicit YCbCr encoding information (there
isn't really a specification?).
- The USB devices I have that output MJPEG do not output JFIF (no APP0
with 'JFIF').

About color in UVC:
- MJPEG in UVC is required to be 8-bit YCbCr encoded
[USB_Video_Payload_MJPEG_1.5.pdf, section 3.3] with the color encoding
information specified via the Color Matching descriptor
[USB_Video_Payload_MJPEG_1.5.pdf, section 3].
- The UVC Color Matching descriptor cites BT.601 and other standards
without mentioning any changes to them [UVC Class Specification
1.5.pdf, 3.9.2.6].
- BT.601 and BT.709 require limited-range YCbCr encoding.

Real-world observations:
- The USB webcams I have (Logitech) output limited-range UVC MJPEG.
- The ATEM Mini outputs full-range UVC MJPEG, but this is considered
to be incorrect behavior
(https://forum.blackmagicdesign.com/viewtopic.php?f=4&t=108315,
https://www.youtube.com/watch?v=BEXQ5s2HpwE).
- Chrome, Firefox, and Safari interpret UVC MJPEG as limited-range.


I would agree that if the MJPEG coming out of UVC has JFIF headers, we
would have a problem, because we would have conflicting YCbCr encoding
metadata.

But since:
1. UVC is pretty clear about how to encode color,
2. JPEG-without-JFIF doesn't define YCbCr encoding,
3. MJPEG devices output non-JFIF JPEG,
4. UVC only specifies limited-range encoding for YCbCr,

I would conclude that UVC MJPEG should be expected to be limited-range.



Thanks,

Adam

> >
> >>>>> +            */
> >>>>> +           format->quantization = V4L2_QUANTIZATION_LIM_RANGE;
> >>
> >> What about sRGB? That uses full range.
> >>
> >
> > It is a little confusing in the code, but I only set the quantization
> > explicitly when we get a Color Matching descriptor from the device. My
> > reading of the spec says that this descriptor isn't present for RGB
> > formats, only YCrCb. When the spec mentions sRGB in Color Matching, it
> > is referring only to primaries or gamma.
> >
> >> Regards,
> >>
> >>         Hans
> >>
> >>>>>
> >>>>>             buflen -= buffer[0];
> >>>>>             buffer += buffer[0];
> >>>>> diff --git a/drivers/media/usb/uvc/uvc_v4l2.c b/drivers/media/usb/uvc/uvc_v4l2.c
> >>>>> index 7f14096cb44d..79daf46b3dcd 100644
> >>>>> --- a/drivers/media/usb/uvc/uvc_v4l2.c
> >>>>> +++ b/drivers/media/usb/uvc/uvc_v4l2.c
> >>>>> @@ -279,6 +279,9 @@ static int uvc_v4l2_try_format(struct uvc_streaming *stream,
> >>>>>     fmt->fmt.pix.sizeimage = probe->dwMaxVideoFrameSize;
> >>>>>     fmt->fmt.pix.pixelformat = format->fcc;
> >>>>>     fmt->fmt.pix.colorspace = format->colorspace;
> >>>>> +   fmt->fmt.pix.xfer_func = format->xfer_func;
> >>>>> +   fmt->fmt.pix.ycbcr_enc = format->ycbcr_enc;
> >>>>> +   fmt->fmt.pix.quantization = format->quantization;
> >>>>>
> >>>>>     if (uvc_format != NULL)
> >>>>>             *uvc_format = format;
> >>>>> @@ -315,6 +318,9 @@ static int uvc_v4l2_get_format(struct uvc_streaming *stream,
> >>>>>     fmt->fmt.pix.bytesperline = uvc_v4l2_get_bytesperline(format, frame);
> >>>>>     fmt->fmt.pix.sizeimage = stream->ctrl.dwMaxVideoFrameSize;
> >>>>>     fmt->fmt.pix.colorspace = format->colorspace;
> >>>>> +   fmt->fmt.pix.xfer_func = format->xfer_func;
> >>>>> +   fmt->fmt.pix.ycbcr_enc = format->ycbcr_enc;
> >>>>> +   fmt->fmt.pix.quantization = format->quantization;
> >>>>>
> >>>>>  done:
> >>>>>     mutex_unlock(&stream->mutex);
> >>>>> diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
> >>>>> index 6ab972c643e3..6508192173dd 100644
> >>>>> --- a/drivers/media/usb/uvc/uvcvideo.h
> >>>>> +++ b/drivers/media/usb/uvc/uvcvideo.h
> >>>>> @@ -370,7 +370,10 @@ struct uvc_format {
> >>>>>     u8 type;
> >>>>>     u8 index;
> >>>>>     u8 bpp;
> >>>>> -   u8 colorspace;
> >>>>> +   enum v4l2_colorspace colorspace;
> >>>>> +   enum v4l2_xfer_func xfer_func;
> >>>>> +   enum v4l2_ycbcr_encoding ycbcr_enc;
> >>>>> +   enum v4l2_quantization quantization;
> >>>>>     u32 fcc;
> >>>>>     u32 flags;
> >>>>>
> >>>
> >>
>

Re: [PATCH 2/2] media: uvcvideo: Convey full ycbcr colorspace information to v4l2

[Adam Goode]

Hi,

I sent a v2 patch last night, thanks for all the comments here!


Adam

Re: [PATCH 2/2] media: uvcvideo: Convey full ycbcr colorspace information to v4l2

[Laurent Pinchart]

Hi Adam,

On Mon, Aug 24, 2020 at 01:31:54PM -0400, Adam Goode wrote:
> On Mon, Aug 24, 2020 at 10:38 AM Hans Verkuil wrote:
> > On 24/08/2020 15:56, Adam Goode wrote:
> > > On Mon, Aug 24, 2020 at 4:48 AM Hans Verkuil wrote:
> > >> On 23/08/2020 17:08, Laurent Pinchart wrote:
> > >>> On Sun, Aug 23, 2020 at 05:54:24PM +0300, Laurent Pinchart wrote:
> > >>>> On Sat, Aug 22, 2020 at 09:21:34PM -0400, Adam Goode wrote:
> > >>>>> The Color Matching Descriptor has been present in USB cameras since
> > >>>>> the original version of UVC, but it has never been fully used
> > >>>>> in Linux.
> > >>>>>
> > >>>>> This change informs V4L2 of all of the critical colorspace parameters:
> > >>>>> colorspace (called "color primaries" in UVC), transfer function
> > >>>>> (called "transfer characteristics" in UVC), ycbcr encoding (called
> > >>>>> "matrix coefficients" in UVC), and quantization, which is always
> > >>>>> LIMITED for UVC, see section 2.26 in USB_Video_FAQ_1.5.pdf.
> > >>>>
> > >>>> Isn't this valid for MJPEG only though ? There's not much else we can do
> > >>>> though, as UVC cameras don't report quantization information. Shouldn't
> > >>>> we however reflect this in the commit message, and in the comment below,
> > >>>> to state that UVC requires limited quantization range for MJPEG, and
> > >>>> while it doesn't explicitly specify the quantization range for other
> > >>>> formats, we can only assume it should be limited as well ?
> > >
> > > Yes, I am happy to improve the comment to be clearer.
> > >
> > >>>> The code otherwise looks good to me.
> > >>>>
> > >>>> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > >>>>
> > >>>> Please let me know if you'd like to address the above issue.
> > >>>>
> > >>>>> The quantization is the most important improvement for this patch,
> > >>>>> because V4L2 will otherwise interpret MJPEG as FULL range. Web browsers
> > >>>>> such as Chrome and Firefox already ignore V4L2's quantization for USB
> > >>>>> devices and use the correct LIMITED value, but other programs such
> > >>>>> as qv4l2 will incorrectly interpret the output of MJPEG from USB
> > >>>>> cameras without this change.
> > >>>>>
> > >>>>> Signed-off-by: Adam Goode <agoode@google.com>
> > >>>>> ---
> > >>>>>  drivers/media/usb/uvc/uvc_driver.c | 52 +++++++++++++++++++++++++++---
> > >>>>>  drivers/media/usb/uvc/uvc_v4l2.c   |  6 ++++
> > >>>>>  drivers/media/usb/uvc/uvcvideo.h   |  5 ++-
> > >>>>>  3 files changed, 58 insertions(+), 5 deletions(-)
> > >>>>>
> > >>>>> diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
> > >>>>> index 431d86e1c94b..c0c81b089b7d 100644
> > >>>>> --- a/drivers/media/usb/uvc/uvc_driver.c
> > >>>>> +++ b/drivers/media/usb/uvc/uvc_driver.c
> > >>>>> @@ -248,10 +248,10 @@ static struct uvc_format_desc *uvc_format_by_guid(const u8 guid[16])
> > >>>>>     return NULL;
> > >>>>>  }
> > >>>>>
> > >>>>> -static u32 uvc_colorspace(const u8 primaries)
> > >>>>> +static enum v4l2_colorspace uvc_colorspace(const u8 primaries)
> > >>>>>  {
> > >>>>> -   static const u8 colorprimaries[] = {
> > >>>>> -           0,
> > >>>>> +   static const enum v4l2_colorspace colorprimaries[] = {
> > >>>>> +           V4L2_COLORSPACE_DEFAULT,  /* Unspecified */
> > >>>>>             V4L2_COLORSPACE_SRGB,
> > >>>>>             V4L2_COLORSPACE_470_SYSTEM_M,
> > >>>>>             V4L2_COLORSPACE_470_SYSTEM_BG,
> > >>>>> @@ -262,7 +262,43 @@ static u32 uvc_colorspace(const u8 primaries)
> > >>>>>     if (primaries < ARRAY_SIZE(colorprimaries))
> > >>>>>             return colorprimaries[primaries];
> > >>>>>
> > >>>>> -   return 0;
> > >>>>> +   return V4L2_COLORSPACE_DEFAULT;  /* Reserved */
> > >>>>> +}
> > >>>>> +
> > >>>>> +static enum v4l2_xfer_func uvc_xfer_func(const u8 transfer_characteristics)
> > >>>>> +{
> > >>>>> +   static const enum v4l2_xfer_func xfer_funcs[] = {
> > >>>>> +           V4L2_XFER_FUNC_DEFAULT,  /* Unspecified */
> > >>>>> +           V4L2_XFER_FUNC_709,
> > >>>>> +           V4L2_XFER_FUNC_709,      /* BT.470-2 M */
> > >>>>> +           V4L2_XFER_FUNC_709,      /* BT.470-2 B, G */
> > >>>>> +           V4L2_XFER_FUNC_709,      /* SMPTE 170M */
> > >>>>> +           V4L2_XFER_FUNC_SMPTE240M,
> > >>>>> +           V4L2_XFER_FUNC_NONE,     /* Linear (V = Lc) */
> > >>>>> +           V4L2_XFER_FUNC_SRGB,
> > >>>>> +   };
> > >>>>> +
> > >>>>> +   if (transfer_characteristics < ARRAY_SIZE(xfer_funcs))
> > >>>>> +           return xfer_funcs[transfer_characteristics];
> > >>>>> +
> > >>>>> +   return V4L2_XFER_FUNC_DEFAULT;  /* Reserved */
> > >>>>> +}
> > >>>>> +
> > >>>>> +static enum v4l2_ycbcr_encoding uvc_ycbcr_enc(const u8 matrix_coefficients)
> > >>>>> +{
> > >>>>> +   static const enum v4l2_ycbcr_encoding ycbcr_encs[] = {
> > >>>>> +           V4L2_YCBCR_ENC_DEFAULT,  /* Unspecified */
> > >>>>> +           V4L2_YCBCR_ENC_709,
> > >>>>> +           V4L2_YCBCR_ENC_601,      /* FCC */
> > >>>
> > >>> I may have spoken a bit too fast. Doesn't FCC differ from BT.601 ?
> > >>> According to https://en.wikipedia.org/wiki/Talk%3AYCbCr, the former uses
> > >>>
> > >>>  E'Y = 0.59 E'G + 0.11 E'B + 0.30 E'R
> > >>>  E'PB = – 0.331 E'G + 0.500 E'B – 0.169 E'R
> > >>>  E'PR = – 0.421 E'G – 0.079 E'B + 0.500 E'R
> > >>>
> > >>> while the latter uses
> > >>>
> > >>>  E'Y = 0.587 E'G + 0.114 E'B + 0.299 E'R
> > >>>  E'PB = – 0.331 E'G + 0.500 E'B – 0.169 E'R
> > >>>  E'PR = – 0.419 E'G – 0.081 E'B + 0.500 E'R
> > >>>
> > >>> We seems to be missing FCC in the V4L2 color space definitions.
> > >>
> > >> The differences between the two are minuscule. Add to that that NTSC 1953
> > >> hasn't been in use for many decades. So I have no plans to add another YCC
> > >> encoding for this. I'll double check this in a few weeks time when I have
> > >> access to a better book on colorimetry.
> > >>
> > >
> > > I can add a comment directly to clarify, but I am following the
> > > mappings described in videodev2.h (with the assumption that "FCC" is
> > > close enough to 601):
> > >
> > > /*
> > > * Mapping of V4L2_XFER_FUNC_DEFAULT to actual transfer functions
> > > * for the various colorspaces:
> > > *
> > > * V4L2_COLORSPACE_SMPTE170M, V4L2_COLORSPACE_470_SYSTEM_M,
> > > * V4L2_COLORSPACE_470_SYSTEM_BG, V4L2_COLORSPACE_REC709 and
> > > * V4L2_COLORSPACE_BT2020: V4L2_XFER_FUNC_709
> > > *
> > > * V4L2_COLORSPACE_SRGB, V4L2_COLORSPACE_JPEG: V4L2_XFER_FUNC_SRGB
> > > *
> > > * V4L2_COLORSPACE_OPRGB: V4L2_XFER_FUNC_OPRGB
> > > *
> > > * V4L2_COLORSPACE_SMPTE240M: V4L2_XFER_FUNC_SMPTE240M
> > > *
> > > * V4L2_COLORSPACE_RAW: V4L2_XFER_FUNC_NONE
> > > *
> > > * V4L2_COLORSPACE_DCI_P3: V4L2_XFER_FUNC_DCI_P3
> > > */
> > >
> > > /*
> > > * Mapping of V4L2_YCBCR_ENC_DEFAULT to actual encodings for the
> > > * various colorspaces:
> > > *
> > > * V4L2_COLORSPACE_SMPTE170M, V4L2_COLORSPACE_470_SYSTEM_M,
> > > * V4L2_COLORSPACE_470_SYSTEM_BG, V4L2_COLORSPACE_SRGB,
> > > * V4L2_COLORSPACE_OPRGB and V4L2_COLORSPACE_JPEG: V4L2_YCBCR_ENC_601
> > > *
> > > * V4L2_COLORSPACE_REC709 and V4L2_COLORSPACE_DCI_P3: V4L2_YCBCR_ENC_709
> > > *
> > > * V4L2_COLORSPACE_BT2020: V4L2_YCBCR_ENC_BT2020
> > > *
> > > * V4L2_COLORSPACE_SMPTE240M: V4L2_YCBCR_ENC_SMPTE240M
> > > */
> > >
> > > We could potentially do with some more xfer functions, though.
> > >
> > >>>
> > >>>>> +           V4L2_YCBCR_ENC_601,      /* BT.470-2 B, G */
> > >>>>> +           V4L2_YCBCR_ENC_601,      /* SMPTE 170M */
> > >>>>> +           V4L2_YCBCR_ENC_SMPTE240M,
> > >>>>> +   };
> > >>>>> +
> > >>>>> +   if (matrix_coefficients < ARRAY_SIZE(ycbcr_encs))
> > >>>>> +           return ycbcr_encs[matrix_coefficients];
> > >>>>> +
> > >>>>> +   return V4L2_YCBCR_ENC_DEFAULT;  /* Reserved */
> > >>>>>  }
> > >>>>>
> > >>>>>  /* Simplify a fraction using a simple continued fraction decomposition. The
> > >>>>> @@ -704,6 +740,14 @@ static int uvc_parse_format(struct uvc_device *dev,
> > >>>>>             }
> > >>>>>
> > >>>>>             format->colorspace = uvc_colorspace(buffer[3]);
> > >>>>> +           format->xfer_func = uvc_xfer_func(buffer[4]);
> > >>>>> +           format->ycbcr_enc = uvc_ycbcr_enc(buffer[5]);
> > >>>>> +           /* All USB YCbCr encodings use LIMITED range as of UVC 1.5.
> > >>>>> +            * This is true even for MJPEG, which V4L2 otherwise assumes to
> > >>>>> +            * be FULL.
> > >>>>> +            * See section 2.26 in USB_Video_FAQ_1.5.pdf.
> > >>
> > >> Not true. I checked the FAQ: the FAQ describes what happens when a video renderer
> > >> incorrectly interprets the decoded JPEG color components as limited range instead
> > >> of full range (which they are to be JPEG compliant). JPEG always encodes YCbCr as
> > >> full range.
> > >>
> > >
> > > Here is what the FAQ says:
> > >
> > > "If the images are encoded with the luma and chroma units in the 0-255
> > > range that is used
> > > for typical JPEG still images, then the saturation and contrast will
> > > look artificially boosted
> > > when the video is rendered under the assumption that the levels were
> > > in the YCbCr color
> > > space. BT601 specifies eight-bit coding where Y is in the range of 16
> > > (black) to 235 (white)
> > > inclusive."
> > >
> > > I read this as saying "if you encode MJPEG the same as typical JPEG
> > > still images, it is wrong because Y must be in the range 16-235". Am I
> > > reading this incorrectly?
> >
> > I think so. It's the 'when the video is rendered under the assumption that
> > the levels were in the YCbCr color space.' that is the reason why the colors
> > are boosted. Normally a JPEG is either decoded to full range RGB or limited
> > range YCbCr. If it is decoded to full range YCbCr, then the renderer should
> > take that into account, otherwise the colors will be wrong ('boosted').
> >
> > The text is a bit ambiguous, but JPEG always uses full range YCbCr, that's
> > just part of the JPEG standard.
> 
> Hmm. The text is definitely confusing. Here are some facts I know:
> 
> About JPEG and JFIF:
> - JPEG itself doesn't mandate the YCbCr encoding (this is specified in
> JFIF). [https://www.w3.org/Graphics/JPEG/jfif3.pdf]
> - JFIF specifies YCbCr encoding and range as 601 but with an explicit
> change: "Y, Cb, and Cr are converted from R, G, and B as defined in
> CCIR Recommendation 601 but are normalized so as to occupy the full
> 256 levels of a 8-bit binary encoding".
> - A JPEG file isn't JFIF unless it has an APP0 with 'JFIF'.
> 
> About MJPEG:
> - MJPEG doesn't specify explicit YCbCr encoding information (there
> isn't really a specification?).
> - The USB devices I have that output MJPEG do not output JFIF (no APP0
> with 'JFIF').
> 
> About color in UVC:
> - MJPEG in UVC is required to be 8-bit YCbCr encoded
> [USB_Video_Payload_MJPEG_1.5.pdf, section 3.3] with the color encoding
> information specified via the Color Matching descriptor
> [USB_Video_Payload_MJPEG_1.5.pdf, section 3].
> - The UVC Color Matching descriptor cites BT.601 and other standards
> without mentioning any changes to them [UVC Class Specification
> 1.5.pdf, 3.9.2.6].
> - BT.601 and BT.709 require limited-range YCbCr encoding.
> 
> Real-world observations:
> - The USB webcams I have (Logitech) output limited-range UVC MJPEG.

How have you determined that ? I'd like to run tests locally, and before
writing an application that decompresses JPEG without any colorspace
conversion, I'd like to know if one already exists.

> - The ATEM Mini outputs full-range UVC MJPEG, but this is considered
> to be incorrect behavior
> (https://forum.blackmagicdesign.com/viewtopic.php?f=4&t=108315,
> https://www.youtube.com/watch?v=BEXQ5s2HpwE).
> - Chrome, Firefox, and Safari interpret UVC MJPEG as limited-range.
> 
> 
> I would agree that if the MJPEG coming out of UVC has JFIF headers, we
> would have a problem, because we would have conflicting YCbCr encoding
> metadata.
> 
> But since:
> 1. UVC is pretty clear about how to encode color,
> 2. JPEG-without-JFIF doesn't define YCbCr encoding,
> 3. MJPEG devices output non-JFIF JPEG,
> 4. UVC only specifies limited-range encoding for YCbCr,
> 
> I would conclude that UVC MJPEG should be expected to be limited-range.
>
> > >>>>> +            */
> > >>>>> +           format->quantization = V4L2_QUANTIZATION_LIM_RANGE;
> > >>
> > >> What about sRGB? That uses full range.
> > >>
> > >
> > > It is a little confusing in the code, but I only set the quantization
> > > explicitly when we get a Color Matching descriptor from the device. My
> > > reading of the spec says that this descriptor isn't present for RGB
> > > formats, only YCrCb. When the spec mentions sRGB in Color Matching, it
> > > is referring only to primaries or gamma.
> > >
> > >>>>>
> > >>>>>             buflen -= buffer[0];
> > >>>>>             buffer += buffer[0];
> > >>>>> diff --git a/drivers/media/usb/uvc/uvc_v4l2.c b/drivers/media/usb/uvc/uvc_v4l2.c
> > >>>>> index 7f14096cb44d..79daf46b3dcd 100644
> > >>>>> --- a/drivers/media/usb/uvc/uvc_v4l2.c
> > >>>>> +++ b/drivers/media/usb/uvc/uvc_v4l2.c
> > >>>>> @@ -279,6 +279,9 @@ static int uvc_v4l2_try_format(struct uvc_streaming *stream,
> > >>>>>     fmt->fmt.pix.sizeimage = probe->dwMaxVideoFrameSize;
> > >>>>>     fmt->fmt.pix.pixelformat = format->fcc;
> > >>>>>     fmt->fmt.pix.colorspace = format->colorspace;
> > >>>>> +   fmt->fmt.pix.xfer_func = format->xfer_func;
> > >>>>> +   fmt->fmt.pix.ycbcr_enc = format->ycbcr_enc;
> > >>>>> +   fmt->fmt.pix.quantization = format->quantization;
> > >>>>>
> > >>>>>     if (uvc_format != NULL)
> > >>>>>             *uvc_format = format;
> > >>>>> @@ -315,6 +318,9 @@ static int uvc_v4l2_get_format(struct uvc_streaming *stream,
> > >>>>>     fmt->fmt.pix.bytesperline = uvc_v4l2_get_bytesperline(format, frame);
> > >>>>>     fmt->fmt.pix.sizeimage = stream->ctrl.dwMaxVideoFrameSize;
> > >>>>>     fmt->fmt.pix.colorspace = format->colorspace;
> > >>>>> +   fmt->fmt.pix.xfer_func = format->xfer_func;
> > >>>>> +   fmt->fmt.pix.ycbcr_enc = format->ycbcr_enc;
> > >>>>> +   fmt->fmt.pix.quantization = format->quantization;
> > >>>>>
> > >>>>>  done:
> > >>>>>     mutex_unlock(&stream->mutex);
> > >>>>> diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
> > >>>>> index 6ab972c643e3..6508192173dd 100644
> > >>>>> --- a/drivers/media/usb/uvc/uvcvideo.h
> > >>>>> +++ b/drivers/media/usb/uvc/uvcvideo.h
> > >>>>> @@ -370,7 +370,10 @@ struct uvc_format {
> > >>>>>     u8 type;
> > >>>>>     u8 index;
> > >>>>>     u8 bpp;
> > >>>>> -   u8 colorspace;
> > >>>>> +   enum v4l2_colorspace colorspace;
> > >>>>> +   enum v4l2_xfer_func xfer_func;
> > >>>>> +   enum v4l2_ycbcr_encoding ycbcr_enc;
> > >>>>> +   enum v4l2_quantization quantization;
> > >>>>>     u32 fcc;
> > >>>>>     u32 flags;
> > >>>>>

-- 
Regards,

Laurent Pinchart

Re: [PATCH 2/2] media: uvcvideo: Convey full ycbcr colorspace information to v4l2

[Adam Goode]

On Mon, Aug 31, 2020 at 9:17 PM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi Adam,
>
> On Mon, Aug 24, 2020 at 01:31:54PM -0400, Adam Goode wrote:
> > On Mon, Aug 24, 2020 at 10:38 AM Hans Verkuil wrote:
> > > On 24/08/2020 15:56, Adam Goode wrote:
> > > > On Mon, Aug 24, 2020 at 4:48 AM Hans Verkuil wrote:
> > > >> On 23/08/2020 17:08, Laurent Pinchart wrote:
> > > >>> On Sun, Aug 23, 2020 at 05:54:24PM +0300, Laurent Pinchart wrote:
> > > >>>> On Sat, Aug 22, 2020 at 09:21:34PM -0400, Adam Goode wrote:
> > > >>>>> The Color Matching Descriptor has been present in USB cameras since
> > > >>>>> the original version of UVC, but it has never been fully used
> > > >>>>> in Linux.
> > > >>>>>
> > > >>>>> This change informs V4L2 of all of the critical colorspace parameters:
> > > >>>>> colorspace (called "color primaries" in UVC), transfer function
> > > >>>>> (called "transfer characteristics" in UVC), ycbcr encoding (called
> > > >>>>> "matrix coefficients" in UVC), and quantization, which is always
> > > >>>>> LIMITED for UVC, see section 2.26 in USB_Video_FAQ_1.5.pdf.
> > > >>>>
> > > >>>> Isn't this valid for MJPEG only though ? There's not much else we can do
> > > >>>> though, as UVC cameras don't report quantization information. Shouldn't
> > > >>>> we however reflect this in the commit message, and in the comment below,
> > > >>>> to state that UVC requires limited quantization range for MJPEG, and
> > > >>>> while it doesn't explicitly specify the quantization range for other
> > > >>>> formats, we can only assume it should be limited as well ?
> > > >
> > > > Yes, I am happy to improve the comment to be clearer.
> > > >
> > > >>>> The code otherwise looks good to me.
> > > >>>>
> > > >>>> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > >>>>
> > > >>>> Please let me know if you'd like to address the above issue.
> > > >>>>
> > > >>>>> The quantization is the most important improvement for this patch,
> > > >>>>> because V4L2 will otherwise interpret MJPEG as FULL range. Web browsers
> > > >>>>> such as Chrome and Firefox already ignore V4L2's quantization for USB
> > > >>>>> devices and use the correct LIMITED value, but other programs such
> > > >>>>> as qv4l2 will incorrectly interpret the output of MJPEG from USB
> > > >>>>> cameras without this change.
> > > >>>>>
> > > >>>>> Signed-off-by: Adam Goode <agoode@google.com>
> > > >>>>> ---
> > > >>>>>  drivers/media/usb/uvc/uvc_driver.c | 52 +++++++++++++++++++++++++++---
> > > >>>>>  drivers/media/usb/uvc/uvc_v4l2.c   |  6 ++++
> > > >>>>>  drivers/media/usb/uvc/uvcvideo.h   |  5 ++-
> > > >>>>>  3 files changed, 58 insertions(+), 5 deletions(-)
> > > >>>>>
> > > >>>>> diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
> > > >>>>> index 431d86e1c94b..c0c81b089b7d 100644
> > > >>>>> --- a/drivers/media/usb/uvc/uvc_driver.c
> > > >>>>> +++ b/drivers/media/usb/uvc/uvc_driver.c
> > > >>>>> @@ -248,10 +248,10 @@ static struct uvc_format_desc *uvc_format_by_guid(const u8 guid[16])
> > > >>>>>     return NULL;
> > > >>>>>  }
> > > >>>>>
> > > >>>>> -static u32 uvc_colorspace(const u8 primaries)
> > > >>>>> +static enum v4l2_colorspace uvc_colorspace(const u8 primaries)
> > > >>>>>  {
> > > >>>>> -   static const u8 colorprimaries[] = {
> > > >>>>> -           0,
> > > >>>>> +   static const enum v4l2_colorspace colorprimaries[] = {
> > > >>>>> +           V4L2_COLORSPACE_DEFAULT,  /* Unspecified */
> > > >>>>>             V4L2_COLORSPACE_SRGB,
> > > >>>>>             V4L2_COLORSPACE_470_SYSTEM_M,
> > > >>>>>             V4L2_COLORSPACE_470_SYSTEM_BG,
> > > >>>>> @@ -262,7 +262,43 @@ static u32 uvc_colorspace(const u8 primaries)
> > > >>>>>     if (primaries < ARRAY_SIZE(colorprimaries))
> > > >>>>>             return colorprimaries[primaries];
> > > >>>>>
> > > >>>>> -   return 0;
> > > >>>>> +   return V4L2_COLORSPACE_DEFAULT;  /* Reserved */
> > > >>>>> +}
> > > >>>>> +
> > > >>>>> +static enum v4l2_xfer_func uvc_xfer_func(const u8 transfer_characteristics)
> > > >>>>> +{
> > > >>>>> +   static const enum v4l2_xfer_func xfer_funcs[] = {
> > > >>>>> +           V4L2_XFER_FUNC_DEFAULT,  /* Unspecified */
> > > >>>>> +           V4L2_XFER_FUNC_709,
> > > >>>>> +           V4L2_XFER_FUNC_709,      /* BT.470-2 M */
> > > >>>>> +           V4L2_XFER_FUNC_709,      /* BT.470-2 B, G */
> > > >>>>> +           V4L2_XFER_FUNC_709,      /* SMPTE 170M */
> > > >>>>> +           V4L2_XFER_FUNC_SMPTE240M,
> > > >>>>> +           V4L2_XFER_FUNC_NONE,     /* Linear (V = Lc) */
> > > >>>>> +           V4L2_XFER_FUNC_SRGB,
> > > >>>>> +   };
> > > >>>>> +
> > > >>>>> +   if (transfer_characteristics < ARRAY_SIZE(xfer_funcs))
> > > >>>>> +           return xfer_funcs[transfer_characteristics];
> > > >>>>> +
> > > >>>>> +   return V4L2_XFER_FUNC_DEFAULT;  /* Reserved */
> > > >>>>> +}
> > > >>>>> +
> > > >>>>> +static enum v4l2_ycbcr_encoding uvc_ycbcr_enc(const u8 matrix_coefficients)
> > > >>>>> +{
> > > >>>>> +   static const enum v4l2_ycbcr_encoding ycbcr_encs[] = {
> > > >>>>> +           V4L2_YCBCR_ENC_DEFAULT,  /* Unspecified */
> > > >>>>> +           V4L2_YCBCR_ENC_709,
> > > >>>>> +           V4L2_YCBCR_ENC_601,      /* FCC */
> > > >>>
> > > >>> I may have spoken a bit too fast. Doesn't FCC differ from BT.601 ?
> > > >>> According to https://en.wikipedia.org/wiki/Talk%3AYCbCr, the former uses
> > > >>>
> > > >>>  E'Y = 0.59 E'G + 0.11 E'B + 0.30 E'R
> > > >>>  E'PB = – 0.331 E'G + 0.500 E'B – 0.169 E'R
> > > >>>  E'PR = – 0.421 E'G – 0.079 E'B + 0.500 E'R
> > > >>>
> > > >>> while the latter uses
> > > >>>
> > > >>>  E'Y = 0.587 E'G + 0.114 E'B + 0.299 E'R
> > > >>>  E'PB = – 0.331 E'G + 0.500 E'B – 0.169 E'R
> > > >>>  E'PR = – 0.419 E'G – 0.081 E'B + 0.500 E'R
> > > >>>
> > > >>> We seems to be missing FCC in the V4L2 color space definitions.
> > > >>
> > > >> The differences between the two are minuscule. Add to that that NTSC 1953
> > > >> hasn't been in use for many decades. So I have no plans to add another YCC
> > > >> encoding for this. I'll double check this in a few weeks time when I have
> > > >> access to a better book on colorimetry.
> > > >>
> > > >
> > > > I can add a comment directly to clarify, but I am following the
> > > > mappings described in videodev2.h (with the assumption that "FCC" is
> > > > close enough to 601):
> > > >
> > > > /*
> > > > * Mapping of V4L2_XFER_FUNC_DEFAULT to actual transfer functions
> > > > * for the various colorspaces:
> > > > *
> > > > * V4L2_COLORSPACE_SMPTE170M, V4L2_COLORSPACE_470_SYSTEM_M,
> > > > * V4L2_COLORSPACE_470_SYSTEM_BG, V4L2_COLORSPACE_REC709 and
> > > > * V4L2_COLORSPACE_BT2020: V4L2_XFER_FUNC_709
> > > > *
> > > > * V4L2_COLORSPACE_SRGB, V4L2_COLORSPACE_JPEG: V4L2_XFER_FUNC_SRGB
> > > > *
> > > > * V4L2_COLORSPACE_OPRGB: V4L2_XFER_FUNC_OPRGB
> > > > *
> > > > * V4L2_COLORSPACE_SMPTE240M: V4L2_XFER_FUNC_SMPTE240M
> > > > *
> > > > * V4L2_COLORSPACE_RAW: V4L2_XFER_FUNC_NONE
> > > > *
> > > > * V4L2_COLORSPACE_DCI_P3: V4L2_XFER_FUNC_DCI_P3
> > > > */
> > > >
> > > > /*
> > > > * Mapping of V4L2_YCBCR_ENC_DEFAULT to actual encodings for the
> > > > * various colorspaces:
> > > > *
> > > > * V4L2_COLORSPACE_SMPTE170M, V4L2_COLORSPACE_470_SYSTEM_M,
> > > > * V4L2_COLORSPACE_470_SYSTEM_BG, V4L2_COLORSPACE_SRGB,
> > > > * V4L2_COLORSPACE_OPRGB and V4L2_COLORSPACE_JPEG: V4L2_YCBCR_ENC_601
> > > > *
> > > > * V4L2_COLORSPACE_REC709 and V4L2_COLORSPACE_DCI_P3: V4L2_YCBCR_ENC_709
> > > > *
> > > > * V4L2_COLORSPACE_BT2020: V4L2_YCBCR_ENC_BT2020
> > > > *
> > > > * V4L2_COLORSPACE_SMPTE240M: V4L2_YCBCR_ENC_SMPTE240M
> > > > */
> > > >
> > > > We could potentially do with some more xfer functions, though.
> > > >
> > > >>>
> > > >>>>> +           V4L2_YCBCR_ENC_601,      /* BT.470-2 B, G */
> > > >>>>> +           V4L2_YCBCR_ENC_601,      /* SMPTE 170M */
> > > >>>>> +           V4L2_YCBCR_ENC_SMPTE240M,
> > > >>>>> +   };
> > > >>>>> +
> > > >>>>> +   if (matrix_coefficients < ARRAY_SIZE(ycbcr_encs))
> > > >>>>> +           return ycbcr_encs[matrix_coefficients];
> > > >>>>> +
> > > >>>>> +   return V4L2_YCBCR_ENC_DEFAULT;  /* Reserved */
> > > >>>>>  }
> > > >>>>>
> > > >>>>>  /* Simplify a fraction using a simple continued fraction decomposition. The
> > > >>>>> @@ -704,6 +740,14 @@ static int uvc_parse_format(struct uvc_device *dev,
> > > >>>>>             }
> > > >>>>>
> > > >>>>>             format->colorspace = uvc_colorspace(buffer[3]);
> > > >>>>> +           format->xfer_func = uvc_xfer_func(buffer[4]);
> > > >>>>> +           format->ycbcr_enc = uvc_ycbcr_enc(buffer[5]);
> > > >>>>> +           /* All USB YCbCr encodings use LIMITED range as of UVC 1.5.
> > > >>>>> +            * This is true even for MJPEG, which V4L2 otherwise assumes to
> > > >>>>> +            * be FULL.
> > > >>>>> +            * See section 2.26 in USB_Video_FAQ_1.5.pdf.
> > > >>
> > > >> Not true. I checked the FAQ: the FAQ describes what happens when a video renderer
> > > >> incorrectly interprets the decoded JPEG color components as limited range instead
> > > >> of full range (which they are to be JPEG compliant). JPEG always encodes YCbCr as
> > > >> full range.
> > > >>
> > > >
> > > > Here is what the FAQ says:
> > > >
> > > > "If the images are encoded with the luma and chroma units in the 0-255
> > > > range that is used
> > > > for typical JPEG still images, then the saturation and contrast will
> > > > look artificially boosted
> > > > when the video is rendered under the assumption that the levels were
> > > > in the YCbCr color
> > > > space. BT601 specifies eight-bit coding where Y is in the range of 16
> > > > (black) to 235 (white)
> > > > inclusive."
> > > >
> > > > I read this as saying "if you encode MJPEG the same as typical JPEG
> > > > still images, it is wrong because Y must be in the range 16-235". Am I
> > > > reading this incorrectly?
> > >
> > > I think so. It's the 'when the video is rendered under the assumption that
> > > the levels were in the YCbCr color space.' that is the reason why the colors
> > > are boosted. Normally a JPEG is either decoded to full range RGB or limited
> > > range YCbCr. If it is decoded to full range YCbCr, then the renderer should
> > > take that into account, otherwise the colors will be wrong ('boosted').
> > >
> > > The text is a bit ambiguous, but JPEG always uses full range YCbCr, that's
> > > just part of the JPEG standard.
> >
> > Hmm. The text is definitely confusing. Here are some facts I know:
> >
> > About JPEG and JFIF:
> > - JPEG itself doesn't mandate the YCbCr encoding (this is specified in
> > JFIF). [https://www.w3.org/Graphics/JPEG/jfif3.pdf]
> > - JFIF specifies YCbCr encoding and range as 601 but with an explicit
> > change: "Y, Cb, and Cr are converted from R, G, and B as defined in
> > CCIR Recommendation 601 but are normalized so as to occupy the full
> > 256 levels of a 8-bit binary encoding".
> > - A JPEG file isn't JFIF unless it has an APP0 with 'JFIF'.
> >
> > About MJPEG:
> > - MJPEG doesn't specify explicit YCbCr encoding information (there
> > isn't really a specification?).
> > - The USB devices I have that output MJPEG do not output JFIF (no APP0
> > with 'JFIF').
> >
> > About color in UVC:
> > - MJPEG in UVC is required to be 8-bit YCbCr encoded
> > [USB_Video_Payload_MJPEG_1.5.pdf, section 3.3] with the color encoding
> > information specified via the Color Matching descriptor
> > [USB_Video_Payload_MJPEG_1.5.pdf, section 3].
> > - The UVC Color Matching descriptor cites BT.601 and other standards
> > without mentioning any changes to them [UVC Class Specification
> > 1.5.pdf, 3.9.2.6].
> > - BT.601 and BT.709 require limited-range YCbCr encoding.
> >
> > Real-world observations:
> > - The USB webcams I have (Logitech) output limited-range UVC MJPEG.
>
> How have you determined that ? I'd like to run tests locally, and before
> writing an application that decompresses JPEG without any colorspace
> conversion, I'd like to know if one already exists.
>

I spoke too soon! I did some additional tests and found that I am
getting full-range JPEG out of the Logitech C922 camera. (I think the
camera was accidentally in uncompressed mode for one test.) Thank you
so much for keeping me honest here, and sorry for confusion.

My methodology is this:

1. Use qv4l2 and make sure the camera is in MJPEG mode. Make sure
OpenGL decoding is turned on, otherwise the MJPEG image is always
full-range on the screen in qv4l2.
2. Cover the camera with my finger.
3. Use a screen capture device (gpick is ok) to measure the black values.
4. Flip between override of full range and limited range.

The RGB values should be between 1-15 (not 0) for black when the range
is correct.

You can switch to the uncompressed format to verify that the black
values are in the same range. Uncompressed YUV should be decoded as
limited range to get the same black value as full-range decoded MJPEG.

If you have a light to shine into the camera, you can do the same for
the white value. Again, the RGB values should be >235 but not 255.

I've also used v4l2-ctl to capture a direct JPEG frame (useful for
reading JPEG headers):

v4l2-ctl --stream-mmap --stream-count=1 --stream-to=file.jpg

> > - The ATEM Mini outputs full-range UVC MJPEG, but this is considered
> > to be incorrect behavior
> > (https://forum.blackmagicdesign.com/viewtopic.php?f=4&t=108315,
> > https://www.youtube.com/watch?v=BEXQ5s2HpwE).

I think that the ATEM Mini is one of the first high-quality devices to
use MJPEG, so even though it is outputting the same full-range image
as other devices, people are noticing it for the first time.


> > - Chrome, Firefox, and Safari interpret UVC MJPEG as limited-range.
> >

I guess even though the UVC spec seems to say otherwise, MJPEG UVC is
(at least for some devices) full range. I will at least try to fix
Chrome. Zoom also is decoding this wrong. I haven't looked at the
behavior on Windows yet, just webbrowsers on Mac and Linux (and Zoom
on Linux).

Probably we'll find some MJPEG devices that are limited range, in
which case, we are stuck in the same mess as now.

> >
> > I would agree that if the MJPEG coming out of UVC has JFIF headers, we
> > would have a problem, because we would have conflicting YCbCr encoding
> > metadata.
> >
> > But since:
> > 1. UVC is pretty clear about how to encode color,
> > 2. JPEG-without-JFIF doesn't define YCbCr encoding,
> > 3. MJPEG devices output non-JFIF JPEG,
> > 4. UVC only specifies limited-range encoding for YCbCr,
> >
> > I would conclude that UVC MJPEG should be expected to be limited-range.
> >
> > > >>>>> +            */
> > > >>>>> +           format->quantization = V4L2_QUANTIZATION_LIM_RANGE;
> > > >>
> > > >> What about sRGB? That uses full range.
> > > >>
> > > >
> > > > It is a little confusing in the code, but I only set the quantization
> > > > explicitly when we get a Color Matching descriptor from the device. My
> > > > reading of the spec says that this descriptor isn't present for RGB
> > > > formats, only YCrCb. When the spec mentions sRGB in Color Matching, it
> > > > is referring only to primaries or gamma.
> > > >
> > > >>>>>
> > > >>>>>             buflen -= buffer[0];
> > > >>>>>             buffer += buffer[0];
> > > >>>>> diff --git a/drivers/media/usb/uvc/uvc_v4l2.c b/drivers/media/usb/uvc/uvc_v4l2.c
> > > >>>>> index 7f14096cb44d..79daf46b3dcd 100644
> > > >>>>> --- a/drivers/media/usb/uvc/uvc_v4l2.c
> > > >>>>> +++ b/drivers/media/usb/uvc/uvc_v4l2.c
> > > >>>>> @@ -279,6 +279,9 @@ static int uvc_v4l2_try_format(struct uvc_streaming *stream,
> > > >>>>>     fmt->fmt.pix.sizeimage = probe->dwMaxVideoFrameSize;
> > > >>>>>     fmt->fmt.pix.pixelformat = format->fcc;
> > > >>>>>     fmt->fmt.pix.colorspace = format->colorspace;
> > > >>>>> +   fmt->fmt.pix.xfer_func = format->xfer_func;
> > > >>>>> +   fmt->fmt.pix.ycbcr_enc = format->ycbcr_enc;
> > > >>>>> +   fmt->fmt.pix.quantization = format->quantization;
> > > >>>>>
> > > >>>>>     if (uvc_format != NULL)
> > > >>>>>             *uvc_format = format;
> > > >>>>> @@ -315,6 +318,9 @@ static int uvc_v4l2_get_format(struct uvc_streaming *stream,
> > > >>>>>     fmt->fmt.pix.bytesperline = uvc_v4l2_get_bytesperline(format, frame);
> > > >>>>>     fmt->fmt.pix.sizeimage = stream->ctrl.dwMaxVideoFrameSize;
> > > >>>>>     fmt->fmt.pix.colorspace = format->colorspace;
> > > >>>>> +   fmt->fmt.pix.xfer_func = format->xfer_func;
> > > >>>>> +   fmt->fmt.pix.ycbcr_enc = format->ycbcr_enc;
> > > >>>>> +   fmt->fmt.pix.quantization = format->quantization;
> > > >>>>>
> > > >>>>>  done:
> > > >>>>>     mutex_unlock(&stream->mutex);
> > > >>>>> diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
> > > >>>>> index 6ab972c643e3..6508192173dd 100644
> > > >>>>> --- a/drivers/media/usb/uvc/uvcvideo.h
> > > >>>>> +++ b/drivers/media/usb/uvc/uvcvideo.h
> > > >>>>> @@ -370,7 +370,10 @@ struct uvc_format {
> > > >>>>>     u8 type;
> > > >>>>>     u8 index;
> > > >>>>>     u8 bpp;
> > > >>>>> -   u8 colorspace;
> > > >>>>> +   enum v4l2_colorspace colorspace;
> > > >>>>> +   enum v4l2_xfer_func xfer_func;
> > > >>>>> +   enum v4l2_ycbcr_encoding ycbcr_enc;
> > > >>>>> +   enum v4l2_quantization quantization;
> > > >>>>>     u32 fcc;
> > > >>>>>     u32 flags;
> > > >>>>>
>
> --
> Regards,
>
> Laurent Pinchart

Thanks again for working through this with me. I will revise the patch
to remove the quantization change. I think the color space information
is still useful to expose (though maybe not, given how confused things
are).


Adam

Re: [PATCH 1/2] media: uvcvideo: Ensure all probed info is returned to v4l2

[Laurent Pinchart]

Hi Adam,

Thank you for the patch.

On Sat, Aug 22, 2020 at 09:21:33PM -0400, Adam Goode wrote:
> bFrameIndex and bFormatIndex can be negotiated by the camera during
> probing, resulting in the camera choosing a different format than
> expected. v4l2 can already accommodate such changes, but the code was
> not updating the proper fields.
> 
> Without such a change, v4l2 would potentially interpret the payload
> incorrectly, causing corrupted output. This was happening on the
> Elgato HD60 S+, which currently always renegotiates to format 1.
> 
> As an aside, the Elgato firmware is buggy and should not be renegotating,
> but it is still a valid thing for the camera to do. Both macOS and Windows
> will properly probe and read uncorrupted images from this camera.
> 
> With this change, both qv4l2 and chromium can now read uncorrupted video
> from the Elgato HD60 S+.

Good catch. I've seen my share of buggy cameras, just not this
particular bug I suppose :-)

> Signed-off-by: Adam Goode <agoode@google.com>
> ---
>  drivers/media/usb/uvc/uvc_v4l2.c | 26 ++++++++++++++++++++++++++
>  1 file changed, 26 insertions(+)
> 
> diff --git a/drivers/media/usb/uvc/uvc_v4l2.c b/drivers/media/usb/uvc/uvc_v4l2.c
> index 0335e69b70ab..7f14096cb44d 100644
> --- a/drivers/media/usb/uvc/uvc_v4l2.c
> +++ b/drivers/media/usb/uvc/uvc_v4l2.c
> @@ -247,11 +247,37 @@ static int uvc_v4l2_try_format(struct uvc_streaming *stream,
>  	if (ret < 0)
>  		goto done;
>  
> +	/* After the probe, update fmt with the values returned from
> +	 * negotiation with the device.
> +	 */
> +	for (i = 0; i < stream->nformats; ++i) {
> +		if (probe->bFormatIndex == stream->format[i].index) {
> +			format = &stream->format[i];
> +			break;
> +		}
> +	}
> +	if (i == stream->nformats) {
> +		uvc_trace(UVC_TRACE_FORMAT, "Unknown bFormatIndex %u.\n",
> +			  probe->bFormatIndex);
> +		return -EINVAL;
> +	}
> +	for (i = 0; i < format->nframes; ++i) {
> +		if (probe->bFrameIndex == format->frame[i].bFrameIndex) {
> +			frame = &format->frame[i];
> +			break;
> +		}
> +	}
> +	if (i == format->nframes) {
> +		uvc_trace(UVC_TRACE_FORMAT, "Unknown bFrameIndex %u.\n",
> +			  probe->bFrameIndex);
> +		return -EINVAL;
> +	}

This looks good to me. Blank lines between the different blocks would be
good to let the code breathe a little bit :-) Apart from that,

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

There's no need to resubmit the patch for such a trivial change, unless
you object, I'll add the blank lines locally.

I may submit an additional patch on top of this to share the above code
with the identical implementation in uvc_fixup_video_ctrl().

>  	fmt->fmt.pix.width = frame->wWidth;
>  	fmt->fmt.pix.height = frame->wHeight;
>  	fmt->fmt.pix.field = V4L2_FIELD_NONE;
>  	fmt->fmt.pix.bytesperline = uvc_v4l2_get_bytesperline(format, frame);
>  	fmt->fmt.pix.sizeimage = probe->dwMaxVideoFrameSize;
> +	fmt->fmt.pix.pixelformat = format->fcc;
>  	fmt->fmt.pix.colorspace = format->colorspace;
>  
>  	if (uvc_format != NULL)

-- 
Regards,

Laurent Pinchart