Re: [PATCH v4 1/2] media: v4l2-ctrls: Add intra-refresh period control

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

On 02/06/2021 11:47, Stanimir Varbanov wrote:
> Add a control to set intra-refresh period.
> 
> Signed-off-by: Stanimir Varbanov <stanimir.varbanov@linaro.org>
> ---
>  .../media/v4l/ext-ctrls-codec.rst              | 18 +++++++++++++++++-
>  drivers/media/v4l2-core/v4l2-ctrls-defs.c      |  2 ++
>  include/uapi/linux/v4l2-controls.h             |  1 +
>  3 files changed, 20 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
> index 0b8061666c57..803fb655af1c 100644
> --- a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
> +++ b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
> @@ -1174,7 +1174,23 @@ enum v4l2_mpeg_video_h264_entropy_mode -
>      Cyclic intra macroblock refresh. This is the number of continuous
>      macroblocks refreshed every frame. Each frame a successive set of
>      macroblocks is refreshed until the cycle completes and starts from
> -    the top of the frame. Applicable to H264, H263 and MPEG4 encoder.
> +    the top of the frame.  Note that intra-refresh period is

intra-refresh period -> this control

> +    mutually exclusive with V4L2_CID_MPEG_VIDEO_INTRA_REFRESH_PERIOD

with -> with the

> +    control, aka the client must set this one or the other but not both.

'aka' is not really the right abbreviation to use. How about 'i.e.'?

> +    Applicable to H264, H263 and MPEG4 encoder.
> +
> +``V4L2_CID_MPEG_VIDEO_INTRA_REFRESH_PERIOD (integer)``
> +    Intra macroblock refresh period. This sets the period to refresh
> +    the whole frame. In other words, this defines the number of frames
> +    for which the whole frame will be intra-refreshed.  An example:
> +    setting period to 1 means that the whole frame will be refreshed,
> +    setting period to 2 means that the half of macroblocks will be
> +    intra-refreshed on frameX and the other half of macroblocks
> +    will be refreshed in frameX + 1 and so on. Setting period to zero

period -> the period

> +    means no period is specified.  Note that intra-refresh period is

intra-refresh period -> this control

> +    mutually exclusive with V4L2_CID_MPEG_VIDEO_CYCLIC_INTRA_REFRESH_MB

with -> with the

> +    control, aka the client must set this one or the other but not both.

aka -> i.e.

> +    Applicable to H264 and HEVC encoders.

This text is still problematic. Saying that 'the client must set this one or
the other but not both' makes no sense, since controls are *always* set to
some value. Now, V4L2_CID_MPEG_VIDEO_INTRA_REFRESH_PERIOD can be set to 0
to indicate that no period is specified. Can the same be done with
V4L2_CID_MPEG_VIDEO_CYCLIC_INTRA_REFRESH_MB? The description makes no mention
of that, perhaps it should be updated?

If V4L2_CID_MPEG_VIDEO_CYCLIC_INTRA_REFRESH_MB can be 0, then you can write
something like: 'both controls cannot both have non-0 values' (but better
phrased, but you get my drift). Or setting one should set the other to
0? Or if both are non-0, then control X takes priority. I'm not entirely sure
what works best. I think having one control have a higher prio might make the
most sense. I.e. "if V4L2_CID_MPEG_VIDEO_INTRA_REFRESH_PERIOD is not 0, then
V4L2_CID_MPEG_VIDEO_CYCLIC_INTRA_REFRESH_MB shall be ignored."

It's surprisingly hard to unambiguously define things like this!

Regards,

	Hans

>  
>  ``V4L2_CID_MPEG_VIDEO_FRAME_RC_ENABLE (boolean)``
>      Frame level rate control enable. If this control is disabled then
> diff --git a/drivers/media/v4l2-core/v4l2-ctrls-defs.c b/drivers/media/v4l2-core/v4l2-ctrls-defs.c
> index 7963c7b43450..3d73c28bafa7 100644
> --- a/drivers/media/v4l2-core/v4l2-ctrls-defs.c
> +++ b/drivers/media/v4l2-core/v4l2-ctrls-defs.c
> @@ -833,6 +833,7 @@ const char *v4l2_ctrl_get_name(u32 id)
>  	case V4L2_CID_MPEG_VIDEO_DECODER_SLICE_INTERFACE:	return "Decoder Slice Interface";
>  	case V4L2_CID_MPEG_VIDEO_DECODER_MPEG4_DEBLOCK_FILTER:	return "MPEG4 Loop Filter Enable";
>  	case V4L2_CID_MPEG_VIDEO_CYCLIC_INTRA_REFRESH_MB:	return "Number of Intra Refresh MBs";
> +	case V4L2_CID_MPEG_VIDEO_INTRA_REFRESH_PERIOD:		return "Intra Refresh Period";
>  	case V4L2_CID_MPEG_VIDEO_FRAME_RC_ENABLE:		return "Frame Level Rate Control Enable";
>  	case V4L2_CID_MPEG_VIDEO_MB_RC_ENABLE:			return "H264 MB Level Rate Control";
>  	case V4L2_CID_MPEG_VIDEO_HEADER_MODE:			return "Sequence Header Mode";
> @@ -1257,6 +1258,7 @@ void v4l2_ctrl_fill(u32 id, const char **name, enum v4l2_ctrl_type *type,
>  	case V4L2_CID_MPEG_VIDEO_MV_H_SEARCH_RANGE:
>  	case V4L2_CID_MPEG_VIDEO_MV_V_SEARCH_RANGE:
>  	case V4L2_CID_MPEG_VIDEO_DEC_DISPLAY_DELAY:
> +	case V4L2_CID_MPEG_VIDEO_INTRA_REFRESH_PERIOD:
>  		*type = V4L2_CTRL_TYPE_INTEGER;
>  		break;
>  	case V4L2_CID_MPEG_VIDEO_LTR_COUNT:
> diff --git a/include/uapi/linux/v4l2-controls.h b/include/uapi/linux/v4l2-controls.h
> index f96bea19c991..a34a18ec7f69 100644
> --- a/include/uapi/linux/v4l2-controls.h
> +++ b/include/uapi/linux/v4l2-controls.h
> @@ -434,6 +434,7 @@ enum v4l2_mpeg_video_multi_slice_mode {
>  #define V4L2_CID_MPEG_VIDEO_FRAME_LTR_INDEX		(V4L2_CID_CODEC_BASE+233)
>  #define V4L2_CID_MPEG_VIDEO_USE_LTR_FRAMES		(V4L2_CID_CODEC_BASE+234)
>  #define V4L2_CID_MPEG_VIDEO_DEC_CONCEAL_COLOR		(V4L2_CID_CODEC_BASE+235)
> +#define V4L2_CID_MPEG_VIDEO_INTRA_REFRESH_PERIOD	(V4L2_CID_CODEC_BASE+236)
>  
>  /* CIDs for the MPEG-2 Part 2 (H.262) codec */
>  #define V4L2_CID_MPEG_VIDEO_MPEG2_LEVEL			(V4L2_CID_CODEC_BASE+270)
>