Complete (but inefficient) implementation of the image retargetting method.

skia/ext/convolver.h

Index: skia/ext/convolver.h
diff --git a/skia/ext/convolver.h b/skia/ext/convolver.h
index 14974e52938690f340fb67a2f157ab035ff76c67..f4501b158bfb917db46957c378f35d6adef98cb0 100644
--- a/skia/ext/convolver.h
+++ b/skia/ext/convolver.h
@@ -10,6 +10,7 @@
 
 #include "base/basictypes.h"
 #include "base/cpu.h"
+#include "third_party/skia/include/core/SkSize.h"

[Stephen White, 2013/04/10 15:20:31]

I think you could forward-declare SkISize instead of #including here.

[motek., 2013/04/11 16:18:38]

SkISize is declared:
typedef struct SkTSize<int> SkISize

The compiler will want me to forward declare the actual type first, right? I
thought it would way too much detail for a forward declaration. 

[Stephen White, 2013/04/11 17:16:38]

Ahh, you're right.  I'd forgotten it was a typedef.

 #include "third_party/skia/include/core/SkTypes.h"
 
 #if defined(ARCH_CPU_X86_FAMILY)
@@ -107,6 +108,25 @@ class ConvolutionFilter1D {
     return &filter_values_[filter.data_location];
   }
 
+  // Retrieves the filter for the offset 0, presumed to be the one and only.
+  // The offset and length of the filter values are put into the corresponding
+  // out arguments (see AddFilter). Note that |filter_legth| and
+  // |specified_filter_size| may be different if leading/trailing zeros of the

[Stephen White, 2013/04/10 15:20:31]

Maybe this comment should be on the declaration for filter_size/length
themselves?

[motek., 2013/04/11 16:18:38]

Added it there.

+  // original floating point form were clipped.
+  // There will be |filter_length| values in the return array.

[reed1, 2013/04/09 15:43:18]

Do you want to comment that this can return NULL (and why)?

[motek., 2013/04/11 16:18:38]

Done.

+  inline const Fixed* GetSingleFilter(int* specified_filter_size,

[Stephen White, 2013/04/10 15:20:31]

Nit:  prefer out-of-line here unless performance is critical (I realize the
above is inline too; should probably be checked.)

[motek., 2013/04/11 16:18:38]

The original is processed in a loop, so I assume this is intentional. Moved this
one out-of-line.

+                                      int* filter_offset,
+                                      int* filter_length) const {
+    const FilterInstance& filter = filters_[0];
+    *filter_offset = filter.offset;
+    *filter_length = filter.length;
+    *specified_filter_size = filter.filter_size;
+    if (filter.length == 0) {
+      return NULL;
+    }
+    return &filter_values_[filter.data_location];
+  }
+
 
   inline void PaddingForSIMD(int padding_count) {
     // Padding |padding_count| of more dummy coefficients after the coefficients
@@ -128,6 +148,9 @@ class ConvolutionFilter1D {
 
     // Number of values in this filter instance.
     int length;
+
+    // Given filter size.
+    int filter_size;

[Stephen White, 2013/04/10 15:20:31]

A bit confusing.. maybe filter_size -> length, and length -> trimmed_length?

[motek., 2013/04/11 16:18:38]

Done.

   };
 
   // Stores the information for each filter added to this class.
@@ -168,6 +191,39 @@ SK_API void BGRAConvolve2D(const unsigned char* source_data,
                            int output_byte_row_stride,
                            unsigned char* output,
                            bool use_sse2);
+
+// Does a 1D convolution of the given source image along the X dimension on
+// a single channel of the bitmap.
+//
+// The function uses the same convolution kernel for each pixel. That kernel
+// must be added to |filter| at offset 0. This is a most straightforward
+// implementation of convolution, intended chiefly for development purposes.
+SK_API void SingleChannelConvolve1D_X(const unsigned char* source_data,

[Stephen White, 2013/04/10 15:20:31]

Style guide says no underscores in function names, although I can see why you
did it.  Perhaps SingleChannelConvolveX1D/Y1D?

Or you could also use a single function for both by passing a pixel stride as
well as a row stride (could be a template param if there's a perf impact).

[motek., 2013/04/11 16:18:38]

Renamed as suggested. 

As for the other suggestion: templatizing strides could indeed have influence
over performance, but I am not sure how much exactly. I do intend to make a
performance-focused pass over this, I'll change at that time if this turns out
to make any difference. Would that be OK with you?

+                                      int source_byte_row_stride,
+                                      int input_channel_index,
+                                      int input_channel_count,
+                                      const ConvolutionFilter1D& filter,
+                                      const SkISize& image_size,
+                                      unsigned char* output,
+                                      int output_byte_row_stride,
+                                      int output_channel_index,
+                                      int output_channel_count,
+                                      bool absolute_values);
+
+// Does a 1D convolution of the given source image along the Y dimension on
+// a single channel of the bitmap.
+SK_API void SingleChannelConvolve1D_Y(const unsigned char* source_data,
+                                      int source_byte_row_stride,
+                                      int input_channel_index,
+                                      int input_channel_count,
+                                      const ConvolutionFilter1D& filter,
+                                      const SkISize& image_size,
+                                      unsigned char* output,
+                                      int output_byte_row_stride,
+                                      int output_channel_index,
+                                      int output_channel_count,
+                                      bool absolute_values);
+
 }  // namespace skia
 
 #endif  // SKIA_EXT_CONVOLVER_H_