linux-kernel - Re: [PATCH v3 2/2] media: docs-rst: Document memory-to-memory video encoder interface

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 5 Apr 2019 09:09:11 +0200
From:   Hans Verkuil <hverkuil@...all.nl>
To:     Tomasz Figa <tfiga@...omium.org>
Cc:     Linux Media Mailing List <linux-media@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Mauro Carvalho Chehab <mchehab@...nel.org>,
        Pawel Osciak <posciak@...omium.org>,
        Alexandre Courbot <acourbot@...omium.org>,
        Kamil Debski <kamil@...as.org>,
        Andrzej Hajda <a.hajda@...sung.com>,
        Kyungmin Park <kyungmin.park@...sung.com>,
        Jeongtae Park <jtp.park@...sung.com>,
        Philipp Zabel <p.zabel@...gutronix.de>,
        Tiffany Lin (林慧珊) 
        <tiffany.lin@...iatek.com>,
        Andrew-CT Chen (陳智迪) 
        <andrew-ct.chen@...iatek.com>,
        Stanimir Varbanov <stanimir.varbanov@...aro.org>,
        Todor Tomov <todor.tomov@...aro.org>,
        Nicolas Dufresne <nicolas@...fresne.ca>,
        Paul Kocialkowski <paul.kocialkowski@...tlin.com>,
        Laurent Pinchart <laurent.pinchart@...asonboard.com>,
        dave.stevenson@...pberrypi.org,
        Ezequiel Garcia <ezequiel@...labora.com>,
        Maxime Jourdan <maxi.jourdan@...adoo.fr>
Subject: Re: [PATCH v3 2/2] media: docs-rst: Document memory-to-memory video
 encoder interface

On 4/5/19 7:53 AM, Tomasz Figa wrote:
> On Tue, Jan 29, 2019 at 10:53 PM Hans Verkuil <hverkuil@...all.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@...omium.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