Received: by 2002:ac0:a582:0:0:0:0:0 with SMTP id m2-v6csp1789434imm; Thu, 18 Oct 2018 04:23:10 -0700 (PDT) X-Google-Smtp-Source: ACcGV60tMfDF1xLAlTiwCO5X9yvAn9PPQgVMw43OnXrfaE1z7B0FHK6Q0xkNINwLmthfRRXl2Mvh X-Received: by 2002:a63:1555:: with SMTP id 21-v6mr27857956pgv.383.1539861790316; Thu, 18 Oct 2018 04:23:10 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1539861790; cv=none; d=google.com; s=arc-20160816; b=x4pTP1u3BXV+3Z0vsxOMEqZJ3viAMHYC0QVPeqIf/VyY1VkID+HijaGTszd0fH4ecd iSJxalaVm4WAFsY8jIut+icccTenSfskvS8Uu0t2fM9biSQBWMjoovjJmRZGqJuyz5VV LkYnbwEUDFC+sqWoBqaKWo2J95sLFxaOmd2WkveUhdjja+IIHbBzde5fOl4yEJnurmpl Z5ychkShh0MToZ9xyfQrLaSSKR+Xy2PoVC4JO0Lot0Dh2pgDFJEBq1bLQe9WccBvS7iv vkS1H+SSixvoh0Ten9cLvhgJ2A1TLywiz68HpS88C+U25yvyqowsKaZ/azH50mB5xT2U qKIA== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:sender:content-transfer-encoding:mime-version :references:in-reply-to:organization:message-id:date:subject:cc:to :from:dkim-signature; bh=TfyEQ3T5rUsQoWWeWex856COxF2znB0CGHJ/GZ588QU=; b=UVucCVHKIfNQ7HR59fh7Dk2ubbVfkHhF0OSM1KJVpCljTil7oeDBWnBKzbMPu7Wj6T Re1SQsnF5ZkW62g97v50UIbGVtuFps2PlCL0EXHegiCYuiQlLtP8O0oQ/+R6DgrJjB8u 2AgOSOGMOwKTSpra47C/3/TGXBs2/wb5nsoptoWALx0EzdmvSX5L9iY3RjMSK0ip3PCE QCG35IJad9D7hRbBqN4C1ec3OhJFX679d6mZ5Tt8eBTBmosdLXTyNOrw4Z+wxMDHjzBm hf6fMAHKtYz/O9M+mtTII4NzfOiqWebFmqpckJfHX/IocRTAgSrqNSXUwOYlWWdGsymu DB6w== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass (test mode) header.i=@ideasonboard.com header.s=mail header.b=WtqeCxp4; spf=pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Return-Path: Received: from vger.kernel.org (vger.kernel.org. [209.132.180.67]) by mx.google.com with ESMTP id n9-v6si21052756pgt.267.2018.10.18.04.22.54; Thu, 18 Oct 2018 04:23:10 -0700 (PDT) Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67; Authentication-Results: mx.google.com; dkim=pass (test mode) header.i=@ideasonboard.com header.s=mail header.b=WtqeCxp4; spf=pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1728232AbeJRTWh (ORCPT + 99 others); Thu, 18 Oct 2018 15:22:37 -0400 Received: from perceval.ideasonboard.com ([213.167.242.64]:33162 "EHLO perceval.ideasonboard.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1727470AbeJRTWg (ORCPT ); Thu, 18 Oct 2018 15:22:36 -0400 Received: from avalon.localnet (dfj612ybrt5fhg77mgycy-3.rev.dnainternet.fi [IPv6:2001:14ba:21f5:5b00:2e86:4862:ef6a:2804]) by perceval.ideasonboard.com (Postfix) with ESMTPSA id 80CD21277; Thu, 18 Oct 2018 13:21:58 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com; s=mail; t=1539861719; bh=ck5PxZV4rZqtWXskLE3lrP7U4XRYC4+tzQb4yWfGflY=; h=From:To:Cc:Subject:Date:In-Reply-To:References:From; b=WtqeCxp4GBP0rEsJ8FSZ9wfjM1l5nz96M8m4dcpY0n/JrQKHztlLIOP3UzoANUIue 6ziXJBXE20scw9GRuq20dw7HMBN+w30t0urJO1a3p46U00Ykmr6+BX4kjlYOHh35g8 2FMBezNkJzWxPtj2VQ4C/fR9/O+f6AMlxnXBv/4c= From: Laurent Pinchart To: Tomasz Figa Cc: Linux Media Mailing List , Linux Kernel Mailing List , Stanimir Varbanov , Mauro Carvalho Chehab , Hans Verkuil , Pawel Osciak , Alexandre Courbot , kamil@wypas.org, a.hajda@samsung.com, Kyungmin Park , jtp.park@samsung.com, Philipp Zabel , Tiffany Lin =?utf-8?B?KOael+aFp+ePiik=?= , Andrew-CT Chen =?utf-8?B?KOmZs+aZuui/qik=?= , todor.tomov@linaro.org, nicolas@ndufresne.ca, Paul Kocialkowski , dave.stevenson@raspberrypi.org, Ezequiel Garcia Subject: Re: [PATCH 1/2] media: docs-rst: Document memory-to-memory video decoder interface Date: Thu, 18 Oct 2018 14:22:08 +0300 Message-ID: <4766921.vZCsLckqXW@avalon> Organization: Ideas on Board Oy In-Reply-To: References: <20180724140621.59624-1-tfiga@chromium.org> <9696282.0ldyWdpzWo@avalon> MIME-Version: 1.0 Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset="iso-8859-1" Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi Tomasz, I've stripped out all the parts on which I have no specific comment or just= =20 agree with your proposal. Please see below for a few additional remarks. On Thursday, 18 October 2018 13:03:33 EEST Tomasz Figa wrote: > On Wed, Oct 17, 2018 at 10:34 PM Laurent Pinchart wrote: > > On Tuesday, 24 July 2018 17:06:20 EEST 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. > >>=20 > >> 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=FCsseldorf. 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. > >>=20 > >> 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. > >>=20 > >> Signed-off-by: Tomasz Figa > >> --- > >>=20 > >> Documentation/media/uapi/v4l/dev-decoder.rst | 872 +++++++++++++++++++ > >> Documentation/media/uapi/v4l/devices.rst | 1 + > >> Documentation/media/uapi/v4l/v4l2.rst | 10 +- > >> 3 files changed, 882 insertions(+), 1 deletion(-) > >> create mode 100644 Documentation/media/uapi/v4l/dev-decoder.rst > >>=20 > >> diff --git a/Documentation/media/uapi/v4l/dev-decoder.rst > >> b/Documentation/media/uapi/v4l/dev-decoder.rst new file mode 100644 > >> index 000000000000..f55d34d2f860 > >> --- /dev/null > >> +++ b/Documentation/media/uapi/v4l/dev-decoder.rst > >> @@ -0,0 +1,872 @@ [snip] > >> +4. Allocate source (bitstream) buffers via :c:func:`VIDIOC_REQBUFS` = on > >> + ``OUTPUT``. > >> + > >> + * **Required fields:** > >> + > >> + ``count`` > >> + requested number of buffers to allocate; greater than zero > >> + > >> + ``type`` > >> + a ``V4L2_BUF_TYPE_*`` enum appropriate for ``OUTPUT`` > >> + > >> + ``memory`` > >> + follows standard semantics > >> + > >> + ``sizeimage`` > >> + follows standard semantics; the client is free to choose any > >> + suitable size, however, it may be subject to change by the > >> + driver > >> + > >> + * **Return fields:** > >> + > >> + ``count`` > >> + actual number of buffers allocated > >> + > >> + * The driver must adjust count to minimum of required number of > >> + ``OUTPUT`` buffers for given format and count passed. > >=20 > > Isn't it the maximum, not the minimum ? >=20 > It's actually neither. All we can generally say here is that the > number will be adjusted and the client must note it. I expect it to be clamp(requested count, driver minimum, driver maximum). I= 'm=20 not sure it's worth capturing this in the document though, but we could say "The driver must clam count to the minimum and maximum number of required=20 ``OUTPUT`` buffers for the given format ." > >> The client must > >> + check this value after the ioctl returns to get the number of > >> + buffers allocated. > >> + > >> + .. note:: > >> + > >> + To allocate more than minimum number of buffers (for pipeline > >> + depth), use G_CTRL(``V4L2_CID_MIN_BUFFERS_FOR_OUTPUT``) to > >> + get minimum number of buffers required by the driver/format, > >> + and pass the obtained value plus the number of additional > >> + buffers needed in count to :c:func:`VIDIOC_REQBUFS`. > >> + > >> +5. Start streaming on ``OUTPUT`` queue via :c:func:`VIDIOC_STREAMON`. > >> + > >> +6. 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 driver must keep processing and > >> returning > >> + each buffer to the client until required metadata to configure the > >> + ``CAPTURE`` queue are found. This is indicated by the driver > >> sending > >> + a ``V4L2_EVENT_SOURCE_CHANGE`` event with > >> + ``V4L2_EVENT_SRC_CH_RESOLUTION`` source change type. There is no > >> + requirement to pass enough data for this to occur in the first > >> buffer > >> + and the driver must be able to process any number. > >> + > >> + * If data in a buffer that triggers the event is required to deco= de > >> + the first frame, the driver must not return it to the client, > >> + but must retain it for further decoding. > >> + > >> + * If the client set width and height of ``OUTPUT`` format to 0, > >> calling > >> + :c:func:`VIDIOC_G_FMT` on the ``CAPTURE`` queue will return > >> -EPERM, > >> + until the driver configures ``CAPTURE`` format according to str= eam > >> + metadata. > >=20 > > That's a pretty harsh handling for this condition. What's the rationale > > for returning -EPERM instead of for instance succeeding with width and > > height set to 0 ? >=20 > I don't like it, but the error condition must stay for compatibility > reasons as that's what current drivers implement and applications > expect. (Technically current drivers would return -EINVAL, but we > concluded that existing applications don't care about the exact value, > so we can change it to make more sense.) =46air enough :-/ A bit of a shame though. Should we try to use an error co= de=20 that would have less chance of being confused with an actual permission=20 problem ? -EILSEQ could be an option for "illegal sequence" of operations, = but=20 better options could exist. > >> + * If the client subscribes to ``V4L2_EVENT_SOURCE_CHANGE`` events > >> and > >> + the event is signaled, the decoding process will not continue > >> until > >> + it is acknowledged by either (re-)starting streaming on > >> ``CAPTURE``, > >> + or via :c:func:`VIDIOC_DECODER_CMD` with ``V4L2_DEC_CMD_START`` > >> + command. > >> + > >> + .. note:: > >> + > >> + No decoded frames are produced during this phase. > >> + [snip] > >> +8. Call :c:func:`VIDIOC_G_FMT` for ``CAPTURE`` queue to get format f= or > >> the + destination buffers parsed/decoded from the bitstream. > >> + > >> + * **Required fields:** > >> + > >> + ``type`` > >> + a ``V4L2_BUF_TYPE_*`` enum appropriate for ``CAPTURE`` > >> + > >> + * **Return fields:** > >> + > >> + ``width``, ``height`` > >> + frame buffer resolution for the decoded frames > >> + > >> + ``pixelformat`` > >> + pixel format for decoded frames > >> + > >> + ``num_planes`` (for _MPLANE ``type`` only) > >> + number of planes for pixelformat > >> + > >> + ``sizeimage``, ``bytesperline`` > >> + as per standard semantics; matching frame buffer format > >> + > >> + .. note:: > >> + > >> + The value of ``pixelformat`` may be any pixel format supported > >> and > >> + must be supported for current stream, based on the information > >> + parsed from the stream and hardware capabilities. It is > >> suggested > >> + that driver chooses the preferred/optimal format for given > >=20 > > In compliance with RFC 2119, how about using "Drivers should choose" > > instead of "It is suggested that driver chooses" ? >=20 > The whole paragraph became: >=20 > The value of ``pixelformat`` may be any pixel format supported by = the > decoder for the current stream. It is expected that the decoder chooses a > preferred/optimal format for the default configuration. For example, a YUV > format may be preferred over an RGB format, if additional conversion step > would be required. How about using "should" instead of "it is expected that" ? [snip] > >> +10. *[optional]* Choose a different ``CAPTURE`` format than suggested > >> via > >> + :c:func:`VIDIOC_S_FMT` on ``CAPTURE`` queue. It is possible for > >> the > >> + client to choose a different format than selected/suggested by t= he > >=20 > > And here, "A client may choose" ? > >=20 > >> + driver in :c:func:`VIDIOC_G_FMT`. > >> + > >> + * **Required fields:** > >> + > >> + ``type`` > >> + a ``V4L2_BUF_TYPE_*`` enum appropriate for ``CAPTURE`` > >> + > >> + ``pixelformat`` > >> + a raw pixel format > >> + > >> + .. note:: > >> + > >> + Calling :c:func:`VIDIOC_ENUM_FMT` to discover currently > >> available > >> + formats after receiving ``V4L2_EVENT_SOURCE_CHANGE`` is useful > >> to > >> + find out a set of allowed formats for given configuration, but > >> not > >> + required, if the client can accept the defaults. > >=20 > > s/required/required,/ >=20 > That would become "[...]but not required,, if the client[...]". Is > that your suggestion? ;) Oops, the other way around of course :-) [snip] > >> +3. Start queuing buffers to ``OUTPUT`` queue containing stream data > >> after > >> + the seek until a suitable resume point is found. > >> + > >> + .. note:: > >> + > >> + There is no requirement to begin queuing stream starting exactly > >> from > >=20 > > s/stream/buffers/ ? >=20 > Perhaps "stream data"? The buffers don't have a resume point, the stream > does. Maybe "coded data" ? > >> + a resume point (e.g. SPS or a keyframe). The driver must handle > >> any > >> + data queued and must keep processing the queued buffers until it > >> + finds a suitable resume point. While looking for a resume point, > >> the > >> + driver processes ``OUTPUT`` buffers and returns them to the > >> client > >> + without producing any decoded frames. > >> + > >> + For hardware known to be mishandling seeks to a non-resume poin= t, > >> + e.g. by returning corrupted decoded frames, the driver must be > >> able > >> + to handle such seeks without a crash or any fatal decode error. > >=20 > > This should be true for any hardware, there should never be any crash or > > fatal decode error. I'd write it as > >=20 > > Some hardware is known to mishandle seeks to a non-resume point. Such an > > operation may result in an unspecified number of corrupted decoded fram= es > > being made available on ``CAPTURE``. Drivers must ensure that no fatal > > decoding errors or crashes occur, and implement any necessary handling = and > > work-arounds for hardware issues related to seek operations. >=20 > Done. [snip] > >> +2. After all buffers containing decoded frames from before the > >> resolution > >> + change point are ready to be dequeued on the ``CAPTURE`` queue, t= he > >> + driver sends a ``V4L2_EVENT_SOURCE_CHANGE`` event for source chan= ge > >> + type ``V4L2_EVENT_SRC_CH_RESOLUTION``. > >> + > >> + * The last buffer from before the change must be marked with > >> + :c:type:`v4l2_buffer` ``flags`` flag ``V4L2_BUF_FLAG_LAST`` as = in > >> the + drain sequence. The last buffer might be empty (with > >> + :c:type:`v4l2_buffer` ``bytesused`` =3D 0) and must be ignored = by > >> the > >> + client, since it does not contain any decoded frame. > >> + > >> + * Any client query issued after the driver queues the event must > >> return > >> + values applying to the stream after the resolution change, > >> including > >> + queue formats, selection rectangles and controls. > >> + > >> + * If the client subscribes to ``V4L2_EVENT_SOURCE_CHANGE`` events > >> and > >> + the event is signaled, the decoding process will not continue > >> until > >> + it is acknowledged by either (re-)starting streaming on > >> ``CAPTURE``, > >> + or via :c:func:`VIDIOC_DECODER_CMD` with ``V4L2_DEC_CMD_START`` > >> + command. > >=20 > > This usage of V4L2_DEC_CMD_START isn't aligned with the documentation of > > the command. I'm not opposed to this, but I think the use cases of > > decoder commands for codecs should be explained in the VIDIOC_DECODER_C= MD > > documentation. What bothers me in particular is usage of > > V4L2_DEC_CMD_START to restart the decoder, while no V4L2_DEC_CMD_STOP h= as > > been issued. Should we add a section that details the decoder state > > machine with the implicit and explicit ways in which it is started and > > stopped ? >=20 > Yes, we should probably extend the VIDIOC_DECODER_CMD documentation. >=20 > As for diagrams, they would indeed be nice to have, but maybe we could > add them in a follow up patch? That's another way to say it won't happen, right ? ;-) I'm OK with that, bu= t I=20 think we should still clarify that the source change generates an implicit= =20 V4L2_DEC_CMD_STOP. > > I would also reference step 7 here. > >=20 > >> + .. note:: > >> + > >> + Any attempts to dequeue more buffers beyond the buffer marked > >> + with ``V4L2_BUF_FLAG_LAST`` will result in a -EPIPE error from > >> + :c:func:`VIDIOC_DQBUF`. > >> + > >> +3. The client calls :c:func:`VIDIOC_G_FMT` for ``CAPTURE`` to get the > >> new > >> + format information. This is identical to calling > >> :c:func:`VIDIOC_G_FMT` + after ``V4L2_EVENT_SRC_CH_RESOLUTION`` in > >> the initialization sequence + and should be handled similarly. > >=20 > > As the source resolution change event is mentioned in multiple places, = how > > about extracting the related ioctls sequence to a specific section, and > > referencing it where needed (at least from the initialization sequence = and > > here) ? >=20 > I made the text here refer to the Initialization sequence. Wouldn't it be clearer if those steps were extracted to a standalone sequen= ce=20 referenced from both locations ? > >> + .. note:: > >> + > >> + It is allowed for the driver not to support the same pixel > >> format as > >=20 > > "Drivers may not support ..." > >=20 > >> + previously used (before the resolution change) for the new > >> + resolution. The driver must select a default supported pixel > >> format, > >> + return it, if queried using :c:func:`VIDIOC_G_FMT`, and the > >> client > >> + must take note of it. > >> + > >> +4. The client acquires visible resolution as in initialization > >> sequence. > >> + > >> +5. *[optional]* The client is allowed to enumerate available formats > >> and > >=20 > > s/is allowed to/may/ > >=20 > >> + select a different one than currently chosen (returned via > >> + :c:func:`VIDIOC_G_FMT)`. This is identical to a corresponding step > >> in > >> + the initialization sequence. > >> + > >> +6. *[optional]* The client acquires minimum number of buffers as in > >> + initialization sequence. > >> + > >> +7. If all the following conditions are met, the client may resume the > >> + decoding instantly, by using :c:func:`VIDIOC_DECODER_CMD` with > >> + ``V4L2_DEC_CMD_START`` command, as in case of resuming after the > >> drain > >> + sequence: > >> + > >> + * ``sizeimage`` of new format is less than or equal to the size of > >> + currently allocated buffers, > >> + > >> + * the number of buffers currently allocated is greater than or > >> equal to > >> + the minimum number of buffers acquired in step 6. > >> + > >> + In such case, the remaining steps do not apply. > >> + > >> + However, if the client intends to change the buffer set, to lower > >> + memory usage or for any other reasons, it may be achieved by > >> following > >> + the steps below. > >> + > >> +8. After dequeuing all remaining buffers from the ``CAPTURE`` queue, > >=20 > > This is optional, isn't it ? >=20 > I wouldn't call it optional, since it depends on what the client does > and what the decoder supports. That's why the point above just states > that the remaining steps do not apply. I meant isn't the "After dequeuing all remaining buffers from the CAPTURE=20 queue" part optional ? As far as I understand, the client may decide not to= =20 dequeue them. > Also added a note: >=20 > To fulfill those requirements, the client may attempt to use > :c:func:`VIDIOC_CREATE_BUFS` to add more buffers. However, due to > hardware limitations, the decoder may not support adding buffers at > this point and the client must be able to handle a failure using t= he > steps below. I wonder if there could be a way to work around those limitations on the=20 driver side. At the beginning of step 7, the decoder is effectively stopped= =2E=20 If the hardware doesn't support adding new buffers on the fly, can't the=20 driver support the VIDIOC_CREATE_BUFS + V4L2_DEC_CMD_START sequence the sam= e=20 way it would support the VIDIOC_STREAMOFF + VIDIOC_REQBUFS(0) +=20 VIDIOC_REQBUFS(n) + VIDIOC_STREAMON ? > >> the > >> + client must call :c:func:`VIDIOC_STREAMOFF` on the ``CAPTURE`` > >> queue. > >> + The ``OUTPUT`` queue must remain streaming (calling STREAMOFF on = it > > > > :c:func:`VIDIOC_STREAMOFF` > > > >> + would trigger a seek). [snip] =2D-=20 Regards, Laurent Pinchart