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

[Tomasz Figa <tfiga@chromium.org>]

Hi Hans,

On Wed, May 20, 2020 at 12:01:58PM +0200, Hans Verkuil wrote:
> 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
> 

Thanks a lot for taking care of the remaining updates. Just one comment
inline.

[snip]
> +4. Set the raw and coded frame interval on the ``OUTPUT`` queue via
> +   :c:func:`VIDIOC_S_PARM`
> +
> +   * ** Required fields:**
> +
> +     ``type``
> +	 a ``V4L2_BUF_TYPE_*`` enum appropriate for ``OUTPUT``.
> +
> +     ``parm.output``
> +	 set all fields except ``parm.output.timeperframe`` to 0.
> +
> +     ``parm.output.timeperframe``
> +	 the desired frame interval; the encoder may adjust it to
> +	 match hardware requirements.
> +
> +   * **Return fields:**
> +
> +     ``parm.output.timeperframe``
> +	 the adjusted frame interval.
> +
> +   .. important::
> +
> +      Changing the ``OUTPUT`` frame interval *also* sets the framerate that
> +      the encoder uses to encode the video. So setting the frame interval
> +      to 1/24 (or 24 frames per second) will produce a coded video stream
> +      that can be played back at that speed. The frame interval for the
> +      ``OUTPUT`` queue is just a hint, the application may provide raw
> +      frames at a different rate. It can be used by the driver to help
> +      schedule multiple encoders running in parallel.
> +
> +      In the next step the ``CAPTURE`` frame interval can optionally be
> +      changed to a different value. This is useful for off-line encoding
> +      were the coded frame interval can be different from the rate at
> +      which raw frames are supplied.
> +
> +   .. 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::
> +
> +      It is due to historical reasons that changing the ``OUTPUT`` frame
> +      interval also changes the coded frame interval on the ``CAPTURE``
> +      queue. Ideally these would be independent settings, but that would
> +      break the existing API.
> +

Should we explicitly state that setting the OUTPUT frame interval resets
the CAPTURE frame interval to the same value?

Best regards,
Tomasz