From ace7f4276997235abe559b70620d9f89737d2518 Mon Sep 17 00:00:00 2001 From: rosca Date: Thu, 20 Nov 2014 07:24:32 -0800 Subject: [PATCH] Preventing division by 0 in non-separable blend mode shaders. In the software path, the same issue has been fixed some time ago: https://codereview.chromium.org/114173002 BUG=skia: Review URL: https://codereview.chromium.org/666043003 --- AUTHORS | 1 + gm/mixedxfermodes.cpp | 120 ++++++++++++++++++++++++++++++++---------------- src/core/SkXfermode.cpp | 4 +- 3 files changed, 84 insertions(+), 41 deletions(-) diff --git a/AUTHORS b/AUTHORS index 51d2bf5..c36ea2c 100644 --- a/AUTHORS +++ b/AUTHORS @@ -31,3 +31,4 @@ Steve Singer The Chromium Authors <*@chromium.org> Thiago Fransosi Farina Pavel Krajcevski +Ion Rosca diff --git a/gm/mixedxfermodes.cpp b/gm/mixedxfermodes.cpp index a5023e1..3e5ee7d 100644 --- a/gm/mixedxfermodes.cpp +++ b/gm/mixedxfermodes.cpp @@ -22,6 +22,15 @@ public: } protected: + enum ShapeType { + kShapeTypeCircle, + kShapeTypeRoundRect, + kShapeTypeRect, + kShapeTypeConvexPath, + kShapeTypeConcavePath, + kNumShapeTypes + }; + virtual SkString onShortName() SK_OVERRIDE { return SkString("mixed_xfermodes"); } @@ -32,48 +41,49 @@ protected: void drawShape(SkCanvas* canvas, const SkPaint& paint, - SkRandom* random) { + ShapeType type) { static const SkRect kRect = SkRect::MakeXYWH(SkIntToScalar(-50), SkIntToScalar(-50), SkIntToScalar(75), SkIntToScalar(105)); - int shape = random->nextULessThan(5); - switch (shape) { - case 0: - canvas->drawCircle(0, 0, 50, paint); - break; - case 1: - canvas->drawRoundRect(kRect, SkIntToScalar(10), SkIntToScalar(20), paint); - break; - case 2: - canvas->drawRect(kRect, paint); - break; - case 3: - if (fConvexPath.isEmpty()) { - SkPoint points[4]; - kRect.toQuad(points); - fConvexPath.moveTo(points[0]); - fConvexPath.quadTo(points[1], points[2]); - fConvexPath.quadTo(points[3], points[0]); - SkASSERT(fConvexPath.isConvex()); - } - canvas->drawPath(fConvexPath, paint); - break; - case 4: - if (fConcavePath.isEmpty()) { - SkPoint points[5] = {{0, SkIntToScalar(-50)} }; - SkMatrix rot; - rot.setRotate(SkIntToScalar(360) / 5); - for (int i = 1; i < 5; ++i) { - rot.mapPoints(points + i, points + i - 1, 1); + switch (type) { + case kShapeTypeCircle: + canvas->drawCircle(0, 0, 50, paint); + break; + case kShapeTypeRoundRect: + canvas->drawRoundRect(kRect, SkIntToScalar(10), SkIntToScalar(20), paint); + break; + case kShapeTypeRect: + canvas->drawRect(kRect, paint); + break; + case kShapeTypeConvexPath: + if (fConvexPath.isEmpty()) { + SkPoint points[4]; + kRect.toQuad(points); + fConvexPath.moveTo(points[0]); + fConvexPath.quadTo(points[1], points[2]); + fConvexPath.quadTo(points[3], points[0]); + SkASSERT(fConvexPath.isConvex()); } - fConcavePath.moveTo(points[0]); - for (int i = 0; i < 5; ++i) { - fConcavePath.lineTo(points[(2 * i) % 5]); + canvas->drawPath(fConvexPath, paint); + break; + case kShapeTypeConcavePath: + if (fConcavePath.isEmpty()) { + SkPoint points[5] = {{0, SkIntToScalar(-50)} }; + SkMatrix rot; + rot.setRotate(SkIntToScalar(360) / 5); + for (int i = 1; i < 5; ++i) { + rot.mapPoints(points + i, points + i - 1, 1); + } + fConcavePath.moveTo(points[0]); + for (int i = 0; i < 5; ++i) { + fConcavePath.lineTo(points[(2 * i) % 5]); + } + fConcavePath.setFillType(SkPath::kEvenOdd_FillType); + SkASSERT(!fConcavePath.isConvex()); } - fConcavePath.setFillType(SkPath::kEvenOdd_FillType); - SkASSERT(!fConcavePath.isConvex()); - } - canvas->drawPath(fConcavePath, paint); - break; + canvas->drawPath(fConcavePath, paint); + break; + default: + break; } } @@ -108,6 +118,7 @@ protected: SkColor color = random.nextU(); SkXfermode::Mode mode = static_cast(random.nextULessThan(SkXfermode::kLastMode + 1)); + ShapeType shapeType = static_cast(random.nextULessThan(kNumShapeTypes)); SkPaint p; p.setAntiAlias(true); @@ -117,9 +128,40 @@ protected: canvas->translate(dx, dy); canvas->scale(s, s); canvas->rotate(r); - this->drawShape(canvas, p, &random); + this->drawShape(canvas, p, shapeType); canvas->restore(); } + + // This draw should not affect the test's result. + drawWithHueOnWhite(canvas); + } + + /** + * Draws white color into a white square using the hue blend mode. + * The result color should be white, so it doesn't change the expectations. + * This will test a divide by 0 bug in shaders' setLum function, + * which used to output black pixels. + */ + void drawWithHueOnWhite(SkCanvas* canvas) { + SkColor color = SkColorSetARGBMacro(225, 255, 255, 255); + SkXfermode::Mode mode = SkXfermode::kHue_Mode; + ShapeType shapeType = kShapeTypeConvexPath; + + // Make it fit into a square. + SkScalar s = 0.15f; + // Look for a clean white square. + SkScalar dx = 30.f; + SkScalar dy = 350.f; + + SkPaint p; + p.setAntiAlias(true); + p.setColor(color); + p.setXfermodeMode(mode); + canvas->save(); + canvas->translate(dx, dy); + canvas->scale(s, s); + this->drawShape(canvas, p, shapeType); + canvas->restore(); } virtual uint32_t onGetFlags() const { diff --git a/src/core/SkXfermode.cpp b/src/core/SkXfermode.cpp index 36be29e..77c6684 100644 --- a/src/core/SkXfermode.cpp +++ b/src/core/SkXfermode.cpp @@ -1101,10 +1101,10 @@ public: setLumBody.appendf("\tfloat outLum = %s(outColor);\n", getFunction.c_str()); setLumBody.append("\tfloat minComp = min(min(outColor.r, outColor.g), outColor.b);\n" "\tfloat maxComp = max(max(outColor.r, outColor.g), outColor.b);\n" - "\tif (minComp < 0.0) {\n" + "\tif (minComp < 0.0 && outLum != minComp) {\n" "\t\toutColor = outLum + ((outColor - vec3(outLum, outLum, outLum)) * outLum) / (outLum - minComp);\n" "\t}\n" - "\tif (maxComp > alpha) {\n" + "\tif (maxComp > alpha && maxComp != outLum) {\n" "\t\toutColor = outLum + ((outColor - vec3(outLum, outLum, outLum)) * (alpha - outLum)) / (maxComp - outLum);\n" "\t}\n" "\treturn outColor;\n"); -- 2.7.4