All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v2] [RFC v2] v4l2: add support for colorspace conversion for video capture
@ 2018-09-05 17:09 Philipp Zabel
  2018-09-06  9:02 ` Hans Verkuil
  0 siblings, 1 reply; 9+ messages in thread
From: Philipp Zabel @ 2018-09-05 17:09 UTC (permalink / raw)
  To: linux-media; +Cc: kernel, Hans Verkuil

For video capture it is the driver that reports the colorspace,
Y'CbCr/HSV encoding and quantization range used by the video, and there
is no way to request something different, even though many HDTV
receivers have some sort of colorspace conversion capabilities.

For output video this feature already exists since the application
specifies this information for the video format it will send out, and
the transmitter will enable any available CSC if a format conversion has
to be performed in order to match the capabilities of the sink.

For video capture we propose adding new pix_format flags:
V4L2_PIX_FMT_FLAG_CSC_COLORSPACE, V4L2_PIX_FMT_FLAG_CSC_YCBCR_ENC,
V4L2_PIX_FMT_FLAG_CSC_HSV_ENC, V4L2_PIX_FMT_FLAG_CSC_QUANTIZATION, and
V4L2_PIX_FMT_FLAG_CSC_XFER_FUNC. These are set by the driver to indicate
its conversion features. When set by the application, the driver will
interpret the colorspace, ycbcr_enc/hsv_enc, quantization and xfer_func
fields as the requested colorspace information and will attempt to do
the conversion it supports.

Drivers do not have to actually look at the flags: if the flags are not
set, then the colorspace, ycbcr_enc and quantization fields are set to
the default values by the core, i.e. just pass on the received format
without conversion.

Signed-off-by: Hans Verkuil <Hans Verkuil@cisco.com>
Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
---
Changes since v1 [1]
 - convert to rst
 - split V4L2_PIX_FMT_FLAG_REQUEST_CSC into four separate flags for
   colorspace, ycbcr_enc/hsv_enc, quantization, and xfer_func
 - let driver set flags to indicate supported features

[1] https://patchwork.linuxtv.org/patch/28847/
---
 .../media/uapi/v4l/pixfmt-reserved.rst        | 41 +++++++++++++++++++
 .../media/uapi/v4l/pixfmt-v4l2-mplane.rst     | 16 ++------
 Documentation/media/uapi/v4l/pixfmt-v4l2.rst  | 37 ++++++++++++++---
 drivers/media/v4l2-core/v4l2-ioctl.c          | 12 ++++++
 include/uapi/linux/videodev2.h                |  5 +++
 5 files changed, 94 insertions(+), 17 deletions(-)

diff --git a/Documentation/media/uapi/v4l/pixfmt-reserved.rst b/Documentation/media/uapi/v4l/pixfmt-reserved.rst
index 38af1472a4b4..c1090027626c 100644
--- a/Documentation/media/uapi/v4l/pixfmt-reserved.rst
+++ b/Documentation/media/uapi/v4l/pixfmt-reserved.rst
@@ -261,3 +261,44 @@ please make a proposal on the linux-media mailing list.
 	by RGBA values (128, 192, 255, 128), the same pixel described with
 	premultiplied colors would be described by RGBA values (64, 96,
 	128, 128)
+    * - ``V4L2_PIX_FMT_FLAG_CSC_COLORSPACE``
+      - 0x00000002
+      - Set by the driver to indicate colorspace conversion support. Set by the
+	application to request conversion to the specified colorspace. It is
+	only used for capture and is ignored for output streams. If set by the
+	application, then request the driver to do colorspace conversion from
+	the received colorspace to the requested colorspace by setting the
+	``colorspace`` field of struct :c:type:`v4l2_pix_format`.
+    * - ``V4L2_PIX_FMT_FLAG_CSC_YCBCR_ENC``
+      - 0x00000004
+      - Set by the driver to indicate Y'CbCr encoding conversion support. Set
+	by the application to request conversion to the specified Y'CbCr
+	encoding.  It is only used for capture and is ignored for output
+	streams. If set by the application, then request the driver to convert
+	from the received Y'CbCr encoding to the requested encoding by setting
+	the ``ycbcr_enc`` field of struct :c:type:`v4l2_pix_format`.
+    * - ``V4L2_PIX_FMT_FLAG_CSC_HSV_ENC``
+      - 0x00000004
+      - Set by the driver to indicate HSV encoding conversion support. Set
+	by the application to request conversion to the specified HSV encoding.
+	It is only used for capture and is ignored for output streams. If set
+	by the application, then request the driver to convert from the
+	received HSV encoding to the requested encoding by setting the
+	``hsv_enc`` field of struct :c:type:`v4l2_pix_format`.
+    * - ``V4L2_PIX_FMT_FLAG_CSC_QUANTIZATION``
+      - 0x00000008
+      - Set by the driver to indicate quantization range conversion support.
+	Set by the application to request conversion to the specified
+	quantization range. It is only used for capture and is ignored for
+	output streams. If set by the application, then request the driver to
+	convert from the received quantization range to the requested
+	quantization by setting the ``quantization`` field of struct
+	:c:type:`v4l2_pix_format`.
+    * - ``V4L2_PIX_FMT_FLAG_CSC_XFER_FUNC``
+      - 0x00000010
+      - Set by the driver to indicate transfer function conversion support.
+	Set by the application to request conversion to the specified transfer
+	function. It is only used for capture and is ignored for output
+	streams. If set by the application, then request the driver to convert
+	from the received transfer function to the requested transfer function
+	by setting the ``xfer_func`` field of struct :c:type:`v4l2_pix_format`.
diff --git a/Documentation/media/uapi/v4l/pixfmt-v4l2-mplane.rst b/Documentation/media/uapi/v4l/pixfmt-v4l2-mplane.rst
index ef52f637d8e9..7ff07411db77 100644
--- a/Documentation/media/uapi/v4l/pixfmt-v4l2-mplane.rst
+++ b/Documentation/media/uapi/v4l/pixfmt-v4l2-mplane.rst
@@ -81,30 +81,22 @@ describing all planes of that format.
     * - __u8
       - ``ycbcr_enc``
       - Y'CbCr encoding, from enum :c:type:`v4l2_ycbcr_encoding`.
-        This information supplements the ``colorspace`` and must be set by
-	the driver for capture streams and by the application for output
-	streams, see :ref:`colorspaces`.
+	See struct :c:type:`v4l2_pix_format`.
     * - __u8
       - ``hsv_enc``
       - HSV encoding, from enum :c:type:`v4l2_hsv_encoding`.
-        This information supplements the ``colorspace`` and must be set by
-	the driver for capture streams and by the application for output
-	streams, see :ref:`colorspaces`.
+	See struct :c:type:`v4l2_pix_format`.
     * - }
       -
       -
     * - __u8
       - ``quantization``
       - Quantization range, from enum :c:type:`v4l2_quantization`.
-        This information supplements the ``colorspace`` and must be set by
-	the driver for capture streams and by the application for output
-	streams, see :ref:`colorspaces`.
+	See struct :c:type:`v4l2_pix_format`.
     * - __u8
       - ``xfer_func``
       - Transfer function, from enum :c:type:`v4l2_xfer_func`.
-        This information supplements the ``colorspace`` and must be set by
-	the driver for capture streams and by the application for output
-	streams, see :ref:`colorspaces`.
+	See struct :c:type:`v4l2_pix_format`.
     * - __u8
       - ``reserved[7]``
       - Reserved for future extensions. Should be zeroed by drivers and
diff --git a/Documentation/media/uapi/v4l/pixfmt-v4l2.rst b/Documentation/media/uapi/v4l/pixfmt-v4l2.rst
index 826f2305da01..932b6a546e61 100644
--- a/Documentation/media/uapi/v4l/pixfmt-v4l2.rst
+++ b/Documentation/media/uapi/v4l/pixfmt-v4l2.rst
@@ -88,7 +88,12 @@ Single-planar format structure
       - Image colorspace, from enum :c:type:`v4l2_colorspace`.
         This information supplements the ``pixelformat`` and must be set
 	by the driver for capture streams and by the application for
-	output streams, see :ref:`colorspaces`.
+	output streams, see :ref:`colorspaces`. If the application sets the
+	flag ``V4L2_PIX_FMT_FLAG_CSC_COLORSPACE`` then the application can set
+	this field for a capture stream to request a specific colorspace for
+	the captured image data. The driver will attempt to do colorspace
+	conversion to the specified colorspace or return the colorspace it will
+	use if it can't do the conversion.
     * - __u32
       - ``priv``
       - This field indicates whether the remaining fields of the
@@ -126,13 +131,25 @@ Single-planar format structure
       - Y'CbCr encoding, from enum :c:type:`v4l2_ycbcr_encoding`.
         This information supplements the ``colorspace`` and must be set by
 	the driver for capture streams and by the application for output
-	streams, see :ref:`colorspaces`.
+	streams, see :ref:`colorspaces`. If the application sets the
+	flag ``V4L2_PIX_FMT_FLAG_CSC_YCBCR_ENC`` then the application can set
+	this field for a capture stream to request a specific Y'CbCr encoding
+	for the captured image data. The driver will attempt to do the
+	conversion to the specified Y'CbCr encoding or return the encoding it
+	will use if it can't do the conversion. This field is ignored for
+	non-Y'CbCr pixelformats.
     * - __u32
       - ``hsv_enc``
       - HSV encoding, from enum :c:type:`v4l2_hsv_encoding`.
         This information supplements the ``colorspace`` and must be set by
 	the driver for capture streams and by the application for output
-	streams, see :ref:`colorspaces`.
+	streams, see :ref:`colorspaces`. If the application sets the flag
+	``V4L2_PIX_FMT_FLAG_CSC_HSV_ENC`` then the application can set this
+	field for a capture stream to request a specific HSV encoding for the
+	captured image data. The driver will attempt to do the conversion to
+	the specified HSV encoding or return the encoding it will use if it
+	can't do the conversion. This field is ignored for non-HSV
+	pixelformats.
     * - }
       -
       -
@@ -141,10 +158,20 @@ Single-planar format structure
       - Quantization range, from enum :c:type:`v4l2_quantization`.
         This information supplements the ``colorspace`` and must be set by
 	the driver for capture streams and by the application for output
-	streams, see :ref:`colorspaces`.
+	streams, see :ref:`colorspaces`. If the application sets the flag
+	``V4L2_PIX_FMT_FLAG_CSC_QUANTIZATION`` then the application can set
+	this field for a capture stream to request a specific quantization
+	range for the captured image data. The driver will attempt to do the
+	conversion to the specified quantization range or return the
+	quantization it will use if it can't do the conversion.
     * - __u32
       - ``xfer_func``
       - Transfer function, from enum :c:type:`v4l2_xfer_func`.
         This information supplements the ``colorspace`` and must be set by
 	the driver for capture streams and by the application for output
-	streams, see :ref:`colorspaces`.
+	streams, see :ref:`colorspaces`. If the application sets the flag
+	``V4L2_PIX_FMT_FLAG_CSC_XFER_FUNC`` then the application can set
+	this field for a capture stream to request a specific transfer function
+	for the captured image data. The driver will attempt to do the
+	conversion to the specified transfer function or return the transfer
+	function it will use if it can't do the conversion.
diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c
index 54afc9c7ee6e..39def068f13e 100644
--- a/drivers/media/v4l2-core/v4l2-ioctl.c
+++ b/drivers/media/v4l2-core/v4l2-ioctl.c
@@ -1019,6 +1019,18 @@ static void v4l_sanitize_format(struct v4l2_format *fmt)
 	 * isn't used by applications.
 	 */
 
+	if (fmt->type == V4L2_BUF_TYPE_VIDEO_CAPTURE ||
+	    fmt->type == V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE) {
+		if (!(fmt->fmt.pix.flags & V4L2_PIX_FMT_FLAG_CSC_COLORSPACE))
+			fmt->fmt.pix.colorspace = V4L2_COLORSPACE_DEFAULT;
+		if (!(fmt->fmt.pix.flags & V4L2_PIX_FMT_FLAG_CSC_YCBCR_ENC))
+			fmt->fmt.pix.ycbcr_enc = V4L2_YCBCR_ENC_DEFAULT;
+		if (!(fmt->fmt.pix.flags & V4L2_PIX_FMT_FLAG_CSC_QUANTIZATION))
+			fmt->fmt.pix.quantization = V4L2_QUANTIZATION_DEFAULT;
+		if (!(fmt->fmt.pix.flags & V4L2_PIX_FMT_FLAG_CSC_XFER_FUNC))
+			fmt->fmt.pix.xfer_func = V4L2_XFER_FUNC_DEFAULT;
+	}
+
 	if (fmt->type != V4L2_BUF_TYPE_VIDEO_CAPTURE &&
 	    fmt->type != V4L2_BUF_TYPE_VIDEO_OUTPUT)
 		return;
diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
index 622f0479d668..4cbc8f23b828 100644
--- a/include/uapi/linux/videodev2.h
+++ b/include/uapi/linux/videodev2.h
@@ -709,6 +709,11 @@ struct v4l2_pix_format {
 
 /* Flags */
 #define V4L2_PIX_FMT_FLAG_PREMUL_ALPHA	0x00000001
+#define V4L2_PIX_FMT_FLAG_CSC_COLORSPACE	0x00000002
+#define V4L2_PIX_FMT_FLAG_CSC_YCBCR_ENC		0x00000004
+#define V4L2_PIX_FMT_FLAG_CSC_HSV_ENC		0x00000004
+#define V4L2_PIX_FMT_FLAG_CSC_QUANTIZATION	0x00000008
+#define V4L2_PIX_FMT_FLAG_CSC_XFER_FUNC		0x00000010
 
 /*
  *	F O R M A T   E N U M E R A T I O N
-- 
2.18.0

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH v2] [RFC v2] v4l2: add support for colorspace conversion for video capture
  2018-09-05 17:09 [PATCH v2] [RFC v2] v4l2: add support for colorspace conversion for video capture Philipp Zabel
@ 2018-09-06  9:02 ` Hans Verkuil
  2018-09-06  9:50   ` Philipp Zabel
  2018-12-07 14:30   ` Mauro Carvalho Chehab
  0 siblings, 2 replies; 9+ messages in thread
From: Hans Verkuil @ 2018-09-06  9:02 UTC (permalink / raw)
  To: Philipp Zabel, linux-media; +Cc: kernel, Hans Verkuil

Hi Philipp,

It is much appreciated that this old RFC of mine is picked up again.
I always wanted to get this in, but I never had a driver where it would
make sense to do so.

On 09/05/2018 07:09 PM, Philipp Zabel wrote:
> For video capture it is the driver that reports the colorspace,

add: "transfer function,"

> Y'CbCr/HSV encoding and quantization range used by the video, and there
> is no way to request something different, even though many HDTV
> receivers have some sort of colorspace conversion capabilities.
> 
> For output video this feature already exists since the application
> specifies this information for the video format it will send out, and
> the transmitter will enable any available CSC if a format conversion has
> to be performed in order to match the capabilities of the sink.
> 
> For video capture we propose adding new pix_format flags:
> V4L2_PIX_FMT_FLAG_CSC_COLORSPACE, V4L2_PIX_FMT_FLAG_CSC_YCBCR_ENC,
> V4L2_PIX_FMT_FLAG_CSC_HSV_ENC, V4L2_PIX_FMT_FLAG_CSC_QUANTIZATION, and
> V4L2_PIX_FMT_FLAG_CSC_XFER_FUNC. These are set by the driver to indicate
> its conversion features. When set by the application, the driver will
> interpret the colorspace, ycbcr_enc/hsv_enc, quantization and xfer_func
> fields as the requested colorspace information and will attempt to do
> the conversion it supports.
> 
> Drivers do not have to actually look at the flags: if the flags are not
> set, then the colorspace, ycbcr_enc and quantization fields are set to
> the default values by the core, i.e. just pass on the received format
> without conversion.

Thinking about this some more, I don't think this is quite the right approach.
Having userspace set these flags with S_FMT if they want to do explicit
conversions makes sense, and that part we can keep.

But to signal the capabilities I think should be done via new flags for
VIDIOC_ENUM_FMT. Basically the same set of flags, but for the flags field
of struct v4l2_fmtdesc.

One thing that's not clear to me is what happens if userspace sets one or
more flags and calls S_FMT for a driver that doesn't support this. Are the
flags zeroed in that case upon return? I don't think so, but I think that
is already true for the existing flag V4L2_PIX_FMT_FLAG_PREMUL_ALPHA.

I wonder if V4L2_PIX_FMT_FLAG_PREMUL_ALPHA should also get an equivalent
flag for v4l2_fmtdesc.

Then we can just document that v4l2_format flags are only valid if they
are also defined in v4l2_fmtdesc.

Does anyone have better ideas for this?

Regards,

	Hans

> 
> Signed-off-by: Hans Verkuil <Hans Verkuil@cisco.com>
> Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> ---
> Changes since v1 [1]
>  - convert to rst
>  - split V4L2_PIX_FMT_FLAG_REQUEST_CSC into four separate flags for
>    colorspace, ycbcr_enc/hsv_enc, quantization, and xfer_func
>  - let driver set flags to indicate supported features
> 
> [1] https://patchwork.linuxtv.org/patch/28847/
> ---
>  .../media/uapi/v4l/pixfmt-reserved.rst        | 41 +++++++++++++++++++
>  .../media/uapi/v4l/pixfmt-v4l2-mplane.rst     | 16 ++------
>  Documentation/media/uapi/v4l/pixfmt-v4l2.rst  | 37 ++++++++++++++---
>  drivers/media/v4l2-core/v4l2-ioctl.c          | 12 ++++++
>  include/uapi/linux/videodev2.h                |  5 +++
>  5 files changed, 94 insertions(+), 17 deletions(-)
> 
> diff --git a/Documentation/media/uapi/v4l/pixfmt-reserved.rst b/Documentation/media/uapi/v4l/pixfmt-reserved.rst
> index 38af1472a4b4..c1090027626c 100644
> --- a/Documentation/media/uapi/v4l/pixfmt-reserved.rst
> +++ b/Documentation/media/uapi/v4l/pixfmt-reserved.rst
> @@ -261,3 +261,44 @@ please make a proposal on the linux-media mailing list.
>  	by RGBA values (128, 192, 255, 128), the same pixel described with
>  	premultiplied colors would be described by RGBA values (64, 96,
>  	128, 128)
> +    * - ``V4L2_PIX_FMT_FLAG_CSC_COLORSPACE``
> +      - 0x00000002
> +      - Set by the driver to indicate colorspace conversion support. Set by the
> +	application to request conversion to the specified colorspace. It is
> +	only used for capture and is ignored for output streams. If set by the
> +	application, then request the driver to do colorspace conversion from
> +	the received colorspace to the requested colorspace by setting the
> +	``colorspace`` field of struct :c:type:`v4l2_pix_format`.
> +    * - ``V4L2_PIX_FMT_FLAG_CSC_YCBCR_ENC``
> +      - 0x00000004
> +      - Set by the driver to indicate Y'CbCr encoding conversion support. Set
> +	by the application to request conversion to the specified Y'CbCr
> +	encoding.  It is only used for capture and is ignored for output
> +	streams. If set by the application, then request the driver to convert
> +	from the received Y'CbCr encoding to the requested encoding by setting
> +	the ``ycbcr_enc`` field of struct :c:type:`v4l2_pix_format`.
> +    * - ``V4L2_PIX_FMT_FLAG_CSC_HSV_ENC``
> +      - 0x00000004
> +      - Set by the driver to indicate HSV encoding conversion support. Set
> +	by the application to request conversion to the specified HSV encoding.
> +	It is only used for capture and is ignored for output streams. If set
> +	by the application, then request the driver to convert from the
> +	received HSV encoding to the requested encoding by setting the
> +	``hsv_enc`` field of struct :c:type:`v4l2_pix_format`.
> +    * - ``V4L2_PIX_FMT_FLAG_CSC_QUANTIZATION``
> +      - 0x00000008
> +      - Set by the driver to indicate quantization range conversion support.
> +	Set by the application to request conversion to the specified
> +	quantization range. It is only used for capture and is ignored for
> +	output streams. If set by the application, then request the driver to
> +	convert from the received quantization range to the requested
> +	quantization by setting the ``quantization`` field of struct
> +	:c:type:`v4l2_pix_format`.
> +    * - ``V4L2_PIX_FMT_FLAG_CSC_XFER_FUNC``
> +      - 0x00000010
> +      - Set by the driver to indicate transfer function conversion support.
> +	Set by the application to request conversion to the specified transfer
> +	function. It is only used for capture and is ignored for output
> +	streams. If set by the application, then request the driver to convert
> +	from the received transfer function to the requested transfer function
> +	by setting the ``xfer_func`` field of struct :c:type:`v4l2_pix_format`.
> diff --git a/Documentation/media/uapi/v4l/pixfmt-v4l2-mplane.rst b/Documentation/media/uapi/v4l/pixfmt-v4l2-mplane.rst
> index ef52f637d8e9..7ff07411db77 100644
> --- a/Documentation/media/uapi/v4l/pixfmt-v4l2-mplane.rst
> +++ b/Documentation/media/uapi/v4l/pixfmt-v4l2-mplane.rst
> @@ -81,30 +81,22 @@ describing all planes of that format.
>      * - __u8
>        - ``ycbcr_enc``
>        - Y'CbCr encoding, from enum :c:type:`v4l2_ycbcr_encoding`.
> -        This information supplements the ``colorspace`` and must be set by
> -	the driver for capture streams and by the application for output
> -	streams, see :ref:`colorspaces`.
> +	See struct :c:type:`v4l2_pix_format`.
>      * - __u8
>        - ``hsv_enc``
>        - HSV encoding, from enum :c:type:`v4l2_hsv_encoding`.
> -        This information supplements the ``colorspace`` and must be set by
> -	the driver for capture streams and by the application for output
> -	streams, see :ref:`colorspaces`.
> +	See struct :c:type:`v4l2_pix_format`.
>      * - }
>        -
>        -
>      * - __u8
>        - ``quantization``
>        - Quantization range, from enum :c:type:`v4l2_quantization`.
> -        This information supplements the ``colorspace`` and must be set by
> -	the driver for capture streams and by the application for output
> -	streams, see :ref:`colorspaces`.
> +	See struct :c:type:`v4l2_pix_format`.
>      * - __u8
>        - ``xfer_func``
>        - Transfer function, from enum :c:type:`v4l2_xfer_func`.
> -        This information supplements the ``colorspace`` and must be set by
> -	the driver for capture streams and by the application for output
> -	streams, see :ref:`colorspaces`.
> +	See struct :c:type:`v4l2_pix_format`.
>      * - __u8
>        - ``reserved[7]``
>        - Reserved for future extensions. Should be zeroed by drivers and
> diff --git a/Documentation/media/uapi/v4l/pixfmt-v4l2.rst b/Documentation/media/uapi/v4l/pixfmt-v4l2.rst
> index 826f2305da01..932b6a546e61 100644
> --- a/Documentation/media/uapi/v4l/pixfmt-v4l2.rst
> +++ b/Documentation/media/uapi/v4l/pixfmt-v4l2.rst
> @@ -88,7 +88,12 @@ Single-planar format structure
>        - Image colorspace, from enum :c:type:`v4l2_colorspace`.
>          This information supplements the ``pixelformat`` and must be set
>  	by the driver for capture streams and by the application for
> -	output streams, see :ref:`colorspaces`.
> +	output streams, see :ref:`colorspaces`. If the application sets the
> +	flag ``V4L2_PIX_FMT_FLAG_CSC_COLORSPACE`` then the application can set
> +	this field for a capture stream to request a specific colorspace for
> +	the captured image data. The driver will attempt to do colorspace
> +	conversion to the specified colorspace or return the colorspace it will
> +	use if it can't do the conversion.
>      * - __u32
>        - ``priv``
>        - This field indicates whether the remaining fields of the
> @@ -126,13 +131,25 @@ Single-planar format structure
>        - Y'CbCr encoding, from enum :c:type:`v4l2_ycbcr_encoding`.
>          This information supplements the ``colorspace`` and must be set by
>  	the driver for capture streams and by the application for output
> -	streams, see :ref:`colorspaces`.
> +	streams, see :ref:`colorspaces`. If the application sets the
> +	flag ``V4L2_PIX_FMT_FLAG_CSC_YCBCR_ENC`` then the application can set
> +	this field for a capture stream to request a specific Y'CbCr encoding
> +	for the captured image data. The driver will attempt to do the
> +	conversion to the specified Y'CbCr encoding or return the encoding it
> +	will use if it can't do the conversion. This field is ignored for
> +	non-Y'CbCr pixelformats.
>      * - __u32
>        - ``hsv_enc``
>        - HSV encoding, from enum :c:type:`v4l2_hsv_encoding`.
>          This information supplements the ``colorspace`` and must be set by
>  	the driver for capture streams and by the application for output
> -	streams, see :ref:`colorspaces`.
> +	streams, see :ref:`colorspaces`. If the application sets the flag
> +	``V4L2_PIX_FMT_FLAG_CSC_HSV_ENC`` then the application can set this
> +	field for a capture stream to request a specific HSV encoding for the
> +	captured image data. The driver will attempt to do the conversion to
> +	the specified HSV encoding or return the encoding it will use if it
> +	can't do the conversion. This field is ignored for non-HSV
> +	pixelformats.
>      * - }
>        -
>        -
> @@ -141,10 +158,20 @@ Single-planar format structure
>        - Quantization range, from enum :c:type:`v4l2_quantization`.
>          This information supplements the ``colorspace`` and must be set by
>  	the driver for capture streams and by the application for output
> -	streams, see :ref:`colorspaces`.
> +	streams, see :ref:`colorspaces`. If the application sets the flag
> +	``V4L2_PIX_FMT_FLAG_CSC_QUANTIZATION`` then the application can set
> +	this field for a capture stream to request a specific quantization
> +	range for the captured image data. The driver will attempt to do the
> +	conversion to the specified quantization range or return the
> +	quantization it will use if it can't do the conversion.
>      * - __u32
>        - ``xfer_func``
>        - Transfer function, from enum :c:type:`v4l2_xfer_func`.
>          This information supplements the ``colorspace`` and must be set by
>  	the driver for capture streams and by the application for output
> -	streams, see :ref:`colorspaces`.
> +	streams, see :ref:`colorspaces`. If the application sets the flag
> +	``V4L2_PIX_FMT_FLAG_CSC_XFER_FUNC`` then the application can set
> +	this field for a capture stream to request a specific transfer function
> +	for the captured image data. The driver will attempt to do the
> +	conversion to the specified transfer function or return the transfer
> +	function it will use if it can't do the conversion.
> diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c
> index 54afc9c7ee6e..39def068f13e 100644
> --- a/drivers/media/v4l2-core/v4l2-ioctl.c
> +++ b/drivers/media/v4l2-core/v4l2-ioctl.c
> @@ -1019,6 +1019,18 @@ static void v4l_sanitize_format(struct v4l2_format *fmt)
>  	 * isn't used by applications.
>  	 */
>  
> +	if (fmt->type == V4L2_BUF_TYPE_VIDEO_CAPTURE ||
> +	    fmt->type == V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE) {
> +		if (!(fmt->fmt.pix.flags & V4L2_PIX_FMT_FLAG_CSC_COLORSPACE))
> +			fmt->fmt.pix.colorspace = V4L2_COLORSPACE_DEFAULT;
> +		if (!(fmt->fmt.pix.flags & V4L2_PIX_FMT_FLAG_CSC_YCBCR_ENC))
> +			fmt->fmt.pix.ycbcr_enc = V4L2_YCBCR_ENC_DEFAULT;
> +		if (!(fmt->fmt.pix.flags & V4L2_PIX_FMT_FLAG_CSC_QUANTIZATION))
> +			fmt->fmt.pix.quantization = V4L2_QUANTIZATION_DEFAULT;
> +		if (!(fmt->fmt.pix.flags & V4L2_PIX_FMT_FLAG_CSC_XFER_FUNC))
> +			fmt->fmt.pix.xfer_func = V4L2_XFER_FUNC_DEFAULT;
> +	}
> +
>  	if (fmt->type != V4L2_BUF_TYPE_VIDEO_CAPTURE &&
>  	    fmt->type != V4L2_BUF_TYPE_VIDEO_OUTPUT)
>  		return;
> diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
> index 622f0479d668..4cbc8f23b828 100644
> --- a/include/uapi/linux/videodev2.h
> +++ b/include/uapi/linux/videodev2.h
> @@ -709,6 +709,11 @@ struct v4l2_pix_format {
>  
>  /* Flags */
>  #define V4L2_PIX_FMT_FLAG_PREMUL_ALPHA	0x00000001
> +#define V4L2_PIX_FMT_FLAG_CSC_COLORSPACE	0x00000002
> +#define V4L2_PIX_FMT_FLAG_CSC_YCBCR_ENC		0x00000004
> +#define V4L2_PIX_FMT_FLAG_CSC_HSV_ENC		0x00000004
> +#define V4L2_PIX_FMT_FLAG_CSC_QUANTIZATION	0x00000008
> +#define V4L2_PIX_FMT_FLAG_CSC_XFER_FUNC		0x00000010
>  
>  /*
>   *	F O R M A T   E N U M E R A T I O N
> 

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v2] [RFC v2] v4l2: add support for colorspace conversion for video capture
  2018-09-06  9:02 ` Hans Verkuil
@ 2018-09-06  9:50   ` Philipp Zabel
  2018-09-06 10:54     ` Hans Verkuil
  2018-12-07 14:30   ` Mauro Carvalho Chehab
  1 sibling, 1 reply; 9+ messages in thread
From: Philipp Zabel @ 2018-09-06  9:50 UTC (permalink / raw)
  To: Hans Verkuil, linux-media; +Cc: Hans Verkuil, kernel

On Thu, 2018-09-06 at 11:02 +0200, Hans Verkuil wrote:
> Hi Philipp,
> 
> It is much appreciated that this old RFC of mine

Right, I should have made clearer that this is just a rework of Hans'
original RFC in [1].

[1] https://patchwork.linuxtv.org/patch/28847/

> is picked up again. I always wanted to get this in, but I never had a
> driver where it would make sense to do so.

I'll test this with i.MX PXP and IPU mem2mem drivers and follow up with
per-driver patches to enable this feature once we know where this should
be going.

> On 09/05/2018 07:09 PM, Philipp Zabel wrote:
> > For video capture it is the driver that reports the colorspace,
> 
> add: "transfer function,"

Will do.

> > Y'CbCr/HSV encoding and quantization range used by the video, and there
> > is no way to request something different, even though many HDTV
> > receivers have some sort of colorspace conversion capabilities.
> > 
> > For output video this feature already exists since the application
> > specifies this information for the video format it will send out, and
> > the transmitter will enable any available CSC if a format conversion has
> > to be performed in order to match the capabilities of the sink.
> > 
> > For video capture we propose adding new pix_format flags:
> > V4L2_PIX_FMT_FLAG_CSC_COLORSPACE, V4L2_PIX_FMT_FLAG_CSC_YCBCR_ENC,
> > V4L2_PIX_FMT_FLAG_CSC_HSV_ENC, V4L2_PIX_FMT_FLAG_CSC_QUANTIZATION, and
> > V4L2_PIX_FMT_FLAG_CSC_XFER_FUNC. These are set by the driver to indicate
> > its conversion features. When set by the application, the driver will
> > interpret the colorspace, ycbcr_enc/hsv_enc, quantization and xfer_func
> > fields as the requested colorspace information and will attempt to do
> > the conversion it supports.
> > 
> > Drivers do not have to actually look at the flags: if the flags are not
> > set, then the colorspace, ycbcr_enc and quantization fields are set to
> > the default values by the core, i.e. just pass on the received format
> > without conversion.
> 
> Thinking about this some more, I don't think this is quite the right approach.
> Having userspace set these flags with S_FMT if they want to do explicit
> conversions makes sense, and that part we can keep.
> 
> But to signal the capabilities I think should be done via new flags for
> VIDIOC_ENUM_FMT. Basically the same set of flags, but for the flags field
> of struct v4l2_fmtdesc.

In that case, I think the V4L2_PIX_FMT_FLAG_CSC_* should be purely a
signal from the application to the driver, and the driver should not
(have to) touch them at all.

An equivalent set of v4l2_fmtdesc flags could be used to signal
conversion support via VIDIOC_ENUM_FMT:

#define V4L2_FMT_FLAG_CSC_COLORSPACE	0x0004
#define V4L2_FMT_FLAG_CSC_YCBCR_ENC	0x0008
#define V4L2_FMT_FLAG_CSC_HSV_ENC	0x0008
#define V4L2_FMT_FLAG_CSC_QUANTIZATION	0x0010
#define V4L2_FMT_FLAG_CSC_XFER_FUNC	0x0020

What is the expected use case for these reported flags? Applications
that see them set to zero can skip enumerating capture side colorimetry.
Is there anything else?

> One thing that's not clear to me is what happens if userspace sets one or
> more flags and calls S_FMT for a driver that doesn't support this. Are the
> flags zeroed in that case upon return?

I'd say no. Drivers are free to silently ignore the flag.
The effect is the same as if the driver supports the flag in principle,
but has to change a requested value anyway because of some limitation.
The application can check whether the driver changed its requested
colorspace, xfer_func, ycbcr_enc, or quantization.

The application usually doesn't need to know whether the driver changed
the requested ycbcr_enc because it doesn't have CSC matrix support at
all, or because it doesn't implement a specific conversion. And if the
application needs to know for some reason, it can always check
VIDIOC_ENUM_FMT.

> I don't think so, but I think that
> is already true for the existing flag V4L2_PIX_FMT_FLAG_PREMUL_ALPHA.

The only drivers using V4L2_PIX_FMT_FLAG_PREMUL_ALPHA I can see are
vsp1_brx and vsp1_rpf. They never write to the v4l2_pix_format flags
field.

> I wonder if V4L2_PIX_FMT_FLAG_PREMUL_ALPHA should also get an equivalent
> flag for v4l2_fmtdesc.

Isn't this useless to introduce after the fact, if there are already
applications that use this feature? They can't depend on the existence
of this flag to check for support anyway.

> Then we can just document that v4l2_format flags are only valid if they
> are also defined in v4l2_fmtdesc.
> 
> Does anyone have better ideas for this?

I'd just say the driver is free to ignore the flag if it doesn't support
the specific requested value and leave it at that.

regards
Philipp

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v2] [RFC v2] v4l2: add support for colorspace conversion for video capture
  2018-09-06  9:50   ` Philipp Zabel
@ 2018-09-06 10:54     ` Hans Verkuil
  2018-09-06 12:59       ` Philipp Zabel
  2020-04-10 10:26       ` Dafna Hirschfeld
  0 siblings, 2 replies; 9+ messages in thread
From: Hans Verkuil @ 2018-09-06 10:54 UTC (permalink / raw)
  To: Philipp Zabel, Hans Verkuil, linux-media; +Cc: Hans Verkuil, kernel

On 09/06/18 11:50, Philipp Zabel wrote:
> On Thu, 2018-09-06 at 11:02 +0200, Hans Verkuil wrote:
>> Hi Philipp,
>>
>> It is much appreciated that this old RFC of mine
> 
> Right, I should have made clearer that this is just a rework of Hans'
> original RFC in [1].
> 
> [1] https://patchwork.linuxtv.org/patch/28847/
> 
>> is picked up again. I always wanted to get this in, but I never had a
>> driver where it would make sense to do so.
> 
> I'll test this with i.MX PXP and IPU mem2mem drivers and follow up with
> per-driver patches to enable this feature once we know where this should
> be going.
> 
>> On 09/05/2018 07:09 PM, Philipp Zabel wrote:
>>> For video capture it is the driver that reports the colorspace,
>>
>> add: "transfer function,"
> 
> Will do.
> 
>>> Y'CbCr/HSV encoding and quantization range used by the video, and there
>>> is no way to request something different, even though many HDTV
>>> receivers have some sort of colorspace conversion capabilities.
>>>
>>> For output video this feature already exists since the application
>>> specifies this information for the video format it will send out, and
>>> the transmitter will enable any available CSC if a format conversion has
>>> to be performed in order to match the capabilities of the sink.
>>>
>>> For video capture we propose adding new pix_format flags:
>>> V4L2_PIX_FMT_FLAG_CSC_COLORSPACE, V4L2_PIX_FMT_FLAG_CSC_YCBCR_ENC,
>>> V4L2_PIX_FMT_FLAG_CSC_HSV_ENC, V4L2_PIX_FMT_FLAG_CSC_QUANTIZATION, and
>>> V4L2_PIX_FMT_FLAG_CSC_XFER_FUNC. These are set by the driver to indicate
>>> its conversion features. When set by the application, the driver will
>>> interpret the colorspace, ycbcr_enc/hsv_enc, quantization and xfer_func
>>> fields as the requested colorspace information and will attempt to do
>>> the conversion it supports.
>>>
>>> Drivers do not have to actually look at the flags: if the flags are not
>>> set, then the colorspace, ycbcr_enc and quantization fields are set to
>>> the default values by the core, i.e. just pass on the received format
>>> without conversion.
>>
>> Thinking about this some more, I don't think this is quite the right approach.
>> Having userspace set these flags with S_FMT if they want to do explicit
>> conversions makes sense, and that part we can keep.
>>
>> But to signal the capabilities I think should be done via new flags for
>> VIDIOC_ENUM_FMT. Basically the same set of flags, but for the flags field
>> of struct v4l2_fmtdesc.
> 
> In that case, I think the V4L2_PIX_FMT_FLAG_CSC_* should be purely a
> signal from the application to the driver, and the driver should not
> (have to) touch them at all.

Right. The code in v4l2-ioctl.c that checks these flags and replaces
the corresponding field with 0 (DEFAULT) would be sufficient.

Drivers can just check the fields for non-default values.

> An equivalent set of v4l2_fmtdesc flags could be used to signal
> conversion support via VIDIOC_ENUM_FMT:
> 
> #define V4L2_FMT_FLAG_CSC_COLORSPACE	0x0004
> #define V4L2_FMT_FLAG_CSC_YCBCR_ENC	0x0008
> #define V4L2_FMT_FLAG_CSC_HSV_ENC	0x0008
> #define V4L2_FMT_FLAG_CSC_QUANTIZATION	0x0010
> #define V4L2_FMT_FLAG_CSC_XFER_FUNC	0x0020
> 
> What is the expected use case for these reported flags? Applications
> that see them set to zero can skip enumerating capture side colorimetry.
> Is there anything else?

That's about it. It just signals if the HW is capable of doing such
conversions.

>> One thing that's not clear to me is what happens if userspace sets one or
>> more flags and calls S_FMT for a driver that doesn't support this. Are the
>> flags zeroed in that case upon return?
> 
> I'd say no. Drivers are free to silently ignore the flag.
> The effect is the same as if the driver supports the flag in principle,
> but has to change a requested value anyway because of some limitation.
> The application can check whether the driver changed its requested
> colorspace, xfer_func, ycbcr_enc, or quantization.
> 
> The application usually doesn't need to know whether the driver changed
> the requested ycbcr_enc because it doesn't have CSC matrix support at
> all, or because it doesn't implement a specific conversion. And if the
> application needs to know for some reason, it can always check
> VIDIOC_ENUM_FMT.
> 
>> I don't think so, but I think that
>> is already true for the existing flag V4L2_PIX_FMT_FLAG_PREMUL_ALPHA.
> 
> The only drivers using V4L2_PIX_FMT_FLAG_PREMUL_ALPHA I can see are
> vsp1_brx and vsp1_rpf. They never write to the v4l2_pix_format flags
> field.

But they honor it. The problem is that I can set this flag and call S_FMT
on e.g. the vivid driver, and S_FMT will return the flag. But it actually
doesn't use the flag at all, so userspace has no way of knowing if the
flag is actually used. Although it can call G_FMT and then the flag is
cleared.

> 
>> I wonder if V4L2_PIX_FMT_FLAG_PREMUL_ALPHA should also get an equivalent
>> flag for v4l2_fmtdesc.
> 
> Isn't this useless to introduce after the fact, if there are already
> applications that use this feature? They can't depend on the existence
> of this flag to check for support anyway.

Those applications are already hardcoded for the vsp1. So they won't break
by adding v4l2_fmtdesc flags.

But apps like gstreamer and friends can start using these flags and deduce
what the HW is capable of.

> 
>> Then we can just document that v4l2_format flags are only valid if they
>> are also defined in v4l2_fmtdesc.
>>
>> Does anyone have better ideas for this?
> 
> I'd just say the driver is free to ignore the flag if it doesn't support
> the specific requested value and leave it at that.

It's probably the best option.

Regards,

	Hans

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v2] [RFC v2] v4l2: add support for colorspace conversion for video capture
  2018-09-06 10:54     ` Hans Verkuil
@ 2018-09-06 12:59       ` Philipp Zabel
  2020-04-10 10:26       ` Dafna Hirschfeld
  1 sibling, 0 replies; 9+ messages in thread
From: Philipp Zabel @ 2018-09-06 12:59 UTC (permalink / raw)
  To: Hans Verkuil, Hans Verkuil, linux-media; +Cc: Hans Verkuil, kernel

On Thu, 2018-09-06 at 12:54 +0200, Hans Verkuil wrote:
[...]
> > The application usually doesn't need to know whether the driver changed
> > the requested ycbcr_enc because it doesn't have CSC matrix support at
> > all, or because it doesn't implement a specific conversion. And if the
> > application needs to know for some reason, it can always check
> > VIDIOC_ENUM_FMT.
> > 
> > > I don't think so, but I think that
> > > is already true for the existing flag V4L2_PIX_FMT_FLAG_PREMUL_ALPHA.
> > 
> > The only drivers using V4L2_PIX_FMT_FLAG_PREMUL_ALPHA I can see are
> > vsp1_brx and vsp1_rpf. They never write to the v4l2_pix_format flags
> > field.
> 
> But they honor it. The problem is that I can set this flag and call S_FMT
> on e.g. the vivid driver, and S_FMT will return the flag. But it actually
> doesn't use the flag at all, so userspace has no way of knowing if the
> flag is actually used.

Userspace can see if its requested colorspace, etc. were changed by the
driver, though.

> Although it can call G_FMT and then the flag is cleared.

So if drivers were to clear the flag, would they clear it if they don't
support the flag at all, or also if they support the flag in principle,
but can't convert into the specific requested value?

Would all drivers have to be modified to clear those flags? I suppose
rather the framework should be extended to set the flags on ENUM_FMT and
to mask the TRY/S_FMT flags with those.

> > > I wonder if V4L2_PIX_FMT_FLAG_PREMUL_ALPHA should also get an equivalent
> > > flag for v4l2_fmtdesc.
> > 
> > Isn't this useless to introduce after the fact, if there are already
> > applications that use this feature? They can't depend on the existence
> > of this flag to check for support anyway.
> 
> Those applications are already hardcoded for the vsp1. So they won't break
> by adding v4l2_fmtdesc flags.
> 
> But apps like gstreamer and friends can start using these flags and deduce
> what the HW is capable of.

I see. In that case I agree.

regards
Philipp

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v2] [RFC v2] v4l2: add support for colorspace conversion for video capture
  2018-09-06  9:02 ` Hans Verkuil
  2018-09-06  9:50   ` Philipp Zabel
@ 2018-12-07 14:30   ` Mauro Carvalho Chehab
  2018-12-07 14:38     ` Hans Verkuil
  1 sibling, 1 reply; 9+ messages in thread
From: Mauro Carvalho Chehab @ 2018-12-07 14:30 UTC (permalink / raw)
  To: Hans Verkuil; +Cc: Philipp Zabel, linux-media, kernel, Hans Verkuil

Em Thu, 6 Sep 2018 11:02:28 +0200
Hans Verkuil <hverkuil@xs4all.nl> escreveu:

> Hi Philipp,
> 
> It is much appreciated that this old RFC of mine is picked up again.
> I always wanted to get this in, but I never had a driver where it would
> make sense to do so.

What's the status of this?

Hans,
As this is an old RFC from you, I'll delegate it to you at patchwork,
for you to track it.

Regards,
Mauro

> 
> On 09/05/2018 07:09 PM, Philipp Zabel wrote:
> > For video capture it is the driver that reports the colorspace,  
> 
> add: "transfer function,"
> 
> > Y'CbCr/HSV encoding and quantization range used by the video, and there
> > is no way to request something different, even though many HDTV
> > receivers have some sort of colorspace conversion capabilities.
> > 
> > For output video this feature already exists since the application
> > specifies this information for the video format it will send out, and
> > the transmitter will enable any available CSC if a format conversion has
> > to be performed in order to match the capabilities of the sink.
> > 
> > For video capture we propose adding new pix_format flags:
> > V4L2_PIX_FMT_FLAG_CSC_COLORSPACE, V4L2_PIX_FMT_FLAG_CSC_YCBCR_ENC,
> > V4L2_PIX_FMT_FLAG_CSC_HSV_ENC, V4L2_PIX_FMT_FLAG_CSC_QUANTIZATION, and
> > V4L2_PIX_FMT_FLAG_CSC_XFER_FUNC. These are set by the driver to indicate
> > its conversion features. When set by the application, the driver will
> > interpret the colorspace, ycbcr_enc/hsv_enc, quantization and xfer_func
> > fields as the requested colorspace information and will attempt to do
> > the conversion it supports.
> > 
> > Drivers do not have to actually look at the flags: if the flags are not
> > set, then the colorspace, ycbcr_enc and quantization fields are set to
> > the default values by the core, i.e. just pass on the received format
> > without conversion.  
> 
> Thinking about this some more, I don't think this is quite the right approach.
> Having userspace set these flags with S_FMT if they want to do explicit
> conversions makes sense, and that part we can keep.
> 
> But to signal the capabilities I think should be done via new flags for
> VIDIOC_ENUM_FMT. Basically the same set of flags, but for the flags field
> of struct v4l2_fmtdesc.
> 
> One thing that's not clear to me is what happens if userspace sets one or
> more flags and calls S_FMT for a driver that doesn't support this. Are the
> flags zeroed in that case upon return? I don't think so, but I think that
> is already true for the existing flag V4L2_PIX_FMT_FLAG_PREMUL_ALPHA.
> 
> I wonder if V4L2_PIX_FMT_FLAG_PREMUL_ALPHA should also get an equivalent
> flag for v4l2_fmtdesc.
> 
> Then we can just document that v4l2_format flags are only valid if they
> are also defined in v4l2_fmtdesc.
> 
> Does anyone have better ideas for this?
> 
> Regards,
> 
> 	Hans
> 
> > 
> > Signed-off-by: Hans Verkuil <Hans Verkuil@cisco.com>
> > Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> > ---
> > Changes since v1 [1]
> >  - convert to rst
> >  - split V4L2_PIX_FMT_FLAG_REQUEST_CSC into four separate flags for
> >    colorspace, ycbcr_enc/hsv_enc, quantization, and xfer_func
> >  - let driver set flags to indicate supported features
> > 
> > [1] https://patchwork.linuxtv.org/patch/28847/
> > ---
> >  .../media/uapi/v4l/pixfmt-reserved.rst        | 41 +++++++++++++++++++
> >  .../media/uapi/v4l/pixfmt-v4l2-mplane.rst     | 16 ++------
> >  Documentation/media/uapi/v4l/pixfmt-v4l2.rst  | 37 ++++++++++++++---
> >  drivers/media/v4l2-core/v4l2-ioctl.c          | 12 ++++++
> >  include/uapi/linux/videodev2.h                |  5 +++
> >  5 files changed, 94 insertions(+), 17 deletions(-)
> > 
> > diff --git a/Documentation/media/uapi/v4l/pixfmt-reserved.rst b/Documentation/media/uapi/v4l/pixfmt-reserved.rst
> > index 38af1472a4b4..c1090027626c 100644
> > --- a/Documentation/media/uapi/v4l/pixfmt-reserved.rst
> > +++ b/Documentation/media/uapi/v4l/pixfmt-reserved.rst
> > @@ -261,3 +261,44 @@ please make a proposal on the linux-media mailing list.
> >  	by RGBA values (128, 192, 255, 128), the same pixel described with
> >  	premultiplied colors would be described by RGBA values (64, 96,
> >  	128, 128)
> > +    * - ``V4L2_PIX_FMT_FLAG_CSC_COLORSPACE``
> > +      - 0x00000002
> > +      - Set by the driver to indicate colorspace conversion support. Set by the
> > +	application to request conversion to the specified colorspace. It is
> > +	only used for capture and is ignored for output streams. If set by the
> > +	application, then request the driver to do colorspace conversion from
> > +	the received colorspace to the requested colorspace by setting the
> > +	``colorspace`` field of struct :c:type:`v4l2_pix_format`.
> > +    * - ``V4L2_PIX_FMT_FLAG_CSC_YCBCR_ENC``
> > +      - 0x00000004
> > +      - Set by the driver to indicate Y'CbCr encoding conversion support. Set
> > +	by the application to request conversion to the specified Y'CbCr
> > +	encoding.  It is only used for capture and is ignored for output
> > +	streams. If set by the application, then request the driver to convert
> > +	from the received Y'CbCr encoding to the requested encoding by setting
> > +	the ``ycbcr_enc`` field of struct :c:type:`v4l2_pix_format`.
> > +    * - ``V4L2_PIX_FMT_FLAG_CSC_HSV_ENC``
> > +      - 0x00000004
> > +      - Set by the driver to indicate HSV encoding conversion support. Set
> > +	by the application to request conversion to the specified HSV encoding.
> > +	It is only used for capture and is ignored for output streams. If set
> > +	by the application, then request the driver to convert from the
> > +	received HSV encoding to the requested encoding by setting the
> > +	``hsv_enc`` field of struct :c:type:`v4l2_pix_format`.
> > +    * - ``V4L2_PIX_FMT_FLAG_CSC_QUANTIZATION``
> > +      - 0x00000008
> > +      - Set by the driver to indicate quantization range conversion support.
> > +	Set by the application to request conversion to the specified
> > +	quantization range. It is only used for capture and is ignored for
> > +	output streams. If set by the application, then request the driver to
> > +	convert from the received quantization range to the requested
> > +	quantization by setting the ``quantization`` field of struct
> > +	:c:type:`v4l2_pix_format`.
> > +    * - ``V4L2_PIX_FMT_FLAG_CSC_XFER_FUNC``
> > +      - 0x00000010
> > +      - Set by the driver to indicate transfer function conversion support.
> > +	Set by the application to request conversion to the specified transfer
> > +	function. It is only used for capture and is ignored for output
> > +	streams. If set by the application, then request the driver to convert
> > +	from the received transfer function to the requested transfer function
> > +	by setting the ``xfer_func`` field of struct :c:type:`v4l2_pix_format`.
> > diff --git a/Documentation/media/uapi/v4l/pixfmt-v4l2-mplane.rst b/Documentation/media/uapi/v4l/pixfmt-v4l2-mplane.rst
> > index ef52f637d8e9..7ff07411db77 100644
> > --- a/Documentation/media/uapi/v4l/pixfmt-v4l2-mplane.rst
> > +++ b/Documentation/media/uapi/v4l/pixfmt-v4l2-mplane.rst
> > @@ -81,30 +81,22 @@ describing all planes of that format.
> >      * - __u8
> >        - ``ycbcr_enc``
> >        - Y'CbCr encoding, from enum :c:type:`v4l2_ycbcr_encoding`.
> > -        This information supplements the ``colorspace`` and must be set by
> > -	the driver for capture streams and by the application for output
> > -	streams, see :ref:`colorspaces`.
> > +	See struct :c:type:`v4l2_pix_format`.
> >      * - __u8
> >        - ``hsv_enc``
> >        - HSV encoding, from enum :c:type:`v4l2_hsv_encoding`.
> > -        This information supplements the ``colorspace`` and must be set by
> > -	the driver for capture streams and by the application for output
> > -	streams, see :ref:`colorspaces`.
> > +	See struct :c:type:`v4l2_pix_format`.
> >      * - }
> >        -
> >        -
> >      * - __u8
> >        - ``quantization``
> >        - Quantization range, from enum :c:type:`v4l2_quantization`.
> > -        This information supplements the ``colorspace`` and must be set by
> > -	the driver for capture streams and by the application for output
> > -	streams, see :ref:`colorspaces`.
> > +	See struct :c:type:`v4l2_pix_format`.
> >      * - __u8
> >        - ``xfer_func``
> >        - Transfer function, from enum :c:type:`v4l2_xfer_func`.
> > -        This information supplements the ``colorspace`` and must be set by
> > -	the driver for capture streams and by the application for output
> > -	streams, see :ref:`colorspaces`.
> > +	See struct :c:type:`v4l2_pix_format`.
> >      * - __u8
> >        - ``reserved[7]``
> >        - Reserved for future extensions. Should be zeroed by drivers and
> > diff --git a/Documentation/media/uapi/v4l/pixfmt-v4l2.rst b/Documentation/media/uapi/v4l/pixfmt-v4l2.rst
> > index 826f2305da01..932b6a546e61 100644
> > --- a/Documentation/media/uapi/v4l/pixfmt-v4l2.rst
> > +++ b/Documentation/media/uapi/v4l/pixfmt-v4l2.rst
> > @@ -88,7 +88,12 @@ Single-planar format structure
> >        - Image colorspace, from enum :c:type:`v4l2_colorspace`.
> >          This information supplements the ``pixelformat`` and must be set
> >  	by the driver for capture streams and by the application for
> > -	output streams, see :ref:`colorspaces`.
> > +	output streams, see :ref:`colorspaces`. If the application sets the
> > +	flag ``V4L2_PIX_FMT_FLAG_CSC_COLORSPACE`` then the application can set
> > +	this field for a capture stream to request a specific colorspace for
> > +	the captured image data. The driver will attempt to do colorspace
> > +	conversion to the specified colorspace or return the colorspace it will
> > +	use if it can't do the conversion.
> >      * - __u32
> >        - ``priv``
> >        - This field indicates whether the remaining fields of the
> > @@ -126,13 +131,25 @@ Single-planar format structure
> >        - Y'CbCr encoding, from enum :c:type:`v4l2_ycbcr_encoding`.
> >          This information supplements the ``colorspace`` and must be set by
> >  	the driver for capture streams and by the application for output
> > -	streams, see :ref:`colorspaces`.
> > +	streams, see :ref:`colorspaces`. If the application sets the
> > +	flag ``V4L2_PIX_FMT_FLAG_CSC_YCBCR_ENC`` then the application can set
> > +	this field for a capture stream to request a specific Y'CbCr encoding
> > +	for the captured image data. The driver will attempt to do the
> > +	conversion to the specified Y'CbCr encoding or return the encoding it
> > +	will use if it can't do the conversion. This field is ignored for
> > +	non-Y'CbCr pixelformats.
> >      * - __u32
> >        - ``hsv_enc``
> >        - HSV encoding, from enum :c:type:`v4l2_hsv_encoding`.
> >          This information supplements the ``colorspace`` and must be set by
> >  	the driver for capture streams and by the application for output
> > -	streams, see :ref:`colorspaces`.
> > +	streams, see :ref:`colorspaces`. If the application sets the flag
> > +	``V4L2_PIX_FMT_FLAG_CSC_HSV_ENC`` then the application can set this
> > +	field for a capture stream to request a specific HSV encoding for the
> > +	captured image data. The driver will attempt to do the conversion to
> > +	the specified HSV encoding or return the encoding it will use if it
> > +	can't do the conversion. This field is ignored for non-HSV
> > +	pixelformats.
> >      * - }
> >        -
> >        -
> > @@ -141,10 +158,20 @@ Single-planar format structure
> >        - Quantization range, from enum :c:type:`v4l2_quantization`.
> >          This information supplements the ``colorspace`` and must be set by
> >  	the driver for capture streams and by the application for output
> > -	streams, see :ref:`colorspaces`.
> > +	streams, see :ref:`colorspaces`. If the application sets the flag
> > +	``V4L2_PIX_FMT_FLAG_CSC_QUANTIZATION`` then the application can set
> > +	this field for a capture stream to request a specific quantization
> > +	range for the captured image data. The driver will attempt to do the
> > +	conversion to the specified quantization range or return the
> > +	quantization it will use if it can't do the conversion.
> >      * - __u32
> >        - ``xfer_func``
> >        - Transfer function, from enum :c:type:`v4l2_xfer_func`.
> >          This information supplements the ``colorspace`` and must be set by
> >  	the driver for capture streams and by the application for output
> > -	streams, see :ref:`colorspaces`.
> > +	streams, see :ref:`colorspaces`. If the application sets the flag
> > +	``V4L2_PIX_FMT_FLAG_CSC_XFER_FUNC`` then the application can set
> > +	this field for a capture stream to request a specific transfer function
> > +	for the captured image data. The driver will attempt to do the
> > +	conversion to the specified transfer function or return the transfer
> > +	function it will use if it can't do the conversion.
> > diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c
> > index 54afc9c7ee6e..39def068f13e 100644
> > --- a/drivers/media/v4l2-core/v4l2-ioctl.c
> > +++ b/drivers/media/v4l2-core/v4l2-ioctl.c
> > @@ -1019,6 +1019,18 @@ static void v4l_sanitize_format(struct v4l2_format *fmt)
> >  	 * isn't used by applications.
> >  	 */
> >  
> > +	if (fmt->type == V4L2_BUF_TYPE_VIDEO_CAPTURE ||
> > +	    fmt->type == V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE) {
> > +		if (!(fmt->fmt.pix.flags & V4L2_PIX_FMT_FLAG_CSC_COLORSPACE))
> > +			fmt->fmt.pix.colorspace = V4L2_COLORSPACE_DEFAULT;
> > +		if (!(fmt->fmt.pix.flags & V4L2_PIX_FMT_FLAG_CSC_YCBCR_ENC))
> > +			fmt->fmt.pix.ycbcr_enc = V4L2_YCBCR_ENC_DEFAULT;
> > +		if (!(fmt->fmt.pix.flags & V4L2_PIX_FMT_FLAG_CSC_QUANTIZATION))
> > +			fmt->fmt.pix.quantization = V4L2_QUANTIZATION_DEFAULT;
> > +		if (!(fmt->fmt.pix.flags & V4L2_PIX_FMT_FLAG_CSC_XFER_FUNC))
> > +			fmt->fmt.pix.xfer_func = V4L2_XFER_FUNC_DEFAULT;
> > +	}
> > +
> >  	if (fmt->type != V4L2_BUF_TYPE_VIDEO_CAPTURE &&
> >  	    fmt->type != V4L2_BUF_TYPE_VIDEO_OUTPUT)
> >  		return;
> > diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
> > index 622f0479d668..4cbc8f23b828 100644
> > --- a/include/uapi/linux/videodev2.h
> > +++ b/include/uapi/linux/videodev2.h
> > @@ -709,6 +709,11 @@ struct v4l2_pix_format {
> >  
> >  /* Flags */
> >  #define V4L2_PIX_FMT_FLAG_PREMUL_ALPHA	0x00000001
> > +#define V4L2_PIX_FMT_FLAG_CSC_COLORSPACE	0x00000002
> > +#define V4L2_PIX_FMT_FLAG_CSC_YCBCR_ENC		0x00000004
> > +#define V4L2_PIX_FMT_FLAG_CSC_HSV_ENC		0x00000004
> > +#define V4L2_PIX_FMT_FLAG_CSC_QUANTIZATION	0x00000008
> > +#define V4L2_PIX_FMT_FLAG_CSC_XFER_FUNC		0x00000010
> >  
> >  /*
> >   *	F O R M A T   E N U M E R A T I O N
> >   
> 



Thanks,
Mauro

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v2] [RFC v2] v4l2: add support for colorspace conversion for video capture
  2018-12-07 14:30   ` Mauro Carvalho Chehab
@ 2018-12-07 14:38     ` Hans Verkuil
  0 siblings, 0 replies; 9+ messages in thread
From: Hans Verkuil @ 2018-12-07 14:38 UTC (permalink / raw)
  To: Mauro Carvalho Chehab; +Cc: Philipp Zabel, linux-media, kernel, Hans Verkuil

On 12/07/2018 03:30 PM, Mauro Carvalho Chehab wrote:
> Em Thu, 6 Sep 2018 11:02:28 +0200
> Hans Verkuil <hverkuil@xs4all.nl> escreveu:
> 
>> Hi Philipp,
>>
>> It is much appreciated that this old RFC of mine is picked up again.
>> I always wanted to get this in, but I never had a driver where it would
>> make sense to do so.
> 
> What's the status of this?

Changes were requested, and there was some discussion. I'm basically
waiting for an update.

I've delegated it to me.

Regards,

	Hans

> 
> Hans,
> As this is an old RFC from you, I'll delegate it to you at patchwork,
> for you to track it.
> 
> Regards,
> Mauro
> 
>>
>> On 09/05/2018 07:09 PM, Philipp Zabel wrote:
>>> For video capture it is the driver that reports the colorspace,  
>>
>> add: "transfer function,"
>>
>>> Y'CbCr/HSV encoding and quantization range used by the video, and there
>>> is no way to request something different, even though many HDTV
>>> receivers have some sort of colorspace conversion capabilities.
>>>
>>> For output video this feature already exists since the application
>>> specifies this information for the video format it will send out, and
>>> the transmitter will enable any available CSC if a format conversion has
>>> to be performed in order to match the capabilities of the sink.
>>>
>>> For video capture we propose adding new pix_format flags:
>>> V4L2_PIX_FMT_FLAG_CSC_COLORSPACE, V4L2_PIX_FMT_FLAG_CSC_YCBCR_ENC,
>>> V4L2_PIX_FMT_FLAG_CSC_HSV_ENC, V4L2_PIX_FMT_FLAG_CSC_QUANTIZATION, and
>>> V4L2_PIX_FMT_FLAG_CSC_XFER_FUNC. These are set by the driver to indicate
>>> its conversion features. When set by the application, the driver will
>>> interpret the colorspace, ycbcr_enc/hsv_enc, quantization and xfer_func
>>> fields as the requested colorspace information and will attempt to do
>>> the conversion it supports.
>>>
>>> Drivers do not have to actually look at the flags: if the flags are not
>>> set, then the colorspace, ycbcr_enc and quantization fields are set to
>>> the default values by the core, i.e. just pass on the received format
>>> without conversion.  
>>
>> Thinking about this some more, I don't think this is quite the right approach.
>> Having userspace set these flags with S_FMT if they want to do explicit
>> conversions makes sense, and that part we can keep.
>>
>> But to signal the capabilities I think should be done via new flags for
>> VIDIOC_ENUM_FMT. Basically the same set of flags, but for the flags field
>> of struct v4l2_fmtdesc.
>>
>> One thing that's not clear to me is what happens if userspace sets one or
>> more flags and calls S_FMT for a driver that doesn't support this. Are the
>> flags zeroed in that case upon return? I don't think so, but I think that
>> is already true for the existing flag V4L2_PIX_FMT_FLAG_PREMUL_ALPHA.
>>
>> I wonder if V4L2_PIX_FMT_FLAG_PREMUL_ALPHA should also get an equivalent
>> flag for v4l2_fmtdesc.
>>
>> Then we can just document that v4l2_format flags are only valid if they
>> are also defined in v4l2_fmtdesc.
>>
>> Does anyone have better ideas for this?
>>
>> Regards,
>>
>> 	Hans
>>
>>>
>>> Signed-off-by: Hans Verkuil <Hans Verkuil@cisco.com>
>>> Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
>>> ---
>>> Changes since v1 [1]
>>>  - convert to rst
>>>  - split V4L2_PIX_FMT_FLAG_REQUEST_CSC into four separate flags for
>>>    colorspace, ycbcr_enc/hsv_enc, quantization, and xfer_func
>>>  - let driver set flags to indicate supported features
>>>
>>> [1] https://patchwork.linuxtv.org/patch/28847/
>>> ---
>>>  .../media/uapi/v4l/pixfmt-reserved.rst        | 41 +++++++++++++++++++
>>>  .../media/uapi/v4l/pixfmt-v4l2-mplane.rst     | 16 ++------
>>>  Documentation/media/uapi/v4l/pixfmt-v4l2.rst  | 37 ++++++++++++++---
>>>  drivers/media/v4l2-core/v4l2-ioctl.c          | 12 ++++++
>>>  include/uapi/linux/videodev2.h                |  5 +++
>>>  5 files changed, 94 insertions(+), 17 deletions(-)
>>>
>>> diff --git a/Documentation/media/uapi/v4l/pixfmt-reserved.rst b/Documentation/media/uapi/v4l/pixfmt-reserved.rst
>>> index 38af1472a4b4..c1090027626c 100644
>>> --- a/Documentation/media/uapi/v4l/pixfmt-reserved.rst
>>> +++ b/Documentation/media/uapi/v4l/pixfmt-reserved.rst
>>> @@ -261,3 +261,44 @@ please make a proposal on the linux-media mailing list.
>>>  	by RGBA values (128, 192, 255, 128), the same pixel described with
>>>  	premultiplied colors would be described by RGBA values (64, 96,
>>>  	128, 128)
>>> +    * - ``V4L2_PIX_FMT_FLAG_CSC_COLORSPACE``
>>> +      - 0x00000002
>>> +      - Set by the driver to indicate colorspace conversion support. Set by the
>>> +	application to request conversion to the specified colorspace. It is
>>> +	only used for capture and is ignored for output streams. If set by the
>>> +	application, then request the driver to do colorspace conversion from
>>> +	the received colorspace to the requested colorspace by setting the
>>> +	``colorspace`` field of struct :c:type:`v4l2_pix_format`.
>>> +    * - ``V4L2_PIX_FMT_FLAG_CSC_YCBCR_ENC``
>>> +      - 0x00000004
>>> +      - Set by the driver to indicate Y'CbCr encoding conversion support. Set
>>> +	by the application to request conversion to the specified Y'CbCr
>>> +	encoding.  It is only used for capture and is ignored for output
>>> +	streams. If set by the application, then request the driver to convert
>>> +	from the received Y'CbCr encoding to the requested encoding by setting
>>> +	the ``ycbcr_enc`` field of struct :c:type:`v4l2_pix_format`.
>>> +    * - ``V4L2_PIX_FMT_FLAG_CSC_HSV_ENC``
>>> +      - 0x00000004
>>> +      - Set by the driver to indicate HSV encoding conversion support. Set
>>> +	by the application to request conversion to the specified HSV encoding.
>>> +	It is only used for capture and is ignored for output streams. If set
>>> +	by the application, then request the driver to convert from the
>>> +	received HSV encoding to the requested encoding by setting the
>>> +	``hsv_enc`` field of struct :c:type:`v4l2_pix_format`.
>>> +    * - ``V4L2_PIX_FMT_FLAG_CSC_QUANTIZATION``
>>> +      - 0x00000008
>>> +      - Set by the driver to indicate quantization range conversion support.
>>> +	Set by the application to request conversion to the specified
>>> +	quantization range. It is only used for capture and is ignored for
>>> +	output streams. If set by the application, then request the driver to
>>> +	convert from the received quantization range to the requested
>>> +	quantization by setting the ``quantization`` field of struct
>>> +	:c:type:`v4l2_pix_format`.
>>> +    * - ``V4L2_PIX_FMT_FLAG_CSC_XFER_FUNC``
>>> +      - 0x00000010
>>> +      - Set by the driver to indicate transfer function conversion support.
>>> +	Set by the application to request conversion to the specified transfer
>>> +	function. It is only used for capture and is ignored for output
>>> +	streams. If set by the application, then request the driver to convert
>>> +	from the received transfer function to the requested transfer function
>>> +	by setting the ``xfer_func`` field of struct :c:type:`v4l2_pix_format`.
>>> diff --git a/Documentation/media/uapi/v4l/pixfmt-v4l2-mplane.rst b/Documentation/media/uapi/v4l/pixfmt-v4l2-mplane.rst
>>> index ef52f637d8e9..7ff07411db77 100644
>>> --- a/Documentation/media/uapi/v4l/pixfmt-v4l2-mplane.rst
>>> +++ b/Documentation/media/uapi/v4l/pixfmt-v4l2-mplane.rst
>>> @@ -81,30 +81,22 @@ describing all planes of that format.
>>>      * - __u8
>>>        - ``ycbcr_enc``
>>>        - Y'CbCr encoding, from enum :c:type:`v4l2_ycbcr_encoding`.
>>> -        This information supplements the ``colorspace`` and must be set by
>>> -	the driver for capture streams and by the application for output
>>> -	streams, see :ref:`colorspaces`.
>>> +	See struct :c:type:`v4l2_pix_format`.
>>>      * - __u8
>>>        - ``hsv_enc``
>>>        - HSV encoding, from enum :c:type:`v4l2_hsv_encoding`.
>>> -        This information supplements the ``colorspace`` and must be set by
>>> -	the driver for capture streams and by the application for output
>>> -	streams, see :ref:`colorspaces`.
>>> +	See struct :c:type:`v4l2_pix_format`.
>>>      * - }
>>>        -
>>>        -
>>>      * - __u8
>>>        - ``quantization``
>>>        - Quantization range, from enum :c:type:`v4l2_quantization`.
>>> -        This information supplements the ``colorspace`` and must be set by
>>> -	the driver for capture streams and by the application for output
>>> -	streams, see :ref:`colorspaces`.
>>> +	See struct :c:type:`v4l2_pix_format`.
>>>      * - __u8
>>>        - ``xfer_func``
>>>        - Transfer function, from enum :c:type:`v4l2_xfer_func`.
>>> -        This information supplements the ``colorspace`` and must be set by
>>> -	the driver for capture streams and by the application for output
>>> -	streams, see :ref:`colorspaces`.
>>> +	See struct :c:type:`v4l2_pix_format`.
>>>      * - __u8
>>>        - ``reserved[7]``
>>>        - Reserved for future extensions. Should be zeroed by drivers and
>>> diff --git a/Documentation/media/uapi/v4l/pixfmt-v4l2.rst b/Documentation/media/uapi/v4l/pixfmt-v4l2.rst
>>> index 826f2305da01..932b6a546e61 100644
>>> --- a/Documentation/media/uapi/v4l/pixfmt-v4l2.rst
>>> +++ b/Documentation/media/uapi/v4l/pixfmt-v4l2.rst
>>> @@ -88,7 +88,12 @@ Single-planar format structure
>>>        - Image colorspace, from enum :c:type:`v4l2_colorspace`.
>>>          This information supplements the ``pixelformat`` and must be set
>>>  	by the driver for capture streams and by the application for
>>> -	output streams, see :ref:`colorspaces`.
>>> +	output streams, see :ref:`colorspaces`. If the application sets the
>>> +	flag ``V4L2_PIX_FMT_FLAG_CSC_COLORSPACE`` then the application can set
>>> +	this field for a capture stream to request a specific colorspace for
>>> +	the captured image data. The driver will attempt to do colorspace
>>> +	conversion to the specified colorspace or return the colorspace it will
>>> +	use if it can't do the conversion.
>>>      * - __u32
>>>        - ``priv``
>>>        - This field indicates whether the remaining fields of the
>>> @@ -126,13 +131,25 @@ Single-planar format structure
>>>        - Y'CbCr encoding, from enum :c:type:`v4l2_ycbcr_encoding`.
>>>          This information supplements the ``colorspace`` and must be set by
>>>  	the driver for capture streams and by the application for output
>>> -	streams, see :ref:`colorspaces`.
>>> +	streams, see :ref:`colorspaces`. If the application sets the
>>> +	flag ``V4L2_PIX_FMT_FLAG_CSC_YCBCR_ENC`` then the application can set
>>> +	this field for a capture stream to request a specific Y'CbCr encoding
>>> +	for the captured image data. The driver will attempt to do the
>>> +	conversion to the specified Y'CbCr encoding or return the encoding it
>>> +	will use if it can't do the conversion. This field is ignored for
>>> +	non-Y'CbCr pixelformats.
>>>      * - __u32
>>>        - ``hsv_enc``
>>>        - HSV encoding, from enum :c:type:`v4l2_hsv_encoding`.
>>>          This information supplements the ``colorspace`` and must be set by
>>>  	the driver for capture streams and by the application for output
>>> -	streams, see :ref:`colorspaces`.
>>> +	streams, see :ref:`colorspaces`. If the application sets the flag
>>> +	``V4L2_PIX_FMT_FLAG_CSC_HSV_ENC`` then the application can set this
>>> +	field for a capture stream to request a specific HSV encoding for the
>>> +	captured image data. The driver will attempt to do the conversion to
>>> +	the specified HSV encoding or return the encoding it will use if it
>>> +	can't do the conversion. This field is ignored for non-HSV
>>> +	pixelformats.
>>>      * - }
>>>        -
>>>        -
>>> @@ -141,10 +158,20 @@ Single-planar format structure
>>>        - Quantization range, from enum :c:type:`v4l2_quantization`.
>>>          This information supplements the ``colorspace`` and must be set by
>>>  	the driver for capture streams and by the application for output
>>> -	streams, see :ref:`colorspaces`.
>>> +	streams, see :ref:`colorspaces`. If the application sets the flag
>>> +	``V4L2_PIX_FMT_FLAG_CSC_QUANTIZATION`` then the application can set
>>> +	this field for a capture stream to request a specific quantization
>>> +	range for the captured image data. The driver will attempt to do the
>>> +	conversion to the specified quantization range or return the
>>> +	quantization it will use if it can't do the conversion.
>>>      * - __u32
>>>        - ``xfer_func``
>>>        - Transfer function, from enum :c:type:`v4l2_xfer_func`.
>>>          This information supplements the ``colorspace`` and must be set by
>>>  	the driver for capture streams and by the application for output
>>> -	streams, see :ref:`colorspaces`.
>>> +	streams, see :ref:`colorspaces`. If the application sets the flag
>>> +	``V4L2_PIX_FMT_FLAG_CSC_XFER_FUNC`` then the application can set
>>> +	this field for a capture stream to request a specific transfer function
>>> +	for the captured image data. The driver will attempt to do the
>>> +	conversion to the specified transfer function or return the transfer
>>> +	function it will use if it can't do the conversion.
>>> diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c
>>> index 54afc9c7ee6e..39def068f13e 100644
>>> --- a/drivers/media/v4l2-core/v4l2-ioctl.c
>>> +++ b/drivers/media/v4l2-core/v4l2-ioctl.c
>>> @@ -1019,6 +1019,18 @@ static void v4l_sanitize_format(struct v4l2_format *fmt)
>>>  	 * isn't used by applications.
>>>  	 */
>>>  
>>> +	if (fmt->type == V4L2_BUF_TYPE_VIDEO_CAPTURE ||
>>> +	    fmt->type == V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE) {
>>> +		if (!(fmt->fmt.pix.flags & V4L2_PIX_FMT_FLAG_CSC_COLORSPACE))
>>> +			fmt->fmt.pix.colorspace = V4L2_COLORSPACE_DEFAULT;
>>> +		if (!(fmt->fmt.pix.flags & V4L2_PIX_FMT_FLAG_CSC_YCBCR_ENC))
>>> +			fmt->fmt.pix.ycbcr_enc = V4L2_YCBCR_ENC_DEFAULT;
>>> +		if (!(fmt->fmt.pix.flags & V4L2_PIX_FMT_FLAG_CSC_QUANTIZATION))
>>> +			fmt->fmt.pix.quantization = V4L2_QUANTIZATION_DEFAULT;
>>> +		if (!(fmt->fmt.pix.flags & V4L2_PIX_FMT_FLAG_CSC_XFER_FUNC))
>>> +			fmt->fmt.pix.xfer_func = V4L2_XFER_FUNC_DEFAULT;
>>> +	}
>>> +
>>>  	if (fmt->type != V4L2_BUF_TYPE_VIDEO_CAPTURE &&
>>>  	    fmt->type != V4L2_BUF_TYPE_VIDEO_OUTPUT)
>>>  		return;
>>> diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
>>> index 622f0479d668..4cbc8f23b828 100644
>>> --- a/include/uapi/linux/videodev2.h
>>> +++ b/include/uapi/linux/videodev2.h
>>> @@ -709,6 +709,11 @@ struct v4l2_pix_format {
>>>  
>>>  /* Flags */
>>>  #define V4L2_PIX_FMT_FLAG_PREMUL_ALPHA	0x00000001
>>> +#define V4L2_PIX_FMT_FLAG_CSC_COLORSPACE	0x00000002
>>> +#define V4L2_PIX_FMT_FLAG_CSC_YCBCR_ENC		0x00000004
>>> +#define V4L2_PIX_FMT_FLAG_CSC_HSV_ENC		0x00000004
>>> +#define V4L2_PIX_FMT_FLAG_CSC_QUANTIZATION	0x00000008
>>> +#define V4L2_PIX_FMT_FLAG_CSC_XFER_FUNC		0x00000010
>>>  
>>>  /*
>>>   *	F O R M A T   E N U M E R A T I O N
>>>   
>>
> 
> 
> 
> Thanks,
> Mauro
> 


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: Re: [PATCH v2] [RFC v2] v4l2: add support for colorspace conversion for video capture
  2018-09-06 10:54     ` Hans Verkuil
  2018-09-06 12:59       ` Philipp Zabel
@ 2020-04-10 10:26       ` Dafna Hirschfeld
  2020-04-15 10:33         ` Hans Verkuil
  1 sibling, 1 reply; 9+ messages in thread
From: Dafna Hirschfeld @ 2020-04-10 10:26 UTC (permalink / raw)
  To: Hans Verkuil, Philipp Zabel, Hans Verkuil, linux-media
  Cc: Helen Koike, Ezequiel Garcia, Laurent Pinchart

Hi, I am working on sending a v3 of this RFC,
This RFC is needed for the rkisp1 driver. Currently the driver set the
quantization range to full range only instead of the default limited range for
YUV. With this RFC the driver could set the quantization to limited
by default and also allow userspace the change to full range.

I have some inline comments.

On 9/6/18 12:54 PM, Hans Verkuil wrote:
> On 09/06/18 11:50, Philipp Zabel wrote:
>> On Thu, 2018-09-06 at 11:02 +0200, Hans Verkuil wrote:
>>> Hi Philipp,
>>>
>>> It is much appreciated that this old RFC of mine
>>
>> Right, I should have made clearer that this is just a rework of Hans'
>> original RFC in [1].
>>
>> [1] https://patchwork.linuxtv.org/patch/28847/
>>
>>> is picked up again. I always wanted to get this in, but I never had a
>>> driver where it would make sense to do so.
>>
>> I'll test this with i.MX PXP and IPU mem2mem drivers and follow up with
>> per-driver patches to enable this feature once we know where this should
>> be going.
>>
>>> On 09/05/2018 07:09 PM, Philipp Zabel wrote:
>>>> For video capture it is the driver that reports the colorspace,
>>>
>>> add: "transfer function,"
>>
>> Will do.
>>
>>>> Y'CbCr/HSV encoding and quantization range used by the video, and there
>>>> is no way to request something different, even though many HDTV
>>>> receivers have some sort of colorspace conversion capabilities.
>>>>
>>>> For output video this feature already exists since the application
>>>> specifies this information for the video format it will send out, and
>>>> the transmitter will enable any available CSC if a format conversion has
>>>> to be performed in order to match the capabilities of the sink.
>>>>
>>>> For video capture we propose adding new pix_format flags:
>>>> V4L2_PIX_FMT_FLAG_CSC_COLORSPACE, V4L2_PIX_FMT_FLAG_CSC_YCBCR_ENC,
>>>> V4L2_PIX_FMT_FLAG_CSC_HSV_ENC, V4L2_PIX_FMT_FLAG_CSC_QUANTIZATION, and
>>>> V4L2_PIX_FMT_FLAG_CSC_XFER_FUNC. These are set by the driver to indicate
>>>> its conversion features. When set by the application, the driver will
>>>> interpret the colorspace, ycbcr_enc/hsv_enc, quantization and xfer_func
>>>> fields as the requested colorspace information and will attempt to do
>>>> the conversion it supports.
>>>>
>>>> Drivers do not have to actually look at the flags: if the flags are not
>>>> set, then the colorspace, ycbcr_enc and quantization fields are set to
>>>> the default values by the core, i.e. just pass on the received format
>>>> without conversion.
>>>
>>> Thinking about this some more, I don't think this is quite the right approach.
>>> Having userspace set these flags with S_FMT if they want to do explicit
>>> conversions makes sense, and that part we can keep.
>>>
>>> But to signal the capabilities I think should be done via new flags for
>>> VIDIOC_ENUM_FMT. Basically the same set of flags, but for the flags field
>>> of struct v4l2_fmtdesc.
>>
>> In that case, I think the V4L2_PIX_FMT_FLAG_CSC_* should be purely a
>> signal from the application to the driver, and the driver should not
>> (have to) touch them at all.
> 
> Right. The code in v4l2-ioctl.c that checks these flags and replaces
> the corresponding field with 0 (DEFAULT) would be sufficient.
> 
> Drivers can just check the fields for non-default values.> 
>> An equivalent set of v4l2_fmtdesc flags could be used to signal
>> conversion support via VIDIOC_ENUM_FMT:
>>
>> #define V4L2_FMT_FLAG_CSC_COLORSPACE	0x0004
>> #define V4L2_FMT_FLAG_CSC_YCBCR_ENC	0x0008
>> #define V4L2_FMT_FLAG_CSC_HSV_ENC	0x0008
>> #define V4L2_FMT_FLAG_CSC_QUANTIZATION	0x0010
>> #define V4L2_FMT_FLAG_CSC_XFER_FUNC	0x0020
>>
>> What is the expected use case for these reported flags? Applications
>> that see them set to zero can skip enumerating capture side colorimetry.
>> Is there anything else?
> 
> That's about it. It just signals if the HW is capable of doing such
> conversions.
In the enumeration, are the flags allowed to change according to the format?
For example in rkisp1 we want to set the V4L2_FMT_FLAG_CSC_QUANTIZATION
only for yuv formats, and set all flags to 0 for RGB and BAYER formats.

> 
>>> One thing that's not clear to me is what happens if userspace sets one or
>>> more flags and calls S_FMT for a driver that doesn't support this. Are the
>>> flags zeroed in that case upon return?
>>
>> I'd say no. Drivers are free to silently ignore the flag.
>> The effect is the same as if the driver supports the flag in principle,
>> but has to change a requested value anyway because of some limitation.
>> The application can check whether the driver changed its requested
>> colorspace, xfer_func, ycbcr_enc, or quantization.
>>
>> The application usually doesn't need to know whether the driver changed
>> the requested ycbcr_enc because it doesn't have CSC matrix support at
>> all, or because it doesn't implement a specific conversion. And if the
>> application needs to know for some reason, it can always check
>> VIDIOC_ENUM_FMT.
>>
>>> I don't think so, but I think that
>>> is already true for the existing flag V4L2_PIX_FMT_FLAG_PREMUL_ALPHA.
>>
>> The only drivers using V4L2_PIX_FMT_FLAG_PREMUL_ALPHA I can see are
>> vsp1_brx and vsp1_rpf. They never write to the v4l2_pix_format flags
>> field.
> 
> But they honor it. The problem is that I can set this flag and call S_FMT
> on e.g. the vivid driver, and S_FMT will return the flag. But it actually
> doesn't use the flag at all, so userspace has no way of knowing if the
> flag is actually used. Although it can call G_FMT and then the flag is
> cleared.
Is it a bug if usespace receive different values for G_FMT and S_FMT like in
this vivid scenario you describe?
the docs says:
"Finally the VIDIOC_S_FMT ioctl returns the current format parameters as VIDIOC_G_FMT does"

> 
>>
>>> I wonder if V4L2_PIX_FMT_FLAG_PREMUL_ALPHA should also get an equivalent
>>> flag for v4l2_fmtdesc.
>>
>> Isn't this useless to introduce after the fact, if there are already
>> applications that use this feature? They can't depend on the existence
>> of this flag to check for support anyway.
> 
> Those applications are already hardcoded for the vsp1. So they won't break
> by adding v4l2_fmtdesc flags.
> 
> But apps like gstreamer and friends can start using these flags and deduce
> what the HW is capable of.
> 
>>
>>> Then we can just document that v4l2_format flags are only valid if they
>>> are also defined in v4l2_fmtdesc.
>>>
>>> Does anyone have better ideas for this?
>>
>> I'd just say the driver is free to ignore the flag if it doesn't support
>> the specific requested value and leave it at that.
> 
> It's probably the best option.
> 
> Regards,
> 
> 	Hans
> 

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v2] [RFC v2] v4l2: add support for colorspace conversion for video capture
  2020-04-10 10:26       ` Dafna Hirschfeld
@ 2020-04-15 10:33         ` Hans Verkuil
  0 siblings, 0 replies; 9+ messages in thread
From: Hans Verkuil @ 2020-04-15 10:33 UTC (permalink / raw)
  To: Dafna Hirschfeld, Hans Verkuil, Philipp Zabel, linux-media
  Cc: Helen Koike, Ezequiel Garcia, Laurent Pinchart

On 10/04/2020 12:26, Dafna Hirschfeld wrote:
> Hi, I am working on sending a v3 of this RFC,
> This RFC is needed for the rkisp1 driver. Currently the driver set the
> quantization range to full range only instead of the default limited range for
> YUV. With this RFC the driver could set the quantization to limited
> by default and also allow userspace the change to full range.
> 
> I have some inline comments.
> 
> On 9/6/18 12:54 PM, Hans Verkuil wrote:
>> On 09/06/18 11:50, Philipp Zabel wrote:
>>> On Thu, 2018-09-06 at 11:02 +0200, Hans Verkuil wrote:
>>>> Hi Philipp,
>>>>
>>>> It is much appreciated that this old RFC of mine
>>>
>>> Right, I should have made clearer that this is just a rework of Hans'
>>> original RFC in [1].
>>>
>>> [1] https://patchwork.linuxtv.org/patch/28847/
>>>
>>>> is picked up again. I always wanted to get this in, but I never had a
>>>> driver where it would make sense to do so.
>>>
>>> I'll test this with i.MX PXP and IPU mem2mem drivers and follow up with
>>> per-driver patches to enable this feature once we know where this should
>>> be going.
>>>
>>>> On 09/05/2018 07:09 PM, Philipp Zabel wrote:
>>>>> For video capture it is the driver that reports the colorspace,
>>>>
>>>> add: "transfer function,"
>>>
>>> Will do.
>>>
>>>>> Y'CbCr/HSV encoding and quantization range used by the video, and there
>>>>> is no way to request something different, even though many HDTV
>>>>> receivers have some sort of colorspace conversion capabilities.
>>>>>
>>>>> For output video this feature already exists since the application
>>>>> specifies this information for the video format it will send out, and
>>>>> the transmitter will enable any available CSC if a format conversion has
>>>>> to be performed in order to match the capabilities of the sink.
>>>>>
>>>>> For video capture we propose adding new pix_format flags:
>>>>> V4L2_PIX_FMT_FLAG_CSC_COLORSPACE, V4L2_PIX_FMT_FLAG_CSC_YCBCR_ENC,
>>>>> V4L2_PIX_FMT_FLAG_CSC_HSV_ENC, V4L2_PIX_FMT_FLAG_CSC_QUANTIZATION, and
>>>>> V4L2_PIX_FMT_FLAG_CSC_XFER_FUNC. These are set by the driver to indicate
>>>>> its conversion features. When set by the application, the driver will
>>>>> interpret the colorspace, ycbcr_enc/hsv_enc, quantization and xfer_func
>>>>> fields as the requested colorspace information and will attempt to do
>>>>> the conversion it supports.

Instead of having all these flags, I think it would be better to return to
the original patch (https://patchwork.linuxtv.org/patch/28847/) and have just
one flag: V4L2_PIX_FMT_FLAG_REQUEST_CSC.

Userspace sets this flag and then fills in colorspace, ycbcr_enc, quantization
and xfer_func with the desired values (leave to 0 if it should remain unchanged).

>>>>>
>>>>> Drivers do not have to actually look at the flags: if the flags are not
>>>>> set, then the colorspace, ycbcr_enc and quantization fields are set to
>>>>> the default values by the core, i.e. just pass on the received format
>>>>> without conversion.
>>>>
>>>> Thinking about this some more, I don't think this is quite the right approach.
>>>> Having userspace set these flags with S_FMT if they want to do explicit
>>>> conversions makes sense, and that part we can keep.
>>>>
>>>> But to signal the capabilities I think should be done via new flags for
>>>> VIDIOC_ENUM_FMT. Basically the same set of flags, but for the flags field
>>>> of struct v4l2_fmtdesc.
>>>
>>> In that case, I think the V4L2_PIX_FMT_FLAG_CSC_* should be purely a
>>> signal from the application to the driver, and the driver should not
>>> (have to) touch them at all.
>>
>> Right. The code in v4l2-ioctl.c that checks these flags and replaces
>> the corresponding field with 0 (DEFAULT) would be sufficient.
>>
>> Drivers can just check the fields for non-default values.> 
>>> An equivalent set of v4l2_fmtdesc flags could be used to signal
>>> conversion support via VIDIOC_ENUM_FMT:
>>>
>>> #define V4L2_FMT_FLAG_CSC_COLORSPACE	0x0004
>>> #define V4L2_FMT_FLAG_CSC_YCBCR_ENC	0x0008
>>> #define V4L2_FMT_FLAG_CSC_HSV_ENC	0x0008
>>> #define V4L2_FMT_FLAG_CSC_QUANTIZATION	0x0010
>>> #define V4L2_FMT_FLAG_CSC_XFER_FUNC	0x0020
>>>
>>> What is the expected use case for these reported flags? Applications
>>> that see them set to zero can skip enumerating capture side colorimetry.
>>> Is there anything else?
>>
>> That's about it. It just signals if the HW is capable of doing such
>> conversions.
> In the enumeration, are the flags allowed to change according to the format?
> For example in rkisp1 we want to set the V4L2_FMT_FLAG_CSC_QUANTIZATION
> only for yuv formats, and set all flags to 0 for RGB and BAYER formats.

Yes, they can change according to the format. E.g. V4L2_FMT_FLAG_CSC_YCBCR_ENC
is meaningless for non-YUV formats.

The purpose of these flags is to indicate what the hardware can do.

I think we can drop V4L2_FMT_FLAG_CSC_COLORSPACE and V4L2_FMT_FLAG_CSC_XFER_FUNC:
there are no drivers in mainline that can do this AFAIK. They can always be
added later when needed.

> 
>>
>>>> One thing that's not clear to me is what happens if userspace sets one or
>>>> more flags and calls S_FMT for a driver that doesn't support this. Are the
>>>> flags zeroed in that case upon return?
>>>
>>> I'd say no. Drivers are free to silently ignore the flag.
>>> The effect is the same as if the driver supports the flag in principle,
>>> but has to change a requested value anyway because of some limitation.
>>> The application can check whether the driver changed its requested
>>> colorspace, xfer_func, ycbcr_enc, or quantization.
>>>
>>> The application usually doesn't need to know whether the driver changed
>>> the requested ycbcr_enc because it doesn't have CSC matrix support at
>>> all, or because it doesn't implement a specific conversion. And if the
>>> application needs to know for some reason, it can always check
>>> VIDIOC_ENUM_FMT.
>>>
>>>> I don't think so, but I think that
>>>> is already true for the existing flag V4L2_PIX_FMT_FLAG_PREMUL_ALPHA.
>>>
>>> The only drivers using V4L2_PIX_FMT_FLAG_PREMUL_ALPHA I can see are
>>> vsp1_brx and vsp1_rpf. They never write to the v4l2_pix_format flags
>>> field.

I think that a separate V4L2_FMT_FLAG_PREMUL_ALPHA would be desirable,
but that should be done in a separate patch.

>>
>> But they honor it. The problem is that I can set this flag and call S_FMT
>> on e.g. the vivid driver, and S_FMT will return the flag. But it actually
>> doesn't use the flag at all, so userspace has no way of knowing if the
>> flag is actually used. Although it can call G_FMT and then the flag is
>> cleared.
> Is it a bug if usespace receive different values for G_FMT and S_FMT like in
> this vivid scenario you describe?
> the docs says:
> "Finally the VIDIOC_S_FMT ioctl returns the current format parameters as VIDIOC_G_FMT does"

Hmm, that's perhaps a bit ambiguous. What is meant is: "The VIDIOC_S_FMT returns
the updated format parameters."

Regards,

	Hans

> 
>>
>>>
>>>> I wonder if V4L2_PIX_FMT_FLAG_PREMUL_ALPHA should also get an equivalent
>>>> flag for v4l2_fmtdesc.
>>>
>>> Isn't this useless to introduce after the fact, if there are already
>>> applications that use this feature? They can't depend on the existence
>>> of this flag to check for support anyway.
>>
>> Those applications are already hardcoded for the vsp1. So they won't break
>> by adding v4l2_fmtdesc flags.
>>
>> But apps like gstreamer and friends can start using these flags and deduce
>> what the HW is capable of.
>>
>>>
>>>> Then we can just document that v4l2_format flags are only valid if they
>>>> are also defined in v4l2_fmtdesc.
>>>>
>>>> Does anyone have better ideas for this?
>>>
>>> I'd just say the driver is free to ignore the flag if it doesn't support
>>> the specific requested value and leave it at that.
>>
>> It's probably the best option.
>>
>> Regards,
>>
>> 	Hans
>>


^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2020-04-15 10:33 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-09-05 17:09 [PATCH v2] [RFC v2] v4l2: add support for colorspace conversion for video capture Philipp Zabel
2018-09-06  9:02 ` Hans Verkuil
2018-09-06  9:50   ` Philipp Zabel
2018-09-06 10:54     ` Hans Verkuil
2018-09-06 12:59       ` Philipp Zabel
2020-04-10 10:26       ` Dafna Hirschfeld
2020-04-15 10:33         ` Hans Verkuil
2018-12-07 14:30   ` Mauro Carvalho Chehab
2018-12-07 14:38     ` Hans Verkuil

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.