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

skia/ext/convolver.h

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #ifndef SKIA_EXT_CONVOLVER_H_
 #define SKIA_EXT_CONVOLVER_H_
 
 #include <cmath>
 #include <vector>
 
 #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)
 #if defined(__x86_64__) || defined(_M_X64) || defined(__SSE2__) || _M_IX86_FP==2
 // This is where we had compiler support for SSE2 instructions.
 // FIXME: Known buggy, so disabling for M22.
 // #define SIMD_SSE2 1
 #endif
 #endif
 
@@ skipping 77 matching lines @@
                                      int* filter_length) const {
     const FilterInstance& filter = filters_[value_offset];
     *filter_offset = filter.offset;
     *filter_length = filter.length;
     if (filter.length == 0) {
       return NULL;
     }
     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
     // of last filter to prevent SIMD instructions which load 8 or 16 bytes
     // together to access invalid memory areas. We are not trying to align the
     // coefficients right now due to the opaqueness of <vector> implementation.
     // This has to be done after all |AddFilter| calls.
     for (int i = 0; i < padding_count; ++i)
       filter_values_.push_back(static_cast<Fixed>(0));
   }
 
  private:
   struct FilterInstance {
     // Offset within filter_values for this instance of the filter.
     int data_location;
 
     // Distance from the left of the filter to the center. IN PIXELS
     int offset;
 
     // 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.
   std::vector<FilterInstance> filters_;
 
   // We store all the filter values in this flat list, indexed by
   // |FilterInstance.data_location| to avoid the mallocs required for storing
   // each one separately.
   std::vector<Fixed> filter_values_;
 
@@ skipping 20 matching lines @@
 // The layout in memory is assumed to be 4-bytes per pixel in B-G-R-A order
 // (this is ARGB when loaded into 32-bit words on a little-endian machine).
 SK_API void BGRAConvolve2D(const unsigned char* source_data,
                            int source_byte_row_stride,
                            bool source_has_alpha,
                            const ConvolutionFilter1D& xfilter,
                            const ConvolutionFilter1D& yfilter,
                            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_