<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Wed, Feb 17, 2016 at 9:05 AM, chen <span dir="ltr"><<a href="mailto:chenm003@163.com" target="_blank">chenm003@163.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div style="line-height:1.7;color:rgb(0,0,0);font-size:14px;font-family:arial"><br><pre><div><div class="h5">At 2016-02-16 23:11:19,<a href="mailto:ashok@multicorewareinc.com" target="_blank">ashok@multicorewareinc.com</a> wrote:
># HG changeset patch
># User Ashok Kumar Mishra<<a href="mailto:ashok@multicorewareinc.com" target="_blank">ashok@multicorewareinc.com</a>>
># Date 1455633534 -19800
># Tue Feb 16 20:08:54 2016 +0530
># Node ID 5f1817e5ff55689c45b12e1982a4c4fc46201382
># Parent a26f3d8c9758c1ee510c75518304d4c9445a202b
>SAO: modified SignOf2()
>Both SignOf and SignOf2 can be written as macro instead of using a separate namespace
>estSaoDist() is used for sao distortion calculation, it should be part of sao class
>
>diff -r a26f3d8c9758 -r 5f1817e5ff55 source/encoder/sao.cpp
>--- a/source/encoder/sao.cpp Tue Feb 16 20:00:33 2016 +0530
>+++ b/source/encoder/sao.cpp Tue Feb 16 20:08:54 2016 +0530
>@@ -29,6 +29,10 @@
> #include "picyuv.h"
> #include "sao.h"
>
>+/* get the sign of input variable */
>+#define signOf(X) (((X) >> 31) | (((uint32_t)-(X)) >> 31))
>+#define signOf2(A, B) (((A) < (B)) ? -1 : ((A) > (B)) ? 1 : 0)
>+
> namespace {
>
> inline int32_t roundIBDI(int32_t num, int32_t den)
>@@ -36,27 +40,6 @@
> return num >= 0 ? ((num * 2 + den) / (den * 2)) : -((-num * 2 + den) / (den * 2));
> }
>
>-/* get the sign of input variable (TODO: this is a dup, make common) */
>-inline int8_t signOf(int x)
>-{
>- return (x >> 31) | ((int)((((uint32_t)-x)) >> 31));
>-}
>-
>-inline int signOf2(const int a, const int b)
>-{
>- // NOTE: don't reorder below compare, both ICL, VC, GCC optimize strong depends on order!
>- int r = 0;
>- if (a < b)
>- r = -1;
>- if (a > b)
>- r = 1;
>- return r;
>-}
>-
>-inline int64_t estSaoDist(int32_t count, int offset, int32_t offsetOrg)
>-{
>- return (count * offset - offsetOrg * 2) * offset;
>-}
> } // end anonymous namespace
>
>
>@@ -1413,7 +1396,12 @@
> }
> }
>
>-inline int SAO::estIterOffset(int typeIdx, double lambda, int offset, int32_t count, int32_t offsetOrg, int& distBOClasses, double& costBOClasses)
>+inline int64_t SAO::estSaoDist(int32_t count, int offset, int32_t offsetOrg)<br><br></div></div>This modify will made a real function call, the call/ret cost more than compute cost.</pre></div></blockquote><div>No. the estSaoDist() will not make a real function call, still it is a inline function call as it was previously. Yoy can check.</div><div>But now the only difference is it is part of sao class, as it is expected. Previously it was inside anonymous namespace. </div><div>I don't think it is a good coding practice to make a unnecessary namespace and name it as "anonymous".</div><div><br></div><div>And previously, the function estIterOffset() was declared inline. But by looking this function you can say compiler will not</div><div>make it inline. As we know even we force a function to be inline, compiler will not make it inline. Compiler will make a function</div><div>inline based on certain conditions. </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div style="line-height:1.7;color:rgb(0,0,0);font-size:14px;font-family:arial"><pre><span class=""><br>
>+{
>+ return (count * offset - offsetOrg * 2) * offset;
>+}
>+
>+int SAO::estIterOffset(int typeIdx, double lambda, int offset, int32_t count, int32_t offsetOrg, int& distBOClasses, double& costBOClasses)
> {
> int bestOffset = 0;
>
>@@ -1740,7 +1728,6 @@
> for (int x = 0; x < endX; x++)
> {
> int signRight = signOf2(rec[x], rec[x + 1]);
>- X265_CHECK(signRight == signOf(rec[x] - rec[x + 1]), "signDown check failure\n");<br><br></span>Why remove self verify condition?</pre></div></blockquote><div>I believe this self verify code is not required at all. This self verify code was written to check whether signOf2() was working correctly or not.</div><div>If that is the case why we are using two functions signOf() and signOf2() functions ? Can we not manage with only one sign check function ? </div><div>And yes, if you want your self verify condition checks, i can restore in next patch.</div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div style="line-height:1.7;color:rgb(0,0,0);font-size:14px;font-family:arial"><pre><span class=""><br>
> uint32_t edgeType = signRight + signLeft + 2;
> signLeft = -signRight;
>
</span></pre></div><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></blockquote></div><br></div></div>