Re: [PATCH v2 1/2] media: v4l2-ctrls: add encoder maximum bitrate control

[Nicolas Dufresne]

Hi Scahin,

Le mardi 30 janvier 2024 à 16:53 +0530, Sachin Kumar Garg a écrit :
> Introduce V4L2_MPEG_VIDEO_BITRATE_MODE_MBR rate control to
> limit the frame level maximum bit rate.
> Encoder will choose appropriate quantization parameter and
> do the smart bit allocation to set the frame maximum bitrate
> level as per the Bitrate value configured.
> 
> Signed-off-by: Sachin Kumar Garg <quic_sachinku@xxxxxxxxxxx>
> ---
>  Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst | 2 ++
>  drivers/media/v4l2-core/v4l2-ctrls-defs.c                 | 1 +
>  include/uapi/linux/v4l2-controls.h                        | 1 +
>  3 files changed, 4 insertions(+)
> 
> diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
> index 2a165ae063fb..05ef4a70e3f5 100644
> --- a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
> +++ b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
> @@ -576,6 +576,8 @@ enum v4l2_mpeg_video_bitrate_mode -
>        - Constant bitrate
>      * - ``V4L2_MPEG_VIDEO_BITRATE_MODE_CQ``
>        - Constant quality
> +    * - ``V4L2_MPEG_VIDEO_BITRATE_MODE_MBR``
> +      - Maximum bitrate

I'm afraid for this one your documentation is too short. I believe your commit
message helps, but this is not what our uAPI users will read.

My understanding is that this feature is a form of constant quality (smart bit
allocation) but with a maximum rate guaranty. Using a specific mode (rather then
a constraint on top of a constant quality mode) is a Qualcomm specific design. I
think presets are generally easier to use, so I kind of like it. What is missing
(arguably all these modes documentation are also missing it) is the rate
observation window. Would be nice to check if there is a way to specify that (or
even configure it, if so add a cross reference).

So I'd like to see some proper documentation for this one, remember that V4L2
documentation is also a specification and will serve to ensure drivers conforms
to the preset expectations.

regards,
Nicolas

>  
>  
>  
> diff --git a/drivers/media/v4l2-core/v4l2-ctrls-defs.c b/drivers/media/v4l2-core/v4l2-ctrls-defs.c
> index 8696eb1cdd61..e0597b61ffb9 100644
> --- a/drivers/media/v4l2-core/v4l2-ctrls-defs.c
> +++ b/drivers/media/v4l2-core/v4l2-ctrls-defs.c
> @@ -154,6 +154,7 @@ const char * const *v4l2_ctrl_get_menu(u32 id)
>  		"Variable Bitrate",
>  		"Constant Bitrate",
>  		"Constant Quality",
> +		"Maximum Bitrate",
>  		NULL
>  	};
>  	static const char * const mpeg_stream_type[] = {
> diff --git a/include/uapi/linux/v4l2-controls.h b/include/uapi/linux/v4l2-controls.h
> index 99c3f5e99da7..7c74d6c417d1 100644
> --- a/include/uapi/linux/v4l2-controls.h
> +++ b/include/uapi/linux/v4l2-controls.h
> @@ -393,6 +393,7 @@ 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,
> +	V4L2_MPEG_VIDEO_BITRATE_MODE_MBR = 3,
>  };
>  #define V4L2_CID_MPEG_VIDEO_BITRATE		(V4L2_CID_CODEC_BASE+207)
>  #define V4L2_CID_MPEG_VIDEO_BITRATE_PEAK	(V4L2_CID_CODEC_BASE+208)