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
diff --git a/include/core/SkShader.h b/include/core/SkShader.h
index 72bd784..c48c369 100644
--- a/include/core/SkShader.h
+++ b/include/core/SkShader.h
@@ -285,10 +285,15 @@
// 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.
- @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.
+ *
+ * If the bitmap cannot be used (e.g. has no pixels, or its dimensions
+ * exceed implementation limits (currently at 64K - 1)) then SkEmptyShader
+ * 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);
diff --git a/src/core/SkBitmapProcShader.cpp b/src/core/SkBitmapProcShader.cpp
index 7ae6afc..3abfc8d 100644
--- a/src/core/SkBitmapProcShader.cpp
+++ b/src/core/SkBitmapProcShader.cpp
@@ -271,12 +271,22 @@
#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)) {
diff --git a/src/core/SkBitmapProcState_matrixProcs.cpp b/src/core/SkBitmapProcState_matrixProcs.cpp
index 15e7beb..a8a2821 100644
--- a/src/core/SkBitmapProcState_matrixProcs.cpp
+++ b/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_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 TILEY_PROCF(fy, max) SkClampMax((fy) >> 16, max)
+#define TILEX_PROCF(fx, max) SkClampMax(SK_USHIFT16(fx), 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 @@
#endif
#define MAKENAME(suffix) RepeatX_RepeatY ## suffix
-#define TILEX_PROCF(fx, max) (((fx) & 0xFFFF) * ((max) + 1) >> 16)
-#define TILEY_PROCF(fy, max) (((fy) & 0xFFFF) * ((max) + 1) >> 16)
+#define TILEX_PROCF(fx, max) SK_USHIFT16(((fx) & 0xFFFF) * ((max) + 1))
+#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 @@
#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 TILEY_PROCF(fy, max) (tileProcY(fy) * ((max) + 1) >> 16)
+#define TILEX_PROCF(fx, max) SK_USHIFT16(tileProcX(fx) * ((max) + 1))
+#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"
diff --git a/tests/DrawBitmapRectTest.cpp b/tests/DrawBitmapRectTest.cpp
index e5b4463..ec6fa12 100644
--- a/tests/DrawBitmapRectTest.cpp
+++ b/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 @@
REPORTER_ASSERT(reporter, check_for_all_zeros(dst));
test_nan_antihair(reporter);
+ test_giantrepeat_crbug118018(reporter);
}
#include "TestClassDef.h"