Re: [PATCH 1/4] media: v4l2-ctrl: Add frame-skip std encoder control

[Stanimir Varbanov <stanimir.varbanov@linaro.org>]

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?

-- 
regards,
Stan