Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:7 as permitted sender) client-ip=2620:137:e000::3:7;
From:   Detlev Casanova <detlev.casanova@collabora.com>
To:     linux-kernel@vger.kernel.org, Hans Verkuil <hverkuil@xs4all.nl>
Cc:     linux-media@vger.kernel.org,
        Daniel Almeida <daniel.almeida@collabora.com>,
        Mauro Carvalho Chehab <mchehab@kernel.org>
Subject: Re: [PATCH v2 4/5] visl: Add a codec specific variability parameter
Date:   Wed, 22 Nov 2023 11:52:08 -0500
Message-ID: <10376589.nUPlyArG6x@arisu>
In-Reply-To: <bbc673bd-de2c-43eb-81c0-16a9dfad4c4e@xs4all.nl>
References: <20231024191027.305622-1-detlev.casanova@collabora.com>
 <20231024191027.305622-5-detlev.casanova@collabora.com>
 <bbc673bd-de2c-43eb-81c0-16a9dfad4c4e@xs4all.nl>
MIME-Version: 1.0
Content-Type: multipart/signed; boundary="nextPart2250269.iZASKD2KPV";
 micalg="pgp-sha256"; protocol="application/pgp-signature"
Precedence: bulk

--nextPart2250269.iZASKD2KPV
Content-Transfer-Encoding: 7Bit
Content-Type: text/plain; charset="us-ascii"; protected-headers="v1"
From: Detlev Casanova <detlev.casanova@collabora.com>
To: linux-kernel@vger.kernel.org, Hans Verkuil <hverkuil@xs4all.nl>
Date: Wed, 22 Nov 2023 11:52:08 -0500
Message-ID: <10376589.nUPlyArG6x@arisu>
In-Reply-To: <bbc673bd-de2c-43eb-81c0-16a9dfad4c4e@xs4all.nl>
MIME-Version: 1.0

On Wednesday, November 22, 2023 11:07:18 A.M. EST Hans Verkuil wrote:
> On 24/10/2023 21:09, Detlev Casanova wrote:
> > When running tests with different input data, the stable output frames
> > could be too similar and hide possible issues.
> > 
> > This commit adds variation by using some codec specific parameters.
> > 
> > Only HEVC and H.264 support this.
> > 
> > Reviewed-by: Daniel Almeida <daniel.almeida@collabora.com>
> > Signed-off-by: Detlev Casanova <detlev.casanova@collabora.com>
> > ---
> > 
> >  drivers/media/test-drivers/visl/visl-core.c |  5 ++++
> >  drivers/media/test-drivers/visl/visl-dec.c  | 27 +++++++++++++++++++++
> >  drivers/media/test-drivers/visl/visl.h      |  1 +
> >  3 files changed, 33 insertions(+)
> > 
> > diff --git a/drivers/media/test-drivers/visl/visl-core.c
> > b/drivers/media/test-drivers/visl/visl-core.c index
> > d28d50afec02..e7466f6a91e1 100644
> > --- a/drivers/media/test-drivers/visl/visl-core.c
> > +++ b/drivers/media/test-drivers/visl/visl-core.c
> > @@ -93,6 +93,11 @@ module_param(stable_output, bool, 0644);
> > 
> >  MODULE_PARM_DESC(stable_output,
> >  
> >  		 " only write stable data for a given input on the 
output frames");
> > 
> > +bool codec_variability;
> > +module_param(codec_variability, bool, 0644);
> > +MODULE_PARM_DESC(codec_variability,
> > +		 " add codec specific variability data to generate more 
unique frames.
> > (Only h.264 and hevc)");
> Why make this a module parameter instead of always doing this?
> 
> It's not clear from the commit log why a parameter is needed.

I agree with that, I started as a parameter when I wasn't sure what 
variability values or method would be used, but just showing a value can be 
integrated without a parameter and keep it more simple. I'll change that.

> > +
> > 
> >  static const struct visl_ctrl_desc visl_fwht_ctrl_descs[] = {
> >  
> >  	{
> >  	
> >  		.cfg.id = V4L2_CID_STATELESS_FWHT_PARAMS,
> > 
> > diff --git a/drivers/media/test-drivers/visl/visl-dec.c
> > b/drivers/media/test-drivers/visl/visl-dec.c index
> > 61cfca49ead9..002d5e3b0ea4 100644
> > --- a/drivers/media/test-drivers/visl/visl-dec.c
> > +++ b/drivers/media/test-drivers/visl/visl-dec.c
> > @@ -223,6 +223,26 @@ static void visl_tpg_fill_sequence(struct visl_ctx
> > *ctx,> 
> >  	}
> >  
> >  }
> > 
> > +static bool visl_tpg_fill_codec_specific(struct visl_ctx *ctx,
> > +					 struct visl_run *run,
> > +					 char buf[], size_t 
bufsz)
> > +{
> > +	switch (ctx->current_codec) {
> > +	case VISL_CODEC_H264:
> > +		scnprintf(buf, bufsz,
> > +			  "H264: %u", run->h264.dpram-
>pic_order_cnt_lsb);
> > +		break;
> > +	case VISL_CODEC_HEVC:
> > +		scnprintf(buf, bufsz,
> > +			  "HEVC: %d", run->hevc.dpram-
>pic_order_cnt_val);
> > +		break;
> 
> Perhaps mention here why these specific values are chosen?

Will do.

> > +	default:
> > +		return false;
> > +	}
> > +
> > +	return true;
> > +}
> > +
> > 
> >  static void visl_tpg_fill(struct visl_ctx *ctx, struct visl_run *run)
> >  {
> >  
> >  	u8 *basep[TPG_MAX_PLANES][2];
> > 
> > @@ -255,6 +275,13 @@ static void visl_tpg_fill(struct visl_ctx *ctx,
> > struct visl_run *run)> 
> >  	frame_dprintk(ctx->dev, run->dst->sequence, "");
> >  	line++;
> > 
> > +	if (codec_variability && visl_tpg_fill_codec_specific(ctx, run, buf,
> > TPG_STR_BUF_SZ)) { +		tpg_gen_text(&ctx->tpg, basep, line++ *
> > line_height, 16, buf);
> > +		frame_dprintk(ctx->dev, run->dst->sequence, "%s\n", 
buf);
> > +		frame_dprintk(ctx->dev, run->dst->sequence, "");
> > +		line++;
> > +	}
> > +
> > 
> >  	if (!stable_output) {
> >  	
> >  		visl_get_ref_frames(ctx, buf, TPG_STR_BUF_SZ, run);
> > 
> > diff --git a/drivers/media/test-drivers/visl/visl.h
> > b/drivers/media/test-drivers/visl/visl.h index 5a81b493f121..4ac2d1783020
> > 100644
> > --- a/drivers/media/test-drivers/visl/visl.h
> > +++ b/drivers/media/test-drivers/visl/visl.h
> > @@ -86,6 +86,7 @@ extern bool keep_bitstream_buffers;
> > 
> >  extern int bitstream_trace_frame_start;
> >  extern unsigned int bitstream_trace_nframes;
> >  extern bool stable_output;
> > 
> > +extern bool codec_variability;
> > 
> >  #define frame_dprintk(dev, current, fmt, arg...) \
> >  
> >  	do { \
> 
> Regards,
> 
> 	Hans


--nextPart2250269.iZASKD2KPV
Content-Type: application/pgp-signature; name="signature.asc"
Content-Description: This is a digitally signed message part.
Content-Transfer-Encoding: 7Bit

-----BEGIN PGP SIGNATURE-----

iQEzBAABCAAdFiEEonF9IvGrXNkDg+CX5EFKUk4x7bYFAmVeMbgACgkQ5EFKUk4x
7bbEgwf9FHsPocHfueH/kMUZBCVG54oUWXAp8/xnRXclmaGhbQ8W58q4rvF868aU
QaF6Nt63eTSEMj9h9xyqjKQbZX9JT7YgvOtN6C+6og3cGdg5+0k8/wdjww/QCYvl
61EvpvrktJYt9+60g/bB4Vv2m5a1AzJdpuUvFb9vughsrEJpoEFNkaSb1STh5xPL
frGUAPDP35aD2L1+gJa8mshRiadh9pGDp2k/uY1+LEq1AUly09TwxEyiIRSb/9VJ
w2iZCRDeZgRVdZmaV6ImvkzNDxXaiiaYtcXF/Xmtu6A3asNkEDYXFn4vu2C8AjNw
hVo1uyjutdQr2Mv4VM9ZsMc318S6pg==
=buIh
-----END PGP SIGNATURE-----

--nextPart2250269.iZASKD2KPV--