From bf8dc343df4fbdcb8af546eb68b640e011a33489 Mon Sep 17 00:00:00 2001 From: mtklein Date: Wed, 29 Oct 2014 08:12:07 -0700 Subject: [PATCH] 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 Review URL: https://codereview.chromium.org/680363003 --- src/core/SkRecordDraw.cpp | 39 ++++++++++++++++++++++++--------------- tests/RecordDrawTest.cpp | 10 +++++----- 2 files changed, 29 insertions(+), 20 deletions(-) diff --git a/src/core/SkRecordDraw.cpp b/src/core/SkRecordDraw.cpp index 5981245c01..2858cd3021 100644 --- a/src/core/SkRecordDraw.cpp +++ b/src/core/SkRecordDraw.cpp @@ -7,6 +7,7 @@ #include "SkRecordDraw.h" #include "SkPatchUtils.h" +#include "SkTypeface.h" void SkRecordDraw(const SkRecord& record, SkCanvas* canvas, @@ -444,15 +445,13 @@ private: Bounds bounds(const DrawTextOnPath& op) const { SkRect dst = op.path.getBounds(); - // Pad all sides by the maximum padding in any direction we'd normally apply. + // 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. SkRect pad = { 0, 0, 0, 0}; AdjustTextForFontMetrics(&pad, op.paint); - - // 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); + SkScalar max = SkTMax(SkTMax(-pad.fLeft, pad.fRight), + SkTMax(-pad.fTop, pad.fBottom)); + dst.outset(max, max); return this->adjustAndMap(dst, &op.paint); } @@ -464,14 +463,23 @@ 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 - // 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); + SkAutoTUnref 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; #ifdef SK_DEBUG SkPaint::FontMetrics metrics; paint.getFontMetrics(&metrics); @@ -481,9 +489,10 @@ 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\n", - -xPad, -yPad, +xPad, +yPad, - metrics.fXMin, metrics.fTop, metrics.fXMax, metrics.fBottom); + "%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()); #endif } diff --git a/tests/RecordDrawTest.cpp b/tests/RecordDrawTest.cpp index 6105c00f7f..2476c03984 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); - // 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))); + // 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)); } // Base test to ensure start/stop range is respected -- 2.34.1