Re: [PATCH v4 02/13] media: v4l: add some definition of v4l2 colorspace/xfer_func/ycbcr_encoding

From: Hans Verkuil <hverkuil-cisco@xs4all.nl>
To: Ming Qian <ming.qian@nxp.com>,
	mchehab@kernel.org, shawnguo@kernel.org, robh+dt@kernel.org,
	s.hauer@pengutronix.de
Cc: kernel@pengutronix.de, festevam@gmail.com, linux-imx@nxp.com,
	aisheng.dong@nxp.com, linux-media@vger.kernel.org,
	linux-kernel@vger.kernel.org, devicetree@vger.kernel.org,
	linux-arm-kernel@lists.infradead.org
Subject: Re: [PATCH v4 02/13] media: v4l: add some definition of v4l2 colorspace/xfer_func/ycbcr_encoding
Date: Wed, 21 Jul 2021 12:09:28 +0200	[thread overview]
Message-ID: <449dc223-d248-8771-2f5a-46c6bbac401d@xs4all.nl> (raw)
In-Reply-To: <d63b34381eec0ae47bf39dd2b88d2bc8994c269d.1626743758.git.ming.qian@nxp.com>

On 20/07/2021 03:43, Ming Qian wrote:
> Some definition of colorspace/xfer_func/ycbcr_encoding
> are defined in ISO, but missed in V4L2,
> so add some definition according VPU driver's requirement
> 
> Signed-off-by: Ming Qian <ming.qian@nxp.com>
> Signed-off-by: Shijie Qin <shijie.qin@nxp.com>
> Signed-off-by: Zhou Peng <eagle.zhou@nxp.com>
> ---
>  include/uapi/linux/videodev2.h | 30 ++++++++++++++++++++++++++++++
>  1 file changed, 30 insertions(+)
> 
> diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
> index 534eaa4d39bc..545f2c329bc9 100644
> --- a/include/uapi/linux/videodev2.h
> +++ b/include/uapi/linux/videodev2.h
> @@ -247,6 +247,12 @@ enum v4l2_colorspace {
>  
>  	/* DCI-P3 colorspace, used by cinema projectors */
>  	V4L2_COLORSPACE_DCI_P3        = 12,
> +
> +	/* Generic film (colour filters using Illuminant C) */
> +	V4L2_COLORSPACE_GENERIC_FILM  = 13,
> +
> +	/* SMPTE ST 428-1 */
> +	V4L2_COLORSPACE_ST428         = 14,
>  };
>  
>  /*
> @@ -276,6 +282,20 @@ enum v4l2_xfer_func {
>  	 * V4L2_COLORSPACE_RAW: V4L2_XFER_FUNC_NONE
>  	 *
>  	 * V4L2_COLORSPACE_DCI_P3: V4L2_XFER_FUNC_DCI_P3
> +	 *
> +	 * V4L2_XFER_FUNC_LINEAR: Linear transfer characteristics

This exists already: V4L2_XFER_FUNC_NONE

> +	 *
> +	 * V4L2_XFER_FUNC_GAMMA22: Assumed display gamma 2.2
> +	 *
> +	 * V4L2_XFER_FUNC_GAMMA28: Assumed display gamma 2.8
> +	 *
> +	 * V4L2_XFER_FUNC_HLG: STD-B67, Rec. ITU-R BT.2100-2 hybrid-log-gamma
> +	 *
> +	 * V4L2_XFER_FUNC_XVYCC: IEC 61966-2-4

This exists already, it is signaled through V4L2_YCBCR_ENC_XV709 and
V4L2_YCBCR_ENC_XV601. It's not actually a different transfer function,
it's the YCbCr encoding that's different (the transfer function is still
V4L2_XFER_FUNC_709).

> +	 *
> +	 * V4L2_XFER_FUNC_BT1361: Rec. ITU-R BT.1361-0 extended colour gamut
> +	 *
> +	 * V4L2_XFER_FUNC_ST428: SMPTE ST 428-1
>  	 */
>  	V4L2_XFER_FUNC_DEFAULT     = 0,
>  	V4L2_XFER_FUNC_709         = 1,
> @@ -285,6 +305,13 @@ enum v4l2_xfer_func {
>  	V4L2_XFER_FUNC_NONE        = 5,
>  	V4L2_XFER_FUNC_DCI_P3      = 6,
>  	V4L2_XFER_FUNC_SMPTE2084   = 7,
> +	V4L2_XFER_FUNC_LINEAR      = 8,
> +	V4L2_XFER_FUNC_GAMMA22     = 9,
> +	V4L2_XFER_FUNC_GAMMA28     = 10,
> +	V4L2_XFER_FUNC_HLG         = 11,
> +	V4L2_XFER_FUNC_XVYCC       = 12,
> +	V4L2_XFER_FUNC_BT1361      = 13,

This appears to be a variant of xvYCC, it should probably be a YCBCR_ENC variant
since the transfer function defined in bt.1361 is REC709.

> +	V4L2_XFER_FUNC_ST428       = 14,

Not sure what this one is about.

>  };
>  
>  /*
> @@ -345,6 +372,9 @@ enum v4l2_ycbcr_encoding {
>  
>  	/* SMPTE 240M -- Obsolete HDTV */
>  	V4L2_YCBCR_ENC_SMPTE240M      = 8,
> +
> +	/* KR=0.30, KB=0.11 or equivalent */
> +	V4L2_YCBCR_ENC_BT470_6M       = 9,
>  };
>  
>  /*
> 

I'm not opposed to this, but it has to be documented in
Documentation/userspace-api/media/v4l/colorspaces-details.rst.

I would recommend for an initial submission to only add those new colorimetries
that are actually needed, and others can be added later. uAPI additions take a
lot of time, esp. getting the documentation correct.

Regards,

	Hans