Re: [PATCH v2 4/5] visl: Add a codec specific variability parameter

[Detlev Casanova <detlev.casanova@collabora.com>]

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

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


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