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 <[email protected]> wrote: > > > On 07/03, Divya Manivannan wrote: > > > # HG changeset patch > > > # User Divya Manivannan <[email protected]> > > > # 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 > > > [email protected] > > > https://mailman.videolan.org/listinfo/x265-devel > > > > -- > > Steve Borho > > _______________________________________________ > > x265-devel mailing list > > [email protected] > > https://mailman.videolan.org/listinfo/x265-devel > > > _______________________________________________ > x265-devel mailing list > [email protected] > https://mailman.videolan.org/listinfo/x265-devel -- Steve Borho _______________________________________________ x265-devel mailing list [email protected] https://mailman.videolan.org/listinfo/x265-devel
