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@xxxxxxxxxxxxx>
> > Signed-off-by: Detlev Casanova <detlev.casanova@xxxxxxxxxxxxx>
> > ---
> >
> > 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:
signature.asc
Description: This is a digitally signed message part.
