diff options
author | 2014-10-29 08:33:38 -0700 | |
---|---|---|
committer | 2014-10-29 08:33:38 -0700 | |
commit | 27f965a577262deb9182c6935f3af05bb52b817c (patch) | |
tree | f1caf99809406e41da7ae035326fa6193b5ea32f | |
parent | 7aa4d349be08cd622b715b21dd1e7770f7db2c01 (diff) |
Revert of Use SkTypeface::getBounds() in bounding-box calculations. (patchset #3 id:40001 of https://codereview.chromium.org/680363003/)
Reason for revert:
http://build.chromium.org/p/client.skia/builders/Test-Win7-ShuttleA-HD2000-x86-Debug/builds/97/steps/nanobench/logs/stdio
Original issue's description:
> Use SkTypeface::getBounds() in bounding-box calculations.
>
> This should produce tighter conservative bounding boxes for text than the
> approximation code it replaces.
>
> Recording performance is neutral on my desktop. Playback performance
> improves by up to 15% on text heavy pages, e.g.
>
> desk_pokemonwiki.skp_1 3.24ms -> 2.83ms 0.87x
> desk_baidu.skp_1 1.91ms -> 1.58ms 0.83x
>
> Committed: https://skia.googlesource.com/skia/+/bf8dc343df4fbdcb8af546eb68b640e011a33489
TBR=reed@google.com,mtklein@chromium.org
NOTREECHECKS=true
NOTRY=true
Review URL: https://codereview.chromium.org/685173002
-rw-r--r-- | src/core/SkRecordDraw.cpp | 39 | ||||
-rw-r--r-- | tests/RecordDrawTest.cpp | 10 |
2 files changed, 20 insertions, 29 deletions
diff --git a/src/core/SkRecordDraw.cpp b/src/core/SkRecordDraw.cpp index 2858cd3021..5981245c01 100644 --- a/src/core/SkRecordDraw.cpp +++ b/src/core/SkRecordDraw.cpp @@ -7,7 +7,6 @@ #include "SkRecordDraw.h" #include "SkPatchUtils.h" -#include "SkTypeface.h" void SkRecordDraw(const SkRecord& record, SkCanvas* canvas, @@ -445,13 +444,15 @@ private: Bounds bounds(const DrawTextOnPath& op) const { SkRect dst = op.path.getBounds(); - // We don't know how the text will curve aroudn the path, so - // pad all sides by the maximum padding in any direction we'd normally apply. + // Pad all sides by the maximum padding in any direction we'd normally apply. SkRect pad = { 0, 0, 0, 0}; AdjustTextForFontMetrics(&pad, op.paint); - SkScalar max = SkTMax(SkTMax(-pad.fLeft, pad.fRight), - SkTMax(-pad.fTop, pad.fBottom)); - dst.outset(max, max); + + // That maximum padding happens to always be the right pad today. + SkASSERT(pad.fLeft == -pad.fRight); + SkASSERT(pad.fTop == -pad.fBottom); + SkASSERT(pad.fRight > pad.fBottom); + dst.outset(pad.fRight, pad.fRight); return this->adjustAndMap(dst, &op.paint); } @@ -463,23 +464,14 @@ private: } static void AdjustTextForFontMetrics(SkRect* rect, const SkPaint& paint) { - // rect was built from only the text's origin points, so we need - // to outset it by the worst-case bounds of the typeface. #ifdef SK_DEBUG SkRect correct = *rect; #endif - SkAutoTUnref<SkTypeface> au; - SkTypeface* tf = paint.getTypeface(); - if (!tf) { - au.reset(SkTypeface::RefDefault()); - tf = au.get(); - } - const SkScalar size = paint.getTextSize(); - const SkRect tb = tf->getBounds(); - rect->fLeft += size * tb.fLeft; - rect->fRight += size * tb.fRight; - rect->fTop += size * tb.fTop; - rect->fBottom += size * tb.fBottom; + // crbug.com/373785 ~~> xPad = 4x yPad + // crbug.com/424824 ~~> bump yPad from 2x text size to 2.5x + const SkScalar yPad = 2.5f * paint.getTextSize(), + xPad = 4.0f * yPad; + rect->outset(xPad, yPad); #ifdef SK_DEBUG SkPaint::FontMetrics metrics; paint.getFontMetrics(&metrics); @@ -489,10 +481,9 @@ private: correct.fBottom += metrics.fBottom; // See skia:2862 for why we ignore small text sizes. SkASSERTF(paint.getTextSize() < 0.001f || rect->contains(correct), - "%f %f %f %f vs. %f %f %f %f, text size %f\n", - size*tb.fLeft, size*tb.fTop, size*tb.fRight, size*tb.fBottom, - metrics.fXMin, metrics.fTop, metrics.fXMax, metrics.fBottom, - paint.getTextSize()); + "%f %f %f %f vs. %f %f %f %f\n", + -xPad, -yPad, +xPad, +yPad, + metrics.fXMin, metrics.fTop, metrics.fXMax, metrics.fBottom); #endif } diff --git a/tests/RecordDrawTest.cpp b/tests/RecordDrawTest.cpp index 2476c03984..6105c00f7f 100644 --- a/tests/RecordDrawTest.cpp +++ b/tests/RecordDrawTest.cpp @@ -166,11 +166,11 @@ DEF_TEST(RecordDraw_TextBounds, r) { SkRecordFillBounds(record, &bbh); REPORTER_ASSERT(r, bbh.fEntries.count() == 2); - // Font metrics are somewhat platform dependent. These assertions may need to be adjusted. - // However, these particular numbers are left over from the days when we used to wildly - // overestimate font metrics, so these assertions should be pretty conservative. - REPORTER_ASSERT(r, SkRect::MakeLTRB(-110, 0, 140, 60) .contains(bbh.fEntries[0].bounds)); - REPORTER_ASSERT(r, SkRect::MakeLTRB(-80, 20, 180, 100).contains(bbh.fEntries[1].bounds)); + // We can make these next assertions confidently because SkRecordFillBounds + // builds its bounds by overestimating font metrics in a platform-independent way. + // If that changes, these tests will need to be more flexible. + REPORTER_ASSERT(r, sloppy_rect_eq(bbh.fEntries[0].bounds, SkRect::MakeLTRB(-110, 0, 140, 60))); + REPORTER_ASSERT(r, sloppy_rect_eq(bbh.fEntries[1].bounds, SkRect::MakeLTRB(-80, 20, 180, 100))); } // Base test to ensure start/stop range is respected |