Conservative SkTextBlob bounds.

src/core/SkTextBlob.cpp

Index: src/core/SkTextBlob.cpp
diff --git a/src/core/SkTextBlob.cpp b/src/core/SkTextBlob.cpp
index 0b81cb6572bbe288e284d08c49e7e89c62cca6bf..d2ea63d8f5c6249dfe0beaefa22d4738e61e5b38 100644
--- a/src/core/SkTextBlob.cpp
+++ b/src/core/SkTextBlob.cpp
@@ -8,6 +8,7 @@
 #include "SkTextBlob.h"
 
 #include "SkReadBuffer.h"
+#include "SkTypeface.h"
 #include "SkWriteBuffer.h"
 
 //
@@ -304,6 +305,82 @@ SkTextBlobBuilder::~SkTextBlobBuilder() {
     }
 }
 
+SkRect SkTextBlobBuilder::tightRunBounds(const SkTextBlob::RunRecord& run) {
+    SkRect bounds;
+
+    if (SkTextBlob::kDefault_Positioning == run.positioning()) {
+        run.font().measureText(run.glyphBuffer(), run.glyphCount() * sizeof(uint16_t), &bounds);
+        return bounds;
+    }
+
+    SkASSERT(SkTextBlob::kFull_Positioning == run.positioning() ||
+             SkTextBlob::kHorizontal_Positioning == run.positioning());
+
+    SkAutoSTArray<16, SkRect> glyphBounds(run.glyphCount());
+    run.font().getTextWidths(run.glyphBuffer(),
+                             run.glyphCount() * sizeof(uint16_t),
+                             NULL,
+                             glyphBounds.get());
+
+    bounds = SkRect::MakeEmpty();
+    SkScalar* glyphPos = run.posBuffer();
+    for (unsigned i = 0; i < run.glyphCount(); ++i) {
+        if (SkTextBlob::kFull_Positioning == run.positioning()) {
+            // [ x, y, x, y... ]
+            glyphBounds[i].offset(glyphPos[0], glyphPos[1]);
+            SkASSERT(2 == SkTextBlob::ScalarsPerGlyph(run.positioning()));
+            glyphPos += 2;
+        } else {
+            // [ x, x, x... ], const y applied by runBounds.offset(run->offset()) later.
+            glyphBounds[i].offset(glyphPos[0], 0);
+            SkASSERT(1 == SkTextBlob::ScalarsPerGlyph(run.positioning()));
+            glyphPos += 1;
+        }
+
+        bounds.join(glyphBounds[i]);
+    }
+
+    SkASSERT((void*)glyphPos <= SkTextBlob::RunRecord::Next(&run));
+
+    bounds.offset(run.offset());
+
+    return bounds;
+}
+
+SkRect SkTextBlobBuilder::conservativeRunBounds(const SkTextBlob::RunRecord& run) {
+    const SkScalar* glyphPos = run.posBuffer();
+    int posScalars = SkTextBlob::ScalarsPerGlyph(run.positioning());
+
+    SkASSERT(1 == posScalars || 2 == posScalars);
+    SkASSERT(run.glyphCount() > 0);
+    SkASSERT((void*)(glyphPos + run.glyphCount() * posScalars) <= SkTextBlob::RunRecord::Next(&run));
+
+    // First, compute the glyph position bbox.
+    SkRect bounds = SkRect::MakeXYWH(glyphPos[0], (2 == posScalars) ? glyphPos[1] : 0, 0, 0);
+    for (unsigned i = 1; i < run.glyphCount(); ++i) {
+        bounds.growToInclude(glyphPos[i * posScalars],
+                             (2 == posScalars) ? glyphPos[i * posScalars + 1] : 0);
+    }
+
+    SkScalar glyphScale = run.font().getTextSize();
+    SkRect typefaceBounds;
+    if (SkToBool(run.font().getTypeface())) {
+        typefaceBounds = run.font().getTypeface()->getBounds();
+    } else {
+        SkAutoTUnref<SkTypeface> typeface(SkTypeface::RefDefault());
+        typefaceBounds = typeface->getBounds();
+    }
+
+    // Expand by typeface glyph bounds.
+    bounds.fLeft   += glyphScale * typefaceBounds.left();
+    bounds.fTop    += glyphScale * typefaceBounds.top();
+    bounds.fRight  += glyphScale * typefaceBounds.right();
+    bounds.fBottom += glyphScale * typefaceBounds.bottom();
+
+    // Offset by run position.
+    return bounds.makeOffset(run.offset().x(), run.offset().y());
+}

[f(malita), 2015/01/28 14:46:19]

Here used to live an awesome assert: conservative bounds should always contain
the tight bounds.

Except this awesome assert was firing in Chromium for certain pages, because of
minor vertical tight box snapping due to hinting (as Ben explained).

So, err, the awesome assert no longer lives here...

[mtklein, 2015/01/28 14:58:21]

I actually had the same sort of problem over in the bbh building world (no
surprise), and fixing it was the reason we added getBounds() to SkTypeface. 
Turns out, SkTypeface::getBounds() isn't quite good enough in all cases.  It
doesn't account for the paint effects on that box, like x scale and skew.  This
led to a few rendering issues where small skewed text was dropped.

Luckily, we now have SkPaint::getFontBounds(), which applies the scale and skew
and should return the rectangle you actually want.  gm/fontmgr demos it.  I'm
not really sure why I haven't made use of it yet over in the BBH code...

[f(malita), 2015/01/28 15:14:37]

Yay for getFontBounds() - thanks for the tip! Done.

+
 void SkTextBlobBuilder::updateDeferredBounds() {
     SkASSERT(!fDeferredBounds || fRunCount > 0);
 
@@ -316,42 +393,17 @@ void SkTextBlobBuilder::updateDeferredBounds() {
                                                                           fLastRun);
     SkASSERT(SkPaint::kGlyphID_TextEncoding == run->font().getTextEncoding());
 
-    SkRect runBounds = SkRect::MakeEmpty();
+    SkRect runBounds;
+#ifdef SK_SUPPORT_LEGACY_BLOB_BOUNDS
+    runBounds = tightRunBounds(*run);
+#else
+    // FIXME: conservative bounds for default positioning?
     if (SkTextBlob::kDefault_Positioning == run->positioning()) {
-        run->font().measureText(run->glyphBuffer(),
-                                run->glyphCount() * sizeof(uint16_t),
-                                &runBounds);
+        runBounds = tightRunBounds(*run);
     } else {
-        SkASSERT(SkTextBlob::kFull_Positioning == run->positioning() ||
-                 SkTextBlob::kHorizontal_Positioning == run->positioning());
-
-        SkAutoSTArray<16, SkRect> glyphBounds(run->glyphCount());
-        run->font().getTextWidths(run->glyphBuffer(),
-                                  run->glyphCount() * sizeof(uint16_t),
-                                  NULL,
-                                  glyphBounds.get());
-
-        SkScalar* glyphOffset = run->posBuffer();
-        for (unsigned i = 0; i < run->glyphCount(); ++i) {
-            if (SkTextBlob::kFull_Positioning == run->positioning()) {
-                // [ x, y, x, y... ]
-                glyphBounds[i].offset(glyphOffset[0], glyphOffset[1]);
-                SkASSERT(2 == SkTextBlob::ScalarsPerGlyph(run->positioning()));
-                glyphOffset += 2;
-            } else {
-                // [ x, x, x... ], const y applied by runBounds.offset(run->offset()) later.
-                glyphBounds[i].offset(glyphOffset[0], 0);
-                SkASSERT(1 == SkTextBlob::ScalarsPerGlyph(run->positioning()));
-                glyphOffset += 1;
-            }
-
-            runBounds.join(glyphBounds[i]);
-        }
-
-        SkASSERT((void*)glyphOffset <= SkTextBlob::RunRecord::Next(run));
+        runBounds = conservativeRunBounds(*run);
     }
-
-    runBounds.offset(run->offset());
+#endif
 
     fBounds.join(runBounds);
     fDeferredBounds = false;