Hi Tomasz, On Saturday, 20 October 2018 11:52:57 EEST Tomasz Figa wrote: > On Thu, Oct 18, 2018 at 8:22 PM Laurent Pinchart wrote: > > 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. > >>>> > >>>> 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@xxxxxxxxxxxx> > >>>> --- > >>>> > >>>> 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 > >>>> > >>>> 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. > >>> > >>> Isn't it the maximum, not the minimum ? > >> > >> 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 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 > > ``OUTPUT`` buffers for the given format ." > > I'd leave the details to the documentation of VIDIOC_REQBUFS, if > needed. This document focuses on the decoder UAPI and with this note I > want to ensure that the applications don't assume that exactly the > requested number of buffers is always allocated. > > How about making it even simpler: > > The actual number of allocated buffers may differ from the ``count`` > given. The client must check the updated value of ``count`` after the > call returns. That works for me. You may want to see "... given, as specified in the VIDIOC_REQBUFS documentation.". > >>>> 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 > >>>> decode > >>>> + 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 > >>>> stream > >>>> + metadata. > >>> > >>> 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 ? > >> > >> 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.) > > > > Fair enough :-/ A bit of a shame though. Should we try to use an error > > code that would have less chance of being confused with an actual > > permission problem ? -EILSEQ could be an option for "illegal sequence" of > > operations, but better options could exist. > > In Request API we concluded that -EACCES is the right code to return > for G_EXT_CTRLS on a request that has not finished yet. The case here > is similar - the capture queue is not yet set up. What do you think? Good question. -EPERM is documented as "Operation not permitted", while - EACCES is documented as "Permission denied". The former appears to be understood as "This isn't a good idea, I can't let you do that", and the latter as "You don't have sufficient privileges, if you retry with the correct privileges this will succeed". Neither are a perfect match, but -EACCES might be better if you replace getting privileges by performing the required setup. > >>>> + * 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] > >> Also added a note: > >> 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 the steps below. > > > > I wonder if there could be a way to work around those limitations on the > > driver side. At the beginning of step 7, the decoder is effectively > > stopped. If the hardware doesn't support adding new buffers on the fly, > > can't the driver support the VIDIOC_CREATE_BUFS + V4L2_DEC_CMD_START > > sequence the same way it would support the VIDIOC_STREAMOFF + > > VIDIOC_REQBUFS(0) + > > VIDIOC_REQBUFS(n) + VIDIOC_STREAMON ? > > I guess that would work. I would only allow it for the case where > existing buffers are already big enough and just more buffers are > needed. Otherwise it would lead to some weird cases, such as some old > buffers already in the CAPTURE queue, blocking the decode of further > frames. (While it could be handled by the driver returning them with > an error state, it would only complicate the interface.) Good point. I wonder if this could be handled in the framework. If it can't, or with non trivial support code on the driver side, then I would agree with you. Otherwise, handling the workaround in the framework would ensure consistent behaviour across drivers with minimal cost, and simplify the userspace API, so I think it would be a good thing. -- Regards, Laurent Pinchart
