Reland "[skottie] More snug kTop Shaper alignment"
This reverts commit 80658204f417f90711900b2c761d04f9e835c928.
Reason for revert: <INSERT REASONING HERE>
Original change's description:
> Revert "[skottie] More snug kTop Shaper alignment"
>
> This reverts commit 7ef3e145160fbe99223249b3cff74fb64df6e7e7.
>
> Reason for revert: TSAN failures (HB?)
>
> Original change's description:
> > [skottie] More snug kTop Shaper alignment
> >
> > The current implementation relies on SkShaper ascent values for top
> > text box alignment, but the results are not as visually accurate as AE
> > (or Lottie).
> >
> > Use the computed tight bounds instead.
> >
> > Change-Id: I4447a834fe3cae398fc887766daa68802e7f50a5
> > Reviewed-on: https://skia-review.googlesource.com/c/skia/+/206684
> > Reviewed-by: Ben Wagner <bungeman@google.com>
> > Commit-Queue: Florin Malita <fmalita@chromium.org>
>
> TBR=bungeman@google.com,fmalita@chromium.org
>
> Change-Id: Icdcca2f6a7b33c8366c1118be2e842ff3978c8bd
> No-Presubmit: true
> No-Tree-Checks: true
> No-Try: true
> Reviewed-on: https://skia-review.googlesource.com/c/skia/+/206911
> Reviewed-by: Florin Malita <fmalita@chromium.org>
> Commit-Queue: Florin Malita <fmalita@chromium.org>
TBR=bungeman@google.com,fmalita@chromium.org
Change-Id: I8b72a30b7c2d6d6918f768e59e5d7c987ed5c147
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/207300
Reviewed-by: Florin Malita <fmalita@chromium.org>
Reviewed-by: Ben Wagner <bungeman@google.com>
Commit-Queue: Florin Malita <fmalita@chromium.org>
diff --git a/modules/skottie/src/SkottieAdapter.cpp b/modules/skottie/src/SkottieAdapter.cpp
index 6c5804e..097d22e 100644
--- a/modules/skottie/src/SkottieAdapter.cpp
+++ b/modules/skottie/src/SkottieAdapter.cpp
@@ -470,6 +470,24 @@
};
const auto shape_result = Shaper::Shape(fText.fText, text_desc, fText.fBox);
+#if (0)
+ // Enable for text box debugging/visualization.
+ auto box_color = sksg::Color::Make(0xffff0000);
+ box_color->setStyle(SkPaint::kStroke_Style);
+ box_color->setStrokeWidth(1);
+ box_color->setAntiAlias(true);
+
+ auto bounds_color = sksg::Color::Make(0xff00ff00);
+ bounds_color->setStyle(SkPaint::kStroke_Style);
+ bounds_color->setStrokeWidth(1);
+ bounds_color->setAntiAlias(true);
+
+ fRoot->addChild(sksg::Draw::Make(sksg::Rect::Make(fText.fBox),
+ std::move(box_color)));
+ fRoot->addChild(sksg::Draw::Make(sksg::Rect::Make(shape_result.computeBounds()),
+ std::move(bounds_color)));
+#endif
+
fTextNode->setBlob(shape_result.fBlob);
fTextNode->setPosition(shape_result.fPos);
diff --git a/modules/skottie/src/SkottieShaper.cpp b/modules/skottie/src/SkottieShaper.cpp
index 782197f..d801455 100644
--- a/modules/skottie/src/SkottieShaper.cpp
+++ b/modules/skottie/src/SkottieShaper.cpp
@@ -73,9 +73,7 @@
fMaxRunLeading = SkTMax(fMaxRunLeading, metrics.fLeading);
}
- void commitRunInfo() override {
- fCurrentPosition.fY -= fMaxRunAscent;
- }
+ void commitRunInfo() override {}
Buffer runBuffer(const RunInfo& info) override {
int glyphCount = SkTFitsIn<int>(info.glyphCount) ? info.glyphCount : INT_MAX;
@@ -102,11 +100,6 @@
void commitLine() override {
fOffset += { 0, fMaxRunDescent + fMaxRunLeading - fMaxRunAscent };
-
- // Grab the first line ascent for vertical alignment.
- if (SkScalarIsNaN(fFirstLineAscent)) {
- fFirstLineAscent = fMaxRunAscent;
- }
}
Shaper::Result makeBlob() {
@@ -114,12 +107,14 @@
SkPoint pos {fBox.x(), fBox.y()};
+ // By default, first line is vertical-aligned on a baseline of 0.
+ // Perform additional adjustments based on VAlign.
switch (fDesc.fVAlign) {
- case Shaper::VAlign::kTop:
- // Nothing to do (SkShaper default behavior).
- break;
+ case Shaper::VAlign::kTop: {
+ pos.offset(0, -ComputeBlobBounds(blob).fTop);
+ } break;
case Shaper::VAlign::kTopBaseline:
- pos.offset(0, fFirstLineAscent);
+ // Default behavior.
break;
case Shaper::VAlign::kCenter: {
const auto bounds = ComputeBlobBounds(blob).makeOffset(pos.x(), pos.y());
@@ -185,7 +180,6 @@
SkTextBlobBuilder fBuilder;
std::unique_ptr<SkShaper> fShaper;
- SkScalar fFirstLineAscent = SK_ScalarNaN;
SkScalar fMaxRunAscent;
SkScalar fMaxRunDescent;
SkScalar fMaxRunLeading;
diff --git a/modules/skottie/src/SkottieShaper.h b/modules/skottie/src/SkottieShaper.h
index 7005fe9..816141d 100644
--- a/modules/skottie/src/SkottieShaper.h
+++ b/modules/skottie/src/SkottieShaper.h
@@ -30,7 +30,7 @@
};
enum class VAlign : uint8_t {
- // Align the first line ascent with the text box top.
+ // Align the first line visual top with the text box top.
kTop,
// Align the first line baseline with the text box top.
kTopBaseline,
diff --git a/modules/skottie/src/SkottieTest.cpp b/modules/skottie/src/SkottieTest.cpp
index 4b550c7..34a4073 100644
--- a/modules/skottie/src/SkottieTest.cpp
+++ b/modules/skottie/src/SkottieTest.cpp
@@ -222,7 +222,7 @@
REPORTER_ASSERT(reporter, std::get<2>(observer->fMarkers[1]) == 0.75f);
}
-DEF_TEST(Skottie_Shaper, reporter) {
+DEF_TEST(Skottie_Shaper_HAlign, reporter) {
auto typeface = SkTypeface::MakeDefault();
REPORTER_ASSERT(reporter, typeface);
@@ -281,3 +281,64 @@
}
}
}
+
+DEF_TEST(Skottie_Shaper_VAlign, reporter) {
+ auto typeface = SkTypeface::MakeDefault();
+ REPORTER_ASSERT(reporter, typeface);
+
+ static constexpr struct {
+ SkScalar text_size,
+ tolerance;
+ } kTestSizes[] = {
+ // 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 },
+ };
+
+ struct {
+ skottie::Shaper::VAlign align;
+ SkScalar topFactor;
+ } kTestAligns[] = {
+ { skottie::Shaper::VAlign::kTop , 0.0f },
+ { skottie::Shaper::VAlign::kCenter, 0.5f },
+ // TODO: any way to test kTopBaseline?
+ };
+
+ const SkString text("Foo, bar.\rBaz.");
+ const auto text_box = SkRect::MakeXYWH(100, 100, 1000, 1000); // large-enough to avoid breaks.
+
+
+ for (const auto& tsize : kTestSizes) {
+ for (const auto& talign : kTestAligns) {
+ const skottie::Shaper::TextDesc desc = {
+ typeface,
+ tsize.text_size,
+ SkTextUtils::Align::kCenter_Align,
+ talign.align,
+ };
+
+ const auto shape_result = skottie::Shaper::Shape(text, desc, text_box);
+ REPORTER_ASSERT(reporter, shape_result.fBlob);
+
+ const auto shape_bounds = shape_result.computeBounds();
+ REPORTER_ASSERT(reporter, !shape_bounds.isEmpty());
+
+ const auto v_diff = text_box.height() - shape_bounds.height();
+
+ const auto expected_t = text_box.top() + v_diff * talign.topFactor;
+ REPORTER_ASSERT(reporter,
+ std::fabs(shape_bounds.top() - expected_t) < tsize.tolerance,
+ "%f %f %f %f %d", shape_bounds.top(), expected_t, tsize.tolerance,
+ tsize.text_size, SkToU32(talign.align));
+
+ const auto expected_b = text_box.bottom() - v_diff * (1 - talign.topFactor);
+ REPORTER_ASSERT(reporter,
+ std::fabs(shape_bounds.bottom() - expected_b) < tsize.tolerance,
+ "%f %f %f %f %d", shape_bounds.bottom(), expected_b, tsize.tolerance,
+ tsize.text_size, SkToU32(talign.align));
+ }
+ }
+}
