Reland "[skottie] Use metrics for Shaper vertical alignment"
This is a reland of 084fa1b52f30c1b1e349b809deda5f240b90b039
Original change's description:
> [skottie] Use metrics for Shaper vertical alignment
>
> 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>
TBR=
No-Try: true
Bug: skia:9098
Change-Id: Iaba53968840749e35b9c3ed04b15d6e2cda55e72
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/220916
Reviewed-by: Florin Malita <fmalita@chromium.org>
Commit-Queue: Florin Malita <fmalita@chromium.org>
diff --git a/modules/skottie/src/SkottieTest.cpp b/modules/skottie/src/SkottieTest.cpp
index 61b08ad..72069d5 100644
--- 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 @@
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 @@
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 @@
// 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 @@
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
index 172d213..bd2921f 100644
--- a/modules/skottie/src/text/SkottieShaper.cpp
+++ b/modules/skottie/src/text/SkottieShaper.cpp
@@ -7,10 +7,10 @@
#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>
@@ -18,31 +18,6 @@
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 @@
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,14 +100,14 @@
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());
@@ -131,21 +115,26 @@
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 @@
}
}
+ if (shaped_height) {
+ *shaped_height = content_height;
+ }
+
return std::move(fResult);
}
@@ -248,14 +241,17 @@
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 @@
}
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 @@
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 @@
: 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
index e419e6d..a919e43 100644
--- a/modules/skottie/src/text/SkottieShaper.h
+++ b/modules/skottie/src/text/SkottieShaper.h
@@ -33,8 +33,6 @@
struct Result {
std::vector<Fragment> fFragments;
-
- SkRect computeBounds() const;
};
enum class VAlign : uint8_t {
