Re: [PATCH] media: v4l2-ctrls: Add encoder constant quality control

[Hans Verkuil <hverkuil-cisco@xs4all.nl>]

On 23/05/2020 03:05, Maheshwar Ajja wrote:
> When V4L2_CID_MPEG_VIDEO_BITRATE_MODE value is
> V4L2_MPEG_VIDEO_BITRATE_MODE_CQ, encoder will produce
> constant quality output indicated by
> V4L2_CID_MPEG_VIDEO_CONSTANT_QUALITY control value.
> Encoder will choose appropriate quantization parameter
> and bitrate to produce requested frame quality level.
> 
> Signed-off-by: Maheshwar Ajja <majja@codeaurora.org>
> ---
>  Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst | 10 ++++++++++
>  drivers/media/v4l2-core/v4l2-ctrls.c                      |  2 ++
>  include/uapi/linux/v4l2-controls.h                        |  2 ++
>  3 files changed, 14 insertions(+)
> 
> diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
> index d0d506a..b9d3f7a 100644
> --- a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
> +++ b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
> @@ -581,6 +581,8 @@ enum v4l2_mpeg_video_bitrate_mode -
>        - Variable bitrate
>      * - ``V4L2_MPEG_VIDEO_BITRATE_MODE_CBR``
>        - Constant bitrate
> +    * - ``V4L2_MPEG_VIDEO_BITRATE_MODE_CQ``
> +      - Constant quality
>  
>  
>  
> @@ -592,6 +594,14 @@ enum v4l2_mpeg_video_bitrate_mode -
>      the average video bitrate. It is ignored if the video bitrate mode
>      is set to constant bitrate.
>  
> +``V4L2_CID_MPEG_VIDEO_CONSTANT_QUALITY (integer)``
> +    Constant quality level control. This control is applicable when
> +    ``V4L2_CID_MPEG_VIDEO_BITRATE_MODE`` value is
> +    ``V4L2_MPEG_VIDEO_BITRATE_MODE_CQ``. Valid range is 1 to 100
> +    where 1 indicates lowest quality and 100 indicates highest quality.
> +    Encoder will decide the appropriate quantization parameter and
> +    bitrate to produce requested frame quality.
> +
>  ``V4L2_CID_MPEG_VIDEO_TEMPORAL_DECIMATION (integer)``
>      For every captured frame, skip this many subsequent frames (default
>      0).
> diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c
> index 1c617b4..f94cc9d 100644
> --- a/drivers/media/v4l2-core/v4l2-ctrls.c
> +++ b/drivers/media/v4l2-core/v4l2-ctrls.c
> @@ -199,6 +199,7 @@ const char * const *v4l2_ctrl_get_menu(u32 id)
>  	static const char * const mpeg_video_bitrate_mode[] = {
>  		"Variable Bitrate",
>  		"Constant Bitrate",
> +		"Constant Quality",
>  		NULL
>  	};
>  	static const char * const mpeg_stream_type[] = {
> @@ -982,6 +983,7 @@ const char *v4l2_ctrl_get_name(u32 id)
>  	case V4L2_CID_MPEG_VIDEO_HEVC_SLICE_PARAMS:		return "HEVC Slice Parameters";
>  	case V4L2_CID_MPEG_VIDEO_HEVC_DECODE_MODE:		return "HEVC Decode Mode";
>  	case V4L2_CID_MPEG_VIDEO_HEVC_START_CODE:		return "HEVC Start Code";
> +	case V4L2_CID_MPEG_VIDEO_CONSTANT_QUALITY:		return "Constant Quality";

Add this to the codec controls that are earlier in this function (around line 925).

It's not HEVC specific, it can be used by any codec with constant quality support,
so it looks odd making this part of the HEVC controls.

Other than that, this looks good. Before it can be accepted we do need a driver
that supports this feature, so a second patch is needed that adds this to a driver
(I'm guessing that that will be the venus driver).

Regards,

	Hans

>  
>  	/* CAMERA controls */
>  	/* Keep the order of the 'case's the same as in v4l2-controls.h! */
> diff --git a/include/uapi/linux/v4l2-controls.h b/include/uapi/linux/v4l2-controls.h
> index 0ba1005..ca916da 100644
> --- a/include/uapi/linux/v4l2-controls.h
> +++ b/include/uapi/linux/v4l2-controls.h
> @@ -375,6 +375,7 @@ enum v4l2_mpeg_video_aspect {
>  enum v4l2_mpeg_video_bitrate_mode {
>  	V4L2_MPEG_VIDEO_BITRATE_MODE_VBR = 0,
>  	V4L2_MPEG_VIDEO_BITRATE_MODE_CBR = 1,
> +	V4L2_MPEG_VIDEO_BITRATE_MODE_CQ  = 2,
>  };
>  #define V4L2_CID_MPEG_VIDEO_BITRATE		(V4L2_CID_MPEG_BASE+207)
>  #define V4L2_CID_MPEG_VIDEO_BITRATE_PEAK	(V4L2_CID_MPEG_BASE+208)
> @@ -742,6 +743,7 @@ enum v4l2_cid_mpeg_video_hevc_size_of_length_field {
>  #define V4L2_CID_MPEG_VIDEO_HEVC_HIER_CODING_L6_BR	(V4L2_CID_MPEG_BASE + 642)
>  #define V4L2_CID_MPEG_VIDEO_REF_NUMBER_FOR_PFRAMES	(V4L2_CID_MPEG_BASE + 643)
>  #define V4L2_CID_MPEG_VIDEO_PREPEND_SPSPPS_TO_IDR	(V4L2_CID_MPEG_BASE + 644)
> +#define V4L2_CID_MPEG_VIDEO_CONSTANT_QUALITY		(V4L2_CID_MPEG_BASE + 645)
>  
>  /*  MPEG-class control IDs specific to the CX2341x driver as defined by V4L2 */
>  #define V4L2_CID_MPEG_CX2341X_BASE				(V4L2_CTRL_CLASS_MPEG | 0x1000)
>