[x265] [PATCH] primitives :added C primitives to compute SSIM

[Steve Borho]

On Tue, Oct 1, 2013 at 7:07 AM, Aarthi Thirumalai <
aarthi at multicorewareinc.com> wrote:

> # HG changeset patch
> # User Aarthi Thirumalai
> # Date 1380629233 -19800
> #      Tue Oct 01 17:37:13 2013 +0530
> # Node ID bc38d51440330519f411e46d34cf176e7b728069
> # Parent  fc225ea42650867a229d15f41e1f2a495629bb7d
> primitives :added C primitives to compute SSIM
>

nit: space goes after the colon (primitives: )

put a blank line between the summary line and the rest of the commit message


> ssim_4x4x2_core_hb( for high bit depth enabled) , ssim_4x4x2_core_lb(for
> high bit depth disabled)
>

This looks much better, but I think it still needs work


> diff -r fc225ea42650 -r bc38d5144033 source/common/pixel.cpp
> --- a/source/common/pixel.cpp   Tue Oct 01 17:28:19 2013 +0530
> +++ b/source/common/pixel.cpp   Tue Oct 01 17:37:13 2013 +0530
> @@ -636,6 +636,69 @@
>      }
>  }
>
> + /* structural similarity metric */
>

extra space before the comment


> +template<class T1>
> +void ssim_4x4x2_core(const pixel *pix1, intptr_t stride1, const pixel
> *pix2, intptr_t stride2, T1 sums[2][4])
> +{
> +    for (int z = 0; z < 2; z++)
> +    {
> +        T1 s1 = 0, s2 = 0, ss = 0, s12 = 0;
> +        for (int y = 0; y < 4; y++)
> +            for (int x = 0; x < 4; x++)
> +            {
> +                int a = pix1[x + y * stride1];
> +                int b = pix2[x + y * stride2];
> +                s1 += a;
> +                s2 += b;
> +                ss += a * a;
> +                ss += b * b;
> +                s12 += a * b;
> +            }
> +        sums[z][0] = s1;
> +        sums[z][1] = s2;
> +        sums[z][2] = ss;
> +        sums[z][3] = s12;
> +        pix1 += 4;
> +        pix2 += 4;
> +    }
> +}
> +
> +template<class T1>
> +float ssim_end_4(T1 sum0[5][4], T1 sum1[5][4], int width)
> +{
> +    float ssim = 0.0;
> +    for (int i = 0; i < width; i++)
> +        ssim += ssim_end_1(sum0[i][0] + sum0[i+1][0] + sum1[i][0] +
> sum1[i+1][0],
> +                          sum0[i][1] + sum0[i+1][1] + sum1[i][1] +
> sum1[i+1][1],
> +                          sum0[i][2] + sum0[i+1][2] + sum1[i][2] +
> sum1[i+1][2],
> +                          sum0[i][3] + sum0[i+1][3] + sum1[i][3] +
> sum1[i+1][3]);
> +    return ssim;
> +}
> +
> +template<class T1>
> +float ssim_end_1(T1 s1, T1 s2, T1 ss, T1 s12)
> +{
> +    static const uint16_t pixelMax = (1 << X265_DEPTH)-1;
> +#if HIGH_BIT_DEPTH
> +    static const float ssim_c1 = .01 * .01 * pixelMax * pixelMax * 64;
> +    static const float ssim_c2 = .03 * .03 * pixelMax * pixelMax * 64 *
> 63;
> +#else
> +    static const int ssim_c1 = (int)(.01 *.01 * pixelMax * pixelMax * 64
> + .5);
> +    static const int ssim_c2 = (int)(.03 * .03 * pixelMax * pixelMax * 64
> * 63 + .5);
> +#endif
>

I think it would be cleaner to simply write two versions of this function;
even with HIGH_BIT_DEPTH you don't want to do unnecessary float-to-int
conversions if they're not needed.  Even high bit depth builds might need
to use the int version.


> +    T1 fs1 = s1;
> +    T1 fs2 = s2;
> +    T1 fss = ss;
> +    T1 fs12 = s12;
> +    T1 vars = fss * 64 - fs1 * fs1 - fs2 * fs2;
> +    T1 covar = fs12 * 64 - fs1 * fs2;
> +    return (float)(2 * fs1 * fs2 + ssim_c1) * (float)(2 * covar + ssim_c2)
> +         / ((float)(fs1 * fs1 + fs2 * fs2 + ssim_c1) * (float)(vars +
> ssim_c2));
> +#undef type
>

this undef is unnecessary


> +}
> +
> +
> +
>

unnecessary blank lines


>  }  // end anonymous namespace
>
>  namespace x265 {
> @@ -851,5 +914,10 @@
>      p.scale1D_128to64 = scale1D_128to64;
>      p.scale2D_64to32 = scale2D_64to32;
>      p.frame_init_lowres_core = frame_init_lowres_core;
> +
> +    p.ssim_4x4x2_core_hb = ssim_4x4x2_core<float>;
> +    p.ssim_4x4x2_core_lb = ssim_4x4x2_core<int>;
> +    p.ssim_end4_hb     = ssim_end_4<float>;
> +    p.ssim_end4_lb     = ssim_end_4<int>;
>  }
>  }
> diff -r fc225ea42650 -r bc38d5144033 source/common/primitives.h
> --- a/source/common/primitives.h        Tue Oct 01 17:28:19 2013 +0530
> +++ b/source/common/primitives.h        Tue Oct 01 17:37:13 2013 +0530
> @@ -233,6 +233,10 @@
>  typedef void (*scale_t)(pixel *dst, pixel *src, intptr_t stride);
>  typedef void (*downscale_t)(pixel *src0, pixel *dstf, pixel *dsth, pixel
> *dstv, pixel *dstc,
>                              intptr_t src_stride, intptr_t dst_stride, int
> width, int height);
> +typedef void (*ssim_4x4x2_core_lb_t)(const pixel *pix1, intptr_t stride1,
> const pixel *pix2, intptr_t stride2, int sums[2][4]);
> +typedef void (*ssim_4x4x2_core_hb_t)(const pixel *pix1, intptr_t stride1,
> const pixel *pix2, intptr_t stride2, float sums[2][4]);
> +typedef float (*ssim_end4_lb_t)(int sum0[5][4], int sum1[5][4], int
> width);
> +typedef float (*ssim_end4_hb_t)(float sum0[5][4], float sum1[5][4], int
> width);
>
>  /* Define a structure containing function pointers to optimized encoder
>   * primitives.  Each pointer can reference either an assembly routine,
> @@ -297,6 +301,11 @@
>      scale_t         scale1D_128to64;
>      scale_t         scale2D_64to32;
>      downscale_t     frame_init_lowres_core;
> +
> +    ssim_4x4x2_core_lb_t   ssim_4x4x2_core_lb;
> +    ssim_4x4x2_core_hb_t   ssim_4x4x2_core_hb;
> +    ssim_end4_lb_t         ssim_end4_lb;
> +    ssim_end4_hb_t         ssim_end4_hb;
>  };
>

lb and hb are new acronyms, we don't use those anywhere else.  these would
be better as:

+    ssim_4x4x2_core_int_t   ssim_4x4x2_core_int;
+    ssim_4x4x2_core_float_t   ssim_4x4x2_core_float;
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mailman.videolan.org/pipermail/x265-devel/attachments/20131001/a82f96b7/attachment.html>