[x265] [PATCH] motion: fix overflow in mvcost() check failure

[Steve Borho]

On 07/05, Deepthi Nandakumar wrote:
> This patch is right - but I've been thinking if the mvcost() function
> itself is safe, if there is a clear risk of unsigned short overflow and
> wraparound. This could return wrong (low, wrapped around) mvcosts.

if MV dynamic range is an issue, we can lower the hard limit to
12 bits. The max MV size in bits is signaled in the stream header and we
enforce that size in the search range function.

> On Sat, Jul 4, 2015 at 12:54 AM, Steve Borho <steve at borho.org> wrote:
> 
> > On 07/03, Divya Manivannan wrote:
> > > # HG changeset patch
> > > # User Divya Manivannan <divya at multicorewareinc.com>
> > > # Date 1435933202 -19800
> > > #      Fri Jul 03 19:50:02 2015 +0530
> > > # Node ID 5a28e41404c776a0f8f81c301cbb7c0426857624
> > > # Parent  f3571dc7b077e2de084f5118c6f6a9316f40f54a
> > > motion: fix overflow in mvcost() check failure.
> > >
> > > mvcost() funtion always truncate the value to 16 bits but there is a
> > possiblity
> > > of overflow in the mvcost() check failure condition given.
> > >
> > > This check can be done in the cost calculation (s_costs) initially andso
> > it is
> > > removed here.
> >
> > LGTM
> >
> > > diff -r f3571dc7b077 -r 5a28e41404c7 source/encoder/motion.cpp
> > > --- a/source/encoder/motion.cpp       Wed Jul 01 14:36:18 2015 +0530
> > > +++ b/source/encoder/motion.cpp       Fri Jul 03 19:50:02 2015 +0530
> > > @@ -234,14 +234,9 @@
> > >                 pix_base + (m1x) + (m1y) * stride, \
> > >                 pix_base + (m2x) + (m2y) * stride, \
> > >                 stride, costs); \
> > > -        const uint16_t *base_mvx = &m_cost_mvx[(bmv.x + (m0x)) << 2]; \
> > > -        const uint16_t *base_mvy = &m_cost_mvy[(bmv.y + (m0y)) << 2]; \
> > > -        X265_CHECK(mvcost((bmv + MV(m0x, m0y)) << 2) ==
> > (base_mvx[((m0x) - (m0x)) << 2] + base_mvy[((m0y) - (m0y)) << 2]),
> > "mvcost() check failure\n"); \
> > > -        X265_CHECK(mvcost((bmv + MV(m1x, m1y)) << 2) ==
> > (base_mvx[((m1x) - (m0x)) << 2] + base_mvy[((m1y) - (m0y)) << 2]),
> > "mvcost() check failure\n"); \
> > > -        X265_CHECK(mvcost((bmv + MV(m2x, m2y)) << 2) ==
> > (base_mvx[((m2x) - (m0x)) << 2] + base_mvy[((m2y) - (m0y)) << 2]),
> > "mvcost() check failure\n"); \
> > > -        (costs)[0] += (base_mvx[((m0x) - (m0x)) << 2] + base_mvy[((m0y)
> > - (m0y)) << 2]); \
> > > -        (costs)[1] += (base_mvx[((m1x) - (m0x)) << 2] + base_mvy[((m1y)
> > - (m0y)) << 2]); \
> > > -        (costs)[2] += (base_mvx[((m2x) - (m0x)) << 2] + base_mvy[((m2y)
> > - (m0y)) << 2]); \
> > > +        (costs)[0] += mvcost((bmv + MV(m0x, m0y)) << 2); \
> > > +        (costs)[1] += mvcost((bmv + MV(m1x, m1y)) << 2); \
> > > +        (costs)[2] += mvcost((bmv + MV(m2x, m2y)) << 2); \
> > >      }
> > >
> > >  #define COST_MV_PT_DIST_X4(m0x, m0y, p0, d0, m1x, m1y, p1, d1, m2x,
> > m2y, p2, d2, m3x, m3y, p3, d3) \
> > > @@ -271,16 +266,10 @@
> > >                 pix_base + (m2x) + (m2y) * stride, \
> > >                 pix_base + (m3x) + (m3y) * stride, \
> > >                 stride, costs); \
> > > -        const uint16_t *base_mvx = &m_cost_mvx[(omv.x << 2)]; \
> > > -        const uint16_t *base_mvy = &m_cost_mvy[(omv.y << 2)]; \
> > > -        X265_CHECK(mvcost((omv + MV(m0x, m0y)) << 2) == (base_mvx[(m0x)
> > << 2] + base_mvy[(m0y) << 2]), "mvcost() check failure\n"); \
> > > -        X265_CHECK(mvcost((omv + MV(m1x, m1y)) << 2) == (base_mvx[(m1x)
> > << 2] + base_mvy[(m1y) << 2]), "mvcost() check failure\n"); \
> > > -        X265_CHECK(mvcost((omv + MV(m2x, m2y)) << 2) == (base_mvx[(m2x)
> > << 2] + base_mvy[(m2y) << 2]), "mvcost() check failure\n"); \
> > > -        X265_CHECK(mvcost((omv + MV(m3x, m3y)) << 2) == (base_mvx[(m3x)
> > << 2] + base_mvy[(m3y) << 2]), "mvcost() check failure\n"); \
> > > -        costs[0] += (base_mvx[(m0x) << 2] + base_mvy[(m0y) << 2]); \
> > > -        costs[1] += (base_mvx[(m1x) << 2] + base_mvy[(m1y) << 2]); \
> > > -        costs[2] += (base_mvx[(m2x) << 2] + base_mvy[(m2y) << 2]); \
> > > -        costs[3] += (base_mvx[(m3x) << 2] + base_mvy[(m3y) << 2]); \
> > > +        costs[0] += mvcost((omv + MV(m0x, m0y)) << 2); \
> > > +        costs[1] += mvcost((omv + MV(m1x, m1y)) << 2); \
> > > +        costs[2] += mvcost((omv + MV(m2x, m2y)) << 2); \
> > > +        costs[3] += mvcost((omv + MV(m3x, m3y)) << 2); \
> > >          COPY2_IF_LT(bcost, costs[0], bmv, omv + MV(m0x, m0y)); \
> > >          COPY2_IF_LT(bcost, costs[1], bmv, omv + MV(m1x, m1y)); \
> > >          COPY2_IF_LT(bcost, costs[2], bmv, omv + MV(m2x, m2y)); \
> > > @@ -296,17 +285,10 @@
> > >                 pix_base + (m2x) + (m2y) * stride, \
> > >                 pix_base + (m3x) + (m3y) * stride, \
> > >                 stride, costs); \
> > > -        /* TODO: use restrict keyword in ICL */ \
> > > -        const uint16_t *base_mvx = &m_cost_mvx[(bmv.x << 2)]; \
> > > -        const uint16_t *base_mvy = &m_cost_mvy[(bmv.y << 2)]; \
> > > -        X265_CHECK(mvcost((bmv + MV(m0x, m0y)) << 2) == (base_mvx[(m0x)
> > << 2] + base_mvy[(m0y) << 2]), "mvcost() check failure\n"); \
> > > -        X265_CHECK(mvcost((bmv + MV(m1x, m1y)) << 2) == (base_mvx[(m1x)
> > << 2] + base_mvy[(m1y) << 2]), "mvcost() check failure\n"); \
> > > -        X265_CHECK(mvcost((bmv + MV(m2x, m2y)) << 2) == (base_mvx[(m2x)
> > << 2] + base_mvy[(m2y) << 2]), "mvcost() check failure\n"); \
> > > -        X265_CHECK(mvcost((bmv + MV(m3x, m3y)) << 2) == (base_mvx[(m3x)
> > << 2] + base_mvy[(m3y) << 2]), "mvcost() check failure\n"); \
> > > -        (costs)[0] += (base_mvx[(m0x) << 2] + base_mvy[(m0y) << 2]); \
> > > -        (costs)[1] += (base_mvx[(m1x) << 2] + base_mvy[(m1y) << 2]); \
> > > -        (costs)[2] += (base_mvx[(m2x) << 2] + base_mvy[(m2y) << 2]); \
> > > -        (costs)[3] += (base_mvx[(m3x) << 2] + base_mvy[(m3y) << 2]); \
> > > +        (costs)[0] += mvcost((bmv + MV(m0x, m0y)) << 2); \
> > > +        (costs)[1] += mvcost((bmv + MV(m1x, m1y)) << 2); \
> > > +        (costs)[2] += mvcost((bmv + MV(m2x, m2y)) << 2); \
> > > +        (costs)[3] += mvcost((bmv + MV(m3x, m3y)) << 2); \
> > >      }
> > >
> > >  #define DIA1_ITER(mx, my) \
> > > _______________________________________________
> > > x265-devel mailing list
> > > x265-devel at videolan.org
> > > https://mailman.videolan.org/listinfo/x265-devel
> >
> > --
> > Steve Borho
> > _______________________________________________
> > x265-devel mailing list
> > x265-devel at videolan.org
> > https://mailman.videolan.org/listinfo/x265-devel
> >

> _______________________________________________
> x265-devel mailing list
> x265-devel at videolan.org
> https://mailman.videolan.org/listinfo/x265-devel


-- 
Steve Borho