From 60751d7a069fb540aa468068f792fb6f4999832d Mon Sep 17 00:00:00 2001 From: Brian Osman Date: Fri, 12 May 2017 11:21:36 -0400 Subject: [PATCH] Sort all user-supplied rects before computeFastBounds https://codereview.chromium.org/908353002 fixed drawRect 2+ years ago, but drawOval and drawArc were still susceptible. This version ensures that all rects are sorted before we do the bounds check. Added a new makeSorted helper to simplify the code, and an assert to catch any future oversight. All other drawing functions compute their bounds rect in some way that already ensures it is sorted. Bug: skia: Change-Id: I8926b2dbe9d496d0876f1ac5313bd058ae4568b7 Reviewed-on: https://skia-review.googlesource.com/16702 Reviewed-by: Mike Reed Commit-Queue: Brian Osman --- include/core/SkPaint.h | 2 ++ include/core/SkRect.h | 30 +++++++++++++++++++++++++++++- src/core/SkCanvas.cpp | 13 +++++++------ src/core/SkRect.cpp | 9 --------- 4 files changed, 38 insertions(+), 16 deletions(-) diff --git a/include/core/SkPaint.h b/include/core/SkPaint.h index 1d99e4047b..464e535758 100644 --- a/include/core/SkPaint.h +++ b/include/core/SkPaint.h @@ -986,6 +986,8 @@ public: } */ const SkRect& computeFastBounds(const SkRect& orig, SkRect* storage) const { + // Things like stroking, etc... will do math on the bounds rect, assuming that it's sorted. + SkASSERT(orig.isSorted()); SkPaint::Style style = this->getStyle(); // ultra fast-case: filling with no effects that affect geometry if (kFill_Style == style) { diff --git a/include/core/SkRect.h b/include/core/SkRect.h index b8cf846404..a7b8b119d2 100644 --- a/include/core/SkRect.h +++ b/include/core/SkRect.h @@ -376,7 +376,22 @@ struct SK_API SkIRect { and may have crossed over each other. When this returns, left <= right && top <= bottom */ - void sort(); + void sort() { + if (fLeft > fRight) { + SkTSwap(fLeft, fRight); + } + if (fTop > fBottom) { + SkTSwap(fTop, fBottom); + } + } + + /** + * Return a new Rect that is the sorted version of this rect (left <= right, top <= bottom). + */ + SkIRect makeSorted() const { + return MakeLTRB(SkMin32(fLeft, fRight), SkMin32(fTop, fBottom), + SkMax32(fLeft, fRight), SkMax32(fTop, fBottom)); + } static const SkIRect& SK_WARN_UNUSED_RESULT EmptyIRect() { static const SkIRect gEmpty = { 0, 0, 0, 0 }; @@ -456,6 +471,11 @@ struct SK_API SkRect { */ bool isEmpty() const { return fLeft >= fRight || fTop >= fBottom; } + /** + * Return true if the rectangle's width and height are >= 0 + */ + bool isSorted() const { return fLeft <= fRight && fTop <= fBottom; } + bool isLargest() const { return SK_ScalarMin == fLeft && SK_ScalarMin == fTop && SK_ScalarMax == fRight && @@ -887,6 +907,14 @@ public: } } + /** + * Return a new Rect that is the sorted version of this rect (left <= right, top <= bottom). + */ + SkRect makeSorted() const { + return MakeLTRB(SkMinScalar(fLeft, fRight), SkMinScalar(fTop, fBottom), + SkMaxScalar(fLeft, fRight), SkMaxScalar(fTop, fBottom)); + } + /** * cast-safe way to treat the rect as an array of (4) SkScalars. */ diff --git a/src/core/SkCanvas.cpp b/src/core/SkCanvas.cpp index b890ead9cf..10e2181aff 100644 --- a/src/core/SkCanvas.cpp +++ b/src/core/SkCanvas.cpp @@ -1981,11 +1981,8 @@ void SkCanvas::onDrawRect(const SkRect& r, const SkPaint& paint) { if (paint.canComputeFastBounds()) { // Skia will draw an inverted rect, because it explicitly "sorts" it downstream. // To prevent accidental rejecting at this stage, we have to sort it before we check. - SkRect tmp(r); - tmp.sort(); - SkRect storage; - if (this->quickReject(paint.computeFastBounds(tmp, &storage))) { + if (this->quickReject(paint.computeFastBounds(r.makeSorted(), &storage))) { return; } } @@ -2028,8 +2025,10 @@ void SkCanvas::onDrawRegion(const SkRegion& region, const SkPaint& paint) { void SkCanvas::onDrawOval(const SkRect& oval, const SkPaint& paint) { TRACE_EVENT0("disabled-by-default-skia", "SkCanvas::drawOval()"); if (paint.canComputeFastBounds()) { + // Skia will draw an inverted rect, because it explicitly "sorts" it downstream. + // To prevent accidental rejecting at this stage, we have to sort it before we check. SkRect storage; - if (this->quickReject(paint.computeFastBounds(oval, &storage))) { + if (this->quickReject(paint.computeFastBounds(oval.makeSorted(), &storage))) { return; } } @@ -2048,9 +2047,11 @@ void SkCanvas::onDrawArc(const SkRect& oval, SkScalar startAngle, const SkPaint& paint) { TRACE_EVENT0("disabled-by-default-skia", "SkCanvas::drawArc()"); if (paint.canComputeFastBounds()) { + // Skia will draw an inverted rect, because it explicitly "sorts" it downstream. + // To prevent accidental rejecting at this stage, we have to sort it before we check. SkRect storage; // Note we're using the entire oval as the bounds. - if (this->quickReject(paint.computeFastBounds(oval, &storage))) { + if (this->quickReject(paint.computeFastBounds(oval.makeSorted(), &storage))) { return; } } diff --git a/src/core/SkRect.cpp b/src/core/SkRect.cpp index d868bbb859..900b872896 100644 --- a/src/core/SkRect.cpp +++ b/src/core/SkRect.cpp @@ -26,15 +26,6 @@ void SkIRect::join(int32_t left, int32_t top, int32_t right, int32_t bottom) { } } -void SkIRect::sort() { - if (fLeft > fRight) { - SkTSwap(fLeft, fRight); - } - if (fTop > fBottom) { - SkTSwap(fTop, fBottom); - } -} - ///////////////////////////////////////////////////////////////////////////// void SkRect::toQuad(SkPoint quad[4]) const {