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