NEON-ize RGBA to RGB code

third_party/WebKit/Source/platform/image-encoders/JPEGImageEncoder.cpp

 /*
  * Copyright (c) 2010, Google Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions are
  * met:
  *
  *     * Redistributions of source code must retain the above copyright
  * notice, this list of conditions and the following disclaimer.
  *     * Redistributions in binary form must reproduce the above
@@ skipping 15 matching lines @@
  * THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
  * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
  * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
  */
 
 #include "platform/image-encoders/JPEGImageEncoder.h"
 
 #include "SkColorPriv.h"
 #include "platform/geometry/IntSize.h"
 #include "platform/graphics/ImageBuffer.h"
+#include "platform/image-encoders/RGBAtoRGB.h"
 #include "wtf/CurrentTime.h"
 #include "wtf/PtrUtil.h"
 #include <memory>
 
 extern "C" {
 #include <setjmp.h>
 #include <stdio.h>  // jpeglib.h needs stdio.h FILE
 #include "jpeglib.h"
 }
 
 namespace blink {
 
+void RGBAtoRGBScalar(const unsigned char* pixels,

[msarett1, 2016/12/16 13:07:54]

libjpeg-turbo actually supports RGBA input (in addition to RGB).  So RGBA to
RGBA would actually be fine here.  And would probably be faster (in the scalar
code) in the case where we have opaque pixels.

I don't think it would affect the NEON code much either way.

Just a comment, no need to think about this in this change.

[cavalcantii1, 2016/12/17 02:35:33]

Acknowledged.

+                     unsigned pixelCount,
+                     unsigned char* output) {
+  // Per <canvas> spec, composite the input image pixels source-over on black.
+  for (; pixelCount-- > 0; pixels += 4) {
+    unsigned char alpha = pixels[3];
+    if (alpha != 255) {
+      *output++ = SkMulDiv255Round(pixels[0], alpha);
+      *output++ = SkMulDiv255Round(pixels[1], alpha);
+      *output++ = SkMulDiv255Round(pixels[2], alpha);
+    } else {
+      *output++ = pixels[0];
+      *output++ = pixels[1];
+      *output++ = pixels[2];
+    }
+  }
+}
+
+// TODO(cavalcantii): use regular macro, see https://crbug.com/673067.
+#ifdef __ARM_NEON__
+void RGBAtoRGBNeon(const unsigned char* input,
+                   const unsigned pixelCount,
+                   unsigned char* output) {
+  const unsigned pixelsPerLoad = 16;
+  const unsigned rgbaStep = pixelsPerLoad * 4, rgbStep = pixelsPerLoad * 3;
+  // Input registers.
+  uint8x16x4_t rgba;
+  // Output registers.
+  uint8x16x3_t rgb;
+  // Intermediate registers.
+  uint16x8_t low, high;
+  uint8x16_t result;
+  unsigned counter;
+
+  for (counter = 0; counter + pixelsPerLoad <= pixelCount;
+       counter += pixelsPerLoad) {
+    // Reads 16 pixels at once, each color channel in a different
+    // 128 bits register.
+    rgba = vld4q_u8(input);
+
+    // Extracts the low/high part of the 128 bits, multiplying by the
+    // respective alpha channel.
+    low = vmull_u8(vget_low_u8(rgba.val[0]), vget_low_u8(rgba.val[3]));
+    high = vmull_u8(vget_high_u8(rgba.val[0]), vget_high_u8(rgba.val[3]));
+
+    // Original Skia formula is: (x + (x >> 8)) >> 8, where x = a*b + 128.
+    // This shifts and accumulates following by rounding in a single
+    // instruction.
+    low = vrsraq_n_u16(low, low, 8);

[msarett1, 2016/12/16 13:07:54]

Skia has a NEON implementation of "mul and rounded divide by 255".  Probably
worth taking a look, it takes advantage of vraddhn_u16().
https://cs.chromium.org/chromium/src/third_party/skia/src/opts/SkSwizzler_opt...

[cavalcantii1, 2016/12/17 02:35:33]

I searched for SkSwizzler_opts.h and it seems it is used only internally in
Skia?

I also noticed that scale() and div255_round() are both marked as 'static' but
not inline. I wonder what happens at the call sites.

Anyway, I got curious about the difference between using vrsra() X vraddhn().
Notice that in both cases you have to perform a shift, so the number of
instructions is the same.

What I think is troubling is that vraddhn requires a temporary register to keep
the result of shifting x (while vrsra doesn't need that). That being said, ARM
has quite a few registers.
:-)

I decided to have a look on how the assembly would look like for each version.

a) Using vraddhn: https://godbolt.org/g/Ji8cmW

b) Using vrsra: https://godbolt.org/g/XHe9ky

And after next I executed the test4KPerf 20x times (Nexus 6) for each version
and collected this data:
https://gist.github.com/Adenilson/ff2d677ce4cb16662e406fbe497053c9

It seems that using vraddhn is about 6% faster (mraddh/mrsra = 0.9483218), but
not in a conclusive way (reported F was 0.47).

Since both implementations pass the test suite, I'm ok with going with
vraddhn. A question: what would be the right way of adding the Skia
header in JPEGImageEncoder?

+    high = vrsraq_n_u16(high, high, 8);
+
+    // And now to the last shift and combining the vector.
+    result = vcombine_u8(vqrshrn_n_u16(low, 8), vqrshrn_n_u16(high, 8));
+
+    // Write back the Red channel to the first 128 bits register.
+    rgb.val[0] = result;
+
+    // Now the Green channel (don't trust the compiler to unroll the loop).

[msarett1, 2016/12/16 13:07:54]

What about using an inline helper function?  Ex: mul_div_255_round()

[cavalcantii1, 2016/12/17 02:35:33]

I tested moving the pixel manipulation code to a lambda and the compiler was
able to inline it just fine (as long using at least -O1).

+    low = vmull_u8(vget_low_u8(rgba.val[1]), vget_low_u8(rgba.val[3]));
+    high = vmull_u8(vget_high_u8(rgba.val[1]), vget_high_u8(rgba.val[3]));
+    low = vrsraq_n_u16(low, low, 8);
+    high = vrsraq_n_u16(high, high, 8);
+    result = vcombine_u8(vqrshrn_n_u16(low, 8), vqrshrn_n_u16(high, 8));
+    rgb.val[1] = result;
+
+    // Finally the Blue channel.
+    low = vmull_u8(vget_low_u8(rgba.val[2]), vget_low_u8(rgba.val[3]));
+    high = vmull_u8(vget_high_u8(rgba.val[2]), vget_high_u8(rgba.val[3]));
+    low = vrsraq_n_u16(low, low, 8);
+    high = vrsraq_n_u16(high, high, 8);
+    result = vcombine_u8(vqrshrn_n_u16(low, 8), vqrshrn_n_u16(high, 8));
+    rgb.val[2] = result;
+
+    // Write back (interleaved) results to output.
+    vst3q_u8(output, rgb);
+
+    // Advance to next elements (could be avoided loading register with
+    // increment after i.e. "vld4 {vector}, [r1]!").
+    input += rgbaStep;
+    output += rgbStep;
+  }
+
+  // Handle the tail elements.
+  unsigned remaining = pixelCount;
+  remaining -= counter;
+  if (remaining != 0) {
+    RGBAtoRGBScalar(input, remaining, output);
+  }
+}
+#endif
+
 struct JPEGOutputBuffer : public jpeg_destination_mgr {
   DISALLOW_NEW();
   Vector<unsigned char>* output;
   Vector<unsigned char> buffer;
 };
 
 class JPEGImageEncoderStateImpl final : public JPEGImageEncoderState {
  public:
   JPEGImageEncoderStateImpl() {}
   ~JPEGImageEncoderStateImpl() override {
@@ skipping 30 matching lines @@
   JPEGOutputBuffer* out = static_cast<JPEGOutputBuffer*>(cinfo->dest);
   const size_t size = out->buffer.size() - out->free_in_buffer;
   out->output->append(out->buffer.data(), size);
 }
 
 static void handleError(j_common_ptr common) {
   jmp_buf* jumpBufferPtr = static_cast<jmp_buf*>(common->client_data);
   longjmp(*jumpBufferPtr, -1);
 }
 
-static void RGBAtoRGB(const unsigned char* pixels,
-                      unsigned pixelCount,
-                      unsigned char* output) {
-  // Per <canvas> spec, composite the input image pixels source-over on black.
-
-  for (; pixelCount-- > 0; pixels += 4) {
-    unsigned char alpha = pixels[3];
-    if (alpha != 255) {
-      *output++ = SkMulDiv255Round(pixels[0], alpha);
-      *output++ = SkMulDiv255Round(pixels[1], alpha);
-      *output++ = SkMulDiv255Round(pixels[2], alpha);
-    } else {
-      *output++ = pixels[0];
-      *output++ = pixels[1];
-      *output++ = pixels[2];
-    }
-  }
-}
-
 static void disableSubsamplingForHighQuality(jpeg_compress_struct* cinfo,
                                              int quality) {
   if (quality < 100)
     return;
 
   for (int i = 0; i < MAX_COMPONENTS; ++i) {
     cinfo->comp_info[i].h_samp_factor = 1;
     cinfo->comp_info[i].v_samp_factor = 1;
   }
 }
@@ skipping 123 matching lines @@
   std::unique_ptr<JPEGImageEncoderState> encoderState =
       JPEGImageEncoderState::create(imageData.size(), quality, output);
   if (!encoderState)
     return false;
 
   return JPEGImageEncoder::encodeWithPreInitializedState(
       std::move(encoderState), imageData.pixels());
 }
 
 }  // namespace blink