diff options
author | mtklein <mtklein@google.com> | 2014-12-16 06:31:01 -0800 |
---|---|---|
committer | Commit bot <commit-bot@chromium.org> | 2014-12-16 06:31:01 -0800 |
commit | 9a5380dd358a9c559a41beef7cb368cd7f0ca51f (patch) | |
tree | 85fce3373cc0d0982c1a4b7f200149e8b018b843 | |
parent | bdd22a35bfb3bb087ea37a328f14db2c7796f9df (diff) |
Revert of Use SkPaint::getFontBounds() for text bounding boxes in pictures. (patchset #1 id:1 of https://codereview.chromium.org/805983003/)
Reason for revert:
Test-Win7-ShuttleA-HD2000-x86-Debug-GDI
Original issue's description:
> Use SkPaint::getFontBounds() for text bounding boxes in pictures.
>
> Now that SkTextBlobs have landed, this is a perf no-op, but it at least lets us eliminate a bunch of questionable hacks.
>
> CQ_EXTRA_TRYBOTS=client.skia:Test-Win7-ShuttleA-HD2000-x86-Debug-Trybot,Test-Mac10.8-MacMini4.1-GeForce320M-x86_64-Debug-Trybot
>
>
> BUG=skia:
>
> Committed: https://skia.googlesource.com/skia/+/02d2b9831579173e783569530ab7bae08de907e9
TBR=reed@google.com,mtklein@chromium.org
NOTREECHECKS=true
NOTRY=true
BUG=skia:
Review URL: https://codereview.chromium.org/789793004
-rw-r--r-- | src/core/SkRecordDraw.cpp | 37 | ||||
-rw-r--r-- | tests/RecordDrawTest.cpp | 10 |
2 files changed, 27 insertions, 20 deletions
diff --git a/src/core/SkRecordDraw.cpp b/src/core/SkRecordDraw.cpp index ba401005fa..86621f2170 100644 --- a/src/core/SkRecordDraw.cpp +++ b/src/core/SkRecordDraw.cpp @@ -487,13 +487,16 @@ private: Bounds bounds(const DrawTextOnPath& op) const { SkRect dst = op.path.getBounds(); - // We don't know how the text will curve around 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); } @@ -508,21 +511,25 @@ 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 font. - const SkRect bounds = paint.getFontBounds(); - rect->fLeft += bounds.fLeft; - rect->fRight += bounds.fRight; - rect->fTop += bounds.fTop; - rect->fBottom += bounds.fBottom; +#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); #ifdef SK_DEBUG SkPaint::FontMetrics metrics; paint.getFontMetrics(&metrics); - const SkRect correct = { metrics.fXMin, metrics.fTop, metrics.fXMax, metrics.fBottom }; + correct.fLeft += metrics.fXMin; + correct.fTop += metrics.fTop; + correct.fRight += metrics.fXMax; + correct.fBottom += metrics.fBottom; // See skia:2862 for why we ignore small text sizes. - SkASSERTF(paint.getTextSize() < 0.001f || bounds.contains(correct), + SkASSERTF(paint.getTextSize() < 0.001f || rect->contains(correct), "%f %f %f %f vs. %f %f %f %f\n", - bounds.fLeft, bounds.fTop, bounds.fRight, bounds.fBottom, + -xPad, -yPad, +xPad, +yPad, metrics.fXMin, metrics.fTop, metrics.fXMax, metrics.fBottom); #endif } diff --git a/tests/RecordDrawTest.cpp b/tests/RecordDrawTest.cpp index fb5e9cf3dd..e830af4b43 100644 --- a/tests/RecordDrawTest.cpp +++ b/tests/RecordDrawTest.cpp @@ -191,11 +191,11 @@ DEF_TEST(RecordDraw_TextBounds, r) { SkRecordFillBounds(SkRect::MakeWH(SkIntToScalar(W), SkIntToScalar(H)), record, &bbh); REPORTER_ASSERT(r, bbh.fEntries.count() == 2); - // Font metrics are somewhat platform dependent, so these assertions may need to be adjusted. - // But these particular numbers are left over from the days when we used to wildly overestimate - // font metrics, so these assertions should actually be pretty safe. - REPORTER_ASSERT(r, SkRect::MakeLTRB(0, 0,140, 60).contains(bbh.fEntries[0].bounds)); - REPORTER_ASSERT(r, SkRect::MakeLTRB(0,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(0, 0, 140, 60))); + REPORTER_ASSERT(r, sloppy_rect_eq(bbh.fEntries[1].bounds, SkRect::MakeLTRB(0, 20, 180, 100))); } // Base test to ensure start/stop range is respected |