[x265] [PATCH] SAO: modified SignOf2()

[Ashok Kumar Mishra]

On Wed, Feb 17, 2016 at 9:05 AM, chen <chenm003 at 163.com> wrote:

>
> At 2016-02-16 23:11:19,ashok at multicorewareinc.com wrote:
> ># HG changeset patch
> ># User Ashok Kumar Mishra<ashok at multicorewareinc.com>
> ># 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)
>
> This modify will made a real function call, the call/ret cost more than compute cost.
>
> No. the estSaoDist() will not make a real function call, still it is a
inline function call as it was previously. Yoy can check.
But now the only difference is it is part of sao class, as it is expected.
Previously it was inside anonymous namespace.
I don't think it is a good coding practice to make a unnecessary namespace
and name it as "anonymous".

And previously, the function estIterOffset() was declared inline. But by
looking this function you can say compiler will not
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
inline based on certain conditions.

>
>
> >+{
> >+    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");
>
> Why remove self verify condition?
>
> 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.
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 ?
And yes, if you want your self verify condition checks, i can restore in
next patch.

>
>
> >             uint32_t edgeType = signRight + signLeft + 2;
> >             signLeft = -signRight;
> >
>
>
>
> _______________________________________________
> x265-devel mailing list
> x265-devel at videolan.org
> https://mailman.videolan.org/listinfo/x265-devel
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mailman.videolan.org/pipermail/x265-devel/attachments/20160217/2e362d4d/attachment-0001.html>