diff mbox series

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

Message ID 20200717034923.219524-1-ezequiel@collabora.com (mailing list archive)
State New, archived
Headers show
Series [1/2] media: coda: Fix reported H264 profile | expand

Commit Message

Ezequiel Garcia July 17, 2020, 3:49 a.m. UTC 
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(-)

Comments

Philipp Zabel July 17, 2020, 8:14 a.m. UTC | #1 
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
Nicolas Dufresne July 17, 2020, 3:56 p.m. UTC | #2 
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
Philipp Zabel July 20, 2020, 8:40 a.m. UTC | #3 
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
Nicolas Dufresne July 20, 2020, 4:13 p.m. UTC | #4 
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
Nicolas Dufresne July 20, 2020, 4:20 p.m. UTC | #5 
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
diff mbox series

Patch

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);