[1/4] media: v4l2-ctrl: Add frame-skip std encoder control
diff mbox series

Message ID 20200705121128.5250-2-stanimir.varbanov@linaro.org
State New
Headers show
Series
  • Make Frame Skip Mode control a standard
Related show

Commit Message

Stanimir Varbanov July 5, 2020, 12:11 p.m. UTC
Adds encoders standard v4l2 control for frame-skip. The control
is a copy of a custom encoder control so that other v4l2 encoder
drivers can use it.

Signed-off-by: Stanimir Varbanov <stanimir.varbanov@linaro.org>
---
 .../media/v4l/ext-ctrls-codec.rst             | 32 +++++++++++++++++++
 drivers/media/v4l2-core/v4l2-ctrls.c          | 10 ++++++
 include/uapi/linux/v4l2-controls.h            |  6 ++++
 3 files changed, 48 insertions(+)

Comments

Nicolas Dufresne July 7, 2020, 8:53 p.m. UTC | #1
Le dimanche 05 juillet 2020 à 15:11 +0300, Stanimir Varbanov a écrit :
> Adds encoders standard v4l2 control for frame-skip. The control
> is a copy of a custom encoder control so that other v4l2 encoder
> drivers can use it.
> 
> Signed-off-by: Stanimir Varbanov <stanimir.varbanov@linaro.org>
> ---
>  .../media/v4l/ext-ctrls-codec.rst             | 32 +++++++++++++++++++
>  drivers/media/v4l2-core/v4l2-ctrls.c          | 10 ++++++
>  include/uapi/linux/v4l2-controls.h            |  6 ++++
>  3 files changed, 48 insertions(+)
> 
> diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
> index d0d506a444b1..a8b4c0b40747 100644
> --- a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
> +++ b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
> @@ -592,6 +592,38 @@ 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_FRAME_SKIP_MODE (enum)``
> +
> +enum v4l2_mpeg_video_frame_skip_mode -
> +    Indicates in what conditions the encoder should skip frames. If
> +    encoding a frame would cause the encoded stream to be larger then a
> +    chosen data limit then the frame will be skipped. Possible values
> +    are:

I have nothing against this API, in fact it's really nice to generalize
as this is very common. Though, I think we are missing two things. This
documentation refer to the "chosen data limit". Is there controls to
configure these *chosen* limit ? The other issue is the vagueness of
the documented mode, see lower...

> +
> +
> +.. tabularcolumns:: |p{9.2cm}|p{8.3cm}|
> +
> +.. raw:: latex
> +
> +    \small
> +
> +.. flat-table::
> +    :header-rows:  0
> +    :stub-columns: 0
> +
> +    * - ``V4L2_MPEG_FRAME_SKIP_MODE_DISABLED``
> +      - Frame skip mode is disabled.
> +    * - ``V4L2_MPEG_FRAME_SKIP_MODE_LEVEL_LIMIT``
> +      - Frame skip mode enabled and buffer limit is set by the chosen
> +	level and is defined by the standard.

At least for H.264, a level is compose of 3 limits. One is the maximum
number of macroblocks, this is is evidently not use for frame skipping
and already constrained in V4L2 (assuming the driver does not ignore
the level control of course). The two other limits are decoded
macroblocks/s and encoded kbits/s. Both are measure over time, which
means the M2M encoder needs to be timing aware. I think the time source
should be documented. Perhaps it is mandatory to set a frame interval
for this to work ? Or we need some timestamp to allow variable frame
interval ? (I don't think the second is really an option without
extending the API again, and confusingly, since I think we have used
the timestamp for other purpose already)

> +    * - ``V4L2_MPEG_FRAME_SKIP_MODE_BUF_LIMIT``
> +      - Frame skip mode enabled and buffer limit is set by the VBV
> +	(MPEG1/2/4) or CPB (H264) buffer size control.

The notion of VBV an CPB is unlikely well known. If my memory is right,
these are constrained in buffering: in bytes (VBV) or bits per frame
over a window of n-frames (or the gop size for some less flexible
encoder) (CPB). I think these should be somehow chosen by application
(with controls), directly or indirectly, and documented here to ensure
we get consistent implementation across drivers.

> +
> +.. raw:: latex
> +
> +    \normalsize
> +
>  ``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 3f3fbcd60cc6..d088acfa6dd8 100644
> --- a/drivers/media/v4l2-core/v4l2-ctrls.c
> +++ b/drivers/media/v4l2-core/v4l2-ctrls.c
> @@ -590,6 +590,12 @@ const char * const *v4l2_ctrl_get_menu(u32 id)
>  		"External",
>  		NULL,
>  	};
> +	static const char * const mpeg_video_frame_skip[] = {
> +		"Disabled",
> +		"Level Limit",
> +		"VBV/CPB Limit",
> +		NULL,
> +	};
>  
>  	switch (id) {
>  	case V4L2_CID_MPEG_AUDIO_SAMPLING_FREQ:
> @@ -651,6 +657,8 @@ const char * const *v4l2_ctrl_get_menu(u32 id)
>  		return flash_strobe_source;
>  	case V4L2_CID_MPEG_VIDEO_HEADER_MODE:
>  		return header_mode;
> +	case V4L2_CID_MPEG_VIDEO_FRAME_SKIP_MODE:
> +		return mpeg_video_frame_skip;
>  	case V4L2_CID_MPEG_VIDEO_MULTI_SLICE_MODE:
>  		return multi_slice;
>  	case V4L2_CID_MPEG_VIDEO_H264_ENTROPY_MODE:
> @@ -844,6 +852,7 @@ const char *v4l2_ctrl_get_name(u32 id)
>  	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";
>  	case V4L2_CID_MPEG_VIDEO_MAX_REF_PIC:			return "Max Number of Reference Pics";
> +	case V4L2_CID_MPEG_VIDEO_FRAME_SKIP_MODE:		return "Frame Skip Mode";
>  	case V4L2_CID_MPEG_VIDEO_H263_I_FRAME_QP:		return "H263 I-Frame QP Value";
>  	case V4L2_CID_MPEG_VIDEO_H263_P_FRAME_QP:		return "H263 P-Frame QP Value";
>  	case V4L2_CID_MPEG_VIDEO_H263_B_FRAME_QP:		return "H263 B-Frame QP Value";
> @@ -1265,6 +1274,7 @@ void v4l2_ctrl_fill(u32 id, const char **name, enum v4l2_ctrl_type *type,
>  	case V4L2_CID_FLASH_LED_MODE:
>  	case V4L2_CID_FLASH_STROBE_SOURCE:
>  	case V4L2_CID_MPEG_VIDEO_HEADER_MODE:
> +	case V4L2_CID_MPEG_VIDEO_FRAME_SKIP_MODE:
>  	case V4L2_CID_MPEG_VIDEO_MULTI_SLICE_MODE:
>  	case V4L2_CID_MPEG_VIDEO_H264_ENTROPY_MODE:
>  	case V4L2_CID_MPEG_VIDEO_H264_LEVEL:
> diff --git a/include/uapi/linux/v4l2-controls.h b/include/uapi/linux/v4l2-controls.h
> index 62271418c1be..4e1526175a4c 100644
> --- a/include/uapi/linux/v4l2-controls.h
> +++ b/include/uapi/linux/v4l2-controls.h
> @@ -742,6 +742,12 @@ 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_FRAME_SKIP_MODE		(V4L2_CID_MPEG_BASE + 645)
> +enum v4l2_mpeg_video_frame_skip_mode {
> +	V4L2_MPEG_VIDEO_FRAME_SKIP_MODE_DISABLED	= 0,
> +	V4L2_MPEG_VIDEO_FRAME_SKIP_MODE_LEVEL_LIMIT	= 1,
> +	V4L2_MPEG_VIDEO_FRAME_SKIP_MODE_BUF_LIMIT	= 2,
> +};
>  
>  /*  MPEG-class control IDs specific to the CX2341x driver as defined by V4L2 */
>  #define V4L2_CID_MPEG_CX2341X_BASE				(V4L2_CTRL_CLASS_MPEG | 0x1000)
Stanimir Varbanov July 15, 2020, 3:42 p.m. UTC | #2
Hi Nicolas,

On 7/7/20 11:53 PM, Nicolas Dufresne wrote:
> Le dimanche 05 juillet 2020 à 15:11 +0300, Stanimir Varbanov a écrit :
>> Adds encoders standard v4l2 control for frame-skip. The control
>> is a copy of a custom encoder control so that other v4l2 encoder
>> drivers can use it.
>>
>> Signed-off-by: Stanimir Varbanov <stanimir.varbanov@linaro.org>
>> ---
>>  .../media/v4l/ext-ctrls-codec.rst             | 32 +++++++++++++++++++
>>  drivers/media/v4l2-core/v4l2-ctrls.c          | 10 ++++++
>>  include/uapi/linux/v4l2-controls.h            |  6 ++++
>>  3 files changed, 48 insertions(+)
>>
>> diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
>> index d0d506a444b1..a8b4c0b40747 100644
>> --- a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
>> +++ b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
>> @@ -592,6 +592,38 @@ 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_FRAME_SKIP_MODE (enum)``
>> +
>> +enum v4l2_mpeg_video_frame_skip_mode -
>> +    Indicates in what conditions the encoder should skip frames. If
>> +    encoding a frame would cause the encoded stream to be larger then a
>> +    chosen data limit then the frame will be skipped. Possible values
>> +    are:
> 
> I have nothing against this API, in fact it's really nice to generalize
> as this is very common. Though, I think we are missing two things. This
> documentation refer to the "chosen data limit". Is there controls to
> configure these *chosen* limit ? The other issue is the vagueness of
> the documented mode, see lower...
> 
>> +
>> +
>> +.. tabularcolumns:: |p{9.2cm}|p{8.3cm}|
>> +
>> +.. raw:: latex
>> +
>> +    \small
>> +
>> +.. flat-table::
>> +    :header-rows:  0
>> +    :stub-columns: 0
>> +
>> +    * - ``V4L2_MPEG_FRAME_SKIP_MODE_DISABLED``
>> +      - Frame skip mode is disabled.
>> +    * - ``V4L2_MPEG_FRAME_SKIP_MODE_LEVEL_LIMIT``
>> +      - Frame skip mode enabled and buffer limit is set by the chosen
>> +	level and is defined by the standard.
> 
> At least for H.264, a level is compose of 3 limits. One is the maximum
> number of macroblocks, this is is evidently not use for frame skipping
> and already constrained in V4L2 (assuming the driver does not ignore
> the level control of course). The two other limits are decoded
> macroblocks/s and encoded kbits/s. Both are measure over time, which
> means the M2M encoder needs to be timing aware. I think the time source
> should be documented. Perhaps it is mandatory to set a frame interval
> for this to work ? Or we need some timestamp to allow variable frame
> interval ? (I don't think the second is really an option without
> extending the API again, and confusingly, since I think we have used
> the timestamp for other purpose already)

Do you want to say that the encoder input timestamp, bitrate control
(V4L2_CID_MPEG_VIDEO_BITRATE) and S_PARM is not enough to describe
FRAME_SKIP_MODE_LEVEL_LIMIT mode?

> 
>> +    * - ``V4L2_MPEG_FRAME_SKIP_MODE_BUF_LIMIT``
>> +      - Frame skip mode enabled and buffer limit is set by the VBV
>> +	(MPEG1/2/4) or CPB (H264) buffer size control.
> 
> The notion of VBV an CPB is unlikely well known. If my memory is right,
> these are constrained in buffering: in bytes (VBV) or bits per frame
> over a window of n-frames (or the gop size for some less flexible
> encoder) (CPB). I think these should be somehow chosen by application
> (with controls), directly or indirectly, and documented here to ensure
> we get consistent implementation across drivers.

I guess you want me to add here references to the following controls:

V4L2_CID_MPEG_VIDEO_VBV_SIZE
V4L2_CID_MPEG_VIDEO_VBV_DELAY
V4L2_CID_MPEG_VIDEO_H264_CPB_SIZE

?

> 
>> +
>> +.. raw:: latex
>> +
>> +    \normalsize
>> +
>>  ``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 3f3fbcd60cc6..d088acfa6dd8 100644
>> --- a/drivers/media/v4l2-core/v4l2-ctrls.c
>> +++ b/drivers/media/v4l2-core/v4l2-ctrls.c
>> @@ -590,6 +590,12 @@ const char * const *v4l2_ctrl_get_menu(u32 id)
>>  		"External",
>>  		NULL,
>>  	};
>> +	static const char * const mpeg_video_frame_skip[] = {
>> +		"Disabled",
>> +		"Level Limit",
>> +		"VBV/CPB Limit",
>> +		NULL,
>> +	};
>>  
>>  	switch (id) {
>>  	case V4L2_CID_MPEG_AUDIO_SAMPLING_FREQ:
>> @@ -651,6 +657,8 @@ const char * const *v4l2_ctrl_get_menu(u32 id)
>>  		return flash_strobe_source;
>>  	case V4L2_CID_MPEG_VIDEO_HEADER_MODE:
>>  		return header_mode;
>> +	case V4L2_CID_MPEG_VIDEO_FRAME_SKIP_MODE:
>> +		return mpeg_video_frame_skip;
>>  	case V4L2_CID_MPEG_VIDEO_MULTI_SLICE_MODE:
>>  		return multi_slice;
>>  	case V4L2_CID_MPEG_VIDEO_H264_ENTROPY_MODE:
>> @@ -844,6 +852,7 @@ const char *v4l2_ctrl_get_name(u32 id)
>>  	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";
>>  	case V4L2_CID_MPEG_VIDEO_MAX_REF_PIC:			return "Max Number of Reference Pics";
>> +	case V4L2_CID_MPEG_VIDEO_FRAME_SKIP_MODE:		return "Frame Skip Mode";
>>  	case V4L2_CID_MPEG_VIDEO_H263_I_FRAME_QP:		return "H263 I-Frame QP Value";
>>  	case V4L2_CID_MPEG_VIDEO_H263_P_FRAME_QP:		return "H263 P-Frame QP Value";
>>  	case V4L2_CID_MPEG_VIDEO_H263_B_FRAME_QP:		return "H263 B-Frame QP Value";
>> @@ -1265,6 +1274,7 @@ void v4l2_ctrl_fill(u32 id, const char **name, enum v4l2_ctrl_type *type,
>>  	case V4L2_CID_FLASH_LED_MODE:
>>  	case V4L2_CID_FLASH_STROBE_SOURCE:
>>  	case V4L2_CID_MPEG_VIDEO_HEADER_MODE:
>> +	case V4L2_CID_MPEG_VIDEO_FRAME_SKIP_MODE:
>>  	case V4L2_CID_MPEG_VIDEO_MULTI_SLICE_MODE:
>>  	case V4L2_CID_MPEG_VIDEO_H264_ENTROPY_MODE:
>>  	case V4L2_CID_MPEG_VIDEO_H264_LEVEL:
>> diff --git a/include/uapi/linux/v4l2-controls.h b/include/uapi/linux/v4l2-controls.h
>> index 62271418c1be..4e1526175a4c 100644
>> --- a/include/uapi/linux/v4l2-controls.h
>> +++ b/include/uapi/linux/v4l2-controls.h
>> @@ -742,6 +742,12 @@ 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_FRAME_SKIP_MODE		(V4L2_CID_MPEG_BASE + 645)
>> +enum v4l2_mpeg_video_frame_skip_mode {
>> +	V4L2_MPEG_VIDEO_FRAME_SKIP_MODE_DISABLED	= 0,
>> +	V4L2_MPEG_VIDEO_FRAME_SKIP_MODE_LEVEL_LIMIT	= 1,
>> +	V4L2_MPEG_VIDEO_FRAME_SKIP_MODE_BUF_LIMIT	= 2,
>> +};
>>  
>>  /*  MPEG-class control IDs specific to the CX2341x driver as defined by V4L2 */
>>  #define V4L2_CID_MPEG_CX2341X_BASE				(V4L2_CTRL_CLASS_MPEG | 0x1000)
>
Nicolas Dufresne July 15, 2020, 6:12 p.m. UTC | #3
Le mercredi 15 juillet 2020 à 18:42 +0300, Stanimir Varbanov a écrit :
> Hi Nicolas,
> 
> On 7/7/20 11:53 PM, Nicolas Dufresne wrote:
> > Le dimanche 05 juillet 2020 à 15:11 +0300, Stanimir Varbanov a écrit :
> > > Adds encoders standard v4l2 control for frame-skip. The control
> > > is a copy of a custom encoder control so that other v4l2 encoder
> > > drivers can use it.
> > > 
> > > Signed-off-by: Stanimir Varbanov <stanimir.varbanov@linaro.org>
> > > ---
> > >  .../media/v4l/ext-ctrls-codec.rst             | 32 +++++++++++++++++++
> > >  drivers/media/v4l2-core/v4l2-ctrls.c          | 10 ++++++
> > >  include/uapi/linux/v4l2-controls.h            |  6 ++++
> > >  3 files changed, 48 insertions(+)
> > > 
> > > diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
> > > index d0d506a444b1..a8b4c0b40747 100644
> > > --- a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
> > > +++ b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
> > > @@ -592,6 +592,38 @@ 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_FRAME_SKIP_MODE (enum)``
> > > +
> > > +enum v4l2_mpeg_video_frame_skip_mode -
> > > +    Indicates in what conditions the encoder should skip frames. If
> > > +    encoding a frame would cause the encoded stream to be larger then a
> > > +    chosen data limit then the frame will be skipped. Possible values
> > > +    are:
> > 
> > I have nothing against this API, in fact it's really nice to generalize
> > as this is very common. Though, I think we are missing two things. This
> > documentation refer to the "chosen data limit". Is there controls to
> > configure these *chosen* limit ? The other issue is the vagueness of
> > the documented mode, see lower...
> > 
> > > +
> > > +
> > > +.. tabularcolumns:: |p{9.2cm}|p{8.3cm}|
> > > +
> > > +.. raw:: latex
> > > +
> > > +    \small
> > > +
> > > +.. flat-table::
> > > +    :header-rows:  0
> > > +    :stub-columns: 0
> > > +
> > > +    * - ``V4L2_MPEG_FRAME_SKIP_MODE_DISABLED``
> > > +      - Frame skip mode is disabled.
> > > +    * - ``V4L2_MPEG_FRAME_SKIP_MODE_LEVEL_LIMIT``
> > > +      - Frame skip mode enabled and buffer limit is set by the chosen
> > > +	level and is defined by the standard.
> > 
> > At least for H.264, a level is compose of 3 limits. One is the maximum
> > number of macroblocks, this is is evidently not use for frame skipping
> > and already constrained in V4L2 (assuming the driver does not ignore
> > the level control of course). The two other limits are decoded
> > macroblocks/s and encoded kbits/s. Both are measure over time, which
> > means the M2M encoder needs to be timing aware. I think the time source
> > should be documented. Perhaps it is mandatory to set a frame interval
> > for this to work ? Or we need some timestamp to allow variable frame
> > interval ? (I don't think the second is really an option without
> > extending the API again, and confusingly, since I think we have used
> > the timestamp for other purpose already)
> 
> Do you want to say that the encoder input timestamp, bitrate control
> (V4L2_CID_MPEG_VIDEO_BITRATE) and S_PARM is not enough to describe
> FRAME_SKIP_MODE_LEVEL_LIMIT mode?

I don't think we have spec to give the input timestamp a meaning that
driver can interpret. In fact I think we gave it a meaning that the
driver must not interpret it (aka driver opaque). So remain S_PARM to
give a clue, but some stream don't have a framerate (like RTP streams,
unless written in bitstream).

> 
> > > +    * - ``V4L2_MPEG_FRAME_SKIP_MODE_BUF_LIMIT``
> > > +      - Frame skip mode enabled and buffer limit is set by the VBV
> > > +	(MPEG1/2/4) or CPB (H264) buffer size control.
> > 
> > The notion of VBV an CPB is unlikely well known. If my memory is right,
> > these are constrained in buffering: in bytes (VBV) or bits per frame
> > over a window of n-frames (or the gop size for some less flexible
> > encoder) (CPB). I think these should be somehow chosen by application
> > (with controls), directly or indirectly, and documented here to ensure
> > we get consistent implementation across drivers.
> 
> I guess you want me to add here references to the following controls:
> 
> V4L2_CID_MPEG_VIDEO_VBV_SIZE
> V4L2_CID_MPEG_VIDEO_VBV_DELAY
> V4L2_CID_MPEG_VIDEO_H264_CPB_SIZE

Perhaps it would be helpful yes.

> 
> ?
> 
> > > +
> > > +.. raw:: latex
> > > +
> > > +    \normalsize
> > > +
> > >  ``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 3f3fbcd60cc6..d088acfa6dd8 100644
> > > --- a/drivers/media/v4l2-core/v4l2-ctrls.c
> > > +++ b/drivers/media/v4l2-core/v4l2-ctrls.c
> > > @@ -590,6 +590,12 @@ const char * const *v4l2_ctrl_get_menu(u32 id)
> > >  		"External",
> > >  		NULL,
> > >  	};
> > > +	static const char * const mpeg_video_frame_skip[] = {
> > > +		"Disabled",
> > > +		"Level Limit",
> > > +		"VBV/CPB Limit",
> > > +		NULL,
> > > +	};
> > >  
> > >  	switch (id) {
> > >  	case V4L2_CID_MPEG_AUDIO_SAMPLING_FREQ:
> > > @@ -651,6 +657,8 @@ const char * const *v4l2_ctrl_get_menu(u32 id)
> > >  		return flash_strobe_source;
> > >  	case V4L2_CID_MPEG_VIDEO_HEADER_MODE:
> > >  		return header_mode;
> > > +	case V4L2_CID_MPEG_VIDEO_FRAME_SKIP_MODE:
> > > +		return mpeg_video_frame_skip;
> > >  	case V4L2_CID_MPEG_VIDEO_MULTI_SLICE_MODE:
> > >  		return multi_slice;
> > >  	case V4L2_CID_MPEG_VIDEO_H264_ENTROPY_MODE:
> > > @@ -844,6 +852,7 @@ const char *v4l2_ctrl_get_name(u32 id)
> > >  	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";
> > >  	case V4L2_CID_MPEG_VIDEO_MAX_REF_PIC:			return "Max Number of Reference Pics";
> > > +	case V4L2_CID_MPEG_VIDEO_FRAME_SKIP_MODE:		return "Frame Skip Mode";
> > >  	case V4L2_CID_MPEG_VIDEO_H263_I_FRAME_QP:		return "H263 I-Frame QP Value";
> > >  	case V4L2_CID_MPEG_VIDEO_H263_P_FRAME_QP:		return "H263 P-Frame QP Value";
> > >  	case V4L2_CID_MPEG_VIDEO_H263_B_FRAME_QP:		return "H263 B-Frame QP Value";
> > > @@ -1265,6 +1274,7 @@ void v4l2_ctrl_fill(u32 id, const char **name, enum v4l2_ctrl_type *type,
> > >  	case V4L2_CID_FLASH_LED_MODE:
> > >  	case V4L2_CID_FLASH_STROBE_SOURCE:
> > >  	case V4L2_CID_MPEG_VIDEO_HEADER_MODE:
> > > +	case V4L2_CID_MPEG_VIDEO_FRAME_SKIP_MODE:
> > >  	case V4L2_CID_MPEG_VIDEO_MULTI_SLICE_MODE:
> > >  	case V4L2_CID_MPEG_VIDEO_H264_ENTROPY_MODE:
> > >  	case V4L2_CID_MPEG_VIDEO_H264_LEVEL:
> > > diff --git a/include/uapi/linux/v4l2-controls.h b/include/uapi/linux/v4l2-controls.h
> > > index 62271418c1be..4e1526175a4c 100644
> > > --- a/include/uapi/linux/v4l2-controls.h
> > > +++ b/include/uapi/linux/v4l2-controls.h
> > > @@ -742,6 +742,12 @@ 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_FRAME_SKIP_MODE		(V4L2_CID_MPEG_BASE + 645)
> > > +enum v4l2_mpeg_video_frame_skip_mode {
> > > +	V4L2_MPEG_VIDEO_FRAME_SKIP_MODE_DISABLED	= 0,
> > > +	V4L2_MPEG_VIDEO_FRAME_SKIP_MODE_LEVEL_LIMIT	= 1,
> > > +	V4L2_MPEG_VIDEO_FRAME_SKIP_MODE_BUF_LIMIT	= 2,
> > > +};
> > >  
> > >  /*  MPEG-class control IDs specific to the CX2341x driver as defined by V4L2 */
> > >  #define V4L2_CID_MPEG_CX2341X_BASE				(V4L2_CTRL_CLASS_MPEG | 0x1000)
Stanimir Varbanov July 16, 2020, 10:50 a.m. UTC | #4
On 7/15/20 9:12 PM, Nicolas Dufresne wrote:
> Le mercredi 15 juillet 2020 à 18:42 +0300, Stanimir Varbanov a écrit :
>> Hi Nicolas,
>>
>> On 7/7/20 11:53 PM, Nicolas Dufresne wrote:
>>> Le dimanche 05 juillet 2020 à 15:11 +0300, Stanimir Varbanov a écrit :
>>>> Adds encoders standard v4l2 control for frame-skip. The control
>>>> is a copy of a custom encoder control so that other v4l2 encoder
>>>> drivers can use it.
>>>>
>>>> Signed-off-by: Stanimir Varbanov <stanimir.varbanov@linaro.org>
>>>> ---
>>>>  .../media/v4l/ext-ctrls-codec.rst             | 32 +++++++++++++++++++
>>>>  drivers/media/v4l2-core/v4l2-ctrls.c          | 10 ++++++
>>>>  include/uapi/linux/v4l2-controls.h            |  6 ++++
>>>>  3 files changed, 48 insertions(+)
>>>>
>>>> diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
>>>> index d0d506a444b1..a8b4c0b40747 100644
>>>> --- a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
>>>> +++ b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
>>>> @@ -592,6 +592,38 @@ 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_FRAME_SKIP_MODE (enum)``
>>>> +
>>>> +enum v4l2_mpeg_video_frame_skip_mode -
>>>> +    Indicates in what conditions the encoder should skip frames. If
>>>> +    encoding a frame would cause the encoded stream to be larger then a
>>>> +    chosen data limit then the frame will be skipped. Possible values
>>>> +    are:
>>>
>>> I have nothing against this API, in fact it's really nice to generalize
>>> as this is very common. Though, I think we are missing two things. This
>>> documentation refer to the "chosen data limit". Is there controls to
>>> configure these *chosen* limit ? The other issue is the vagueness of
>>> the documented mode, see lower...
>>>
>>>> +
>>>> +
>>>> +.. tabularcolumns:: |p{9.2cm}|p{8.3cm}|
>>>> +
>>>> +.. raw:: latex
>>>> +
>>>> +    \small
>>>> +
>>>> +.. flat-table::
>>>> +    :header-rows:  0
>>>> +    :stub-columns: 0
>>>> +
>>>> +    * - ``V4L2_MPEG_FRAME_SKIP_MODE_DISABLED``
>>>> +      - Frame skip mode is disabled.
>>>> +    * - ``V4L2_MPEG_FRAME_SKIP_MODE_LEVEL_LIMIT``
>>>> +      - Frame skip mode enabled and buffer limit is set by the chosen
>>>> +	level and is defined by the standard.
>>>
>>> At least for H.264, a level is compose of 3 limits. One is the maximum
>>> number of macroblocks, this is is evidently not use for frame skipping
>>> and already constrained in V4L2 (assuming the driver does not ignore
>>> the level control of course). The two other limits are decoded
>>> macroblocks/s and encoded kbits/s. Both are measure over time, which
>>> means the M2M encoder needs to be timing aware. I think the time source
>>> should be documented. Perhaps it is mandatory to set a frame interval
>>> for this to work ? Or we need some timestamp to allow variable frame
>>> interval ? (I don't think the second is really an option without
>>> extending the API again, and confusingly, since I think we have used
>>> the timestamp for other purpose already)
>>
>> Do you want to say that the encoder input timestamp, bitrate control
>> (V4L2_CID_MPEG_VIDEO_BITRATE) and S_PARM is not enough to describe
>> FRAME_SKIP_MODE_LEVEL_LIMIT mode?
> 
> I don't think we have spec to give the input timestamp a meaning that
> driver can interpret. In fact I think we gave it a meaning that the
> driver must not interpret it (aka driver opaque). So remain S_PARM to

At least for Venus the timestamps are passed to the firmware and used by
encoder rate-controller.

> give a clue, but some stream don't have a framerate (like RTP streams,
> unless written in bitstream).
I think v4l2 clients should be able to guess what would be the frame
rate in such cases, no?
Hans Verkuil July 20, 2020, 9:36 a.m. UTC | #5
On 05/07/2020 14:11, Stanimir Varbanov wrote:
> Adds encoders standard v4l2 control for frame-skip. The control
> is a copy of a custom encoder control so that other v4l2 encoder
> drivers can use it.
> 
> Signed-off-by: Stanimir Varbanov <stanimir.varbanov@linaro.org>

Reviewed-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>

But see note at the end.

> ---
>  .../media/v4l/ext-ctrls-codec.rst             | 32 +++++++++++++++++++
>  drivers/media/v4l2-core/v4l2-ctrls.c          | 10 ++++++
>  include/uapi/linux/v4l2-controls.h            |  6 ++++
>  3 files changed, 48 insertions(+)
> 
> diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
> index d0d506a444b1..a8b4c0b40747 100644
> --- a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
> +++ b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
> @@ -592,6 +592,38 @@ 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_FRAME_SKIP_MODE (enum)``
> +
> +enum v4l2_mpeg_video_frame_skip_mode -
> +    Indicates in what conditions the encoder should skip frames. If
> +    encoding a frame would cause the encoded stream to be larger then a
> +    chosen data limit then the frame will be skipped. Possible values
> +    are:
> +
> +
> +.. tabularcolumns:: |p{9.2cm}|p{8.3cm}|
> +
> +.. raw:: latex
> +
> +    \small
> +
> +.. flat-table::
> +    :header-rows:  0
> +    :stub-columns: 0
> +
> +    * - ``V4L2_MPEG_FRAME_SKIP_MODE_DISABLED``
> +      - Frame skip mode is disabled.
> +    * - ``V4L2_MPEG_FRAME_SKIP_MODE_LEVEL_LIMIT``
> +      - Frame skip mode enabled and buffer limit is set by the chosen
> +	level and is defined by the standard.
> +    * - ``V4L2_MPEG_FRAME_SKIP_MODE_BUF_LIMIT``
> +      - Frame skip mode enabled and buffer limit is set by the VBV
> +	(MPEG1/2/4) or CPB (H264) buffer size control.
> +
> +.. raw:: latex
> +
> +    \normalsize
> +
>  ``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 3f3fbcd60cc6..d088acfa6dd8 100644
> --- a/drivers/media/v4l2-core/v4l2-ctrls.c
> +++ b/drivers/media/v4l2-core/v4l2-ctrls.c
> @@ -590,6 +590,12 @@ const char * const *v4l2_ctrl_get_menu(u32 id)
>  		"External",
>  		NULL,
>  	};
> +	static const char * const mpeg_video_frame_skip[] = {
> +		"Disabled",
> +		"Level Limit",
> +		"VBV/CPB Limit",
> +		NULL,
> +	};
>  
>  	switch (id) {
>  	case V4L2_CID_MPEG_AUDIO_SAMPLING_FREQ:
> @@ -651,6 +657,8 @@ const char * const *v4l2_ctrl_get_menu(u32 id)
>  		return flash_strobe_source;
>  	case V4L2_CID_MPEG_VIDEO_HEADER_MODE:
>  		return header_mode;
> +	case V4L2_CID_MPEG_VIDEO_FRAME_SKIP_MODE:
> +		return mpeg_video_frame_skip;
>  	case V4L2_CID_MPEG_VIDEO_MULTI_SLICE_MODE:
>  		return multi_slice;
>  	case V4L2_CID_MPEG_VIDEO_H264_ENTROPY_MODE:
> @@ -844,6 +852,7 @@ const char *v4l2_ctrl_get_name(u32 id)
>  	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";
>  	case V4L2_CID_MPEG_VIDEO_MAX_REF_PIC:			return "Max Number of Reference Pics";
> +	case V4L2_CID_MPEG_VIDEO_FRAME_SKIP_MODE:		return "Frame Skip Mode";
>  	case V4L2_CID_MPEG_VIDEO_H263_I_FRAME_QP:		return "H263 I-Frame QP Value";
>  	case V4L2_CID_MPEG_VIDEO_H263_P_FRAME_QP:		return "H263 P-Frame QP Value";
>  	case V4L2_CID_MPEG_VIDEO_H263_B_FRAME_QP:		return "H263 B-Frame QP Value";
> @@ -1265,6 +1274,7 @@ void v4l2_ctrl_fill(u32 id, const char **name, enum v4l2_ctrl_type *type,
>  	case V4L2_CID_FLASH_LED_MODE:
>  	case V4L2_CID_FLASH_STROBE_SOURCE:
>  	case V4L2_CID_MPEG_VIDEO_HEADER_MODE:
> +	case V4L2_CID_MPEG_VIDEO_FRAME_SKIP_MODE:
>  	case V4L2_CID_MPEG_VIDEO_MULTI_SLICE_MODE:
>  	case V4L2_CID_MPEG_VIDEO_H264_ENTROPY_MODE:
>  	case V4L2_CID_MPEG_VIDEO_H264_LEVEL:
> diff --git a/include/uapi/linux/v4l2-controls.h b/include/uapi/linux/v4l2-controls.h
> index 62271418c1be..4e1526175a4c 100644
> --- a/include/uapi/linux/v4l2-controls.h
> +++ b/include/uapi/linux/v4l2-controls.h
> @@ -742,6 +742,12 @@ 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_FRAME_SKIP_MODE		(V4L2_CID_MPEG_BASE + 645)

I think this now clashes with "media: v4l2-ctrls: Add encoder constant quality control".

I recommend making a new series that combines both series. That avoid this problem.

Regards,

	Hans

> +enum v4l2_mpeg_video_frame_skip_mode {
> +	V4L2_MPEG_VIDEO_FRAME_SKIP_MODE_DISABLED	= 0,
> +	V4L2_MPEG_VIDEO_FRAME_SKIP_MODE_LEVEL_LIMIT	= 1,
> +	V4L2_MPEG_VIDEO_FRAME_SKIP_MODE_BUF_LIMIT	= 2,
> +};
>  
>  /*  MPEG-class control IDs specific to the CX2341x driver as defined by V4L2 */
>  #define V4L2_CID_MPEG_CX2341X_BASE				(V4L2_CTRL_CLASS_MPEG | 0x1000)
>

Patch
diff mbox series

diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
index d0d506a444b1..a8b4c0b40747 100644
--- a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
+++ b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
@@ -592,6 +592,38 @@  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_FRAME_SKIP_MODE (enum)``
+
+enum v4l2_mpeg_video_frame_skip_mode -
+    Indicates in what conditions the encoder should skip frames. If
+    encoding a frame would cause the encoded stream to be larger then a
+    chosen data limit then the frame will be skipped. Possible values
+    are:
+
+
+.. tabularcolumns:: |p{9.2cm}|p{8.3cm}|
+
+.. raw:: latex
+
+    \small
+
+.. flat-table::
+    :header-rows:  0
+    :stub-columns: 0
+
+    * - ``V4L2_MPEG_FRAME_SKIP_MODE_DISABLED``
+      - Frame skip mode is disabled.
+    * - ``V4L2_MPEG_FRAME_SKIP_MODE_LEVEL_LIMIT``
+      - Frame skip mode enabled and buffer limit is set by the chosen
+	level and is defined by the standard.
+    * - ``V4L2_MPEG_FRAME_SKIP_MODE_BUF_LIMIT``
+      - Frame skip mode enabled and buffer limit is set by the VBV
+	(MPEG1/2/4) or CPB (H264) buffer size control.
+
+.. raw:: latex
+
+    \normalsize
+
 ``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 3f3fbcd60cc6..d088acfa6dd8 100644
--- a/drivers/media/v4l2-core/v4l2-ctrls.c
+++ b/drivers/media/v4l2-core/v4l2-ctrls.c
@@ -590,6 +590,12 @@  const char * const *v4l2_ctrl_get_menu(u32 id)
 		"External",
 		NULL,
 	};
+	static const char * const mpeg_video_frame_skip[] = {
+		"Disabled",
+		"Level Limit",
+		"VBV/CPB Limit",
+		NULL,
+	};
 
 	switch (id) {
 	case V4L2_CID_MPEG_AUDIO_SAMPLING_FREQ:
@@ -651,6 +657,8 @@  const char * const *v4l2_ctrl_get_menu(u32 id)
 		return flash_strobe_source;
 	case V4L2_CID_MPEG_VIDEO_HEADER_MODE:
 		return header_mode;
+	case V4L2_CID_MPEG_VIDEO_FRAME_SKIP_MODE:
+		return mpeg_video_frame_skip;
 	case V4L2_CID_MPEG_VIDEO_MULTI_SLICE_MODE:
 		return multi_slice;
 	case V4L2_CID_MPEG_VIDEO_H264_ENTROPY_MODE:
@@ -844,6 +852,7 @@  const char *v4l2_ctrl_get_name(u32 id)
 	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";
 	case V4L2_CID_MPEG_VIDEO_MAX_REF_PIC:			return "Max Number of Reference Pics";
+	case V4L2_CID_MPEG_VIDEO_FRAME_SKIP_MODE:		return "Frame Skip Mode";
 	case V4L2_CID_MPEG_VIDEO_H263_I_FRAME_QP:		return "H263 I-Frame QP Value";
 	case V4L2_CID_MPEG_VIDEO_H263_P_FRAME_QP:		return "H263 P-Frame QP Value";
 	case V4L2_CID_MPEG_VIDEO_H263_B_FRAME_QP:		return "H263 B-Frame QP Value";
@@ -1265,6 +1274,7 @@  void v4l2_ctrl_fill(u32 id, const char **name, enum v4l2_ctrl_type *type,
 	case V4L2_CID_FLASH_LED_MODE:
 	case V4L2_CID_FLASH_STROBE_SOURCE:
 	case V4L2_CID_MPEG_VIDEO_HEADER_MODE:
+	case V4L2_CID_MPEG_VIDEO_FRAME_SKIP_MODE:
 	case V4L2_CID_MPEG_VIDEO_MULTI_SLICE_MODE:
 	case V4L2_CID_MPEG_VIDEO_H264_ENTROPY_MODE:
 	case V4L2_CID_MPEG_VIDEO_H264_LEVEL:
diff --git a/include/uapi/linux/v4l2-controls.h b/include/uapi/linux/v4l2-controls.h
index 62271418c1be..4e1526175a4c 100644
--- a/include/uapi/linux/v4l2-controls.h
+++ b/include/uapi/linux/v4l2-controls.h
@@ -742,6 +742,12 @@  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_FRAME_SKIP_MODE		(V4L2_CID_MPEG_BASE + 645)
+enum v4l2_mpeg_video_frame_skip_mode {
+	V4L2_MPEG_VIDEO_FRAME_SKIP_MODE_DISABLED	= 0,
+	V4L2_MPEG_VIDEO_FRAME_SKIP_MODE_LEVEL_LIMIT	= 1,
+	V4L2_MPEG_VIDEO_FRAME_SKIP_MODE_BUF_LIMIT	= 2,
+};
 
 /*  MPEG-class control IDs specific to the CX2341x driver as defined by V4L2 */
 #define V4L2_CID_MPEG_CX2341X_BASE				(V4L2_CTRL_CLASS_MPEG | 0x1000)