Re: [PATCHv2 1/2] media: docs-rst: Document memory-to-memory video encoder interface

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

On 20/05/2020 22:32, Nicolas Dufresne wrote:
> Le mercredi 20 mai 2020 à 12:01 +0200, Hans Verkuil a écrit :
>> From: Tomasz Figa <tfiga@chromium.org>
>>
>> Due to complexity of the video encoding process, the V4L2 drivers of
>> stateful encoder hardware require specific sequences of V4L2 API calls
>> to be followed. These include capability enumeration, initialization,
>> encoding, encode parameters change, drain and reset.
>>
>> Specifics of the above have been discussed during Media Workshops at
>> LinuxCon Europe 2012 in Barcelona and then later Embedded Linux
>> Conference Europe 2014 in Düsseldorf. The de facto Codec API that
>> originated at those events was later implemented by the drivers we already
>> have merged in mainline, such as s5p-mfc or coda.
>>
>> The only thing missing was the real specification included as a part of
>> Linux Media documentation. Fix it now and document the encoder part of
>> the Codec API.
>>
>> Signed-off-by: Tomasz Figa <tfiga@chromium.org>
>> Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
>> ---
>>  .../userspace-api/media/v4l/dev-encoder.rst   | 727 ++++++++++++++++++
>>  .../userspace-api/media/v4l/dev-mem2mem.rst   |   1 +
>>  .../userspace-api/media/v4l/pixfmt-v4l2.rst   |   5 +
>>  .../userspace-api/media/v4l/v4l2.rst          |   2 +
>>  .../media/v4l/vidioc-encoder-cmd.rst          |  51 +-
>>  5 files changed, 766 insertions(+), 20 deletions(-)
>>  create mode 100644 Documentation/userspace-api/media/v4l/dev-encoder.rst
>>
>> diff --git a/Documentation/userspace-api/media/v4l/dev-encoder.rst b/Documentation/userspace-api/media/v4l/dev-encoder.rst

<snip>

>> +5. **Optional** Set the coded frame interval on the ``CAPTURE`` queue via
>> +   :c:func:`VIDIOC_S_PARM`. This is only necessary if the coded frame
>> +   interval is different from the raw frame interval, which is typically
>> +   the case for off-line encoding.
>> +
>> +   * ** Required fields:**
>> +
>> +     ``type``
>> +	 a ``V4L2_BUF_TYPE_*`` enum appropriate for ``CAPTURE``.
>> +
>> +     ``parm.capture``
>> +	 set all fields except ``parm.capture.timeperframe`` to 0.
>> +
>> +     ``parm.capture.timeperframe``
>> +	 the desired coded frame interval; the encoder may adjust it to
>> +	 match hardware requirements.
>> +
>> +   * **Return fields:**
>> +
>> +     ``parm.capture.timeperframe``
>> +	 the adjusted frame interval.
>> +
>> +   .. important::
>> +
>> +      Changing the ``CAPTURE`` frame interval sets the framerate for the
>> +      coded video. It does *not* set the rate at which buffers arrive on the
>> +      ``CAPTURE`` queue, that depends on how fast the encoder is and how
>> +      fast raw frames are queued on the ``OUTPUT`` queue.
>> +
>> +   .. important::
>> +
>> +      ``timeperframe`` deals with *frames*, not fields. So for interlaced
>> +      formats this is the time per two fields, since a frame consists of
>> +      a top and a bottom field.
>> +
>> +   .. note::
>> +
>> +      Not all drivers support this functionality, in that case just set
>> +      the desired coded frame interval for the ``OUTPUT`` queue.
> 
> There is a slight contorsion in the resulting user-space API. When I
> read this, the logical thing to do for live streams would be to just
> set the OUTPUT and the driver will take care of CAPTURE for me.
> 
> While if I want to do offline, I don't know if this is supported or
> not. So the flow would be a bit special:
> 
>   S_PARM(OUTPUT) with coded video frame rate
>   S_PARM(CAPTURE) width coded video
>   if ^ worked:
>      S_PARM(OUTPUT) with fastest rate possible
> 
> Ideally I would have preferred if there was a more straight forward way
> to configure offline encoding for fastest performance with specific
> coded framerate. I don't think it's a blocker though, performance is
> not critical at all here. Maybe I'm missing something that allow to
> check if this is supported or not without trying it ?

Good point. I considered adding a flag for the v4l2_fmtdesc struct that
reports whether you can set the capture framerate independently from the
OUTPUT framerate. Perhaps V4L2_FMT_FLAG_ENC_CAP_FRAME_INTERVAL?

I actually think it would be best if that's added. It is not enough to
rely on whether S_PARM(CAPTURE) works to determine this feature since
at least one encoder drivers supports both OUTPUT and CAPTURE with S_PARM,
but CAPTURE does the same as OUTPUT, so that would be a red herring.

I'll add this flag for v3.

Regards,

	Hans