Re: [PATCH v3 1/2] media: docs-rst: Document memory-to-memory video decoder interface

[Hans Verkuil <hverkuil@xs4all.nl>]

On 1/31/19 11:45 AM, Hans Verkuil wrote:
> On 1/24/19 11:04 AM, Tomasz Figa wrote:
>> Due to complexity of the video decoding process, the V4L2 drivers of
>> stateful decoder hardware require specific sequences of V4L2 API calls
>> to be followed. These include capability enumeration, initialization,
>> decoding, seek, pause, dynamic resolution change, drain and end of
>> stream.
>>
>> 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 decoder part of
>> the Codec API.
>>
>> Signed-off-by: Tomasz Figa <tfiga@chromium.org>
>> ---
>>  Documentation/media/uapi/v4l/dev-decoder.rst  | 1076 +++++++++++++++++
>>  Documentation/media/uapi/v4l/dev-mem2mem.rst  |    5 +
>>  Documentation/media/uapi/v4l/pixfmt-v4l2.rst  |    5 +
>>  Documentation/media/uapi/v4l/v4l2.rst         |   10 +-
>>  .../media/uapi/v4l/vidioc-decoder-cmd.rst     |   40 +-
>>  Documentation/media/uapi/v4l/vidioc-g-fmt.rst |   14 +
>>  6 files changed, 1135 insertions(+), 15 deletions(-)
>>  create mode 100644 Documentation/media/uapi/v4l/dev-decoder.rst
>>
> 
> <snip>
> 
>> +4.  **This step only applies to coded formats that contain resolution information
>> +    in the stream.** Continue queuing/dequeuing bitstream buffers to/from the
>> +    ``OUTPUT`` queue via :c:func:`VIDIOC_QBUF` and :c:func:`VIDIOC_DQBUF`. The
>> +    buffers will be processed and returned to the client in order, until
>> +    required metadata to configure the ``CAPTURE`` queue are found. This is
>> +    indicated by the decoder sending a ``V4L2_EVENT_SOURCE_CHANGE`` event with
>> +    ``V4L2_EVENT_SRC_CH_RESOLUTION`` source change type.
>> +
>> +    * It is not an error if the first buffer does not contain enough data for
>> +      this to occur. Processing of the buffers will continue as long as more
>> +      data is needed.
>> +
>> +    * If data in a buffer that triggers the event is required to decode the
>> +      first frame, it will not be returned to the client, until the
>> +      initialization sequence completes and the frame is decoded.
>> +
>> +    * If the client sets width and height of the ``OUTPUT`` format to 0,
>> +      calling :c:func:`VIDIOC_G_FMT`, :c:func:`VIDIOC_S_FMT`,
>> +      :c:func:`VIDIOC_TRY_FMT` or :c:func:`VIDIOC_REQBUFS` on the ``CAPTURE``
>> +      queue will return the ``-EACCES`` error code, until the decoder
>> +      configures ``CAPTURE`` format according to stream metadata.
> 
> I think this should also include the G/S_SELECTION ioctls, right?

I've started work on adding compliance tests for codecs to v4l2-compliance and
I quickly discovered that this 'EACCES' error code is not nice at all.

The problem is that it is really inconsistent with V4L2 behavior: the basic
rule is that there always is a format defined, i.e. G_FMT will always return
a format.

Suddenly returning an error is actually quite painful to handle because it is
a weird exception just for the capture queue of a stateful decoder if no
output resolution is known.

Just writing that sentence is painful.

Why not just return some default driver defined format? It will automatically
be updated once the decoder parsed the bitstream and knows the new resolution.

It really is just the same behavior as with a resolution change.

It is also perfectly fine to request buffers for the capture queue for that
default format. It's pointless, but not a bug.

Unless I am missing something I strongly recommend changing this behavior.

Regards,

	Hans