diff mbox series

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

Message ID 20231024191027.305622-5-detlev.casanova@collabora.com (mailing list archive)
State New, archived
Headers show
Series visl: Adapt output frames for reference comparison | expand

Commit Message

Detlev Casanova Oct. 24, 2023, 7:09 p.m. UTC 
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(+)

Comments

Hans Verkuil Nov. 22, 2023, 4:07 p.m. UTC | #1 
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
Detlev Casanova Nov. 22, 2023, 4:52 p.m. UTC | #2 
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
diff mbox series

Patch

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)");
+
 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;
+	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 { \