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

From: Nicolas Dufresne <nicolas@ndufresne.ca>
To: Tomasz Figa <tfiga@chromium.org>
Cc: "Hans Verkuil" <hverkuil@xs4all.nl>,
	"Linux Media Mailing List" <linux-media@vger.kernel.org>,
	"Linux Kernel Mailing List" <linux-kernel@vger.kernel.org>,
	"Mauro Carvalho Chehab" <mchehab@kernel.org>,
	"Pawel Osciak" <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 1/2] media: docs-rst: Document memory-to-memory video decoder interface
Date: Tue, 29 Jan 2019 23:02:38 -0500	[thread overview]
Message-ID: <c145fbf21301d03bdfdd8bf6613f0f68576e66be.camel@ndufresne.ca> (raw)
In-Reply-To: <CAAFQd5C_OD=bvAxG0B_G+T6bnWddPHuiVZApj_8_+4xpMjH9+g@mail.gmail.com>

Le vendredi 25 janvier 2019 à 12:27 +0900, Tomasz Figa a écrit :
> On Fri, Jan 25, 2019 at 4:55 AM Nicolas Dufresne <nicolas@ndufresne.ca> wrote:
> > Le jeudi 24 janvier 2019 à 18:06 +0900, Tomasz Figa a écrit :
> > > > Actually I just realized the last point might not even be achievable
> > > > for some of the decoders (s5p-mfc, mtk-vcodec), as they don't report
> > > > which frame originates from which bitstream buffer and the driver just
> > > > picks the most recently consumed OUTPUT buffer to copy the timestamp
> > > > from. (s5p-mfc actually "forgets" to set the timestamp in some cases
> > > > too...)
> > > > 
> > > > I need to think a bit more about this.
> > > 
> > > Actually I misread the code. Both s5p-mfc and mtk-vcodec seem to
> > > correctly match the buffers.
> > 
> > Ok good, since otherwise it would have been a regression in MFC driver.
> > This timestamp passing thing could in theory be made optional though,
> > it lives under some COPY_TIMESTAMP kind of flag. What that means though
> > is that a driver without such a capability would need to signal dropped
> > frames using some other mean.
> > 
> > In userspace, the main use is to match the produced frame against a
> > userspace specific list of frames. At least this seems to be the case
> > in Gst and Chromium, since the userspace list contains a superset of
> > the metadata found in the v4l2_buffer.
> > 
> > Now, using the produced timestamp, userspace can deduce frame that the
> > driver should have produced but didn't (could be a deadline case codec,
> > or simply the frames where corrupted). It's quite normal for a codec to
> > just keep parsing until it finally find something it can decode.
> > 
> > That's at least one way to do it, but there is other possible
> > mechanism. The sequence number could be used, or even producing buffers
> > with the ERROR flag set. What matters is just to give userspace a way
> > to clear these frames, which would simply grow userspace memory usage
> > over time.
> 
> Is it just me or we were missing some consistent error handling then?
> 
> I feel like the drivers should definitely return the bitstream buffers
> with the ERROR flag, if there is a decode failure of data in the
> buffer. Still, that could become more complicated if there is more
> than 1 frame in that piece of bitstream, but only 1 frame is corrupted
> (or whatever).

I agree, but it might be more difficult then it looks (even FFMPEG does
not do that). I believe the code that is processing the bitstream in
stateful codecs is mostly unrelated from the code actually doing the
decoding. So what might happen is that the decoding part will never
actually allocate a buffer for the skipped / corrupted part of the
bitstream. Also, the notion of a skipped frame is not always evident in
when parsing H264 or HEVC NALs. There is still a full page of text just
to explain how to detect that start of a new frame.

Yet, it would be interesting to study the firmwares we have and see
what they provide that would help making decode errors more explicit.

> 
> Another case is when the bitstream, even if corrupted, is still enough
> to produce some output. My intuition tells me that such CAPTURE buffer
> should be then returned with the ERROR flag. That wouldn't still be
> enough for any more sophisticated userspace error concealment, but
> could still let the userspace know to perhaps drop the frame.

You mean if a frame was concealed (typically the frame was decoded from
a closed by reference instead of the expected reference). That is
something signalled by FFPEG. We should document this possibility. I
actually have something implemented in GStreamer. Basically if we have
the ERROR flag with a payload size smaller then expected, I drop the
frame and produce a drop event message, while if I have a frame with
ERROR flag but of the right payload size, I assume it is corrupted, and
simply flag it as corrupted, leaving to the application the decision to
display it or not. This is a case that used to happen with some UVC
cameras (though some have been fixed, and the UVC camera should drop
smaller payload size buffers now).

> 
> Best regards,
> Tomasz