[FFmpeg-devel] [PATCH v2 2/3] avfilter/vsrc_mptestsrc: simplify the code and change the type of frame

[Moritz Barsnick]

On Mon, Aug 12, 2019 at 23:39:52 +0800, lance.lmwang at gmail.com wrote:

This looks very wrong. Does it work?

> -    if (tt == TEST_ALL && frame%test->max_frames) /* draw a black frame at the beginning of each test */
> -        tt = (frame/test->max_frames)%(TEST_NB-1);
> +    frame = frame/test->max_frames;

Here, you reduce frame to frame/test->max_frames.

> +    if (tt == TEST_ALL && frame) /* draw a black frame at the beginning of each test */
> +        tt = frame%(TEST_NB-1);

Which makes this replacement correct. But:

> -    case TEST_DC_LUMA:       dc_test(picref->data[0], picref->linesize[0], 256, 256, frame%test->max_frames); break;
> -    case TEST_DC_CHROMA:     dc_test(picref->data[1], picref->linesize[1], 256, 256, frame%test->max_frames); break;
> -    case TEST_FREQ_LUMA:   freq_test(picref->data[0], picref->linesize[0], frame%test->max_frames); break;
> -    case TEST_FREQ_CHROMA: freq_test(picref->data[1], picref->linesize[1], frame%test->max_frames); break;
> -    case TEST_AMP_LUMA:     amp_test(picref->data[0], picref->linesize[0], frame%test->max_frames); break;
> -    case TEST_AMP_CHROMA:   amp_test(picref->data[1], picref->linesize[1], frame%test->max_frames); break;
> -    case TEST_CBP:          cbp_test(picref->data   , picref->linesize   , frame%test->max_frames); break;
> -    case TEST_MV:            mv_test(picref->data[0], picref->linesize[0], frame%test->max_frames); break;
> -    case TEST_RING1:      ring1_test(picref->data[0], picref->linesize[0], frame%test->max_frames); break;
> -    case TEST_RING2:      ring2_test(picref->data[0], picref->linesize[0], frame%test->max_frames); break;
> +    case TEST_DC_LUMA:       dc_test(picref->data[0], picref->linesize[0], 256, 256, frame); break;
> +    case TEST_DC_CHROMA:     dc_test(picref->data[1], picref->linesize[1], 256, 256, frame); break;
> +    case TEST_FREQ_LUMA:   freq_test(picref->data[0], picref->linesize[0], frame); break;
> +    case TEST_FREQ_CHROMA: freq_test(picref->data[1], picref->linesize[1], frame); break;
> +    case TEST_AMP_LUMA:     amp_test(picref->data[0], picref->linesize[0], frame); break;
> +    case TEST_AMP_CHROMA:   amp_test(picref->data[1], picref->linesize[1], frame); break;
> +    case TEST_CBP:          cbp_test(picref->data   , picref->linesize   , frame); break;
> +    case TEST_MV:            mv_test(picref->data[0], picref->linesize[0], frame); break;
> +    case TEST_RING1:      ring1_test(picref->data[0], picref->linesize[0], frame); break;
> +    case TEST_RING2:      ring2_test(picref->data[0], picref->linesize[0], frame); break;

Here, you have effectively replaced "frame%test->max_frames" with
"frame/test->max_frames" (now "frame"). Are you sure?

Cheers,
Moritz