We were numerically overflowing our 16bit coordinates that we communicate

between these two procs. The fixes was in two parts:

1. Just don't draw bitmaps larger than 64K-1 in width or height, since we
   can't represent those coordinates in our transport format (yet).
2. Perform an unsigned shift during the calculation, so we don't get
   sign-extension bleed when packing the two values (X,Y) into our 32bit
   slot.
Review URL: https://codereview.appspot.com/6173046

git-svn-id: http://skia.googlecode.com/svn/trunk@3836 2bbb7eff-a529-9590-31e7-b0007b416f81

include/core/SkShader.h

@@ -285,10 +285,15 @@ public:
     //  Factory methods for stock shaders
 
     /** Call this to create a new shader that will draw with the specified bitmap.
-        @param src  The bitmap to use inside the shader
+     *
-        @param tmx  The tiling mode to use when sampling the bitmap in the x-direction.
+     *  If the bitmap cannot be used (e.g. has no pixels, or its dimensions
-        @param tmy  The tiling mode to use when sampling the bitmap in the y-direction.
+     *  exceed implementation limits (currently at 64K - 1)) then SkEmptyShader
-        @return     Returns a new shader object. Note: this function never returns null.
+     *  may be returned.
+     *
+     *  @param src  The bitmap to use inside the shader
+     *  @param tmx  The tiling mode to use when sampling the bitmap in the x-direction.
+     *  @param tmy  The tiling mode to use when sampling the bitmap in the y-direction.
+     *  @return     Returns a new shader object. Note: this function never returns null.
     */
     static SkShader* CreateBitmapShader(const SkBitmap& src,
                                         TileMode tmx, TileMode tmy);

src/core/SkBitmapProcShader.cpp

@@ -271,12 +271,22 @@ static bool canUseColorShader(const SkBitmap& bm, SkColor* color) {
 
 #include "SkTemplatesPriv.h"
 
+static bool bitmapIsTooBig(const SkBitmap& bm) {
+    // SkBitmapProcShader stores bitmap coordinates in a 16bit buffer, as it
+    // communicates between its matrix-proc and its sampler-proc. Until we can
+    // widen that, we have to reject bitmaps that are larger.
+    //
+    const int maxSize = 65535;
+
+    return bm.width() > maxSize || bm.height() > maxSize;
+}
+
 SkShader* SkShader::CreateBitmapShader(const SkBitmap& src,
                                        TileMode tmx, TileMode tmy,
                                        void* storage, size_t storageSize) {
     SkShader* shader;
     SkColor color;
-    if (src.isNull()) {
+    if (src.isNull() || bitmapIsTooBig(src)) {
         SK_PLACEMENT_NEW(shader, SkEmptyShader, storage, storageSize);
     }
     else if (canUseColorShader(src, &color)) {

src/core/SkBitmapProcState_matrixProcs.cpp

@@ -9,6 +9,13 @@
 #include "SkShader.h"
 #include "SkUtils.h"
 
+// Helper to ensure that when we shift down, we do it w/o sign-extension
+// so the caller doesn't have to manually mask off the top 16 bits
+//
+static unsigned SK_USHIFT16(unsigned x) {
+    return x >> 16;
+}
+
 /*  returns 0...(n-1) given any x (positive or negative).
     
     As an example, if n (which is always positive) is 5...
@@ -32,8 +39,8 @@ void decal_nofilter_scale(uint32_t dst[], SkFixed fx, SkFixed dx, int count);
 void decal_filter_scale(uint32_t dst[], SkFixed fx, SkFixed dx, int count);
 
 #define MAKENAME(suffix)        ClampX_ClampY ## suffix
-#define TILEX_PROCF(fx, max)    SkClampMax((fx) >> 16, max)
+#define TILEX_PROCF(fx, max)    SkClampMax(SK_USHIFT16(fx), max)
-#define TILEY_PROCF(fy, max)    SkClampMax((fy) >> 16, max)
+#define TILEY_PROCF(fy, max)    SkClampMax(SK_USHIFT16(fy), max)
 #define TILEX_LOW_BITS(fx, max) (((fx) >> 12) & 0xF)
 #define TILEY_LOW_BITS(fy, max) (((fy) >> 12) & 0xF)
 #define CHECK_FOR_DECAL
@@ -44,8 +51,8 @@ void decal_filter_scale(uint32_t dst[], SkFixed fx, SkFixed dx, int count);
 #endif
 
 #define MAKENAME(suffix)        RepeatX_RepeatY ## suffix
-#define TILEX_PROCF(fx, max)    (((fx) & 0xFFFF) * ((max) + 1) >> 16)
+#define TILEX_PROCF(fx, max)    SK_USHIFT16(((fx) & 0xFFFF) * ((max) + 1))
-#define TILEY_PROCF(fy, max)    (((fy) & 0xFFFF) * ((max) + 1) >> 16)
+#define TILEY_PROCF(fy, max)    SK_USHIFT16(((fy) & 0xFFFF) * ((max) + 1))
 #define TILEX_LOW_BITS(fx, max) ((((fx) & 0xFFFF) * ((max) + 1) >> 12) & 0xF)
 #define TILEY_LOW_BITS(fy, max) ((((fy) & 0xFFFF) * ((max) + 1) >> 12) & 0xF)
 #if	defined(__ARM_HAVE_NEON)
@@ -63,8 +70,8 @@ void decal_filter_scale(uint32_t dst[], SkFixed fx, SkFixed dx, int count);
 #define PREAMBLE_PARAM_Y        , SkBitmapProcState::FixedTileProc tileProcY, SkBitmapProcState::FixedTileLowBitsProc tileLowBitsProcY
 #define PREAMBLE_ARG_X          , tileProcX, tileLowBitsProcX
 #define PREAMBLE_ARG_Y          , tileProcY, tileLowBitsProcY
-#define TILEX_PROCF(fx, max)    (tileProcX(fx) * ((max) + 1) >> 16)
+#define TILEX_PROCF(fx, max)    SK_USHIFT16(tileProcX(fx) * ((max) + 1))
-#define TILEY_PROCF(fy, max)    (tileProcY(fy) * ((max) + 1) >> 16)
+#define TILEY_PROCF(fy, max)    SK_USHIFT16(tileProcY(fy) * ((max) + 1))
 #define TILEX_LOW_BITS(fx, max) tileLowBitsProcX(fx, (max) + 1)
 #define TILEY_LOW_BITS(fy, max) tileLowBitsProcY(fy, (max) + 1)
 #include "SkBitmapProcState_matrix.h"

tests/DrawBitmapRectTest.cpp

@@ -8,6 +8,101 @@
 #include "Test.h"
 #include "SkBitmap.h"
 #include "SkCanvas.h"
+#include "SkShader.h"
+
+static void assert_ifDrawnTo(skiatest::Reporter* reporter,
+                             const SkBitmap& bm, bool shouldBeDrawn) {
+    for (int y = 0; y < bm.height(); ++y) {
+        for (int x = 0; x < bm.width(); ++x) {
+            if (shouldBeDrawn) {
+                if (0 == *bm.getAddr32(x, y)) {
+                    REPORTER_ASSERT(reporter, false);
+                    return;
+                }
+            } else {
+                // should not be drawn
+                if (*bm.getAddr32(x, y)) {
+                    REPORTER_ASSERT(reporter, false);
+                    return;
+                }
+            }
+        }
+    }
+}
+
+static void test_wacky_bitmapshader(skiatest::Reporter* reporter,
+                                    int width, int height, bool shouldBeDrawn) {
+    SkBitmap dev;
+    dev.setConfig(SkBitmap::kARGB_8888_Config, 0x56F, 0x4f6);
+    dev.allocPixels();
+    dev.eraseColor(0);  // necessary, so we know if we draw to it
+    
+    SkMatrix matrix;
+    
+    SkCanvas c(dev);
+    matrix.setAll(-119.34097, -43.436558, 93489.945,
+                  43.436558, -119.34097, 123.98426,
+                  0, 0, 1);
+    c.concat(matrix);
+    
+    SkBitmap bm;
+    bm.setConfig(SkBitmap::kARGB_8888_Config, width, height);
+    bm.allocPixels();
+    bm.eraseColor(SK_ColorRED);
+    
+    SkShader* s = SkShader::CreateBitmapShader(bm, SkShader::kRepeat_TileMode,
+                                               SkShader::kRepeat_TileMode);
+    matrix.setAll(0.0078740157, 0, 249,
+                  0, 0.0078740157, 239,
+                  0, 0, 1);
+    s->setLocalMatrix(matrix);
+    
+    SkPaint paint;
+    paint.setShader(s)->unref();
+    
+    SkRect r = SkRect::MakeXYWH(681, 239, 695, 253);
+    c.drawRect(r, paint);
+    
+    assert_ifDrawnTo(reporter, dev, shouldBeDrawn);
+}
+
+/*
+ *  Original bug was asserting that the matrix-proc had generated a (Y) value
+ *  that was out of range. This led (in the release build) to the sampler-proc
+ *  reading memory out-of-bounds of the original bitmap.
+ *
+ *  We were numerically overflowing our 16bit coordinates that we communicate
+ *  between these two procs. The fixes was in two parts:
+ *
+ *  1. Just don't draw bitmaps larger than 64K-1 in width or height, since we
+ *     can't represent those coordinates in our transport format (yet).
+ *  2. Perform an unsigned shift during the calculation, so we don't get
+ *     sign-extension bleed when packing the two values (X,Y) into our 32bit
+ *     slot.
+ *
+ *  This tests exercises the original setup, plus 3 more to ensure that we can,
+ *  in fact, handle bitmaps at 64K-1 (assuming we don't exceed the total
+ *  memory allocation limit).
+ */
+static void test_giantrepeat_crbug118018(skiatest::Reporter* reporter) {
+    static const struct {
+        int fWidth;
+        int fHeight;
+        bool fExpectedToDraw;
+    } gTests[] = {
+        { 0x1b294, 0x7f,  false },   // crbug 118018 (width exceeds 64K)
+        { 0xFFFF, 0x7f,    true },   // should draw, test max width
+        { 0x7f, 0xFFFF,    true },   // should draw, test max height
+        { 0xFFFF, 0xFFFF, false },   // allocation fails (too much RAM)
+    };
+    
+    for (size_t i = 0; i < SK_ARRAY_COUNT(gTests); ++i) {
+        test_wacky_bitmapshader(reporter,
+                                gTests[i].fWidth, gTests[i].fHeight,
+                                gTests[i].fExpectedToDraw);
+    }
+}
+///////////////////////////////////////////////////////////////////////////////
 
 static void test_nan_antihair(skiatest::Reporter* reporter) {
     SkBitmap bm;
@@ -72,6 +167,7 @@ static void TestDrawBitmapRect(skiatest::Reporter* reporter) {
     REPORTER_ASSERT(reporter, check_for_all_zeros(dst));
 
     test_nan_antihair(reporter);
+    test_giantrepeat_crbug118018(reporter);
 }
 
 #include "TestClassDef.h"