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

diff --git a/src/core/SkRecordDraw.cpp b/src/core/SkRecordDraw.cpp
index 5981245c018700e8ed969e7abc75e1d76c63b7ba..2858cd3021f60a22b3c12dc784203cd5635657a7 100644 (file)
--- 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<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;
 #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 6105c00f7f1093842d9fc2e808a753f2c7595f7b..2476c039843228f413909fd3669123c650b52b3e 100644 (file)
--- 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