[PATCH 1/2] media: coda: Fix reported H264 profile

[PATCH 1/2] media: coda: Fix reported H264 profile

[Ezequiel Garcia]

From: Nicolas Dufresne <nicolas.dufresne@collabora.com>

The CODA960 manual states that ASO/FMO features of baseline are not
supported, so for this reason this driver should only report
constrained baseline support.

This fixes negotiation issue with constrained baseline content
on GStreamer 1.17.1.

Cc: stable@vger.kernel.org
Fixes: 42a68012e67c2 ("media: coda: add read-only h.264 decoder profile/level controls")
Signed-off-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
---
 drivers/media/platform/coda/coda-common.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/media/platform/coda/coda-common.c b/drivers/media/platform/coda/coda-common.c
index 3ab3d976d8ca..c641d1608825 100644
--- a/drivers/media/platform/coda/coda-common.c
+++ b/drivers/media/platform/coda/coda-common.c
@@ -2335,8 +2335,8 @@ static void coda_encode_ctrls(struct coda_ctx *ctx)
 		V4L2_CID_MPEG_VIDEO_H264_CHROMA_QP_INDEX_OFFSET, -12, 12, 1, 0);
 	v4l2_ctrl_new_std_menu(&ctx->ctrls, &coda_ctrl_ops,
 		V4L2_CID_MPEG_VIDEO_H264_PROFILE,
-		V4L2_MPEG_VIDEO_H264_PROFILE_BASELINE, 0x0,
-		V4L2_MPEG_VIDEO_H264_PROFILE_BASELINE);
+		V4L2_MPEG_VIDEO_H264_PROFILE_CONSTRAINED_BASELINE, 0x0,
+		V4L2_MPEG_VIDEO_H264_PROFILE_CONSTRAINED_BASELINE);
 	if (ctx->dev->devtype->product == CODA_HX4 ||
 	    ctx->dev->devtype->product == CODA_7541) {
 		v4l2_ctrl_new_std_menu(&ctx->ctrls, &coda_ctrl_ops,
@@ -2417,7 +2417,7 @@ static void coda_decode_ctrls(struct coda_ctx *ctx)
 	ctx->h264_profile_ctrl = v4l2_ctrl_new_std_menu(&ctx->ctrls,
 		&coda_ctrl_ops, V4L2_CID_MPEG_VIDEO_H264_PROFILE,
 		V4L2_MPEG_VIDEO_H264_PROFILE_HIGH,
-		~((1 << V4L2_MPEG_VIDEO_H264_PROFILE_BASELINE) |
+		~((1 << V4L2_MPEG_VIDEO_H264_PROFILE_CONSTRAINED_BASELINE) |
 		  (1 << V4L2_MPEG_VIDEO_H264_PROFILE_MAIN) |
 		  (1 << V4L2_MPEG_VIDEO_H264_PROFILE_HIGH)),
 		V4L2_MPEG_VIDEO_H264_PROFILE_HIGH);
-- 
2.27.0

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

[Ezequiel Garcia]

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. The constraints on frame size and pixel formats already
cover the limitation.

This fixes negotiation of level on GStreamer 1.17.1.

Cc: stable@vger.kernel.org
Fixes: 42a68012e67c2 ("media: coda: add read-only h.264 decoder profile/level controls")
Signed-off-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
---
 drivers/media/platform/coda/coda-common.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/media/platform/coda/coda-common.c b/drivers/media/platform/coda/coda-common.c
index c641d1608825..d0fc573d6ed9 100644
--- a/drivers/media/platform/coda/coda-common.c
+++ b/drivers/media/platform/coda/coda-common.c
@@ -2355,7 +2355,10 @@ static void coda_encode_ctrls(struct coda_ctx *ctx)
 			  (1 << V4L2_MPEG_VIDEO_H264_LEVEL_3_0) |
 			  (1 << V4L2_MPEG_VIDEO_H264_LEVEL_3_1) |
 			  (1 << V4L2_MPEG_VIDEO_H264_LEVEL_3_2) |
-			  (1 << V4L2_MPEG_VIDEO_H264_LEVEL_4_0)),
+			  (1 << V4L2_MPEG_VIDEO_H264_LEVEL_4_0) |
+			  (1 << V4L2_MPEG_VIDEO_H264_LEVEL_4_1) |
+			  (1 << V4L2_MPEG_VIDEO_H264_LEVEL_4_2) |
+			  (1 << V4L2_MPEG_VIDEO_H264_LEVEL_5_0)),
 			V4L2_MPEG_VIDEO_H264_LEVEL_4_0);
 	}
 	v4l2_ctrl_new_std(&ctx->ctrls, &coda_ctrl_ops,
@@ -2428,7 +2431,7 @@ static void coda_decode_ctrls(struct coda_ctx *ctx)
 	    ctx->dev->devtype->product == CODA_7541)
 		max = V4L2_MPEG_VIDEO_H264_LEVEL_4_0;
 	else if (ctx->dev->devtype->product == CODA_960)
-		max = V4L2_MPEG_VIDEO_H264_LEVEL_4_1;
+		max = V4L2_MPEG_VIDEO_H264_LEVEL_5_0;
 	else
 		return;
 	ctx->h264_level_ctrl = v4l2_ctrl_new_std_menu(&ctx->ctrls,
-- 
2.27.0

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

[Philipp Zabel]

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?

regards
Philipp

Re: [PATCH 1/2] media: coda: Fix reported H264 profile

[Philipp Zabel]

On Fri, 2020-07-17 at 00:49 -0300, Ezequiel Garcia wrote:
> From: Nicolas Dufresne <nicolas.dufresne@collabora.com>
> 
> The CODA960 manual states that ASO/FMO features of baseline are not
> supported, so for this reason this driver should only report
> constrained baseline support.

I know the encoder doesn't support this, but is this also true of the
decoder? The i.MX6DQ Reference Manual explicitly lists H.264/AVC decoder
support for both baseline profile and constrained base line profile.

> This fixes negotiation issue with constrained baseline content
> on GStreamer 1.17.1.
> 
> Cc: stable@vger.kernel.org
> Fixes: 42a68012e67c2 ("media: coda: add read-only h.264 decoder profile/level controls")
> Signed-off-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
> Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
> ---
>  drivers/media/platform/coda/coda-common.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/media/platform/coda/coda-common.c b/drivers/media/platform/coda/coda-common.c
> index 3ab3d976d8ca..c641d1608825 100644
> --- a/drivers/media/platform/coda/coda-common.c
> +++ b/drivers/media/platform/coda/coda-common.c
> @@ -2335,8 +2335,8 @@ static void coda_encode_ctrls(struct coda_ctx *ctx)
>  		V4L2_CID_MPEG_VIDEO_H264_CHROMA_QP_INDEX_OFFSET, -12, 12, 1, 0);
>  	v4l2_ctrl_new_std_menu(&ctx->ctrls, &coda_ctrl_ops,
>  		V4L2_CID_MPEG_VIDEO_H264_PROFILE,
> -		V4L2_MPEG_VIDEO_H264_PROFILE_BASELINE, 0x0,
> -		V4L2_MPEG_VIDEO_H264_PROFILE_BASELINE);
> +		V4L2_MPEG_VIDEO_H264_PROFILE_CONSTRAINED_BASELINE, 0x0,
> +		V4L2_MPEG_VIDEO_H264_PROFILE_CONSTRAINED_BASELINE);

Encoder support is listed as baseline, not constrained baseline, in the
manual, but the SPS NALs produced by the encoder start with:
  00 00 00 01 67 42 40
                    ^
so that is profile_idc=66, constraint_set1_flag==1, constrained baseline
indeed. I think this change is correct.

>  	if (ctx->dev->devtype->product == CODA_HX4 ||
>  	    ctx->dev->devtype->product == CODA_7541) {
>  		v4l2_ctrl_new_std_menu(&ctx->ctrls, &coda_ctrl_ops,
> @@ -2417,7 +2417,7 @@ static void coda_decode_ctrls(struct coda_ctx *ctx)
>  	ctx->h264_profile_ctrl = v4l2_ctrl_new_std_menu(&ctx->ctrls,
>  		&coda_ctrl_ops, V4L2_CID_MPEG_VIDEO_H264_PROFILE,
>  		V4L2_MPEG_VIDEO_H264_PROFILE_HIGH,
> -		~((1 << V4L2_MPEG_VIDEO_H264_PROFILE_BASELINE) |
> +		~((1 << V4L2_MPEG_VIDEO_H264_PROFILE_CONSTRAINED_BASELINE) |
>  		  (1 << V4L2_MPEG_VIDEO_H264_PROFILE_MAIN) |
>  		  (1 << V4L2_MPEG_VIDEO_H264_PROFILE_HIGH)),
>  		V4L2_MPEG_VIDEO_H264_PROFILE_HIGH);

I'm not sure about this one.

regards
Philipp

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

[Nicolas Dufresne]

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

Re: [PATCH 1/2] media: coda: Fix reported H264 profile

[Nicolas Dufresne]

Le vendredi 17 juillet 2020 à 10:14 +0200, Philipp Zabel a écrit :
> On Fri, 2020-07-17 at 00:49 -0300, Ezequiel Garcia wrote:
> > From: Nicolas Dufresne <nicolas.dufresne@collabora.com>
> > 
> > The CODA960 manual states that ASO/FMO features of baseline are not
> > supported, so for this reason this driver should only report
> > constrained baseline support.
> 
> I know the encoder doesn't support this, but is this also true of the
> decoder? The i.MX6DQ Reference Manual explicitly lists H.264/AVC decoder
> support for both baseline profile and constrained base line profile.

Hmm, double checking, you are right this is documented in the encoding tools
sections, not the decoding. But there is extra buffers that need to be passed
for ASO/FMO to work, I greatly doubt you have ever tested it. This is not
supported by GStreamer parser, or FFMPEG parsers either. Again, we need to make
sure in V2 that encoding and decoding capabilities are well seperated.

As for advertising ASO/FMO, I can leave it there, but be aware I won't be
testing it. I can provide you links to streams if you care (they are publicly
accessible throught the ITU conformance streams published by the ITU). But as
for GStreamer and FFMPEG, this is not supported anyway.

> 
> > This fixes negotiation issue with constrained baseline content
> > on GStreamer 1.17.1.
> > 
> > Cc: stable@vger.kernel.org
> > Fixes: 42a68012e67c2 ("media: coda: add read-only h.264 decoder
> > profile/level controls")
> > Signed-off-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
> > Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
> > ---
> >  drivers/media/platform/coda/coda-common.c | 6 +++---
> >  1 file changed, 3 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/media/platform/coda/coda-common.c
> > b/drivers/media/platform/coda/coda-common.c
> > index 3ab3d976d8ca..c641d1608825 100644
> > --- a/drivers/media/platform/coda/coda-common.c
> > +++ b/drivers/media/platform/coda/coda-common.c
> > @@ -2335,8 +2335,8 @@ static void coda_encode_ctrls(struct coda_ctx *ctx)
> >  		V4L2_CID_MPEG_VIDEO_H264_CHROMA_QP_INDEX_OFFSET, -12, 12, 1, 0);
> >  	v4l2_ctrl_new_std_menu(&ctx->ctrls, &coda_ctrl_ops,
> >  		V4L2_CID_MPEG_VIDEO_H264_PROFILE,
> > -		V4L2_MPEG_VIDEO_H264_PROFILE_BASELINE, 0x0,
> > -		V4L2_MPEG_VIDEO_H264_PROFILE_BASELINE);
> > +		V4L2_MPEG_VIDEO_H264_PROFILE_CONSTRAINED_BASELINE, 0x0,
> > +		V4L2_MPEG_VIDEO_H264_PROFILE_CONSTRAINED_BASELINE);
> 
> Encoder support is listed as baseline, not constrained baseline, in the
> manual, but the SPS NALs produced by the encoder start with:
>   00 00 00 01 67 42 40
>                     ^
> so that is profile_idc=66, constraint_set1_flag==1, constrained baseline
> indeed. I think this change is correct.
> 
> >  	if (ctx->dev->devtype->product == CODA_HX4 ||
> >  	    ctx->dev->devtype->product == CODA_7541) {
> >  		v4l2_ctrl_new_std_menu(&ctx->ctrls, &coda_ctrl_ops,
> > @@ -2417,7 +2417,7 @@ static void coda_decode_ctrls(struct coda_ctx *ctx)
> >  	ctx->h264_profile_ctrl = v4l2_ctrl_new_std_menu(&ctx->ctrls,
> >  		&coda_ctrl_ops, V4L2_CID_MPEG_VIDEO_H264_PROFILE,
> >  		V4L2_MPEG_VIDEO_H264_PROFILE_HIGH,
> > -		~((1 << V4L2_MPEG_VIDEO_H264_PROFILE_BASELINE) |
> > +		~((1 << V4L2_MPEG_VIDEO_H264_PROFILE_CONSTRAINED_BASELINE) |
> >  		  (1 << V4L2_MPEG_VIDEO_H264_PROFILE_MAIN) |
> >  		  (1 << V4L2_MPEG_VIDEO_H264_PROFILE_HIGH)),
> >  		V4L2_MPEG_VIDEO_H264_PROFILE_HIGH);
> 
> I'm not sure about this one.
> 
> regards
> Philipp

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

[Philipp Zabel]

Hi Nicolas,

On Fri, 2020-07-17 at 11:50 -0400, Nicolas Dufresne wrote:
> 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?

I didn't see the decoder change, ^ this was just referring to the
encoder level.

> 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.

You are right that the decoder, depending on the individual stream, may
well be capable of playing back a higher level than officially
supported. It is just not guaranteed that any stream of that unsupported
level can play back smoothly.

I suppose on i.MX6 the bottleneck is more likely to be the macroblock
processing rate than the encoded bitrate, especially if the memory bus
is under load. I'm not sure we should increase the advertised level
unless we can reach required MB/s as well. That being said, there is a
kernel option in the i.MX6 vendor kernel that disables CPU and bus
frequency scaling, keeps the SoC voltage high, and overclocks the VPU to
352 MHz. So there might be some headroom left to actually support this.

> 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

I'm not sure the CODA960 VPU on i.MX6 at 264 MHz is as capable as the
CODA966 mentioned on their website.

> 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.

I think negotiation is important for sources that can provide multiple
levels, to choose the right level for the decoder. If there is a given
stream with a fixed level, it might indeed be better to not fail
negotiation (maybe have a warning instead) and just hope for the best,
as for some streams it might just work.

regards
Philipp

Re: [PATCH 1/2] media: coda: Fix reported H264 profile

[Philipp Zabel]

On Fri, 2020-07-17 at 11:56 -0400, Nicolas Dufresne wrote:
> Le vendredi 17 juillet 2020 à 10:14 +0200, Philipp Zabel a écrit :
> > On Fri, 2020-07-17 at 00:49 -0300, Ezequiel Garcia wrote:
> > > From: Nicolas Dufresne <nicolas.dufresne@collabora.com>
> > > 
> > > The CODA960 manual states that ASO/FMO features of baseline are not
> > > supported, so for this reason this driver should only report
> > > constrained baseline support.
> > 
> > I know the encoder doesn't support this, but is this also true of the
> > decoder? The i.MX6DQ Reference Manual explicitly lists H.264/AVC decoder
> > support for both baseline profile and constrained base line profile.
> 
> Hmm, double checking, you are right this is documented in the encoding tools
> sections, not the decoding. But there is extra buffers that need to be passed
> for ASO/FMO to work, I greatly doubt you have ever tested it.

And you are correct, I don't think I use any test streams that have
ASO/FMO enabled.

> This is not supported by GStreamer parser, or FFMPEG parsers either.
> Again, we need to make sure in V2 that encoding and decoding
> capabilities are well seperated.
> 
> As for advertising ASO/FMO, I can leave it there, but be aware I won't be
> testing it. I can provide you links to streams if you care (they are publicly
> accessible throught the ITU conformance streams published by the ITU).

That would be welcome.

> But as for GStreamer and FFMPEG, this is not supported anyway.

Ok, how about changing the commit message to say that this is
unsupported for the encoder and untested for the decoder because there
is no userspace support?

regards
Philipp

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

[Nicolas Dufresne]

[-- Attachment #1: Type: text/plain, Size: 1408 bytes --]

Le lundi 20 juillet 2020 à 10:31 +0200, Philipp Zabel a écrit :
> > 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.
> 
> 
> I think negotiation is important for sources that can provide multiple
> levels, to choose the right level for the decoder. If there is a given
> stream with a fixed level, it might indeed be better to not fail
> negotiation (maybe have a warning instead) and just hope for the best,
> as for some streams it might just work.

Yes, agreed, but I didn't want to use the linux-media list to discuss
GStreamer designs. I have a soltion for that, I'll send a MR and will
CC you. For the general idea, I'll try and keep the levels as
"preferred" capabilities while allowing any levels. Same mechanism used
to proposed an unscaled display resolution, even though scaling might
be supported.

Nicolas

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

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

[Nicolas Dufresne]

[-- Attachment #1: Type: text/plain, Size: 2351 bytes --]

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?

Nothing is very explicit in the user manual, they speak in term of
resolution and framerate. They claim 1080p 30fps for encoding, and
1080p 60fps for decoding. For the encoder, there is an auto selection
for the level, and the documentation is maxed to 4.0, and so I would
agree that 4.0 is the max encoding level. Wikipedia also list "1,920×1,
080@30.1 (4)" so 1080p30 with 4 frame references as being an example of
4.0 maximum. So V2 of this patchset should make sure that for the
encoder this stays there.

On the decoding side, what I found is that there is an error bit
indicator called LEVELID (bit 19) that indicates that SPS level_idc
wasn't accepted. The error is described as "Supported up to 51.". So
basically there is some extra contraints that least to 4.2 as you
describe, and above 5.1 is an hard failure. That imho creates a grey-
zone. If we think of DASH/HLS, the information usually comes with
Resolution/Framerate/Codec/Profile/Level, and in this context, you can
enable 5.1 safely assuming the Resolition/Framerate/Profile are already
verified. But if you only wanted to use the level, then you could
prefer the driver to expose a max of 4.2.

So do you have an opinion on the way forward ? Personally I like the
idea of giving the list of level_idc that won't cause the parser to
reject it, and leave it to the user to validate the
Resolution/Framerate seperatly, we have the V4L2 API for that. Let me
know, as we'll use that for V2.

> 
> regards
> Philipp
> 

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

Re: [PATCH 1/2] media: coda: Fix reported H264 profile

[Nicolas Dufresne]

[-- Attachment #1: Type: text/plain, Size: 3203 bytes --]

Le vendredi 17 juillet 2020 à 10:14 +0200, Philipp Zabel a écrit :
> On Fri, 2020-07-17 at 00:49 -0300, Ezequiel Garcia wrote:
> > From: Nicolas Dufresne <nicolas.dufresne@collabora.com>
> > 
> > The CODA960 manual states that ASO/FMO features of baseline are not
> > supported, so for this reason this driver should only report
> > constrained baseline support.
> 
> I know the encoder doesn't support this, but is this also true of the
> decoder? The i.MX6DQ Reference Manual explicitly lists H.264/AVC decoder
> support for both baseline profile and constrained base line profile.
> 
> > This fixes negotiation issue with constrained baseline content
> > on GStreamer 1.17.1.
> > 
> > Cc: stable@vger.kernel.org
> > Fixes: 42a68012e67c2 ("media: coda: add read-only h.264 decoder profile/level controls")
> > Signed-off-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
> > Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
> > ---
> >  drivers/media/platform/coda/coda-common.c | 6 +++---
> >  1 file changed, 3 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/media/platform/coda/coda-common.c b/drivers/media/platform/coda/coda-common.c
> > index 3ab3d976d8ca..c641d1608825 100644
> > --- a/drivers/media/platform/coda/coda-common.c
> > +++ b/drivers/media/platform/coda/coda-common.c
> > @@ -2335,8 +2335,8 @@ static void coda_encode_ctrls(struct coda_ctx *ctx)
> >  		V4L2_CID_MPEG_VIDEO_H264_CHROMA_QP_INDEX_OFFSET, -12, 12, 1, 0);
> >  	v4l2_ctrl_new_std_menu(&ctx->ctrls, &coda_ctrl_ops,
> >  		V4L2_CID_MPEG_VIDEO_H264_PROFILE,
> > -		V4L2_MPEG_VIDEO_H264_PROFILE_BASELINE, 0x0,
> > -		V4L2_MPEG_VIDEO_H264_PROFILE_BASELINE);
> > +		V4L2_MPEG_VIDEO_H264_PROFILE_CONSTRAINED_BASELINE, 0x0,
> > +		V4L2_MPEG_VIDEO_H264_PROFILE_CONSTRAINED_BASELINE);
> 
> Encoder support is listed as baseline, not constrained baseline, in the
> manual, but the SPS NALs produced by the encoder start with:
>   00 00 00 01 67 42 40
>                     ^
> so that is profile_idc=66, constraint_set1_flag==1, constrained baseline
> indeed. I think this change is correct.
> 
> >  	if (ctx->dev->devtype->product == CODA_HX4 ||
> >  	    ctx->dev->devtype->product == CODA_7541) {
> >  		v4l2_ctrl_new_std_menu(&ctx->ctrls, &coda_ctrl_ops,
> > @@ -2417,7 +2417,7 @@ static void coda_decode_ctrls(struct coda_ctx *ctx)
> >  	ctx->h264_profile_ctrl = v4l2_ctrl_new_std_menu(&ctx->ctrls,
> >  		&coda_ctrl_ops, V4L2_CID_MPEG_VIDEO_H264_PROFILE,
> >  		V4L2_MPEG_VIDEO_H264_PROFILE_HIGH,
> > -		~((1 << V4L2_MPEG_VIDEO_H264_PROFILE_BASELINE) |
> > +		~((1 << V4L2_MPEG_VIDEO_H264_PROFILE_CONSTRAINED_BASELINE) |
> >  		  (1 << V4L2_MPEG_VIDEO_H264_PROFILE_MAIN) |
> >  		  (1 << V4L2_MPEG_VIDEO_H264_PROFILE_HIGH)),
> >  		V4L2_MPEG_VIDEO_H264_PROFILE_HIGH);
> 
> I'm not sure about this one.

Indeed, the decoder does support ASO/FMO, assuming the extra buffer
space is allocated as per the documentation (says that slice_save_size
should be the max_width * max_height * 3 / 4). Did you have that
implemented ? If not, I'd keep that patch, but add a comment to explain
that it can be enabled later.

> 
> regards
> Philipp

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

Re: [PATCH 1/2] media: coda: Fix reported H264 profile

[Nicolas Dufresne]

[-- Attachment #1: Type: text/plain, Size: 2232 bytes --]

Le lundi 20 juillet 2020 à 10:40 +0200, Philipp Zabel a écrit :
> On Fri, 2020-07-17 at 11:56 -0400, Nicolas Dufresne wrote:
> > Le vendredi 17 juillet 2020 à 10:14 +0200, Philipp Zabel a écrit :
> > > On Fri, 2020-07-17 at 00:49 -0300, Ezequiel Garcia wrote:
> > > > From: Nicolas Dufresne <nicolas.dufresne@collabora.com>
> > > > 
> > > > The CODA960 manual states that ASO/FMO features of baseline are not
> > > > supported, so for this reason this driver should only report
> > > > constrained baseline support.
> > > 
> > > I know the encoder doesn't support this, but is this also true of the
> > > decoder? The i.MX6DQ Reference Manual explicitly lists H.264/AVC decoder
> > > support for both baseline profile and constrained base line profile.
> > 
> > Hmm, double checking, you are right this is documented in the encoding tools
> > sections, not the decoding. But there is extra buffers that need to be passed
> > for ASO/FMO to work, I greatly doubt you have ever tested it.
> 
> And you are correct, I don't think I use any test streams that have
> ASO/FMO enabled.
> 
> > This is not supported by GStreamer parser, or FFMPEG parsers either.
> > Again, we need to make sure in V2 that encoding and decoding
> > capabilities are well seperated.
> > 
> > As for advertising ASO/FMO, I can leave it there, but be aware I won't be
> > testing it. I can provide you links to streams if you care (they are publicly
> > accessible throught the ITU conformance streams published by the ITU).
> 
> That would be welcome.

https://www.itu.int/net/ITU-T/sigdb/spevideo/VideoForm-s.aspx?val=102002641

Notably, if you download the AVCv1 series, there is at
least FM2_SVA_C.zip that uses FMO (slice groups). I haven't looked them
up, I barely started harnessing it for GStreamer.

You can find the link to everything else here, including HEVC.

https://www.itu.int/net/ITU-T/sigdb/spevideo/Hseries-s.htm

> 
> > But as for GStreamer and FFMPEG, this is not supported anyway.
> 
> Ok, how about changing the commit message to say that this is
> unsupported for the encoder and untested for the decoder because there
> is no userspace support?

Agreed.

> 
> regards
> Philipp

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

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

[Nicolas Dufresne]

[-- Attachment #1: Type: text/plain, Size: 2914 bytes --]

Le lundi 20 juillet 2020 à 12:09 -0400, Nicolas Dufresne a écrit :
> 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?
> 
> Nothing is very explicit in the user manual, they speak in term of
> resolution and framerate. They claim 1080p 30fps for encoding, and
> 1080p 60fps for decoding. For the encoder, there is an auto selection
> for the level, and the documentation is maxed to 4.0, and so I would
> agree that 4.0 is the max encoding level. Wikipedia also list "1,920×1,
> 080@30.1 (4)" so 1080p30 with 4 frame references as being an example of
> 4.0 maximum. So V2 of this patchset should make sure that for the
> encoder this stays there.
> 
> On the decoding side, what I found is that there is an error bit
> indicator called LEVELID (bit 19) that indicates that SPS level_idc
> wasn't accepted. The error is described as "Supported up to 51.". So
> basically there is some extra contraints that least to 4.2 as you
> describe, and above 5.1 is an hard failure. That imho creates a grey-
> zone. If we think of DASH/HLS, the information usually comes with
> Resolution/Framerate/Codec/Profile/Level, and in this context, you can
> enable 5.1 safely assuming the Resolition/Framerate/Profile are already
> verified. But if you only wanted to use the level, then you could
> prefer the driver to expose a max of 4.2.
> 
> So do you have an opinion on the way forward ? Personally I like the
> idea of giving the list of level_idc that won't cause the parser to
> reject it, and leave it to the user to validate the
> Resolution/Framerate seperatly, we have the V4L2 API for that. Let me
> know, as we'll use that for V2.

Dropping some extra information I received, the CODA960 is designed to
run at 352MHz, but on IMX.6 we run it at 264MHz. Means that CODA960 on
IMX.6 is special, it under-perform the spec by 25%, so it's more of a
1080p45 decoder. At this level of variation, it sounds like that should
endup in per SoC, since you could design an IMX.6 board with better
heat dissipation that could support 352.

> 
> > regards
> > Philipp
> > 

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

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

[Philipp Zabel]

Hi,

[adding Stanimir for some insight, venus also has a decoder h.264 level
control]

On Mon, 2020-07-20 at 12:09 -0400, Nicolas Dufresne wrote:
> 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.
[...]
> 
> So do you have an opinion on the way forward ? Personally I like the
> idea of giving the list of level_idc that won't cause the parser to
> reject it, and leave it to the user to validate the
> Resolution/Framerate seperatly, we have the V4L2 API for that. Let me
> know, as we'll use that for V2.

My opinion was that for decoders the possible values of the
V4L2_CID_MPEG_VIDEO_H264_LEVEL control should reflect the h.264 levels
that the decoder can actually decode in real-time. Otherwise we are
effectively ignoring the MaxMBPS and MaxBR properties of the level,
which makes the control useless for negotiation.

But I am beginning to think that I am wrong. The level control is set to
the level of the stream after parsing the stream header, so arguably it
must be possible to set it to anything that can be decoded at all, real-
time or not.

Further, the documentation says nothing about this. It doesn't even
mention decoders:
                                                                     
  ``V4L2_CID_MPEG_VIDEO_H264_LEVEL``
      (enum)

  enum v4l2_mpeg_video_h264_level -
      The level information for the H264 video elementary stream.
      Applicable to the H264 encoder. Possible values are:

  [...]

So at the moment I would tend towards expanding the list of supported
formats to 4.2, even though we can't decoder > 4.0 in real-time on
i.MX6.

Should we add a note to the V4L2_CID_MPEG_VIDEO_H264_LEVEL documentation
that this is applicable to H264 decoders as well, set by the decoder
after parsing the SPS, and that the list of levels this control can be
set to does not guarantee real-time capability at each level?

regards
Philipp