Re: [PATCH 2/2] media: coda: Add more H264 levels for CODA960

From: Nicolas Dufresne <nicolas.dufresne@collabora.com>
To: Philipp Zabel <p.zabel@pengutronix.de>,
	Ezequiel Garcia <ezequiel@collabora.com>,
	linux-media@vger.kernel.org
Cc: Hans Verkuil <hverkuil@xs4all.nl>,
	Robert Beckett <bob.beckett@collabora.com>,
	kernel@collabora.com, stable@vger.kernel.org
Subject: Re: [PATCH 2/2] media: coda: Add more H264 levels for CODA960
Date: Fri, 17 Jul 2020 11:50:38 -0400	[thread overview]
Message-ID: <f409d4ddad0a352ca7ec84699c94a64e5dbf0407.camel@collabora.com> (raw)
In-Reply-To: <05184a7c923c7e2aacca9da2bafe338ff5a7c16d.camel@pengutronix.de>

Le vendredi 17 juillet 2020 à 09:48 +0200, Philipp Zabel a écrit :
> Hi Ezequiel, Nicolas,
> 
> On Fri, 2020-07-17 at 00:49 -0300, Ezequiel Garcia wrote:
> > From: Nicolas Dufresne <nicolas.dufresne@collabora.com>
> > 
> > This add H264 level 4.1, 4.2 and 5.0 to the list of supported formats.
> > While the hardware does not fully support these levels, it do support
> > most of them.
> 
> Could you clarify this? As far as I understand the hardware supports
> maximum frame size requirement for up to level 4.2 (8704 macroblocks),
> but not 5.0, and at least the implementation on i.MX6 does not support
> the max encoding speed requirements for levels 4.1 and higher.
> 
> I don't think the firmware ever produces any output with a level higher
> than 4.0 either, so what is the purpose of pretending otherwise?

The level is a combination of 3 contraints, frame size, raw bitrate and encoded
bitrate. We have a streams here the decode just fine, that reaches 5.0 for the
endoded bitrate, but is near 4.0 for everything else. This streams works just
fine with the 960. I think the risk with this patch is that it now allow a
stream to underperform in raw bitrate, but that can be controlled otherwise by
the frame interval, so there is no need to limit it through levels.  I could be
wrong.

But in public domain [0], Chips&Media seems to claim 4.2 decode, 4.0 encode. So
yes, claiming 5.0 is off track, we will reduce this to 4.2 in v2.

[0] https://www.chipsnmedia.com/fullhd

Considering how buggy and inconcistent this is going to be in decoder drivers,
I'm tempted to just drop that restriction in GStreamer v4l2 decoders (was added
by Philippe Normand from Igalia). Specially the bitrate limits, since it is
quite clear from testing that this limits is only related to real-time
performance, and that offline decoding should still be possible. Meanwhile, the
driver should still advertise 4.1 and 4.2 decoding. But we should check the
decoding/encoding levels are actually not the same, that I haven't checked, the
code is a bit ... kindly said ... hairy.

> 
> regards
> Philipp