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