diff mbox

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

Message ID 20190812153953.11839-2-lance.lmwang@gmail.com
State Superseded
Headers show

Commit Message

Lance Wang Aug. 12, 2019, 3:39 p.m. UTC 
From: Limin Wang <lance.lmwang@gmail.com>

Signed-off-by: Limin Wang <lance.lmwang@gmail.com>
---
 libavfilter/vsrc_mptestsrc.c | 27 ++++++++++++++-------------
 1 file changed, 14 insertions(+), 13 deletions(-)

Comments

Moritz Barsnick Aug. 22, 2019, 1:04 p.m. UTC | #1 
On Mon, Aug 12, 2019 at 23:39:52 +0800, lance.lmwang@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
Lance Wang Aug. 23, 2019, 12:21 p.m. UTC | #2 
On Thu, Aug 22, 2019 at 03:04:19PM +0200, Moritz Barsnick wrote:
> On Mon, Aug 12, 2019 at 23:39:52 +0800, lance.lmwang@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.

good catch, it's wrong, it's simple replacement, so haven't check it carefully.
Now I have fixed it and update the patch. Have checked with ffplay.

> 
> > +    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
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
diff mbox

Patch

diff --git a/libavfilter/vsrc_mptestsrc.c b/libavfilter/vsrc_mptestsrc.c
index 4a2db18..14e66fd 100644
--- a/libavfilter/vsrc_mptestsrc.c
+++ b/libavfilter/vsrc_mptestsrc.c
@@ -308,7 +308,7 @@  static int request_frame(AVFilterLink *outlink)
     AVFrame *picref;
     int w = WIDTH, h = HEIGHT,
         cw = AV_CEIL_RSHIFT(w, test->hsub), ch = AV_CEIL_RSHIFT(h, test->vsub);
-    unsigned int frame = outlink->frame_count_in;
+    uint64_t  frame = outlink->frame_count_in;
     enum test_type tt = test->test;
     int i;
 
@@ -327,20 +327,21 @@  static int request_frame(AVFilterLink *outlink)
         memset(picref->data[2] + i*picref->linesize[2], 128, cw);
     }
 
-    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;
+    if (tt == TEST_ALL && frame) /* draw a black frame at the beginning of each test */
+        tt = frame%(TEST_NB-1);
 
     switch (tt) {
-    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;
     }
 
     return ff_filter_frame(outlink, picref);