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

[Hans Verkuil <hverkuil@xs4all.nl>]

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.

> +
>  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?

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