diff mbox series

[FFmpeg-devel,2/2] avcodec/motion_est: fix penalty_factor for b frames

Message ID 20200322155525.23410-2-ramiro.polla@gmail.com
State New
Headers show
Series [FFmpeg-devel,1/2] avcodec/mpegvideo_enc: fix multi-threaded motion estimation rounding for mpeg4 | expand

Checks

Context Check Description
andriy/ffmpeg-patchwork success Make fate finished

Commit Message

Ramiro Polla March 22, 2020, 3:55 p.m. UTC 
In ff_estimate_b_frame_motion(), penalty_factor would be used before
being initialized in estimate_motion_b(). Also, the initialization
would happen more than once unnecessarily.
---
 libavcodec/motion_est.c                  | 15 ++++++++-------
 tests/ref/vsynth/vsynth2-mpeg2-422       |  6 +++---
 tests/ref/vsynth/vsynth2-mpeg2-ivlc-qprd |  6 +++---
 tests/ref/vsynth/vsynth2-mpeg4-adap      |  6 +++---
 4 files changed, 17 insertions(+), 16 deletions(-)

Comments

Michael Niedermayer March 23, 2020, 9:42 p.m. UTC | #1 
On Sun, Mar 22, 2020 at 04:55:25PM +0100, Ramiro Polla wrote:
> In ff_estimate_b_frame_motion(), penalty_factor would be used before
> being initialized in estimate_motion_b(). Also, the initialization
> would happen more than once unnecessarily.
> ---
>  libavcodec/motion_est.c                  | 15 ++++++++-------
>  tests/ref/vsynth/vsynth2-mpeg2-422       |  6 +++---
>  tests/ref/vsynth/vsynth2-mpeg2-ivlc-qprd |  6 +++---
>  tests/ref/vsynth/vsynth2-mpeg4-adap      |  6 +++---
>  4 files changed, 17 insertions(+), 16 deletions(-)
> 
> diff --git a/libavcodec/motion_est.c b/libavcodec/motion_est.c
> index 02c75fd470..1feb46cec3 100644
> --- a/libavcodec/motion_est.c
> +++ b/libavcodec/motion_est.c
> @@ -1123,9 +1123,6 @@ static int estimate_motion_b(MpegEncContext *s, int mb_x, int mb_y,
>      uint8_t * const mv_penalty= c->mv_penalty[f_code] + MAX_DMV;
>      int mv_scale;
>  
> -    c->penalty_factor    = get_penalty_factor(s->lambda, s->lambda2, c->avctx->me_cmp);
> -    c->sub_penalty_factor= get_penalty_factor(s->lambda, s->lambda2, c->avctx->me_sub_cmp);
> -    c->mb_penalty_factor = get_penalty_factor(s->lambda, s->lambda2, c->avctx->mb_cmp);
>      c->current_mv_penalty= mv_penalty;
>  
>      get_limits(s, 16*mb_x, 16*mb_y);



> @@ -1491,7 +1488,6 @@ void ff_estimate_b_frame_motion(MpegEncContext * s,
>                               int mb_x, int mb_y)
>  {
>      MotionEstContext * const c= &s->me;
> -    const int penalty_factor= c->mb_penalty_factor;
>      int fmin, bmin, dmin, fbmin, bimin, fimin;
>      int type=0;
>      const int xy = mb_y*s->mb_stride + mb_x;
> @@ -1517,18 +1513,23 @@ void ff_estimate_b_frame_motion(MpegEncContext * s,
>          dmin= direct_search(s, mb_x, mb_y);
>      else
>          dmin= INT_MAX;
> +
> +    c->penalty_factor    = get_penalty_factor(s->lambda, s->lambda2, c->avctx->me_cmp);
> +    c->sub_penalty_factor= get_penalty_factor(s->lambda, s->lambda2, c->avctx->me_sub_cmp);
> +    c->mb_penalty_factor = get_penalty_factor(s->lambda, s->lambda2, c->avctx->mb_cmp);

If mb_penalty_factor isnt correct in this before this maybe isnt enough
as the direct_search() uses mb_penalty_factor 

thx

[...]
Ramiro Polla March 24, 2020, 5:31 a.m. UTC | #2 
On Mon, Mar 23, 2020 at 10:42 PM Michael Niedermayer
<michael@niedermayer.cc> wrote:
> On Sun, Mar 22, 2020 at 04:55:25PM +0100, Ramiro Polla wrote:
> > In ff_estimate_b_frame_motion(), penalty_factor would be used before
> > being initialized in estimate_motion_b(). Also, the initialization
> > would happen more than once unnecessarily.
> > ---
> >  libavcodec/motion_est.c                  | 15 ++++++++-------
> >  tests/ref/vsynth/vsynth2-mpeg2-422       |  6 +++---
> >  tests/ref/vsynth/vsynth2-mpeg2-ivlc-qprd |  6 +++---
> >  tests/ref/vsynth/vsynth2-mpeg4-adap      |  6 +++---
> >  4 files changed, 17 insertions(+), 16 deletions(-)
> >
> > diff --git a/libavcodec/motion_est.c b/libavcodec/motion_est.c
> > index 02c75fd470..1feb46cec3 100644
> > --- a/libavcodec/motion_est.c
> > +++ b/libavcodec/motion_est.c
> > @@ -1123,9 +1123,6 @@ static int estimate_motion_b(MpegEncContext *s, int mb_x, int mb_y,
> >      uint8_t * const mv_penalty= c->mv_penalty[f_code] + MAX_DMV;
> >      int mv_scale;
> >
> > -    c->penalty_factor    = get_penalty_factor(s->lambda, s->lambda2, c->avctx->me_cmp);
> > -    c->sub_penalty_factor= get_penalty_factor(s->lambda, s->lambda2, c->avctx->me_sub_cmp);
> > -    c->mb_penalty_factor = get_penalty_factor(s->lambda, s->lambda2, c->avctx->mb_cmp);
> >      c->current_mv_penalty= mv_penalty;
> >
> >      get_limits(s, 16*mb_x, 16*mb_y);
>
>
>
> > @@ -1491,7 +1488,6 @@ void ff_estimate_b_frame_motion(MpegEncContext * s,
> >                               int mb_x, int mb_y)
> >  {
> >      MotionEstContext * const c= &s->me;
> > -    const int penalty_factor= c->mb_penalty_factor;
> >      int fmin, bmin, dmin, fbmin, bimin, fimin;
> >      int type=0;
> >      const int xy = mb_y*s->mb_stride + mb_x;
> > @@ -1517,18 +1513,23 @@ void ff_estimate_b_frame_motion(MpegEncContext * s,
> >          dmin= direct_search(s, mb_x, mb_y);
> >      else
> >          dmin= INT_MAX;
> > +
> > +    c->penalty_factor    = get_penalty_factor(s->lambda, s->lambda2, c->avctx->me_cmp);
> > +    c->sub_penalty_factor= get_penalty_factor(s->lambda, s->lambda2, c->avctx->me_sub_cmp);
> > +    c->mb_penalty_factor = get_penalty_factor(s->lambda, s->lambda2, c->avctx->mb_cmp);
>
> If mb_penalty_factor isnt correct in this before this maybe isnt enough
> as the direct_search() uses mb_penalty_factor

Fixed.

New patch attached.
diff mbox series

Patch

diff --git a/libavcodec/motion_est.c b/libavcodec/motion_est.c
index 02c75fd470..1feb46cec3 100644
--- a/libavcodec/motion_est.c
+++ b/libavcodec/motion_est.c
@@ -1123,9 +1123,6 @@  static int estimate_motion_b(MpegEncContext *s, int mb_x, int mb_y,
     uint8_t * const mv_penalty= c->mv_penalty[f_code] + MAX_DMV;
     int mv_scale;
 
-    c->penalty_factor    = get_penalty_factor(s->lambda, s->lambda2, c->avctx->me_cmp);
-    c->sub_penalty_factor= get_penalty_factor(s->lambda, s->lambda2, c->avctx->me_sub_cmp);
-    c->mb_penalty_factor = get_penalty_factor(s->lambda, s->lambda2, c->avctx->mb_cmp);
     c->current_mv_penalty= mv_penalty;
 
     get_limits(s, 16*mb_x, 16*mb_y);
@@ -1491,7 +1488,6 @@  void ff_estimate_b_frame_motion(MpegEncContext * s,
                              int mb_x, int mb_y)
 {
     MotionEstContext * const c= &s->me;
-    const int penalty_factor= c->mb_penalty_factor;
     int fmin, bmin, dmin, fbmin, bimin, fimin;
     int type=0;
     const int xy = mb_y*s->mb_stride + mb_x;
@@ -1517,18 +1513,23 @@  void ff_estimate_b_frame_motion(MpegEncContext * s,
         dmin= direct_search(s, mb_x, mb_y);
     else
         dmin= INT_MAX;
+
+    c->penalty_factor    = get_penalty_factor(s->lambda, s->lambda2, c->avctx->me_cmp);
+    c->sub_penalty_factor= get_penalty_factor(s->lambda, s->lambda2, c->avctx->me_sub_cmp);
+    c->mb_penalty_factor = get_penalty_factor(s->lambda, s->lambda2, c->avctx->mb_cmp);
+
 // FIXME penalty stuff for non-MPEG-4
     c->skip=0;
     fmin = estimate_motion_b(s, mb_x, mb_y, s->b_forw_mv_table, 0, s->f_code) +
-           3 * penalty_factor;
+           3 * c->mb_penalty_factor;
 
     c->skip=0;
     bmin = estimate_motion_b(s, mb_x, mb_y, s->b_back_mv_table, 2, s->b_code) +
-           2 * penalty_factor;
+           2 * c->mb_penalty_factor;
     ff_dlog(s, " %d %d ", s->b_forw_mv_table[xy][0], s->b_forw_mv_table[xy][1]);
 
     c->skip=0;
-    fbmin= bidir_refine(s, mb_x, mb_y) + penalty_factor;
+    fbmin= bidir_refine(s, mb_x, mb_y) + c->mb_penalty_factor;
     ff_dlog(s, "%d %d %d %d\n", dmin, fmin, bmin, fbmin);
 
     if (s->avctx->flags & AV_CODEC_FLAG_INTERLACED_ME) {
diff --git a/tests/ref/vsynth/vsynth2-mpeg2-422 b/tests/ref/vsynth/vsynth2-mpeg2-422
index ec7244f9f9..e945a4cc0e 100644
--- a/tests/ref/vsynth/vsynth2-mpeg2-422
+++ b/tests/ref/vsynth/vsynth2-mpeg2-422
@@ -1,4 +1,4 @@ 
-b2fa9b73c3547191ecc01b8163abd4e5 *tests/data/fate/vsynth2-mpeg2-422.mpeg2video
-379164 tests/data/fate/vsynth2-mpeg2-422.mpeg2video
-704f6a96f93c2409219bd48b74169041 *tests/data/fate/vsynth2-mpeg2-422.out.rawvideo
+6fc8dc1d76379e459051ca393101c090 *tests/data/fate/vsynth2-mpeg2-422.mpeg2video
+379173 tests/data/fate/vsynth2-mpeg2-422.mpeg2video
+9199d5aaa1709d2584e21e58d76d44fb *tests/data/fate/vsynth2-mpeg2-422.out.rawvideo
 stddev:    4.17 PSNR: 35.73 MAXDIFF:   70 bytes:  7603200/  7603200
diff --git a/tests/ref/vsynth/vsynth2-mpeg2-ivlc-qprd b/tests/ref/vsynth/vsynth2-mpeg2-ivlc-qprd
index 16de39edfc..f5bbecfcb2 100644
--- a/tests/ref/vsynth/vsynth2-mpeg2-ivlc-qprd
+++ b/tests/ref/vsynth/vsynth2-mpeg2-ivlc-qprd
@@ -1,4 +1,4 @@ 
-907a30295ed8323780eee08e606af0ab *tests/data/fate/vsynth2-mpeg2-ivlc-qprd.mpeg2video
-269722 tests/data/fate/vsynth2-mpeg2-ivlc-qprd.mpeg2video
-d2d9793bf8f3427b5cc17a1be78ddd64 *tests/data/fate/vsynth2-mpeg2-ivlc-qprd.out.rawvideo
+f612ea89aa79a7f7b93a8acf332705c4 *tests/data/fate/vsynth2-mpeg2-ivlc-qprd.mpeg2video
+269723 tests/data/fate/vsynth2-mpeg2-ivlc-qprd.mpeg2video
+88e17886e6383755829d7da519fd5e79 *tests/data/fate/vsynth2-mpeg2-ivlc-qprd.out.rawvideo
 stddev:    5.54 PSNR: 33.25 MAXDIFF:   94 bytes:  7603200/  7603200
diff --git a/tests/ref/vsynth/vsynth2-mpeg4-adap b/tests/ref/vsynth/vsynth2-mpeg4-adap
index a3223f6363..1ae0a65e4f 100644
--- a/tests/ref/vsynth/vsynth2-mpeg4-adap
+++ b/tests/ref/vsynth/vsynth2-mpeg4-adap
@@ -1,4 +1,4 @@ 
-4bff98da2342836476da817428594403 *tests/data/fate/vsynth2-mpeg4-adap.avi
-213508 tests/data/fate/vsynth2-mpeg4-adap.avi
-0c709f2b81f4593eaa29490332c2cb39 *tests/data/fate/vsynth2-mpeg4-adap.out.rawvideo
+fcb79c0dcc00b306b79c354e589b6b69 *tests/data/fate/vsynth2-mpeg4-adap.avi
+213526 tests/data/fate/vsynth2-mpeg4-adap.avi
+71a34a48a81485f938d2c60a3d34ed39 *tests/data/fate/vsynth2-mpeg4-adap.out.rawvideo
 stddev:    4.87 PSNR: 34.36 MAXDIFF:   86 bytes:  7603200/  7603200