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

[Nicolas Dufresne <nicolas.dufresne@collabora.com>]

[-- 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 --]