ARM Skia NEON patches - 16/17 - Blitmask

src/opts/SkBlitMask_opts_arm.cpp

Index: src/opts/SkBlitMask_opts_arm.cpp
diff --git a/src/opts/SkBlitMask_opts_arm.cpp b/src/opts/SkBlitMask_opts_arm.cpp
index 0ad091938712e18b58a6b6917f1abb30536e4a5d..1e6fe0d07d7ee9fe2fcc604f28e5459a682af6c9 100644
--- a/src/opts/SkBlitMask_opts_arm.cpp
+++ b/src/opts/SkBlitMask_opts_arm.cpp
@@ -1,14 +1,48 @@
 
+#include "SkColor.h"
+#include "SkColorPriv.h"
 #include "SkBlitMask.h"
+#include "SkUtilsArm.h"
+
+
+extern SkBlitMask::ColorProc D32_A8_Factory(SkColor color);

[mtklein, 2013/11/11 17:17:04]

Can we do these using the already exported SkBlitMask::{ColorFactory,
PlatformColorProcs}?  Making this guy non-static seems like we're just slicing
right through any abstraction barrier we've tried to set up here.

[kevin.petit.not.used.account, 2013/11/11 18:10:34]

Yes, you're right. Done.

+extern SkBlitMask::ColorProc D32_A8_Factory_neon(SkColor color);
+
+extern void SkBlitLCD16OpaqueRow_neon(SkPMColor dst[], const uint16_t src[],

[mtklein, 2013/11/11 17:17:04]

Add a header for these two?

[kevin.petit.not.used.account, 2013/11/11 18:10:34]

Done.

+                                      SkColor color, int width,
+                                      SkPMColor opaqueDst);
+
+extern void SkBlitLCD16Row_neon(SkPMColor dst[], const uint16_t src[],
+                                SkColor color, int width, SkPMColor);
 
 SkBlitMask::ColorProc SkBlitMask::PlatformColorProcs(SkBitmap::Config dstConfig,
                                                      SkMask::Format maskFormat,
                                                      SkColor color) {

[reed1, 2013/11/11 17:32:55]

How about we just add the following as the first check in this function:

if (!sk_cpu_arm_has_neon()) {
    return NULL;
}

Then the rest of this code can just be switch statements etc, assuming neon is
valid. This will free us in several ways:

1. no need to import other people's factories (e.g. D32_A8_Factor)
2. no need to follow any predetermined naming convention (e.g. _neon)

The design goal for runtime PlatformProcs is to give maximal freedom to
platform-specific optimizations, while giving maximal isolation between that
logic and the core.

[kevin.petit.not.used.account, 2013/11/11 18:10:34]

Thanks for the tip. Done.

+    switch (dstConfig) {

[mtklein, 2013/11/11 17:17:04]

Given the limited support we've got here, something like this seems
significantly easier to follow:

if (SkBitmap::kARGB_8888_Config == dst_config &&
    SkMask::kA8_Format == maskFormat) {
   return ...;
}
return NULL;

[kevin.petit.not.used.account, 2013/11/11 18:10:34]

Done.

+        case SkBitmap::kARGB_8888_Config:
+            switch (maskFormat) {
+                case SkMask::kA8_Format:
+                    return SK_ARM_NEON_WRAP(D32_A8_Factory)(color);
+// We don't need to handle the following case, the default LCD16
+// will call us through SkBlitMask::PlatformBlitRowProcs16()
+//                case SkMask::kLCD16_Format:
+//                    return NULL;
+                default:
+                    break;
+            }
+            break;
+        default:
+            break;
+    }
     return NULL;
 }
 
 SkBlitMask::BlitLCD16RowProc SkBlitMask::PlatformBlitRowProcs16(bool isOpaque) {
-    return NULL;
+    if (isOpaque) {
+        return SK_ARM_NEON_WRAP(SkBlitLCD16OpaqueRow);
+    } else {
+        return SK_ARM_NEON_WRAP(SkBlitLCD16Row);
+    }
 }
 
 SkBlitMask::RowProc SkBlitMask::PlatformRowProcs(SkBitmap::Config dstConfig,