[skottie] Use metrics for Shaper vertical alignment

fmalita · Skia Commit-Bot · commit 084fa1b52f30 · 2019-06-13T13:04:53.000Z
Relying on visual bounds yields incorrect results in some cases (e.g. leading/trailing empty lines). Update the vertical alignment logic to use metrics instead: - track the first line ascent and last line descent - compute content height as first_ascent + last_descent + line_height * (line_count - 1) - relocate Result::computeBounds() to the unit test (only user) Empirically, this causes top-alignment to be less snug (likely due to ascent slack in the tested fonts). Bug: skia:9098 Change-Id: Ib92bf907af8889d6b0d0fda22ef41a2cc8b50901 Reviewed-on: https://skia-review.googlesource.com/c/skia/+/220656 Reviewed-by: Mike Reed <reed@google.com> Commit-Queue: Florin Malita <fmalita@chromium.org>
diff --git a/modules/skottie/src/SkottieTest.cpp b/modules/skottie/src/SkottieTest.cpp
@@ -12,6 +12,7 @@
 #include "modules/skottie/include/Skottie.h"
 #include "modules/skottie/include/SkottieProperty.h"
 #include "modules/skottie/src/text/SkottieShaper.h"
+#include "src/core/SkTextBlobPriv.h"
 
 #include "tests/Test.h"
 
@@ -222,6 +223,42 @@ DEF_TEST(Skottie_Annotations, reporter) {
     REPORTER_ASSERT(reporter, std::get<2>(observer->fMarkers[1]) == 0.75f);
 }
 
+static SkRect ComputeBlobBounds(const sk_sp<SkTextBlob>& blob) {
+    auto bounds = SkRect::MakeEmpty();
+
+    if (!blob) {
+        return bounds;
+    }
+
+    SkAutoSTArray<16, SkRect> glyphBounds;
+
+    SkTextBlobRunIterator it(blob.get());
+
+    for (SkTextBlobRunIterator it(blob.get()); !it.done(); it.next()) {
+        glyphBounds.reset(SkToInt(it.glyphCount()));
+        it.font().getBounds(it.glyphs(), it.glyphCount(), glyphBounds.get(), nullptr);
+
+        SkASSERT(it.positioning() == SkTextBlobRunIterator::kFull_Positioning);
+        for (uint32_t i = 0; i < it.glyphCount(); ++i) {
+            bounds.join(glyphBounds[i].makeOffset(it.pos()[i * 2    ],
+                                                  it.pos()[i * 2 + 1]));
+        }
+    }
+
+    return bounds;
+}
+
+static SkRect ComputeShapeResultBounds(const skottie::Shaper::Result& res) {
+    auto bounds = SkRect::MakeEmpty();
+
+    for (const auto& fragment : res.fFragments) {
+        bounds.join(ComputeBlobBounds(fragment.fBlob).makeOffset(fragment.fPos.x(),
+                                                                 fragment.fPos.y()));
+    }
+
+    return bounds;
+}
+
 DEF_TEST(Skottie_Shaper_HAlign, reporter) {
     auto typeface = SkTypeface::MakeDefault();
     REPORTER_ASSERT(reporter, typeface);
@@ -266,7 +303,7 @@ DEF_TEST(Skottie_Shaper_HAlign, reporter) {
             REPORTER_ASSERT(reporter, shape_result.fFragments.size() == 1ul);
             REPORTER_ASSERT(reporter, shape_result.fFragments[0].fBlob);
 
-            const auto shape_bounds = shape_result.computeBounds();
+            const auto shape_bounds = ComputeShapeResultBounds(shape_result);
             REPORTER_ASSERT(reporter, !shape_bounds.isEmpty());
 
             const auto expected_l = text_point.x() - shape_bounds.width() * talign.l_selector;
@@ -296,9 +333,9 @@ DEF_TEST(Skottie_Shaper_VAlign, reporter) {
         // These gross tolerances are required for the test to pass on NativeFonts bots.
         // Might be worth investigating why we need so much slack.
         {  5, 2.0f },
-        { 10, 2.0f },
-        { 15, 2.4f },
-        { 25, 4.4f },
+        { 10, 4.0f },
+        { 15, 5.5f },
+        { 25, 8.0f },
     };
 
     struct {
@@ -329,7 +366,7 @@ DEF_TEST(Skottie_Shaper_VAlign, reporter) {
             REPORTER_ASSERT(reporter, shape_result.fFragments.size() == 1ul);
             REPORTER_ASSERT(reporter, shape_result.fFragments[0].fBlob);
 
-            const auto shape_bounds = shape_result.computeBounds();
+            const auto shape_bounds = ComputeShapeResultBounds(shape_result);
             REPORTER_ASSERT(reporter, !shape_bounds.isEmpty());
 
             const auto v_diff = text_box.height() - shape_bounds.height();
diff --git a/modules/skottie/src/text/SkottieShaper.cpp b/modules/skottie/src/text/SkottieShaper.cpp
@@ -7,42 +7,17 @@
 
 #include "modules/skottie/src/text/SkottieShaper.h"
 
+#include "include/core/SkFontMetrics.h"
 #include "include/core/SkTextBlob.h"
 #include "include/private/SkTemplates.h"
 #include "modules/skshaper/include/SkShaper.h"
-#include "src/core/SkTextBlobPriv.h"
 #include "src/utils/SkUTF.h"
 
 #include <limits.h>
 
 namespace skottie {
 namespace {
 
-SkRect ComputeBlobBounds(const sk_sp<SkTextBlob>& blob) {
-    auto bounds = SkRect::MakeEmpty();
-
-    if (!blob) {
-        return bounds;
-    }
-
-    SkAutoSTArray<16, SkRect> glyphBounds;
-
-    SkTextBlobRunIterator it(blob.get());
-
-    for (SkTextBlobRunIterator it(blob.get()); !it.done(); it.next()) {
-        glyphBounds.reset(SkToInt(it.glyphCount()));
-        it.font().getBounds(it.glyphs(), it.glyphCount(), glyphBounds.get(), nullptr);
-
-        SkASSERT(it.positioning() == SkTextBlobRunIterator::kFull_Positioning);
-        for (uint32_t i = 0; i < it.glyphCount(); ++i) {
-            bounds.join(glyphBounds[i].makeOffset(it.pos()[i * 2    ],
-                                                  it.pos()[i * 2 + 1]));
-        }
-    }
-
-    return bounds;
-}
-
 // Helper for interfacing with SkShaper: buffers shaper-fed runs and performs
 // per-line position adjustments (for external line breaking, horizontal alignment, etc).
 class BlobMaker final : public SkShaper::RunHandler {
@@ -68,10 +43,19 @@ class BlobMaker final : public SkShaper::RunHandler {
 
         fCurrentPosition = fOffset;
         fPendingLineAdvance  = { 0, 0 };
+
+        fLastLineDescent = 0;
     }
 
     void runInfo(const RunInfo& info) override {
         fPendingLineAdvance += info.fAdvance;
+
+        SkFontMetrics metrics;
+        info.fFont.getMetrics(&metrics);
+        if (!fLineCount) {
+            fFirstLineAscent = SkTMin(fFirstLineAscent, metrics.fAscent);
+        }
+        fLastLineDescent = SkTMax(fLastLineDescent, metrics.fDescent);
     }
 
     void commitRunInfo() override {}
@@ -116,36 +100,41 @@ class BlobMaker final : public SkShaper::RunHandler {
                         fLineGlyphs.get()   + run_offset,
                         fLinePos.get()      + run_offset,
                         fLineClusters.get() + run_offset,
-                        fLineIndex);
+                        fLineCount);
             run_offset += rec.fGlyphCount;
         }
 
-        fLineIndex++;
+        fLineCount++;
     }
 
-    Shaper::Result finalize() {
+    Shaper::Result finalize(float* shaped_height) {
         if (!(fDesc.fFlags & Shaper::Flags::kFragmentGlyphs)) {
             // All glyphs are pending in a single blob.
             SkASSERT(fResult.fFragments.empty());
             fResult.fFragments.reserve(1);
             fResult.fFragments.push_back({fBuilder.make(), {fBox.x(), fBox.y()}, 0, false});
         }
 
-        // By default, first line is vertical-aligned on a baseline of 0.
+        // By default, first line is vertically-aligned on a baseline of 0.
+        // The content height considered for vertical alignment is the distance between the first
+        // line top (ascent) to the last line bottom (descent).
+        const auto content_height = fLastLineDescent - fFirstLineAscent +
+                                    fDesc.fLineHeight * (fLineCount > 0 ? fLineCount - 1 : 0ul);
+
         // Perform additional adjustments based on VAlign.
         float v_offset = 0;
         switch (fDesc.fVAlign) {
         case Shaper::VAlign::kTop:
-            v_offset = fBox.fTop - fResult.computeBounds().fTop;
+            v_offset = -fFirstLineAscent;
             break;
         case Shaper::VAlign::kTopBaseline:
             // Default behavior.
             break;
         case Shaper::VAlign::kCenter:
-            v_offset = fBox.centerY() - fResult.computeBounds().centerY();
+            v_offset = -fFirstLineAscent + (fBox.height() - content_height) * 0.5f;
             break;
         case Shaper::VAlign::kBottom:
-            v_offset = fBox.fBottom - fResult.computeBounds().fBottom;
+            v_offset = -fFirstLineAscent + (fBox.height() - content_height);
             break;
         case Shaper::VAlign::kResizeToFit:
             SkASSERT(false);
@@ -158,6 +147,10 @@ class BlobMaker final : public SkShaper::RunHandler {
             }
         }
 
+        if (shaped_height) {
+            *shaped_height = content_height;
+        }
+
         return std::move(fResult);
     }
 
@@ -248,14 +241,17 @@ class BlobMaker final : public SkShaper::RunHandler {
     SkPoint  fCurrentPosition{ 0, 0 };
     SkPoint  fOffset{ 0, 0 };
     SkVector fPendingLineAdvance{ 0, 0 };
-    uint32_t fLineIndex = 0;
+    uint32_t fLineCount = 0;
+    float    fFirstLineAscent = 0,
+             fLastLineDescent = 0;
 
     const char* fUTF8 = nullptr; // only valid during shapeLine() calls
 
     Shaper::Result fResult;
 };
 
-Shaper::Result ShapeImpl(const SkString& txt, const Shaper::TextDesc& desc, const SkRect& box) {
+Shaper::Result ShapeImpl(const SkString& txt, const Shaper::TextDesc& desc,
+                         const SkRect& box, float* shaped_height = nullptr) {
     SkASSERT(desc.fVAlign != Shaper::VAlign::kResizeToFit);
 
     const auto& is_line_break = [](SkUnichar uch) {
@@ -276,7 +272,7 @@ Shaper::Result ShapeImpl(const SkString& txt, const Shaper::TextDesc& desc, cons
     }
     blobMaker.shapeLine(line_start, ptr);
 
-    return blobMaker.finalize();
+    return blobMaker.finalize(shaped_height);
 }
 
 Shaper::Result ShapeToFit(const SkString& txt, const Shaper::TextDesc& orig_desc,
@@ -307,8 +303,8 @@ Shaper::Result ShapeToFit(const SkString& txt, const Shaper::TextDesc& orig_desc
         desc.fTextSize   = try_scale * orig_desc.fTextSize;
         desc.fLineHeight = try_scale * orig_desc.fLineHeight;
 
-        auto res = ShapeImpl(txt, desc, box);
-        auto res_height = res.computeBounds().height();
+        float res_height = 0;
+        auto res = ShapeImpl(txt, desc, box, &res_height);
 
         if (res_height > box.height()) {
             out_scale = try_scale;
@@ -348,15 +344,4 @@ Shaper::Result Shaper::Shape(const SkString& txt, const TextDesc& desc, const Sk
             : ShapeImpl(txt, desc, box);
 }
 
-SkRect Shaper::Result::computeBounds() const {
-    auto bounds = SkRect::MakeEmpty();
-
-    for (const auto& fragment : fFragments) {
-        bounds.join(ComputeBlobBounds(fragment.fBlob).makeOffset(fragment.fPos.x(),
-                                                                 fragment.fPos.y()));
-    }
-
-    return bounds;
-}
-
 } // namespace skottie
diff --git a/modules/skottie/src/text/SkottieShaper.h b/modules/skottie/src/text/SkottieShaper.h
@@ -33,8 +33,6 @@ class Shaper final {
 
     struct Result {
         std::vector<Fragment> fFragments;
-
-        SkRect computeBounds() const;
     };
 
     enum class VAlign : uint8_t {
