[PATCH v4 5/6] media: Add controls for JPEG quantization tables

[PATCH v4 5/6] media: Add controls for JPEG quantization tables

[Ezequiel Garcia]

From: Shunqian Zheng <zhengsq-TNX95d0MmH7DzftRWevZcw@public.gmane.org>

Add V4L2_CID_JPEG_QUANTIZATION compound control to allow userspace
configure the JPEG quantization tables.

Signed-off-by: Shunqian Zheng <zhengsq-TNX95d0MmH7DzftRWevZcw@public.gmane.org>
Signed-off-by: Ezequiel Garcia <ezequiel-ZGY8ohtN/8qB+jHODAdFcQ@public.gmane.org>
---
 .../media/uapi/v4l/extended-controls.rst      | 23 +++++++++++++++++++
 .../media/videodev2.h.rst.exceptions          |  1 +
 drivers/media/v4l2-core/v4l2-ctrls.c          | 10 ++++++++
 include/uapi/linux/v4l2-controls.h            |  5 ++++
 include/uapi/linux/videodev2.h                |  1 +
 5 files changed, 40 insertions(+)

diff --git a/Documentation/media/uapi/v4l/extended-controls.rst b/Documentation/media/uapi/v4l/extended-controls.rst
index 9f7312bf3365..e0dd03e452de 100644
--- a/Documentation/media/uapi/v4l/extended-controls.rst
+++ b/Documentation/media/uapi/v4l/extended-controls.rst
@@ -3354,7 +3354,30 @@ JPEG Control IDs
     Specify which JPEG markers are included in compressed stream. This
     control is valid only for encoders.
 
+.. _jpeg-quant-tables-control:
 
+``V4L2_CID_JPEG_QUANTIZATION (struct)``
+    Specifies the luma and chroma quantization matrices for encoding
+    or decoding a V4L2_PIX_FMT_JPEG_RAW format buffer. The two matrices
+    must be set in JPEG zigzag order, as per the JPEG specification.
+
+
+.. c:type:: struct v4l2_ctrl_jpeg_quantization
+
+.. cssclass:: longtable
+
+.. flat-table:: struct v4l2_ctrl_jpeg_quantization
+    :header-rows:  0
+    :stub-columns: 0
+    :widths:       1 1 2
+
+    * - __u8
+      - ``luma_quantization_matrix[64]``
+      - Sets the luma quantization table.
+
+    * - __u8
+      - ``chroma_quantization_matrix[64]``
+      - Sets the chroma quantization table.
 
 .. flat-table::
     :header-rows:  0
diff --git a/Documentation/media/videodev2.h.rst.exceptions b/Documentation/media/videodev2.h.rst.exceptions
index ca9f0edc579e..a0a38e92bf38 100644
--- a/Documentation/media/videodev2.h.rst.exceptions
+++ b/Documentation/media/videodev2.h.rst.exceptions
@@ -129,6 +129,7 @@ replace symbol V4L2_CTRL_TYPE_STRING :c:type:`v4l2_ctrl_type`
 replace symbol V4L2_CTRL_TYPE_U16 :c:type:`v4l2_ctrl_type`
 replace symbol V4L2_CTRL_TYPE_U32 :c:type:`v4l2_ctrl_type`
 replace symbol V4L2_CTRL_TYPE_U8 :c:type:`v4l2_ctrl_type`
+replace symbol V4L2_CTRL_TYPE_JPEG_QUANTIZATION :c:type:`v4l2_ctrl_type`
 
 # V4L2 capability defines
 replace define V4L2_CAP_VIDEO_CAPTURE device-capabilities
diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c
index 599c1cbff3b9..305bd7a9b7f1 100644
--- a/drivers/media/v4l2-core/v4l2-ctrls.c
+++ b/drivers/media/v4l2-core/v4l2-ctrls.c
@@ -999,6 +999,7 @@ const char *v4l2_ctrl_get_name(u32 id)
 	case V4L2_CID_JPEG_RESTART_INTERVAL:	return "Restart Interval";
 	case V4L2_CID_JPEG_COMPRESSION_QUALITY:	return "Compression Quality";
 	case V4L2_CID_JPEG_ACTIVE_MARKER:	return "Active Markers";
+	case V4L2_CID_JPEG_QUANTIZATION:	return "JPEG Quantization Tables";
 
 	/* Image source controls */
 	/* Keep the order of the 'case's the same as in v4l2-controls.h! */
@@ -1286,6 +1287,9 @@ void v4l2_ctrl_fill(u32 id, const char **name, enum v4l2_ctrl_type *type,
 	case V4L2_CID_DETECT_MD_REGION_GRID:
 		*type = V4L2_CTRL_TYPE_U8;
 		break;
+	case V4L2_CID_JPEG_QUANTIZATION:
+		*type = V4L2_CTRL_TYPE_JPEG_QUANTIZATION;
+		break;
 	case V4L2_CID_DETECT_MD_THRESHOLD_GRID:
 		*type = V4L2_CTRL_TYPE_U16;
 		break;
@@ -1612,6 +1616,9 @@ static int std_validate(const struct v4l2_ctrl *ctrl, u32 idx,
 			return -ERANGE;
 		return 0;
 
+	case V4L2_CTRL_TYPE_JPEG_QUANTIZATION:
+		return 0;
+
 	default:
 		return -EINVAL;
 	}
@@ -2133,6 +2140,9 @@ static struct v4l2_ctrl *v4l2_ctrl_new(struct v4l2_ctrl_handler *hdl,
 	case V4L2_CTRL_TYPE_U32:
 		elem_size = sizeof(u32);
 		break;
+	case V4L2_CTRL_TYPE_JPEG_QUANTIZATION:
+		elem_size = sizeof(struct v4l2_ctrl_jpeg_quantization);
+		break;
 	default:
 		if (type < V4L2_CTRL_COMPOUND_TYPES)
 			elem_size = sizeof(s32);
diff --git a/include/uapi/linux/v4l2-controls.h b/include/uapi/linux/v4l2-controls.h
index e4ee10ee917d..fcb288bb05c7 100644
--- a/include/uapi/linux/v4l2-controls.h
+++ b/include/uapi/linux/v4l2-controls.h
@@ -987,6 +987,11 @@ enum v4l2_jpeg_chroma_subsampling {
 #define	V4L2_JPEG_ACTIVE_MARKER_DQT		(1 << 17)
 #define	V4L2_JPEG_ACTIVE_MARKER_DHT		(1 << 18)
 
+#define V4L2_CID_JPEG_QUANTIZATION		(V4L2_CID_JPEG_CLASS_BASE + 5)
+struct v4l2_ctrl_jpeg_quantization {
+	__u8	luma_quantization_matrix[64];
+	__u8	chroma_quantization_matrix[64];
+};
 
 /* Image source controls */
 #define V4L2_CID_IMAGE_SOURCE_CLASS_BASE	(V4L2_CTRL_CLASS_IMAGE_SOURCE | 0x900)
diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
index f271048c89c4..e998d07464cb 100644
--- a/include/uapi/linux/videodev2.h
+++ b/include/uapi/linux/videodev2.h
@@ -1630,6 +1630,7 @@ enum v4l2_ctrl_type {
 	V4L2_CTRL_TYPE_U8	     = 0x0100,
 	V4L2_CTRL_TYPE_U16	     = 0x0101,
 	V4L2_CTRL_TYPE_U32	     = 0x0102,
+	V4L2_CTRL_TYPE_JPEG_QUANTIZATION = 0x0103,
 };
 
 /*  Used in the VIDIOC_QUERYCTRL ioctl for querying controls */
-- 
2.18.0

Re: [PATCH v4 5/6] media: Add controls for JPEG quantization tables

[Hans Verkuil]

On 08/31/2018 05:52 PM, Ezequiel Garcia wrote:
> From: Shunqian Zheng <zhengsq-TNX95d0MmH7DzftRWevZcw@public.gmane.org>
> 
> Add V4L2_CID_JPEG_QUANTIZATION compound control to allow userspace
> configure the JPEG quantization tables.
> 
> Signed-off-by: Shunqian Zheng <zhengsq-TNX95d0MmH7DzftRWevZcw@public.gmane.org>
> Signed-off-by: Ezequiel Garcia <ezequiel-ZGY8ohtN/8qB+jHODAdFcQ@public.gmane.org>
> ---
>  .../media/uapi/v4l/extended-controls.rst      | 23 +++++++++++++++++++
>  .../media/videodev2.h.rst.exceptions          |  1 +
>  drivers/media/v4l2-core/v4l2-ctrls.c          | 10 ++++++++
>  include/uapi/linux/v4l2-controls.h            |  5 ++++
>  include/uapi/linux/videodev2.h                |  1 +
>  5 files changed, 40 insertions(+)
> 
> diff --git a/Documentation/media/uapi/v4l/extended-controls.rst b/Documentation/media/uapi/v4l/extended-controls.rst
> index 9f7312bf3365..e0dd03e452de 100644
> --- a/Documentation/media/uapi/v4l/extended-controls.rst
> +++ b/Documentation/media/uapi/v4l/extended-controls.rst
> @@ -3354,7 +3354,30 @@ JPEG Control IDs
>      Specify which JPEG markers are included in compressed stream. This
>      control is valid only for encoders.
>  
> +.. _jpeg-quant-tables-control:
>  
> +``V4L2_CID_JPEG_QUANTIZATION (struct)``
> +    Specifies the luma and chroma quantization matrices for encoding
> +    or decoding a V4L2_PIX_FMT_JPEG_RAW format buffer. The two matrices
> +    must be set in JPEG zigzag order, as per the JPEG specification.

Can you change "JPEG specification" to a reference to the JPEG spec entry
in bibio.rst?

> +
> +
> +.. c:type:: struct v4l2_ctrl_jpeg_quantization
> +
> +.. cssclass:: longtable
> +
> +.. flat-table:: struct v4l2_ctrl_jpeg_quantization
> +    :header-rows:  0
> +    :stub-columns: 0
> +    :widths:       1 1 2
> +
> +    * - __u8
> +      - ``luma_quantization_matrix[64]``
> +      - Sets the luma quantization table.
> +
> +    * - __u8
> +      - ``chroma_quantization_matrix[64]``
> +      - Sets the chroma quantization table.

Just checking: the JPEG standard specifies this as unsigned 8-bit values as well?

>  
>  .. flat-table::
>      :header-rows:  0
> diff --git a/Documentation/media/videodev2.h.rst.exceptions b/Documentation/media/videodev2.h.rst.exceptions
> index ca9f0edc579e..a0a38e92bf38 100644
> --- a/Documentation/media/videodev2.h.rst.exceptions
> +++ b/Documentation/media/videodev2.h.rst.exceptions
> @@ -129,6 +129,7 @@ replace symbol V4L2_CTRL_TYPE_STRING :c:type:`v4l2_ctrl_type`
>  replace symbol V4L2_CTRL_TYPE_U16 :c:type:`v4l2_ctrl_type`
>  replace symbol V4L2_CTRL_TYPE_U32 :c:type:`v4l2_ctrl_type`
>  replace symbol V4L2_CTRL_TYPE_U8 :c:type:`v4l2_ctrl_type`
> +replace symbol V4L2_CTRL_TYPE_JPEG_QUANTIZATION :c:type:`v4l2_ctrl_type`
>  
>  # V4L2 capability defines
>  replace define V4L2_CAP_VIDEO_CAPTURE device-capabilities
> diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c
> index 599c1cbff3b9..305bd7a9b7f1 100644
> --- a/drivers/media/v4l2-core/v4l2-ctrls.c
> +++ b/drivers/media/v4l2-core/v4l2-ctrls.c
> @@ -999,6 +999,7 @@ const char *v4l2_ctrl_get_name(u32 id)
>  	case V4L2_CID_JPEG_RESTART_INTERVAL:	return "Restart Interval";
>  	case V4L2_CID_JPEG_COMPRESSION_QUALITY:	return "Compression Quality";
>  	case V4L2_CID_JPEG_ACTIVE_MARKER:	return "Active Markers";
> +	case V4L2_CID_JPEG_QUANTIZATION:	return "JPEG Quantization Tables";
>  
>  	/* Image source controls */
>  	/* Keep the order of the 'case's the same as in v4l2-controls.h! */
> @@ -1286,6 +1287,9 @@ void v4l2_ctrl_fill(u32 id, const char **name, enum v4l2_ctrl_type *type,
>  	case V4L2_CID_DETECT_MD_REGION_GRID:
>  		*type = V4L2_CTRL_TYPE_U8;
>  		break;
> +	case V4L2_CID_JPEG_QUANTIZATION:
> +		*type = V4L2_CTRL_TYPE_JPEG_QUANTIZATION;
> +		break;
>  	case V4L2_CID_DETECT_MD_THRESHOLD_GRID:
>  		*type = V4L2_CTRL_TYPE_U16;
>  		break;
> @@ -1612,6 +1616,9 @@ static int std_validate(const struct v4l2_ctrl *ctrl, u32 idx,
>  			return -ERANGE;
>  		return 0;
>  
> +	case V4L2_CTRL_TYPE_JPEG_QUANTIZATION:
> +		return 0;
> +
>  	default:
>  		return -EINVAL;
>  	}
> @@ -2133,6 +2140,9 @@ static struct v4l2_ctrl *v4l2_ctrl_new(struct v4l2_ctrl_handler *hdl,
>  	case V4L2_CTRL_TYPE_U32:
>  		elem_size = sizeof(u32);
>  		break;
> +	case V4L2_CTRL_TYPE_JPEG_QUANTIZATION:
> +		elem_size = sizeof(struct v4l2_ctrl_jpeg_quantization);
> +		break;
>  	default:
>  		if (type < V4L2_CTRL_COMPOUND_TYPES)
>  			elem_size = sizeof(s32);
> diff --git a/include/uapi/linux/v4l2-controls.h b/include/uapi/linux/v4l2-controls.h
> index e4ee10ee917d..fcb288bb05c7 100644
> --- a/include/uapi/linux/v4l2-controls.h
> +++ b/include/uapi/linux/v4l2-controls.h
> @@ -987,6 +987,11 @@ enum v4l2_jpeg_chroma_subsampling {
>  #define	V4L2_JPEG_ACTIVE_MARKER_DQT		(1 << 17)
>  #define	V4L2_JPEG_ACTIVE_MARKER_DHT		(1 << 18)
>  
> +#define V4L2_CID_JPEG_QUANTIZATION		(V4L2_CID_JPEG_CLASS_BASE + 5)
> +struct v4l2_ctrl_jpeg_quantization {
> +	__u8	luma_quantization_matrix[64];
> +	__u8	chroma_quantization_matrix[64];
> +};
>  
>  /* Image source controls */
>  #define V4L2_CID_IMAGE_SOURCE_CLASS_BASE	(V4L2_CTRL_CLASS_IMAGE_SOURCE | 0x900)
> diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
> index f271048c89c4..e998d07464cb 100644
> --- a/include/uapi/linux/videodev2.h
> +++ b/include/uapi/linux/videodev2.h
> @@ -1630,6 +1630,7 @@ enum v4l2_ctrl_type {
>  	V4L2_CTRL_TYPE_U8	     = 0x0100,
>  	V4L2_CTRL_TYPE_U16	     = 0x0101,
>  	V4L2_CTRL_TYPE_U32	     = 0x0102,
> +	V4L2_CTRL_TYPE_JPEG_QUANTIZATION = 0x0103,
>  };
>  
>  /*  Used in the VIDIOC_QUERYCTRL ioctl for querying controls */
> 

Regards,

	Hans

Re: [PATCH v4 5/6] media: Add controls for JPEG quantization tables

[Ian Arkver]

Hi,

On 03/09/2018 10:50, Hans Verkuil wrote:
> On 08/31/2018 05:52 PM, Ezequiel Garcia wrote:
>> From: Shunqian Zheng <zhengsq-TNX95d0MmH7DzftRWevZcw@public.gmane.org>
>>
>> Add V4L2_CID_JPEG_QUANTIZATION compound control to allow userspace
>> configure the JPEG quantization tables.
>>
>> Signed-off-by: Shunqian Zheng <zhengsq-TNX95d0MmH7DzftRWevZcw@public.gmane.org>
>> Signed-off-by: Ezequiel Garcia <ezequiel-ZGY8ohtN/8qB+jHODAdFcQ@public.gmane.org>
>> ---
>>   .../media/uapi/v4l/extended-controls.rst      | 23 +++++++++++++++++++
>>   .../media/videodev2.h.rst.exceptions          |  1 +
>>   drivers/media/v4l2-core/v4l2-ctrls.c          | 10 ++++++++
>>   include/uapi/linux/v4l2-controls.h            |  5 ++++
>>   include/uapi/linux/videodev2.h                |  1 +
>>   5 files changed, 40 insertions(+)
>>
>> diff --git a/Documentation/media/uapi/v4l/extended-controls.rst b/Documentation/media/uapi/v4l/extended-controls.rst
>> index 9f7312bf3365..e0dd03e452de 100644
>> --- a/Documentation/media/uapi/v4l/extended-controls.rst
>> +++ b/Documentation/media/uapi/v4l/extended-controls.rst
>> @@ -3354,7 +3354,30 @@ JPEG Control IDs
>>       Specify which JPEG markers are included in compressed stream. This
>>       control is valid only for encoders.
>>   
>> +.. _jpeg-quant-tables-control:
>>   
>> +``V4L2_CID_JPEG_QUANTIZATION (struct)``
>> +    Specifies the luma and chroma quantization matrices for encoding
>> +    or decoding a V4L2_PIX_FMT_JPEG_RAW format buffer. The two matrices
>> +    must be set in JPEG zigzag order, as per the JPEG specification.
> 
> Can you change "JPEG specification" to a reference to the JPEG spec entry
> in bibio.rst?
> 
>> +
>> +
>> +.. c:type:: struct v4l2_ctrl_jpeg_quantization
>> +
>> +.. cssclass:: longtable
>> +
>> +.. flat-table:: struct v4l2_ctrl_jpeg_quantization
>> +    :header-rows:  0
>> +    :stub-columns: 0
>> +    :widths:       1 1 2
>> +
>> +    * - __u8
>> +      - ``luma_quantization_matrix[64]``
>> +      - Sets the luma quantization table.
>> +
>> +    * - __u8
>> +      - ``chroma_quantization_matrix[64]``
>> +      - Sets the chroma quantization table.
> 
> Just checking: the JPEG standard specifies this as unsigned 8-bit values as well?

As far as I can see ISO/IEC 10918-1 does not specify the precision or 
signedness of the quantisation value Qvu. The default tables for 8-bit 
baseline JPEG all fit into __u8 though.

However there can be four sets of tables in non-baseline JPEG and it's 
not clear (to me) whether 12-bit JPEG would need more precision (I'd 
guess it would). Since this patch is defining UAPI I think it might be 
good to build in some additional information, eg. number of tables, 
element size. Maybe this can all be inferred from the selected pixel 
format? If so then it would need documented that the above structure 
only applies to baseline.

Regards,
Ian

> 
>>   
>>   .. flat-table::
>>       :header-rows:  0
>> diff --git a/Documentation/media/videodev2.h.rst.exceptions b/Documentation/media/videodev2.h.rst.exceptions
>> index ca9f0edc579e..a0a38e92bf38 100644
>> --- a/Documentation/media/videodev2.h.rst.exceptions
>> +++ b/Documentation/media/videodev2.h.rst.exceptions
>> @@ -129,6 +129,7 @@ replace symbol V4L2_CTRL_TYPE_STRING :c:type:`v4l2_ctrl_type`
>>   replace symbol V4L2_CTRL_TYPE_U16 :c:type:`v4l2_ctrl_type`
>>   replace symbol V4L2_CTRL_TYPE_U32 :c:type:`v4l2_ctrl_type`
>>   replace symbol V4L2_CTRL_TYPE_U8 :c:type:`v4l2_ctrl_type`
>> +replace symbol V4L2_CTRL_TYPE_JPEG_QUANTIZATION :c:type:`v4l2_ctrl_type`
>>   
>>   # V4L2 capability defines
>>   replace define V4L2_CAP_VIDEO_CAPTURE device-capabilities
>> diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c
>> index 599c1cbff3b9..305bd7a9b7f1 100644
>> --- a/drivers/media/v4l2-core/v4l2-ctrls.c
>> +++ b/drivers/media/v4l2-core/v4l2-ctrls.c
>> @@ -999,6 +999,7 @@ const char *v4l2_ctrl_get_name(u32 id)
>>   	case V4L2_CID_JPEG_RESTART_INTERVAL:	return "Restart Interval";
>>   	case V4L2_CID_JPEG_COMPRESSION_QUALITY:	return "Compression Quality";
>>   	case V4L2_CID_JPEG_ACTIVE_MARKER:	return "Active Markers";
>> +	case V4L2_CID_JPEG_QUANTIZATION:	return "JPEG Quantization Tables";
>>   
>>   	/* Image source controls */
>>   	/* Keep the order of the 'case's the same as in v4l2-controls.h! */
>> @@ -1286,6 +1287,9 @@ void v4l2_ctrl_fill(u32 id, const char **name, enum v4l2_ctrl_type *type,
>>   	case V4L2_CID_DETECT_MD_REGION_GRID:
>>   		*type = V4L2_CTRL_TYPE_U8;
>>   		break;
>> +	case V4L2_CID_JPEG_QUANTIZATION:
>> +		*type = V4L2_CTRL_TYPE_JPEG_QUANTIZATION;
>> +		break;
>>   	case V4L2_CID_DETECT_MD_THRESHOLD_GRID:
>>   		*type = V4L2_CTRL_TYPE_U16;
>>   		break;
>> @@ -1612,6 +1616,9 @@ static int std_validate(const struct v4l2_ctrl *ctrl, u32 idx,
>>   			return -ERANGE;
>>   		return 0;
>>   
>> +	case V4L2_CTRL_TYPE_JPEG_QUANTIZATION:
>> +		return 0;
>> +
>>   	default:
>>   		return -EINVAL;
>>   	}
>> @@ -2133,6 +2140,9 @@ static struct v4l2_ctrl *v4l2_ctrl_new(struct v4l2_ctrl_handler *hdl,
>>   	case V4L2_CTRL_TYPE_U32:
>>   		elem_size = sizeof(u32);
>>   		break;
>> +	case V4L2_CTRL_TYPE_JPEG_QUANTIZATION:
>> +		elem_size = sizeof(struct v4l2_ctrl_jpeg_quantization);
>> +		break;
>>   	default:
>>   		if (type < V4L2_CTRL_COMPOUND_TYPES)
>>   			elem_size = sizeof(s32);
>> diff --git a/include/uapi/linux/v4l2-controls.h b/include/uapi/linux/v4l2-controls.h
>> index e4ee10ee917d..fcb288bb05c7 100644
>> --- a/include/uapi/linux/v4l2-controls.h
>> +++ b/include/uapi/linux/v4l2-controls.h
>> @@ -987,6 +987,11 @@ enum v4l2_jpeg_chroma_subsampling {
>>   #define	V4L2_JPEG_ACTIVE_MARKER_DQT		(1 << 17)
>>   #define	V4L2_JPEG_ACTIVE_MARKER_DHT		(1 << 18)
>>   
>> +#define V4L2_CID_JPEG_QUANTIZATION		(V4L2_CID_JPEG_CLASS_BASE + 5)
>> +struct v4l2_ctrl_jpeg_quantization {
>> +	__u8	luma_quantization_matrix[64];
>> +	__u8	chroma_quantization_matrix[64];
>> +};
>>   
>>   /* Image source controls */
>>   #define V4L2_CID_IMAGE_SOURCE_CLASS_BASE	(V4L2_CTRL_CLASS_IMAGE_SOURCE | 0x900)
>> diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
>> index f271048c89c4..e998d07464cb 100644
>> --- a/include/uapi/linux/videodev2.h
>> +++ b/include/uapi/linux/videodev2.h
>> @@ -1630,6 +1630,7 @@ enum v4l2_ctrl_type {
>>   	V4L2_CTRL_TYPE_U8	     = 0x0100,
>>   	V4L2_CTRL_TYPE_U16	     = 0x0101,
>>   	V4L2_CTRL_TYPE_U32	     = 0x0102,
>> +	V4L2_CTRL_TYPE_JPEG_QUANTIZATION = 0x0103,
>>   };
>>   
>>   /*  Used in the VIDIOC_QUERYCTRL ioctl for querying controls */
>>
> 
> Regards,
> 
> 	Hans
> 

Re: [PATCH v4 5/6] media: Add controls for JPEG quantization tables

[Ezequiel Garcia]

Hi Ian, Hans:

On Mon, 2018-09-03 at 14:29 +0100, Ian Arkver wrote:
> Hi,
> 
> On 03/09/2018 10:50, Hans Verkuil wrote:
> > On 08/31/2018 05:52 PM, Ezequiel Garcia wrote:
> > > From: Shunqian Zheng <zhengsq-TNX95d0MmH7DzftRWevZcw@public.gmane.org>
> > > 
> > > Add V4L2_CID_JPEG_QUANTIZATION compound control to allow userspace
> > > configure the JPEG quantization tables.
> > > 
> > > Signed-off-by: Shunqian Zheng <zhengsq-TNX95d0MmH7DzftRWevZcw@public.gmane.org>
> > > Signed-off-by: Ezequiel Garcia <ezequiel-ZGY8ohtN/8qB+jHODAdFcQ@public.gmane.org>
> > > ---
> > >   .../media/uapi/v4l/extended-controls.rst      | 23 +++++++++++++++++++
> > >   .../media/videodev2.h.rst.exceptions          |  1 +
> > >   drivers/media/v4l2-core/v4l2-ctrls.c          | 10 ++++++++
> > >   include/uapi/linux/v4l2-controls.h            |  5 ++++
> > >   include/uapi/linux/videodev2.h                |  1 +
> > >   5 files changed, 40 insertions(+)
> > > 
> > > diff --git a/Documentation/media/uapi/v4l/extended-controls.rst b/Documentation/media/uapi/v4l/extended-controls.rst
> > > index 9f7312bf3365..e0dd03e452de 100644
> > > --- a/Documentation/media/uapi/v4l/extended-controls.rst
> > > +++ b/Documentation/media/uapi/v4l/extended-controls.rst
> > > @@ -3354,7 +3354,30 @@ JPEG Control IDs
> > >       Specify which JPEG markers are included in compressed stream. This
> > >       control is valid only for encoders.
> > >   
> > > +.. _jpeg-quant-tables-control:
> > >   
> > > +``V4L2_CID_JPEG_QUANTIZATION (struct)``
> > > +    Specifies the luma and chroma quantization matrices for encoding
> > > +    or decoding a V4L2_PIX_FMT_JPEG_RAW format buffer. The two matrices
> > > +    must be set in JPEG zigzag order, as per the JPEG specification.
> > 
> > Can you change "JPEG specification" to a reference to the JPEG spec entry
> > in bibio.rst?
> > 
> > > +
> > > +
> > > +.. c:type:: struct v4l2_ctrl_jpeg_quantization
> > > +
> > > +.. cssclass:: longtable
> > > +
> > > +.. flat-table:: struct v4l2_ctrl_jpeg_quantization
> > > +    :header-rows:  0
> > > +    :stub-columns: 0
> > > +    :widths:       1 1 2
> > > +
> > > +    * - __u8
> > > +      - ``luma_quantization_matrix[64]``
> > > +      - Sets the luma quantization table.
> > > +
> > > +    * - __u8
> > > +      - ``chroma_quantization_matrix[64]``
> > > +      - Sets the chroma quantization table.
> > 
> > Just checking: the JPEG standard specifies this as unsigned 8-bit values as well?
> 

I thought this was already discussed, but I think the only thing I've added
is this comment in one of the driver's headers:

 JPEG encoder
 ------------
 The VPU JPEG encoder produces JPEG baseline sequential format.
 The quantization coefficients are 8-bit values, complying with
 the baseline specification. Therefore, it requires application-defined
 luma and chroma quantization tables. The hardware does entrophy
 encoding using internal Huffman tables, as specified in the JPEG
 specification.

Certainly controls should be specified better.

> As far as I can see ISO/IEC 10918-1 does not specify the precision or 
> signedness of the quantisation value Qvu. The default tables for 8-bit 
> baseline JPEG all fit into __u8 though.
> 

Paragraph 4.7 of that spec, indicates the "sample" precision:
8-bit for baseline; 8-bit or 12-bit for extended.

For the quantization coefficients, the DQT segment contains a bit
that indicates if the quantization coefficients are 8-bit or 16-bit.
See B.2.4.1 for details.

> However there can be four sets of tables in non-baseline JPEG and it's 

You lost me here, which four sets of tables are you refering to?

> not clear (to me) whether 12-bit JPEG would need more precision (I'd 
> guess it would).

It seems it would. From B.2.4.1:

"An 8-bit DCT-based process shall not use a 16-bit precision quantization table."

> Since this patch is defining UAPI I think it might be 
> good to build in some additional information, eg. number of tables, 
> element size. Maybe this can all be inferred from the selected pixel 
> format? If so then it would need documented that the above structure 
> only applies to baseline.
> 

For quantization coefficients, I can only see two tables: one for luma
one for chroma. Huffman coefficients are a different story and we are
not really adding them here.

Thanks,
Eze

Re: [PATCH v4 5/6] media: Add controls for JPEG quantization tables

[Hans Verkuil]

On 09/03/2018 05:27 PM, Ezequiel Garcia wrote:
> Hi Ian, Hans:
> 
> On Mon, 2018-09-03 at 14:29 +0100, Ian Arkver wrote:
>> Hi,
>>
>> On 03/09/2018 10:50, Hans Verkuil wrote:
>>> On 08/31/2018 05:52 PM, Ezequiel Garcia wrote:
>>>> From: Shunqian Zheng <zhengsq-TNX95d0MmH7DzftRWevZcw@public.gmane.org>
>>>>
>>>> Add V4L2_CID_JPEG_QUANTIZATION compound control to allow userspace
>>>> configure the JPEG quantization tables.
>>>>
>>>> Signed-off-by: Shunqian Zheng <zhengsq-TNX95d0MmH7DzftRWevZcw@public.gmane.org>
>>>> Signed-off-by: Ezequiel Garcia <ezequiel-ZGY8ohtN/8qB+jHODAdFcQ@public.gmane.org>
>>>> ---
>>>>   .../media/uapi/v4l/extended-controls.rst      | 23 +++++++++++++++++++
>>>>   .../media/videodev2.h.rst.exceptions          |  1 +
>>>>   drivers/media/v4l2-core/v4l2-ctrls.c          | 10 ++++++++
>>>>   include/uapi/linux/v4l2-controls.h            |  5 ++++
>>>>   include/uapi/linux/videodev2.h                |  1 +
>>>>   5 files changed, 40 insertions(+)
>>>>
>>>> diff --git a/Documentation/media/uapi/v4l/extended-controls.rst b/Documentation/media/uapi/v4l/extended-controls.rst
>>>> index 9f7312bf3365..e0dd03e452de 100644
>>>> --- a/Documentation/media/uapi/v4l/extended-controls.rst
>>>> +++ b/Documentation/media/uapi/v4l/extended-controls.rst
>>>> @@ -3354,7 +3354,30 @@ JPEG Control IDs
>>>>       Specify which JPEG markers are included in compressed stream. This
>>>>       control is valid only for encoders.
>>>>   
>>>> +.. _jpeg-quant-tables-control:
>>>>   
>>>> +``V4L2_CID_JPEG_QUANTIZATION (struct)``
>>>> +    Specifies the luma and chroma quantization matrices for encoding
>>>> +    or decoding a V4L2_PIX_FMT_JPEG_RAW format buffer. The two matrices
>>>> +    must be set in JPEG zigzag order, as per the JPEG specification.
>>>
>>> Can you change "JPEG specification" to a reference to the JPEG spec entry
>>> in bibio.rst?
>>>
>>>> +
>>>> +
>>>> +.. c:type:: struct v4l2_ctrl_jpeg_quantization
>>>> +
>>>> +.. cssclass:: longtable
>>>> +
>>>> +.. flat-table:: struct v4l2_ctrl_jpeg_quantization
>>>> +    :header-rows:  0
>>>> +    :stub-columns: 0
>>>> +    :widths:       1 1 2
>>>> +
>>>> +    * - __u8
>>>> +      - ``luma_quantization_matrix[64]``
>>>> +      - Sets the luma quantization table.
>>>> +
>>>> +    * - __u8
>>>> +      - ``chroma_quantization_matrix[64]``
>>>> +      - Sets the chroma quantization table.
>>>
>>> Just checking: the JPEG standard specifies this as unsigned 8-bit values as well?
>>
> 
> I thought this was already discussed, but I think the only thing I've added
> is this comment in one of the driver's headers:
> 
>  JPEG encoder
>  ------------
>  The VPU JPEG encoder produces JPEG baseline sequential format.
>  The quantization coefficients are 8-bit values, complying with
>  the baseline specification. Therefore, it requires application-defined
>  luma and chroma quantization tables. The hardware does entrophy
>  encoding using internal Huffman tables, as specified in the JPEG
>  specification.
> 
> Certainly controls should be specified better.
> 
>> As far as I can see ISO/IEC 10918-1 does not specify the precision or 
>> signedness of the quantisation value Qvu. The default tables for 8-bit 
>> baseline JPEG all fit into __u8 though.
>>
> 
> Paragraph 4.7 of that spec, indicates the "sample" precision:
> 8-bit for baseline; 8-bit or 12-bit for extended.
> 
> For the quantization coefficients, the DQT segment contains a bit
> that indicates if the quantization coefficients are 8-bit or 16-bit.
> See B.2.4.1 for details.
> 
>> However there can be four sets of tables in non-baseline JPEG and it's 
> 
> You lost me here, which four sets of tables are you refering to?
> 
>> not clear (to me) whether 12-bit JPEG would need more precision (I'd 
>> guess it would).
> 
> It seems it would. From B.2.4.1:
> 
> "An 8-bit DCT-based process shall not use a 16-bit precision quantization table."
> 
>> Since this patch is defining UAPI I think it might be 
>> good to build in some additional information, eg. number of tables, 
>> element size. Maybe this can all be inferred from the selected pixel 
>> format? If so then it would need documented that the above structure 
>> only applies to baseline.
>>
> 
> For quantization coefficients, I can only see two tables: one for luma
> one for chroma. Huffman coefficients are a different story and we are
> not really adding them here.

Since (if I understand this correctly) we would need u16 for extended precision
JPEG, shouldn't we use u16 instead of u8? That makes the control more generic.

BTW, are the coefficients always unsigned? I think so, but I never read the
JPEG spec.

Regards,

	Hans

> 
> Thanks,
> Eze
> 

Re: [PATCH v4 5/6] media: Add controls for JPEG quantization tables

[Ian Arkver]

Hi Hans, Ezequiel,

On 03/09/2018 16:33, Hans Verkuil wrote:
> On 09/03/2018 05:27 PM, Ezequiel Garcia wrote:
>> Hi Ian, Hans:
>>
>> On Mon, 2018-09-03 at 14:29 +0100, Ian Arkver wrote:
>>> Hi,
>>>
>>> On 03/09/2018 10:50, Hans Verkuil wrote:
>>>> On 08/31/2018 05:52 PM, Ezequiel Garcia wrote:
>>>>> From: Shunqian Zheng <zhengsq-TNX95d0MmH7DzftRWevZcw@public.gmane.org>
>>>>>
>>>>> Add V4L2_CID_JPEG_QUANTIZATION compound control to allow userspace
>>>>> configure the JPEG quantization tables.
>>>>>
>>>>> Signed-off-by: Shunqian Zheng <zhengsq-TNX95d0MmH7DzftRWevZcw@public.gmane.org>
>>>>> Signed-off-by: Ezequiel Garcia <ezequiel-ZGY8ohtN/8qB+jHODAdFcQ@public.gmane.org>
>>>>> ---
>>>>>    .../media/uapi/v4l/extended-controls.rst      | 23 +++++++++++++++++++
>>>>>    .../media/videodev2.h.rst.exceptions          |  1 +
>>>>>    drivers/media/v4l2-core/v4l2-ctrls.c          | 10 ++++++++
>>>>>    include/uapi/linux/v4l2-controls.h            |  5 ++++
>>>>>    include/uapi/linux/videodev2.h                |  1 +
>>>>>    5 files changed, 40 insertions(+)
>>>>>
>>>>> diff --git a/Documentation/media/uapi/v4l/extended-controls.rst b/Documentation/media/uapi/v4l/extended-controls.rst
>>>>> index 9f7312bf3365..e0dd03e452de 100644
>>>>> --- a/Documentation/media/uapi/v4l/extended-controls.rst
>>>>> +++ b/Documentation/media/uapi/v4l/extended-controls.rst
>>>>> @@ -3354,7 +3354,30 @@ JPEG Control IDs
>>>>>        Specify which JPEG markers are included in compressed stream. This
>>>>>        control is valid only for encoders.
>>>>>    
>>>>> +.. _jpeg-quant-tables-control:
>>>>>    
>>>>> +``V4L2_CID_JPEG_QUANTIZATION (struct)``
>>>>> +    Specifies the luma and chroma quantization matrices for encoding
>>>>> +    or decoding a V4L2_PIX_FMT_JPEG_RAW format buffer. The two matrices
>>>>> +    must be set in JPEG zigzag order, as per the JPEG specification.
>>>>
>>>> Can you change "JPEG specification" to a reference to the JPEG spec entry
>>>> in bibio.rst?
>>>>
>>>>> +
>>>>> +
>>>>> +.. c:type:: struct v4l2_ctrl_jpeg_quantization
>>>>> +
>>>>> +.. cssclass:: longtable
>>>>> +
>>>>> +.. flat-table:: struct v4l2_ctrl_jpeg_quantization
>>>>> +    :header-rows:  0
>>>>> +    :stub-columns: 0
>>>>> +    :widths:       1 1 2
>>>>> +
>>>>> +    * - __u8
>>>>> +      - ``luma_quantization_matrix[64]``
>>>>> +      - Sets the luma quantization table.
>>>>> +
>>>>> +    * - __u8
>>>>> +      - ``chroma_quantization_matrix[64]``
>>>>> +      - Sets the chroma quantization table.
>>>>
>>>> Just checking: the JPEG standard specifies this as unsigned 8-bit values as well?
>>>
>>
>> I thought this was already discussed, but I think the only thing I've added
>> is this comment in one of the driver's headers:
>>
>>   JPEG encoder
>>   ------------
>>   The VPU JPEG encoder produces JPEG baseline sequential format.
>>   The quantization coefficients are 8-bit values, complying with
>>   the baseline specification. Therefore, it requires application-defined
>>   luma and chroma quantization tables. The hardware does entrophy
>>   encoding using internal Huffman tables, as specified in the JPEG
>>   specification.
>>
>> Certainly controls should be specified better.
>>
>>> As far as I can see ISO/IEC 10918-1 does not specify the precision or
>>> signedness of the quantisation value Qvu. The default tables for 8-bit
>>> baseline JPEG all fit into __u8 though.
>>>
>>
>> Paragraph 4.7 of that spec, indicates the "sample" precision:
>> 8-bit for baseline; 8-bit or 12-bit for extended.
>>
>> For the quantization coefficients, the DQT segment contains a bit
>> that indicates if the quantization coefficients are 8-bit or 16-bit.
>> See B.2.4.1 for details.

See below (and Tq which follows the Pq field)

>>
>>> However there can be four sets of tables in non-baseline JPEG and it's
>>
>> You lost me here, which four sets of tables are you refering to?
>>
>>> not clear (to me) whether 12-bit JPEG would need more precision (I'd
>>> guess it would).
>>
>> It seems it would. From B.2.4.1:
>>
>> "An 8-bit DCT-based process shall not use a 16-bit precision quantization table."
>>
>>> Since this patch is defining UAPI I think it might be
>>> good to build in some additional information, eg. number of tables,
>>> element size. Maybe this can all be inferred from the selected pixel
>>> format? If so then it would need documented that the above structure
>>> only applies to baseline.
>>>
>>
>> For quantization coefficients, I can only see two tables: one for luma
>> one for chroma. Huffman coefficients are a different story and we are
>> not really adding them here.

I was looking at the definition of Tqi in the frame header in B.2.2 
which seems to allow up to four (sets of?) quantization tables. 
Rereading it, it seems these are per component. Table B.2 implies that 
this applies to Baseline Sequential too. In the DQT marker description 
there's a Tq field to specify the destination for the new table. I think 
this means that an encoder can use up to four (sets of) tables and a 
decoder should be able to store four from the stream.

This may well not be relevant to the encoder under discussion, but if 
it's not allowed for in UAPI it's almost a given that it'll need to be 
added later.

BTW, my copy of the spec is dated 1993(E). Maybe it's out of date?

> 
> Since (if I understand this correctly) we would need u16 for extended precision
> JPEG, shouldn't we use u16 instead of u8? That makes the control more generic.

This seems like a safer option to me.

Regards,
Ian

> 
> BTW, are the coefficients always unsigned? I think so, but I never read the
> JPEG spec.
> 
> Regards,
> 
> 	Hans
> 
>>
>> Thanks,
>> Eze
>>
> 

Re: [PATCH v4 5/6] media: Add controls for JPEG quantization tables

[Ezequiel Garcia]

On Mon, 2018-09-03 at 16:51 +0100, Ian Arkver wrote:
> Hi Hans, Ezequiel,
> 
> On 03/09/2018 16:33, Hans Verkuil wrote:
> > On 09/03/2018 05:27 PM, Ezequiel Garcia wrote:
> > > Hi Ian, Hans:
> > > 
> > > On Mon, 2018-09-03 at 14:29 +0100, Ian Arkver wrote:
> > > > Hi,
> > > > 
> > > > On 03/09/2018 10:50, Hans Verkuil wrote:
> > > > > On 08/31/2018 05:52 PM, Ezequiel Garcia wrote:
> > > > > > From: Shunqian Zheng <zhengsq-TNX95d0MmH7DzftRWevZcw@public.gmane.org>
> > > > > > 
> > > > > > Add V4L2_CID_JPEG_QUANTIZATION compound control to allow userspace
> > > > > > configure the JPEG quantization tables.
> > > > > > 
> > > > > > Signed-off-by: Shunqian Zheng <zhengsq-TNX95d0MmH7DzftRWevZcw@public.gmane.org>
> > > > > > Signed-off-by: Ezequiel Garcia <ezequiel-ZGY8ohtN/8qB+jHODAdFcQ@public.gmane.org>
> > > > > > ---
> > > > > >    .../media/uapi/v4l/extended-controls.rst      | 23 +++++++++++++++++++
> > > > > >    .../media/videodev2.h.rst.exceptions          |  1 +
> > > > > >    drivers/media/v4l2-core/v4l2-ctrls.c          | 10 ++++++++
> > > > > >    include/uapi/linux/v4l2-controls.h            |  5 ++++
> > > > > >    include/uapi/linux/videodev2.h                |  1 +
> > > > > >    5 files changed, 40 insertions(+)
> > > > > > 
> > > > > > diff --git a/Documentation/media/uapi/v4l/extended-controls.rst b/Documentation/media/uapi/v4l/extended-controls.rst
> > > > > > index 9f7312bf3365..e0dd03e452de 100644
> > > > > > --- a/Documentation/media/uapi/v4l/extended-controls.rst
> > > > > > +++ b/Documentation/media/uapi/v4l/extended-controls.rst
> > > > > > @@ -3354,7 +3354,30 @@ JPEG Control IDs
> > > > > >        Specify which JPEG markers are included in compressed stream. This
> > > > > >        control is valid only for encoders.
> > > > > >    
> > > > > > +.. _jpeg-quant-tables-control:
> > > > > >    
> > > > > > +``V4L2_CID_JPEG_QUANTIZATION (struct)``
> > > > > > +    Specifies the luma and chroma quantization matrices for encoding
> > > > > > +    or decoding a V4L2_PIX_FMT_JPEG_RAW format buffer. The two matrices
> > > > > > +    must be set in JPEG zigzag order, as per the JPEG specification.
> > > > > 
> > > > > Can you change "JPEG specification" to a reference to the JPEG spec entry
> > > > > in bibio.rst?
> > > > > 
> > > > > > +
> > > > > > +
> > > > > > +.. c:type:: struct v4l2_ctrl_jpeg_quantization
> > > > > > +
> > > > > > +.. cssclass:: longtable
> > > > > > +
> > > > > > +.. flat-table:: struct v4l2_ctrl_jpeg_quantization
> > > > > > +    :header-rows:  0
> > > > > > +    :stub-columns: 0
> > > > > > +    :widths:       1 1 2
> > > > > > +
> > > > > > +    * - __u8
> > > > > > +      - ``luma_quantization_matrix[64]``
> > > > > > +      - Sets the luma quantization table.
> > > > > > +
> > > > > > +    * - __u8
> > > > > > +      - ``chroma_quantization_matrix[64]``
> > > > > > +      - Sets the chroma quantization table.
> > > > > 
> > > > > Just checking: the JPEG standard specifies this as unsigned 8-bit values as well?
> > > 
> > > I thought this was already discussed, but I think the only thing I've added
> > > is this comment in one of the driver's headers:
> > > 
> > >   JPEG encoder
> > >   ------------
> > >   The VPU JPEG encoder produces JPEG baseline sequential format.
> > >   The quantization coefficients are 8-bit values, complying with
> > >   the baseline specification. Therefore, it requires application-defined
> > >   luma and chroma quantization tables. The hardware does entrophy
> > >   encoding using internal Huffman tables, as specified in the JPEG
> > >   specification.
> > > 
> > > Certainly controls should be specified better.
> > > 
> > > > As far as I can see ISO/IEC 10918-1 does not specify the precision or
> > > > signedness of the quantisation value Qvu. The default tables for 8-bit
> > > > baseline JPEG all fit into __u8 though.
> > > > 
> > > 
> > > Paragraph 4.7 of that spec, indicates the "sample" precision:
> > > 8-bit for baseline; 8-bit or 12-bit for extended.
> > > 
> > > For the quantization coefficients, the DQT segment contains a bit
> > > that indicates if the quantization coefficients are 8-bit or 16-bit.
> > > See B.2.4.1 for details.
> 
> See below (and Tq which follows the Pq field)
> 
> > > 
> > > > However there can be four sets of tables in non-baseline JPEG and it's
> > > 
> > > You lost me here, which four sets of tables are you refering to?
> > > 
> > > > not clear (to me) whether 12-bit JPEG would need more precision (I'd
> > > > guess it would).
> > > 
> > > It seems it would. From B.2.4.1:
> > > 
> > > "An 8-bit DCT-based process shall not use a 16-bit precision quantization table."
> > > 
> > > > Since this patch is defining UAPI I think it might be
> > > > good to build in some additional information, eg. number of tables,
> > > > element size. Maybe this can all be inferred from the selected pixel
> > > > format? If so then it would need documented that the above structure
> > > > only applies to baseline.
> > > > 
> > > 
> > > For quantization coefficients, I can only see two tables: one for luma
> > > one for chroma. Huffman coefficients are a different story and we are
> > > not really adding them here.
> 
> I was looking at the definition of Tqi in the frame header in B.2.2 
> which seems to allow up to four (sets of?) quantization tables. 
> Rereading it, it seems these are per component. Table B.2 implies that 
> this applies to Baseline Sequential too. In the DQT marker description 
> there's a Tq field to specify the destination for the new table. I think 
> this means that an encoder can use up to four (sets of) tables and a 
> decoder should be able to store four from the stream.
> 
> This may well not be relevant to the encoder under discussion, but if 
> it's not allowed for in UAPI it's almost a given that it'll need to be 
> added later.
> 

Hm, I think it's not really relevant for us, either on the encoding
or decoding side. Let me explain how I read the spec.

First of all, keep in mind it seems to be written with streams
in mind, which explains different start-of-image and start-of-frames
segments (SOI and SOF).

The way I read the four tables thing, the decoder expects to be first
transmitted a set of quantization tables, via DQT segments. Each DQT
segment contains a 4-bit index (0-3), allowing up to four quantization
tables to be defined.

Then, each frame is transmitted in a SOF segment. The SOF header
contains an 8-bit field (Tq_i) indicating which quantization table has
to be used for this frame.

In Video4Linux, we are frame-based, and the JPEG segments have no meaning,
because these controls are meant to be used with the JPEG RAW format,
as specified.

For the decoder side, the application is expected to parse all the segments,
and set quantization tables and compressed bitstream, via legacy or
request APIs (probably the latter).

Same goes for encoding, the user will set a quantization table for the
encoding process and then take care of the JPEG segments creation.

> BTW, my copy of the spec is dated 1993(E). Maybe it's out of date?
> 

My copy is also 1992, so even more dated :-)

> > 
> > Since (if I understand this correctly) we would need u16 for extended precision
> > JPEG, shouldn't we use u16 instead of u8? That makes the control more generic.
> 
> This seems like a safer option to me.
> 

Yes, I agree too.

> 
> > 
> > BTW, are the coefficients always unsigned? I think so, but I never read the
> > JPEG spec.
> > 

I don't think signed quantization coefficients make sense, so perhaps
this is not explicitly specified?

Regards,
Eze

Re: [PATCH v4 5/6] media: Add controls for JPEG quantization tables

[Ian Arkver]

Hi Ezequiel,

On 03/09/2018 18:15, Ezequiel Garcia wrote:
> On Mon, 2018-09-03 at 16:51 +0100, Ian Arkver wrote:
>> Hi Hans, Ezequiel,
>>
>> On 03/09/2018 16:33, Hans Verkuil wrote:
>>> On 09/03/2018 05:27 PM, Ezequiel Garcia wrote:
>>>> Hi Ian, Hans:
>>>>
>>>> On Mon, 2018-09-03 at 14:29 +0100, Ian Arkver wrote:
>>>>> Hi,
>>>>>
>>>>> On 03/09/2018 10:50, Hans Verkuil wrote:
>>>>>> On 08/31/2018 05:52 PM, Ezequiel Garcia wrote:
>>>>>>> From: Shunqian Zheng <zhengsq-TNX95d0MmH7DzftRWevZcw@public.gmane.org>
>>>>>>>
>>>>>>> Add V4L2_CID_JPEG_QUANTIZATION compound control to allow userspace
>>>>>>> configure the JPEG quantization tables.
>>>>>>>
>>>>>>> Signed-off-by: Shunqian Zheng <zhengsq-TNX95d0MmH7DzftRWevZcw@public.gmane.org>
>>>>>>> Signed-off-by: Ezequiel Garcia <ezequiel-ZGY8ohtN/8qB+jHODAdFcQ@public.gmane.org>
>>>>>>> ---
>>>>>>>     .../media/uapi/v4l/extended-controls.rst      | 23 +++++++++++++++++++
>>>>>>>     .../media/videodev2.h.rst.exceptions          |  1 +
>>>>>>>     drivers/media/v4l2-core/v4l2-ctrls.c          | 10 ++++++++
>>>>>>>     include/uapi/linux/v4l2-controls.h            |  5 ++++
>>>>>>>     include/uapi/linux/videodev2.h                |  1 +
>>>>>>>     5 files changed, 40 insertions(+)
>>>>>>>
>>>>>>> diff --git a/Documentation/media/uapi/v4l/extended-controls.rst b/Documentation/media/uapi/v4l/extended-controls.rst
>>>>>>> index 9f7312bf3365..e0dd03e452de 100644
>>>>>>> --- a/Documentation/media/uapi/v4l/extended-controls.rst
>>>>>>> +++ b/Documentation/media/uapi/v4l/extended-controls.rst
>>>>>>> @@ -3354,7 +3354,30 @@ JPEG Control IDs
>>>>>>>         Specify which JPEG markers are included in compressed stream. This
>>>>>>>         control is valid only for encoders.
>>>>>>>     
>>>>>>> +.. _jpeg-quant-tables-control:
>>>>>>>     
>>>>>>> +``V4L2_CID_JPEG_QUANTIZATION (struct)``
>>>>>>> +    Specifies the luma and chroma quantization matrices for encoding
>>>>>>> +    or decoding a V4L2_PIX_FMT_JPEG_RAW format buffer. The two matrices
>>>>>>> +    must be set in JPEG zigzag order, as per the JPEG specification.
>>>>>>
>>>>>> Can you change "JPEG specification" to a reference to the JPEG spec entry
>>>>>> in bibio.rst?
>>>>>>
>>>>>>> +
>>>>>>> +
>>>>>>> +.. c:type:: struct v4l2_ctrl_jpeg_quantization
>>>>>>> +
>>>>>>> +.. cssclass:: longtable
>>>>>>> +
>>>>>>> +.. flat-table:: struct v4l2_ctrl_jpeg_quantization
>>>>>>> +    :header-rows:  0
>>>>>>> +    :stub-columns: 0
>>>>>>> +    :widths:       1 1 2
>>>>>>> +
>>>>>>> +    * - __u8
>>>>>>> +      - ``luma_quantization_matrix[64]``
>>>>>>> +      - Sets the luma quantization table.
>>>>>>> +
>>>>>>> +    * - __u8
>>>>>>> +      - ``chroma_quantization_matrix[64]``
>>>>>>> +      - Sets the chroma quantization table.
>>>>>>
>>>>>> Just checking: the JPEG standard specifies this as unsigned 8-bit values as well?
>>>>
>>>> I thought this was already discussed, but I think the only thing I've added
>>>> is this comment in one of the driver's headers:
>>>>
>>>>    JPEG encoder
>>>>    ------------
>>>>    The VPU JPEG encoder produces JPEG baseline sequential format.
>>>>    The quantization coefficients are 8-bit values, complying with
>>>>    the baseline specification. Therefore, it requires application-defined
>>>>    luma and chroma quantization tables. The hardware does entrophy
>>>>    encoding using internal Huffman tables, as specified in the JPEG
>>>>    specification.
>>>>
>>>> Certainly controls should be specified better.
>>>>
>>>>> As far as I can see ISO/IEC 10918-1 does not specify the precision or
>>>>> signedness of the quantisation value Qvu. The default tables for 8-bit
>>>>> baseline JPEG all fit into __u8 though.
>>>>>
>>>>
>>>> Paragraph 4.7 of that spec, indicates the "sample" precision:
>>>> 8-bit for baseline; 8-bit or 12-bit for extended.
>>>>
>>>> For the quantization coefficients, the DQT segment contains a bit
>>>> that indicates if the quantization coefficients are 8-bit or 16-bit.
>>>> See B.2.4.1 for details.
>>
>> See below (and Tq which follows the Pq field)
>>
>>>>
>>>>> However there can be four sets of tables in non-baseline JPEG and it's
>>>>
>>>> You lost me here, which four sets of tables are you refering to?
>>>>
>>>>> not clear (to me) whether 12-bit JPEG would need more precision (I'd
>>>>> guess it would).
>>>>
>>>> It seems it would. From B.2.4.1:
>>>>
>>>> "An 8-bit DCT-based process shall not use a 16-bit precision quantization table."
>>>>
>>>>> Since this patch is defining UAPI I think it might be
>>>>> good to build in some additional information, eg. number of tables,
>>>>> element size. Maybe this can all be inferred from the selected pixel
>>>>> format? If so then it would need documented that the above structure
>>>>> only applies to baseline.
>>>>>
>>>>
>>>> For quantization coefficients, I can only see two tables: one for luma
>>>> one for chroma. Huffman coefficients are a different story and we are
>>>> not really adding them here.
>>
>> I was looking at the definition of Tqi in the frame header in B.2.2
>> which seems to allow up to four (sets of?) quantization tables.
>> Rereading it, it seems these are per component. Table B.2 implies that
>> this applies to Baseline Sequential too. In the DQT marker description
>> there's a Tq field to specify the destination for the new table. I think
>> this means that an encoder can use up to four (sets of) tables and a
>> decoder should be able to store four from the stream.
>>
>> This may well not be relevant to the encoder under discussion, but if
>> it's not allowed for in UAPI it's almost a given that it'll need to be
>> added later.
>>
> 
> Hm, I think it's not really relevant for us, either on the encoding
> or decoding side. Let me explain how I read the spec.
> 
> First of all, keep in mind it seems to be written with streams
> in mind, which explains different start-of-image and start-of-frames
> segments (SOI and SOF).
> 
> The way I read the four tables thing, the decoder expects to be first
> transmitted a set of quantization tables, via DQT segments. Each DQT
> segment contains a 4-bit index (0-3), allowing up to four quantization
> tables to be defined.
> 
> Then, each frame is transmitted in a SOF segment. The SOF header
> contains an 8-bit field (Tq_i) indicating which quantization table has
> to be used for this frame.
> 
> In Video4Linux, we are frame-based, and the JPEG segments have no meaning,
> because these controls are meant to be used with the JPEG RAW format,
> as specified.
> 
> For the decoder side, the application is expected to parse all the segments,
> and set quantization tables and compressed bitstream, via legacy or
> request APIs (probably the latter).
> 
> Same goes for encoding, the user will set a quantization table for the
> encoding process and then take care of the JPEG segments creation.

I hadn't got my head round only one table being in use at a time for 
this frame based API, nor had I appreciated that all the DQT 
parsing/creation etc was handled in userspace.

So, yeah, I agree that this JPEG control only needs a single luma and 
chroma pair of tables for this usage scenario.

Thanks for the explanation.
Ian.

> 
>> BTW, my copy of the spec is dated 1993(E). Maybe it's out of date?
>>
> 
> My copy is also 1992, so even more dated :-)
> 
>>>
>>> Since (if I understand this correctly) we would need u16 for extended precision
>>> JPEG, shouldn't we use u16 instead of u8? That makes the control more generic.
>>
>> This seems like a safer option to me.
>>
> 
> Yes, I agree too.
> 
>>
>>>
>>> BTW, are the coefficients always unsigned? I think so, but I never read the
>>> JPEG spec.
>>>
> 
> I don't think signed quantization coefficients make sense, so perhaps
> this is not explicitly specified?
> 
> Regards,
> Eze
>