lkml.org 
[lkml]   [2019]   [Apr]   [5]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH v3 2/2] media: docs-rst: Document memory-to-memory video encoder interface
From
Date
On 4/5/19 7:53 AM, Tomasz Figa wrote:
> On Tue, Jan 29, 2019 at 10:53 PM Hans Verkuil <hverkuil@xs4all.nl> wrote:
>>
>> Hi Tomasz,
>>
>> Some comments below. Nothing major, so I think a v4 should be ready to be
>> merged.
>>
>> On 1/24/19 11:04 AM, Tomasz Figa wrote:
>>> 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>
>>> ---
>>> Documentation/media/uapi/v4l/dev-encoder.rst | 586 ++++++++++++++++++
>>> Documentation/media/uapi/v4l/dev-mem2mem.rst | 1 +
>>> Documentation/media/uapi/v4l/pixfmt-v4l2.rst | 5 +
>>> Documentation/media/uapi/v4l/v4l2.rst | 2 +
>>> .../media/uapi/v4l/vidioc-encoder-cmd.rst | 38 +-
>>> 5 files changed, 617 insertions(+), 15 deletions(-)
>>> create mode 100644 Documentation/media/uapi/v4l/dev-encoder.rst
>>>
>>> diff --git a/Documentation/media/uapi/v4l/dev-encoder.rst b/Documentation/media/uapi/v4l/dev-encoder.rst
>>> new file mode 100644
>>> index 000000000000..fb8b05a132ee
>>> --- /dev/null
>>> +++ b/Documentation/media/uapi/v4l/dev-encoder.rst
>>> @@ -0,0 +1,586 @@

<snip>

>>> +Initialization
>>> +==============
>>> +
>>> +1. Set the coded format on the ``CAPTURE`` queue via :c:func:`VIDIOC_S_FMT`
>>> +
>>> + * **Required fields:**
>>> +
>>> + ``type``
>>> + a ``V4L2_BUF_TYPE_*`` enum appropriate for ``CAPTURE``
>>> +
>>> + ``pixelformat``
>>> + the coded format to be produced
>>> +
>>> + ``sizeimage``
>>> + desired size of ``CAPTURE`` buffers; the encoder may adjust it to
>>> + match hardware requirements
>>> +
>>> + ``width``, ``height``
>>> + ignored (always zero)
>>> +
>>> + other fields
>>> + follow standard semantics
>>> +
>>> + * **Return fields:**
>>> +
>>> + ``sizeimage``
>>> + adjusted size of ``CAPTURE`` buffers
>>> +
>>> + .. important::
>>> +
>>> + Changing the ``CAPTURE`` format may change the currently set ``OUTPUT``
>>> + format. The encoder will derive a new ``OUTPUT`` format from the
>>> + ``CAPTURE`` format being set, including resolution, colorimetry
>>> + parameters, etc. If the client needs a specific ``OUTPUT`` format, it
>>> + must adjust it afterwards.
>>
>> Hmm, "including resolution": if width and height are set to 0, what should the
>> OUTPUT resolution be? Up to the driver? I think this should be clarified since
>> at a first reading of this paragraph it appears to be contradictory.
>>
>
> Indeed, it's hard to derive some sensible resolution from 0...
>
> The intention here is to prevent the application from making any
> assumptions about the OUTPUT format, if it changes the CAPTURE format.
> Then, it shouldn't actually matter what's the new OUTPUT format, since
> the application needs to set it anyway.
>
> Maybe let's just remove the mention of deriving and say something
> along these lines?
>
> "How the new ``OUTPUT`` format is determined is unspecified and the client must
> ensure it matches its needs afterwards."

I would replace "unspecified" with "driver specific" or possibly "up to the driver".

Regards,

Hans

\
 
 \ /
  Last update: 2019-04-05 09:19    [W:0.100 / U:6.736 seconds]
©2003-2018 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site