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

From: Tomasz Figa <tfiga@chromium.org>
To: Hans Verkuil <hverkuil@xs4all.nl>
Cc: "Nicolas Dufresne" <nicolas@ndufresne.ca>,
	"Linux Media Mailing List" <linux-media@vger.kernel.org>,
	"Linux Kernel Mailing List" <linux-kernel@vger.kernel.org>,
	"Mauro Carvalho Chehab" <mchehab@kernel.org>,
	"Paweł Ościak" <posciak@chromium.org>,
	"Alexandre Courbot" <acourbot@chromium.org>,
	"Kamil Debski" <kamil@wypas.org>,
	"Andrzej Hajda" <a.hajda@samsung.com>,
	"Kyungmin Park" <kyungmin.park@samsung.com>,
	"Jeongtae Park" <jtp.park@samsung.com>,
	"Philipp Zabel" <p.zabel@pengutronix.de>,
	"Tiffany Lin" <tiffany.lin@mediatek.com>,
	"Andrew-CT Chen" <andrew-ct.chen@mediatek.com>,
	"Stanimir Varbanov" <stanimir.varbanov@linaro.org>,
	"Todor Tomov" <todor.tomov@linaro.org>,
	"Paul Kocialkowski" <paul.kocialkowski@bootlin.com>,
	"Laurent Pinchart" <laurent.pinchart@ideasonboard.com>,
	dave.stevenson@raspberrypi.org,
	"Ezequiel Garcia" <ezequiel@collabora.com>,
	"Maxime Jourdan" <maxi.jourdan@wanadoo.fr>
Subject: Re: [PATCH v2 2/2] media: docs-rst: Document memory-to-memory video encoder interface
Date: Wed, 23 Jan 2019 19:00:54 +0900	[thread overview]
Message-ID: <CAAFQd5COddN-YosKyfBJ7n=qt40ONP=YEjBo5HQBOPGhs19h+g@mail.gmail.com> (raw)
In-Reply-To: <d853eb91-c05d-fb10-f154-bc24e4ebb89d@xs4all.nl>

On Sat, Nov 17, 2018 at 8:37 PM Hans Verkuil <hverkuil@xs4all.nl> wrote:
>
> On 11/17/2018 05:18 AM, Nicolas Dufresne wrote:
> > Le lundi 12 novembre 2018 à 14:23 +0100, Hans Verkuil a écrit :
> >> On 10/22/2018 04:49 PM, Tomasz Figa wrote:
[snip]
> >>> +      rely on it. The ``V4L2_BUF_FLAG_LAST`` buffer flag should be used
> >>> +      instead.
> >>
> >> Question: should new codec drivers still implement the EOS event?
> >
> > I'm been asking around, but I think here is a good place. Do we really
> > need the FLAG_LAST in userspace ? Userspace can also wait for the first
> > EPIPE return from DQBUF.
>
> I'm interested in hearing Tomasz' opinion. This flag is used already, so there
> definitely is a backwards compatibility issue here.
>

FWIW, it would add the overhead of 1 more system call, although I
don't think it's of our concern.

My personal feeling is that using error codes for signaling normal
conditions isn't very elegant, though.

> >
> >>
> >>> +
> >>> +3. Once all ``OUTPUT`` buffers queued before the ``V4L2_ENC_CMD_STOP`` call and
> >>> +   the last ``CAPTURE`` buffer are dequeued, the encoder is stopped and it will
> >>> +   accept, but not process any newly queued ``OUTPUT`` buffers until the client
> >>> +   issues any of the following operations:
> >>> +
> >>> +   * ``V4L2_ENC_CMD_START`` - the encoder will resume operation normally,
> >>
> >> Perhaps mention that this does not reset the encoder? It's not immediately clear
> >> when reading this.
> >
> > Which drivers supports this ? I believe I tried with Exynos in the
> > past, and that didn't work. How do we know if a driver supports this or
> > not. Do we make it mandatory ? When it's not supported, it basically
> > mean userspace need to cache and resend the header in userspace, and
> > also need to skip to some sync point.
>
> Once we agree on the spec, then the next step will be to add good compliance
> checks and update drivers that fail the tests.
>
> To check if the driver support this ioctl you can call VIDIOC_TRY_ENCODER_CMD
> to see if the functionality is supported.

There is nothing here for the hardware to support. It's an entirely
driver thing, since it just needs to wait for the encoder to complete
all the pending frames and stop enqueuing more frames to the decoder
until V4L2_ENC_CMD_START is called. Any driver that can't do it must
be fixed, since otherwise you have no way to ensure that you got all
the encoded output.

Best regards,
Tomasz