Fix bug in convolution functions (filter selection)

(In response to Issue 604:
 https://code.google.com/p/webm/issues/detail?id=604)

There were bugs in the convolution code for two cases:

1. Where the filter table was assumed to be aligned to a
   256 byte boundary. The offset of the pixel in the
   source buffer was computed incorrectly.

2. Where no such alignment assumption was made. An
   incorrect address for the filter table base was used.

To fix both problems, I now assume that the filter table is
256-byte aligned and modify the pixel offset calculation to
match.

A later patch should remove the restriction that the filter
table is aligned to a 256-byte boundary.

There was also a bug in the ConvolveTest unit test
(convolve_test.cc).

(Bug & initial fix suggestion submitted by Tero Rintaluoma
and Sami Pietilä).

Change-Id: I71985551e62846e55e40de9e7e3959d4805baa82

1 files changed, 52 insertions, 75 deletions

diff --git a/vp9/common/vp9_convolve.c b/vp9/common/vp9_convolve.c
index 4e79bbc1b..1d9684992 100644
--- a/vp9/common/vp9_convolve.c
+++ b/vp9/common/vp9_convolve.c
@@ -14,63 +14,45 @@
 #include "./vpx_config.h"
 #include "./vp9_rtcd.h"
 #include "vp9/common/vp9_common.h"
+#include "vp9/common/vp9_filter.h"
 #include "vpx/vpx_integer.h"
 #include "vpx_ports/mem.h"
 
-/* Assume a bank of 16 filters to choose from. There are two implementations
- * for filter wrapping behavior, since we want to be able to pick which filter
- * to start with. We could either:
- *
- * 1) make filter_ a pointer to the base of the filter array, and then add an
- *    additional offset parameter, to choose the starting filter.
- * 2) use a pointer to 2 periods worth of filters, so that even if the original
- *    phase offset is at 15/16, we'll have valid data to read. The filter
- *    tables become [32][8], and the second half is duplicated.
- * 3) fix the alignment of the filter tables, so that we know the 0/16 is
- *    always 256 byte aligned.
- *
- * Implementations 2 and 3 are likely preferable, as they avoid an extra 2
- * parameters, and switching between them is trivial, with the
- * ALIGN_FILTERS_256 macro, below.
- */
- #define ALIGN_FILTERS_256 1
-
 static void convolve_horiz_c(const uint8_t *src, ptrdiff_t src_stride,
                              uint8_t *dst, ptrdiff_t dst_stride,
                              const int16_t *filter_x0, int x_step_q4,
                              const int16_t *filter_y, int y_step_q4,
                              int w, int h, int taps) {
   int x, y, k;
-  const int16_t *filter_x_base = filter_x0;
 
-#if ALIGN_FILTERS_256
-  filter_x_base = (const int16_t *)(((intptr_t)filter_x0) & ~(intptr_t)0xff);
-#endif
+  /* NOTE: This assumes that the filter table is 256-byte aligned. */
+  /* TODO(agrange) Modify to make independent of table alignment. */
+  const int16_t *const filter_x_base =
+      (const int16_t *)(((intptr_t)filter_x0) & ~(intptr_t)0xff);
 
   /* Adjust base pointer address for this source line */
   src -= taps / 2 - 1;
 
   for (y = 0; y < h; ++y) {
-    /* Pointer to filter to use */
-    const int16_t *filter_x = filter_x0;
-
     /* Initial phase offset */
-    int x0_q4 = (filter_x - filter_x_base) / taps;
-    int x_q4 = x0_q4;
+    int x_q4 = (filter_x0 - filter_x_base) / taps;
 
     for (x = 0; x < w; ++x) {
       /* Per-pixel src offset */
-      int src_x = (x_q4 - x0_q4) >> 4;
+      const int src_x = x_q4 >> SUBPEL_BITS;
       int sum = 0;
 
+      /* Pointer to filter to use */
+      const int16_t *const filter_x = filter_x_base +
+          (x_q4 & SUBPEL_MASK) * taps;
+
       for (k = 0; k < taps; ++k)
         sum += src[src_x + k] * filter_x[k];
 
       dst[x] = clip_pixel(ROUND_POWER_OF_TWO(sum, VP9_FILTER_BITS));
 
-      /* Adjust source and filter to use for the next pixel */
+      /* Move to the next source pixel */
       x_q4 += x_step_q4;
-      filter_x = filter_x_base + (x_q4 & 0xf) * taps;
     }
     src += src_stride;
     dst += dst_stride;
@@ -83,37 +65,36 @@ static void convolve_avg_horiz_c(const uint8_t *src, ptrdiff_t src_stride,
                                  const int16_t *filter_y, int y_step_q4,
                                  int w, int h, int taps) {
   int x, y, k;
-  const int16_t *filter_x_base = filter_x0;
 
-#if ALIGN_FILTERS_256
-  filter_x_base = (const int16_t *)(((intptr_t)filter_x0) & ~(intptr_t)0xff);
-#endif
+  /* NOTE: This assumes that the filter table is 256-byte aligned. */
+  /* TODO(agrange) Modify to make independent of table alignment. */
+  const int16_t *const filter_x_base =
+      (const int16_t *)(((intptr_t)filter_x0) & ~(intptr_t)0xff);
 
   /* Adjust base pointer address for this source line */
   src -= taps / 2 - 1;
 
   for (y = 0; y < h; ++y) {
-    /* Pointer to filter to use */
-    const int16_t *filter_x = filter_x0;
-
     /* Initial phase offset */
-    int x0_q4 = (filter_x - filter_x_base) / taps;
-    int x_q4 = x0_q4;
+    int x_q4 = (filter_x0 - filter_x_base) / taps;
 
     for (x = 0; x < w; ++x) {
       /* Per-pixel src offset */
-      int src_x = (x_q4 - x0_q4) >> 4;
+      const int src_x = x_q4 >> SUBPEL_BITS;
       int sum = 0;
 
+      /* Pointer to filter to use */
+      const int16_t *const filter_x = filter_x_base +
+          (x_q4 & SUBPEL_MASK) * taps;
+
       for (k = 0; k < taps; ++k)
         sum += src[src_x + k] * filter_x[k];
 
       dst[x] = ROUND_POWER_OF_TWO(dst[x] +
                    clip_pixel(ROUND_POWER_OF_TWO(sum, VP9_FILTER_BITS)), 1);
 
-      /* Adjust source and filter to use for the next pixel */
+      /* Move to the next source pixel */
       x_q4 += x_step_q4;
-      filter_x = filter_x_base + (x_q4 & 0xf) * taps;
     }
     src += src_stride;
     dst += dst_stride;
@@ -127,36 +108,35 @@ static void convolve_vert_c(const uint8_t *src, ptrdiff_t src_stride,
                             int w, int h, int taps) {
   int x, y, k;
 
-  const int16_t *filter_y_base = filter_y0;
-
-#if ALIGN_FILTERS_256
-  filter_y_base = (const int16_t *)(((intptr_t)filter_y0) & ~(intptr_t)0xff);
-#endif
+  /* NOTE: This assumes that the filter table is 256-byte aligned. */
+  /* TODO(agrange) Modify to make independent of table alignment. */
+  const int16_t *const filter_y_base =
+      (const int16_t *)(((intptr_t)filter_y0) & ~(intptr_t)0xff);
 
   /* Adjust base pointer address for this source column */
   src -= src_stride * (taps / 2 - 1);
-  for (x = 0; x < w; ++x) {
-    /* Pointer to filter to use */
-    const int16_t *filter_y = filter_y0;
 
+  for (x = 0; x < w; ++x) {
     /* Initial phase offset */
-    int y0_q4 = (filter_y - filter_y_base) / taps;
-    int y_q4 = y0_q4;
+    int y_q4 = (filter_y0 - filter_y_base) / taps;
 
     for (y = 0; y < h; ++y) {
       /* Per-pixel src offset */
-      int src_y = (y_q4 - y0_q4) >> 4;
+      const int src_y = y_q4 >> SUBPEL_BITS;
       int sum = 0;
 
+      /* Pointer to filter to use */
+      const int16_t *const filter_y = filter_y_base +
+          (y_q4 & SUBPEL_MASK) * taps;
+
       for (k = 0; k < taps; ++k)
         sum += src[(src_y + k) * src_stride] * filter_y[k];
 
       dst[y * dst_stride] =
           clip_pixel(ROUND_POWER_OF_TWO(sum, VP9_FILTER_BITS));
 
-      /* Adjust source and filter to use for the next pixel */
+      /* Move to the next source pixel */
       y_q4 += y_step_q4;
-      filter_y = filter_y_base + (y_q4 & 0xf) * taps;
     }
     ++src;
     ++dst;
@@ -170,36 +150,35 @@ static void convolve_avg_vert_c(const uint8_t *src, ptrdiff_t src_stride,
                                 int w, int h, int taps) {
   int x, y, k;
 
-  const int16_t *filter_y_base = filter_y0;
-
-#if ALIGN_FILTERS_256
-  filter_y_base = (const int16_t *)(((intptr_t)filter_y0) & ~(intptr_t)0xff);
-#endif
+  /* NOTE: This assumes that the filter table is 256-byte aligned. */
+  /* TODO(agrange) Modify to make independent of table alignment. */
+  const int16_t *const filter_y_base =
+      (const int16_t *)(((intptr_t)filter_y0) & ~(intptr_t)0xff);
 
   /* Adjust base pointer address for this source column */
   src -= src_stride * (taps / 2 - 1);
-  for (x = 0; x < w; ++x) {
-    /* Pointer to filter to use */
-    const int16_t *filter_y = filter_y0;
 
+  for (x = 0; x < w; ++x) {
     /* Initial phase offset */
-    int y0_q4 = (filter_y - filter_y_base) / taps;
-    int y_q4 = y0_q4;
+    int y_q4 = (filter_y0 - filter_y_base) / taps;
 
     for (y = 0; y < h; ++y) {
       /* Per-pixel src offset */
-      int src_y = (y_q4 - y0_q4) >> 4;
+      const int src_y = y_q4 >> SUBPEL_BITS;
       int sum = 0;
 
+      /* Pointer to filter to use */
+      const int16_t *const filter_y = filter_y_base +
+          (y_q4 & SUBPEL_MASK) * taps;
+
       for (k = 0; k < taps; ++k)
         sum += src[(src_y + k) * src_stride] * filter_y[k];
 
       dst[y * dst_stride] = ROUND_POWER_OF_TWO(dst[y * dst_stride] +
            clip_pixel(ROUND_POWER_OF_TWO(sum, VP9_FILTER_BITS)), 1);
 
-      /* Adjust source and filter to use for the next pixel */
+      /* Move to the next source pixel */
       y_q4 += y_step_q4;
-      filter_y = filter_y_base + (y_q4 & 0xf) * taps;
     }
     ++src;
     ++dst;
@@ -227,13 +206,11 @@ static void convolve_c(const uint8_t *src, ptrdiff_t src_stride,
   if (intermediate_height < h)
     intermediate_height = h;
 
-  convolve_horiz_c(src - src_stride * (taps / 2 - 1), src_stride,
-                   temp, 64,
-                   filter_x, x_step_q4, filter_y, y_step_q4,
-                   w, intermediate_height, taps);
-  convolve_vert_c(temp + 64 * (taps / 2 - 1), 64, dst, dst_stride,
-                  filter_x, x_step_q4, filter_y, y_step_q4,
-                  w, h, taps);
+  convolve_horiz_c(src - src_stride * (taps / 2 - 1), src_stride, temp, 64,
+                   filter_x, x_step_q4, filter_y, y_step_q4, w,
+                   intermediate_height, taps);
+  convolve_vert_c(temp + 64 * (taps / 2 - 1), 64, dst, dst_stride, filter_x,
+                  x_step_q4, filter_y, y_step_q4, w, h, taps);
 }
 
 void vp9_convolve8_horiz_c(const uint8_t *src, ptrdiff_t src_stride,