Re: [PATCH v5 02/10] media: uapi: mpeg2: rework quantisation matrices semantics

[Hans Verkuil <hverkuil@xs4all.nl>]

On 03/04/2021 20:07, Ezequiel Garcia wrote:
> As stated in the MPEG-2 specification, section 6.3.7 "Quant matrix
> extension":
> 
>   Each quantisation matrix has a default set of values. When a
>   sequence_header_code is decoded all matrices shall be reset to
>   their default values. User defined matrices may be downloaded
>   and this can occur in a sequence_header() or in a
>   quant_matrix_extension().
> 
> The load_intra_quantiser_matrix syntax elements are transmitted
> in the bistream headers, signalling that a quantisation matrix

bistream -> bitstream

> needs to be loaded and used for pictures transmitted afterwards
> (until the matrices are reset).
> 
> This "load" semantics are implemented in the V4L2 interface
> without the need of any "load" flags: passing the control
> is effectively a load.
> 
> Therefore, rework the V4L2_CID_MPEG_VIDEO_MPEG2_QUANTISATION
> semantics to match the MPEG-2 semantics. Quantisation matrices
> values are now initialized by the V4L2 control core to their
> reset default value, and applications are expected to reset
> their values as specified.
> 
> The quantisation control is therefore optional, and used to
> load bitstream-defined values in the quantisation matrices.
> 
> Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
> Co-developed-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
> Signed-off-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
> ---
>  .../media/v4l/ext-ctrls-codec.rst             | 17 ---------
>  drivers/media/v4l2-core/v4l2-ctrls.c          | 21 ++++++++++
>  .../staging/media/sunxi/cedrus/cedrus_mpeg2.c | 38 +------------------
>  include/media/mpeg2-ctrls.h                   |  5 ---
>  4 files changed, 23 insertions(+), 58 deletions(-)
> 
> diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
> index 151d1c676b6e..d9546f0aa2e8 100644
> --- a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
> +++ b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
> @@ -1746,23 +1746,6 @@ enum v4l2_mpeg_video_h264_hierarchical_coding_type -
>      :stub-columns: 0
>      :widths:       1 1 2
>  
> -    * - __u8
> -      - ``load_intra_quantiser_matrix``
> -      - One bit to indicate whether to load the ``intra_quantiser_matrix`` data.
> -    * - __u8
> -      - ``load_non_intra_quantiser_matrix``
> -      - One bit to indicate whether to load the ``non_intra_quantiser_matrix``
> -	data.
> -    * - __u8
> -      - ``load_chroma_intra_quantiser_matrix``
> -      - One bit to indicate whether to load the
> -	``chroma_intra_quantiser_matrix`` data, only relevant for non-4:2:0 YUV
> -	formats.
> -    * - __u8
> -      - ``load_chroma_non_intra_quantiser_matrix``
> -      - One bit to indicate whether to load the
> -	``chroma_non_intra_quantiser_matrix`` data, only relevant for non-4:2:0
> -	YUV formats.
>      * - __u8
>        - ``intra_quantiser_matrix[64]``
>        - The quantisation matrix coefficients for intra-coded frames, in zigzag
> diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c
> index 74f5ca1a5f6c..5d92a2b33a6e 100644
> --- a/drivers/media/v4l2-core/v4l2-ctrls.c
> +++ b/drivers/media/v4l2-core/v4l2-ctrls.c
> @@ -57,6 +57,18 @@ static bool is_new_manual(const struct v4l2_ctrl *master)
>  	return master->is_auto && master->val == master->manual_mode_value;
>  }
>  
> +/* Default MPEG-2 quantisation coefficients, from the specification. */

Default -> Default intra

> +static const u8 mpeg2_intra_quant_matrix[64] = {
> +	8,  16, 16, 19, 16, 19, 22, 22,
> +	22, 22, 22, 22, 26, 24, 26, 27,
> +	27, 27, 26, 26, 26, 26, 27, 27,
> +	27, 29, 29, 29, 34, 34, 34, 29,
> +	29, 29, 27, 27, 29, 29, 32, 32,
> +	34, 34, 37, 38, 37, 35, 35, 34,
> +	35, 38, 38, 40, 40, 40, 48, 48,
> +	46, 46, 56, 56, 58, 69, 69, 83
> +};
> +
>  /* Returns NULL or a character pointer array containing the menu for
>     the given control ID. The pointer array ends with a NULL pointer.
>     An empty string signifies a menu entry that is invalid. This allows
> @@ -1656,6 +1668,7 @@ static void std_init_compound(const struct v4l2_ctrl *ctrl, u32 idx,
>  			      union v4l2_ctrl_ptr ptr)
>  {
>  	struct v4l2_ctrl_mpeg2_slice_params *p_mpeg2_slice_params;
> +	struct v4l2_ctrl_mpeg2_quantisation *p_mpeg2_quant;
>  	struct v4l2_ctrl_vp8_frame *p_vp8_frame;
>  	struct v4l2_ctrl_fwht_params *p_fwht_params;
>  	void *p = ptr.p + idx * ctrl->elem_size;
> @@ -1680,6 +1693,14 @@ static void std_init_compound(const struct v4l2_ctrl *ctrl, u32 idx,
>  		p_mpeg2_slice_params->picture.picture_coding_type =
>  					V4L2_MPEG2_PICTURE_CODING_TYPE_I;
>  		break;
> +	case V4L2_CTRL_TYPE_MPEG2_QUANTISATION:
> +		p_mpeg2_quant = p;
> +
> +		memcpy(p_mpeg2_quant->intra_quantiser_matrix,
> +		       mpeg2_intra_quant_matrix,
> +		       ARRAY_SIZE(mpeg2_intra_quant_matrix));
> +		memset(p_mpeg2_quant->non_intra_quantiser_matrix, 16, 64);

64 -> sizeof(p_mpeg2_quant->non_intra_quantiser_matrix)

Also add a comment before the memset:

	/*
	 * The default non-intra MPEG-2 quantisation coefficients are all 16,
	 * as per the specification.
	 */

> +		break;
>  	case V4L2_CTRL_TYPE_VP8_FRAME:
>  		p_vp8_frame = p;
>  		p_vp8_frame->num_dct_parts = 1;
> diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_mpeg2.c b/drivers/staging/media/sunxi/cedrus/cedrus_mpeg2.c
> index 459f71679a4f..e3154f631858 100644
> --- a/drivers/staging/media/sunxi/cedrus/cedrus_mpeg2.c
> +++ b/drivers/staging/media/sunxi/cedrus/cedrus_mpeg2.c
> @@ -13,30 +13,6 @@
>  #include "cedrus_hw.h"
>  #include "cedrus_regs.h"
>  
> -/* Default MPEG-2 quantisation coefficients, from the specification. */
> -
> -static const u8 intra_quantisation_matrix_default[64] = {
> -	8,  16, 16, 19, 16, 19, 22, 22,
> -	22, 22, 22, 22, 26, 24, 26, 27,
> -	27, 27, 26, 26, 26, 26, 27, 27,
> -	27, 29, 29, 29, 34, 34, 34, 29,
> -	29, 29, 27, 27, 29, 29, 32, 32,
> -	34, 34, 37, 38, 37, 35, 35, 34,
> -	35, 38, 38, 40, 40, 40, 48, 48,
> -	46, 46, 56, 56, 58, 69, 69, 83
> -};
> -
> -static const u8 non_intra_quantisation_matrix_default[64] = {
> -	16, 16, 16, 16, 16, 16, 16, 16,
> -	16, 16, 16, 16, 16, 16, 16, 16,
> -	16, 16, 16, 16, 16, 16, 16, 16,
> -	16, 16, 16, 16, 16, 16, 16, 16,
> -	16, 16, 16, 16, 16, 16, 16, 16,
> -	16, 16, 16, 16, 16, 16, 16, 16,
> -	16, 16, 16, 16, 16, 16, 16, 16,
> -	16, 16, 16, 16, 16, 16, 16, 16
> -};
> -
>  static enum cedrus_irq_status cedrus_mpeg2_irq_status(struct cedrus_ctx *ctx)
>  {
>  	struct cedrus_dev *dev = ctx->dev;
> @@ -99,12 +75,7 @@ static void cedrus_mpeg2_setup(struct cedrus_ctx *ctx, struct cedrus_run *run)
>  	cedrus_engine_enable(ctx, CEDRUS_CODEC_MPEG2);
>  
>  	/* Set intra quantisation matrix. */
> -
> -	if (quantisation && quantisation->load_intra_quantiser_matrix)
> -		matrix = quantisation->intra_quantiser_matrix;
> -	else
> -		matrix = intra_quantisation_matrix_default;
> -
> +	matrix = quantisation->intra_quantiser_matrix;
>  	for (i = 0; i < 64; i++) {
>  		reg = VE_DEC_MPEG_IQMINPUT_WEIGHT(i, matrix[i]);
>  		reg |= VE_DEC_MPEG_IQMINPUT_FLAG_INTRA;
> @@ -113,12 +84,7 @@ static void cedrus_mpeg2_setup(struct cedrus_ctx *ctx, struct cedrus_run *run)
>  	}
>  
>  	/* Set non-intra quantisation matrix. */
> -
> -	if (quantisation && quantisation->load_non_intra_quantiser_matrix)
> -		matrix = quantisation->non_intra_quantiser_matrix;
> -	else
> -		matrix = non_intra_quantisation_matrix_default;
> -
> +	matrix = quantisation->non_intra_quantiser_matrix;
>  	for (i = 0; i < 64; i++) {
>  		reg = VE_DEC_MPEG_IQMINPUT_WEIGHT(i, matrix[i]);
>  		reg |= VE_DEC_MPEG_IQMINPUT_FLAG_NON_INTRA;
> diff --git a/include/media/mpeg2-ctrls.h b/include/media/mpeg2-ctrls.h
> index b8adf3ac2c1d..8ea2c7f3a172 100644
> --- a/include/media/mpeg2-ctrls.h
> +++ b/include/media/mpeg2-ctrls.h
> @@ -68,11 +68,6 @@ struct v4l2_ctrl_mpeg2_slice_params {
>  
>  struct v4l2_ctrl_mpeg2_quantisation {
>  	/* ISO/IEC 13818-2, ITU-T Rec. H.262: Quant matrix extension */
> -	__u8	load_intra_quantiser_matrix;
> -	__u8	load_non_intra_quantiser_matrix;
> -	__u8	load_chroma_intra_quantiser_matrix;
> -	__u8	load_chroma_non_intra_quantiser_matrix;
> -
>  	__u8	intra_quantiser_matrix[64];
>  	__u8	non_intra_quantiser_matrix[64];
>  	__u8	chroma_intra_quantiser_matrix[64];
> 

Regards,

	Hans