From b80da4498df0b3b1a69e2b2020c8cd8b67fd3e25 Mon Sep 17 00:00:00 2001 From: gaaclarke <30870216+gaaclarke@users.noreply.github.com> Date: Wed, 29 Jan 2025 08:31:47 -0800 Subject: [PATCH] Revert "Started adjusting uvs to match pixel snapping. (#162049)" This reverts commit dc580aff7885d4848960d658ed63345e4838e1c6. --- .../flutter/ci/licenses_golden/excluded_files | 1 - .../flutter/impeller/display_list/BUILD.gn | 4 - .../display_list/aiks_dl_text_unittests.cc | 28 +++---- .../display_list/dl_golden_blur_unittests.cc | 79 ++++++++++++++++++- .../display_list/dl_golden_unittests.cc | 64 --------------- .../testing/render_text_in_canvas.cc | 43 ---------- .../testing/render_text_in_canvas.h | 34 -------- .../impeller/display_list/testing/rmse.cc | 47 ----------- .../impeller/display_list/testing/rmse.h | 16 ---- .../impeller/entity/contents/text_contents.cc | 33 +++----- 10 files changed, 95 insertions(+), 254 deletions(-) delete mode 100644 engine/src/flutter/impeller/display_list/testing/render_text_in_canvas.cc delete mode 100644 engine/src/flutter/impeller/display_list/testing/render_text_in_canvas.h delete mode 100644 engine/src/flutter/impeller/display_list/testing/rmse.cc delete mode 100644 engine/src/flutter/impeller/display_list/testing/rmse.h diff --git a/engine/src/flutter/ci/licenses_golden/excluded_files b/engine/src/flutter/ci/licenses_golden/excluded_files index 3d9a1de31f9..1b9049d5992 100644 --- a/engine/src/flutter/ci/licenses_golden/excluded_files +++ b/engine/src/flutter/ci/licenses_golden/excluded_files @@ -152,7 +152,6 @@ ../../../flutter/impeller/display_list/dl_golden_unittests.h ../../../flutter/impeller/display_list/dl_unittests.cc ../../../flutter/impeller/display_list/skia_conversions_unittests.cc -../../../flutter/impeller/display_list/testing ../../../flutter/impeller/docs ../../../flutter/impeller/entity/clip_stack_unittests.cc ../../../flutter/impeller/entity/contents/filters/blend_filter_contents_unittests.cc diff --git a/engine/src/flutter/impeller/display_list/BUILD.gn b/engine/src/flutter/impeller/display_list/BUILD.gn index 3c4f1aefc8e..d52f73ea76a 100644 --- a/engine/src/flutter/impeller/display_list/BUILD.gn +++ b/engine/src/flutter/impeller/display_list/BUILD.gn @@ -78,10 +78,6 @@ template("display_list_unittests_component") { "dl_playground.cc", "dl_playground.h", "dl_unittests.cc", - "testing/render_text_in_canvas.cc", - "testing/render_text_in_canvas.h", - "testing/rmse.cc", - "testing/rmse.h", ] additional_sources = [] if (defined(invoker.sources)) { diff --git a/engine/src/flutter/impeller/display_list/aiks_dl_text_unittests.cc b/engine/src/flutter/impeller/display_list/aiks_dl_text_unittests.cc index 0d03982a35d..f51f56d08e4 100644 --- a/engine/src/flutter/impeller/display_list/aiks_dl_text_unittests.cc +++ b/engine/src/flutter/impeller/display_list/aiks_dl_text_unittests.cc @@ -152,27 +152,17 @@ TEST_P(AiksTest, CanRenderTextFrameWithHalfScaling) { } TEST_P(AiksTest, CanRenderTextFrameWithFractionScaling) { - Scalar fine_scale = 0.f; - auto callback = [&]() -> sk_sp { - if (AiksTest::ImGuiBegin("Controls", nullptr, - ImGuiWindowFlags_AlwaysAutoResize)) { - ImGui::SliderFloat("Fine Scale", &fine_scale, -1, 1); - ImGui::End(); - } + DisplayListBuilder builder; - DisplayListBuilder builder; - DlPaint paint; - paint.setColor(DlColor::ARGB(1, 0.1, 0.1, 0.1)); - builder.DrawPaint(paint); - Scalar scale = 2.625 + fine_scale; - builder.Scale(scale, scale); - RenderTextInCanvasSkia(GetContext(), builder, - "the quick brown fox jumped over the lazy dog!.?", - "Roboto-Regular.ttf"); - return builder.Build(); - }; + DlPaint paint; + paint.setColor(DlColor::ARGB(1, 0.1, 0.1, 0.1)); + builder.DrawPaint(paint); + builder.Scale(2.625, 2.625); - ASSERT_TRUE(OpenPlaygroundHere(callback)); + ASSERT_TRUE(RenderTextInCanvasSkia( + GetContext(), builder, "the quick brown fox jumped over the lazy dog!.?", + "Roboto-Regular.ttf")); + ASSERT_TRUE(OpenPlaygroundHere(builder.Build())); } TEST_P(AiksTest, TextFrameSubpixelAlignment) { diff --git a/engine/src/flutter/impeller/display_list/dl_golden_blur_unittests.cc b/engine/src/flutter/impeller/display_list/dl_golden_blur_unittests.cc index 9b42a4d1bff..b070667adf4 100644 --- a/engine/src/flutter/impeller/display_list/dl_golden_blur_unittests.cc +++ b/engine/src/flutter/impeller/display_list/dl_golden_blur_unittests.cc @@ -6,18 +6,58 @@ #include "flutter/display_list/dl_builder.h" #include "flutter/display_list/effects/dl_mask_filter.h" -#include "flutter/impeller/display_list/testing/render_text_in_canvas.h" -#include "flutter/impeller/display_list/testing/rmse.h" #include "flutter/impeller/geometry/round_rect.h" #include "flutter/impeller/golden_tests/screenshot.h" #include "flutter/testing/testing.h" #include "gtest/gtest.h" +#include "impeller/typographer/backends/skia/text_frame_skia.h" +#include "txt/platform.h" namespace flutter { namespace testing { using impeller::Font; +namespace { +struct TextRenderOptions { + bool stroke = false; + DlScalar font_size = 50; + DlColor color = DlColor::kYellow(); + std::shared_ptr mask_filter; +}; + +bool RenderTextInCanvasSkia(DlCanvas* canvas, + const std::string& text, + const std::string_view& font_fixture, + DlPoint position, + const TextRenderOptions& options = {}) { + auto c_font_fixture = std::string(font_fixture); + auto mapping = flutter::testing::OpenFixtureAsSkData(c_font_fixture.c_str()); + if (!mapping) { + return false; + } + sk_sp font_mgr = txt::GetDefaultFontManager(); + SkFont sk_font(font_mgr->makeFromData(mapping), options.font_size); + auto blob = SkTextBlob::MakeFromString(text.c_str(), sk_font); + if (!blob) { + return false; + } + + auto frame = impeller::MakeTextFrameFromTextBlobSkia(blob); + + DlPaint text_paint; + text_paint.setColor(options.color); + text_paint.setMaskFilter(options.mask_filter); + // text_paint.mask_blur_descriptor = options.mask_blur_descriptor; + // text_paint.stroke_width = 1; + // text_paint.style = + // options.stroke ? Paint::Style::kStroke : Paint::Style::kFill; + canvas->DrawTextFrame(frame, position.x, position.y, text_paint); + return true; +} + +} // namespace + TEST_P(DlGoldenTest, TextBlurMaskFilterRespectCTM) { impeller::Point content_scale = GetContentScale(); auto draw = [&](DlCanvas* canvas, @@ -72,6 +112,41 @@ TEST_P(DlGoldenTest, TextBlurMaskFilterDisrespectCTM) { ASSERT_TRUE(OpenPlaygroundHere(builder.Build())); } +namespace { +double CalculateDistance(const uint8_t* left, const uint8_t* right) { + double diff[4] = { + static_cast(left[0]) - right[0], // + static_cast(left[1]) - right[1], // + static_cast(left[2]) - right[2], // + static_cast(left[3]) - right[3] // + }; + return sqrt((diff[0] * diff[0]) + // + (diff[1] * diff[1]) + // + (diff[2] * diff[2]) + // + (diff[3] * diff[3])); +} + +double RMSE(const impeller::testing::Screenshot* left, + const impeller::testing::Screenshot* right) { + FML_CHECK(left); + FML_CHECK(right); + FML_CHECK(left->GetWidth() == right->GetWidth()); + FML_CHECK(left->GetHeight() == right->GetHeight()); + + int64_t samples = left->GetWidth() * left->GetHeight(); + double tally = 0; + + const uint8_t* left_ptr = left->GetBytes(); + const uint8_t* right_ptr = right->GetBytes(); + for (int64_t i = 0; i < samples; ++i, left_ptr += 4, right_ptr += 4) { + double distance = CalculateDistance(left_ptr, right_ptr); + tally += distance * distance; + } + + return sqrt(tally / static_cast(samples)); +} +} // namespace + // This is a test to make sure that we don't regress "shimmering" in the // gaussian blur. Shimmering is abrupt changes in signal when making tiny // changes to the blur parameters. diff --git a/engine/src/flutter/impeller/display_list/dl_golden_unittests.cc b/engine/src/flutter/impeller/display_list/dl_golden_unittests.cc index 2b149f6135b..f3f3577eff3 100644 --- a/engine/src/flutter/impeller/display_list/dl_golden_unittests.cc +++ b/engine/src/flutter/impeller/display_list/dl_golden_unittests.cc @@ -8,8 +8,6 @@ #include "display_list/dl_paint.h" #include "display_list/geometry/dl_geometry_types.h" #include "flutter/display_list/dl_builder.h" -#include "flutter/impeller/display_list/testing/render_text_in_canvas.h" -#include "flutter/impeller/display_list/testing/rmse.h" #include "flutter/impeller/geometry/path_builder.h" #include "flutter/testing/testing.h" #include "gtest/gtest.h" @@ -352,67 +350,5 @@ TEST_P(DlGoldenTest, SaveLayerAtFractionalValue) { ASSERT_TRUE(OpenPlaygroundHere(builder.Build())); } -namespace { -int32_t CalculateMaxY(const impeller::testing::Screenshot* img) { - const uint32_t* ptr = reinterpret_cast(img->GetBytes()); - int32_t max_y = 0; - for (uint32_t i = 0; i < img->GetHeight(); ++i) { - for (uint32_t j = 0; j < img->GetWidth(); ++j) { - uint32_t pixel = *ptr++; - if ((pixel & 0x00ffffff) != 0) { - max_y = std::max(max_y, static_cast(i)); - } - } - } - return max_y; -} -} // namespace - -// This test makes sure that given a tiny change in scale, a glyph will not do a -// large jump in its drawn y position. This was noticed when performing -// animations as a problematic artifact. We tried to come up with a more -// holistic quantification of the problem but haven't yet been able to. -TEST_P(DlGoldenTest, TextJumpingTest) { - SetWindowSize(impeller::ISize(1024, 200)); - impeller::Scalar font_size = 300; - auto callback = [&](impeller::Scalar scale) -> sk_sp { - DisplayListBuilder builder; - DlPaint paint; - paint.setColor(DlColor::ARGB(1, 0, 0, 0)); - builder.DrawPaint(paint); - builder.Scale(scale, scale); - // If you move this code to a playgrounds test the RenderTextInCanvasSkia - // signature is a bit different there, it will look like this: - // - // RenderTextInCanvasSkia(GetContext(), builder, - // "the quick brown fox jumped over the lazy dog!.?", - // "Roboto-Regular.ttf", - // TextRenderOptions{ - // .font_size = font_size, - // .position = SkPoint::Make(100, 300), - // }); - // Note: The ahem font just has full blocks in it. - RenderTextInCanvasSkia(&builder, "h", "Roboto-Regular.ttf", - DlPoint::MakeXY(10, 300), - TextRenderOptions{ - .font_size = font_size, - }); - return builder.Build(); - }; - - std::unique_ptr right = - MakeScreenshot(callback(0.445)); - if (!right) { - GTEST_SKIP() << "making screenshots not supported."; - } - std::unique_ptr left = - MakeScreenshot(callback(0.444)); - - int32_t left_max_y = CalculateMaxY(left.get()); - int32_t right_max_y = CalculateMaxY(right.get()); - int32_t y_diff = std::abs(left_max_y - right_max_y); - EXPECT_TRUE(y_diff <= 1) << "y diff: " << y_diff; -} - } // namespace testing } // namespace flutter diff --git a/engine/src/flutter/impeller/display_list/testing/render_text_in_canvas.cc b/engine/src/flutter/impeller/display_list/testing/render_text_in_canvas.cc deleted file mode 100644 index aca644d3f2b..00000000000 --- a/engine/src/flutter/impeller/display_list/testing/render_text_in_canvas.cc +++ /dev/null @@ -1,43 +0,0 @@ -// Copyright 2013 The Flutter Authors. All rights reserved. -// Use of this source code is governed by a BSD-style license that can be -// found in the LICENSE file. - -#include "impeller/display_list/testing/render_text_in_canvas.h" - -#include "flutter/testing/testing.h" -#include "txt/platform.h" - -namespace flutter { -namespace testing { - -bool RenderTextInCanvasSkia(DlCanvas* canvas, - const std::string& text, - const std::string_view& font_fixture, - DlPoint position, - const TextRenderOptions& options) { - auto c_font_fixture = std::string(font_fixture); - auto mapping = flutter::testing::OpenFixtureAsSkData(c_font_fixture.c_str()); - if (!mapping) { - return false; - } - sk_sp font_mgr = txt::GetDefaultFontManager(); - SkFont sk_font(font_mgr->makeFromData(mapping), options.font_size); - auto blob = SkTextBlob::MakeFromString(text.c_str(), sk_font); - if (!blob) { - return false; - } - - auto frame = impeller::MakeTextFrameFromTextBlobSkia(blob); - - DlPaint text_paint; - text_paint.setColor(options.color); - text_paint.setMaskFilter(options.mask_filter); - // text_paint.mask_blur_descriptor = options.mask_blur_descriptor; - // text_paint.stroke_width = 1; - // text_paint.style = - // options.stroke ? Paint::Style::kStroke : Paint::Style::kFill; - canvas->DrawTextFrame(frame, position.x, position.y, text_paint); - return true; -} -} // namespace testing -} // namespace flutter diff --git a/engine/src/flutter/impeller/display_list/testing/render_text_in_canvas.h b/engine/src/flutter/impeller/display_list/testing/render_text_in_canvas.h deleted file mode 100644 index cdbf3c2259e..00000000000 --- a/engine/src/flutter/impeller/display_list/testing/render_text_in_canvas.h +++ /dev/null @@ -1,34 +0,0 @@ -// Copyright 2013 The Flutter Authors. All rights reserved. -// Use of this source code is governed by a BSD-style license that can be -// found in the LICENSE file. - -#ifndef FLUTTER_IMPELLER_DISPLAY_LIST_TESTING_RENDER_TEXT_IN_CANVAS_H_ -#define FLUTTER_IMPELLER_DISPLAY_LIST_TESTING_RENDER_TEXT_IN_CANVAS_H_ - -#include - -#include "flutter/display_list/dl_canvas.h" -#include "flutter/display_list/dl_color.h" -#include "flutter/display_list/effects/dl_mask_filter.h" -#include "impeller/typographer/backends/skia/text_frame_skia.h" - -namespace flutter { -namespace testing { - -struct TextRenderOptions { - bool stroke = false; - SkScalar font_size = 50; - DlColor color = DlColor::kYellow(); - std::shared_ptr mask_filter; -}; - -bool RenderTextInCanvasSkia(DlCanvas* canvas, - const std::string& text, - const std::string_view& font_fixture, - DlPoint position, - const TextRenderOptions& options = {}); - -} // namespace testing -} // namespace flutter - -#endif // FLUTTER_IMPELLER_DISPLAY_LIST_TESTING_RENDER_TEXT_IN_CANVAS_H_ diff --git a/engine/src/flutter/impeller/display_list/testing/rmse.cc b/engine/src/flutter/impeller/display_list/testing/rmse.cc deleted file mode 100644 index 45e19c80379..00000000000 --- a/engine/src/flutter/impeller/display_list/testing/rmse.cc +++ /dev/null @@ -1,47 +0,0 @@ -// Copyright 2013 The Flutter Authors. All rights reserved. -// Use of this source code is governed by a BSD-style license that can be -// found in the LICENSE file. - -#include "impeller/display_list/testing/rmse.h" - -#include "flutter/fml/logging.h" - -namespace flutter { -namespace testing { -namespace { -double CalculateDistance(const uint8_t* left, const uint8_t* right) { - double diff[4] = { - static_cast(left[0]) - right[0], // - static_cast(left[1]) - right[1], // - static_cast(left[2]) - right[2], // - static_cast(left[3]) - right[3] // - }; - return sqrt((diff[0] * diff[0]) + // - (diff[1] * diff[1]) + // - (diff[2] * diff[2]) + // - (diff[3] * diff[3])); -} -} // namespace - -double RMSE(const impeller::testing::Screenshot* left, - const impeller::testing::Screenshot* right) { - FML_CHECK(left); - FML_CHECK(right); - FML_CHECK(left->GetWidth() == right->GetWidth()); - FML_CHECK(left->GetHeight() == right->GetHeight()); - - int64_t samples = left->GetWidth() * left->GetHeight(); - double tally = 0; - - const uint8_t* left_ptr = left->GetBytes(); - const uint8_t* right_ptr = right->GetBytes(); - for (int64_t i = 0; i < samples; ++i, left_ptr += 4, right_ptr += 4) { - double distance = CalculateDistance(left_ptr, right_ptr); - tally += distance * distance; - } - - return sqrt(tally / static_cast(samples)); -} - -} // namespace testing -} // namespace flutter diff --git a/engine/src/flutter/impeller/display_list/testing/rmse.h b/engine/src/flutter/impeller/display_list/testing/rmse.h deleted file mode 100644 index aadb8302d85..00000000000 --- a/engine/src/flutter/impeller/display_list/testing/rmse.h +++ /dev/null @@ -1,16 +0,0 @@ -// Copyright 2013 The Flutter Authors. All rights reserved. -// Use of this source code is governed by a BSD-style license that can be -// found in the LICENSE file. - -#ifndef FLUTTER_IMPELLER_DISPLAY_LIST_TESTING_RMSE_H_ -#define FLUTTER_IMPELLER_DISPLAY_LIST_TESTING_RMSE_H_ - -#include "flutter/impeller/golden_tests/screenshot.h" - -namespace flutter { -namespace testing { -double RMSE(const impeller::testing::Screenshot* left, - const impeller::testing::Screenshot* right); -} -} // namespace flutter -#endif // FLUTTER_IMPELLER_DISPLAY_LIST_TESTING_RMSE_H_ diff --git a/engine/src/flutter/impeller/entity/contents/text_contents.cc b/engine/src/flutter/impeller/entity/contents/text_contents.cc index 06ef45ecefb..1835c5b4359 100644 --- a/engine/src/flutter/impeller/entity/contents/text_contents.cc +++ b/engine/src/flutter/impeller/entity/contents/text_contents.cc @@ -74,16 +74,6 @@ void TextContents::SetTextProperties(Color color, } } -namespace { -Matrix MakeRectTransform(Rect from, Rect to) { - return Matrix::MakeTranslation({to.GetLeft(), to.GetTop(), 0}) * - Matrix::MakeScale( - /*s=*/{to.GetWidth() / from.GetWidth(), - to.GetHeight() / from.GetHeight(), 1}) * - Matrix::MakeTranslation({-from.GetLeft(), -from.GetTop(), 0}); -} -} // namespace - void TextContents::ComputeVertexData( VS::PerVertexData* vtx_contents, const std::shared_ptr& frame, @@ -188,25 +178,20 @@ void TextContents::ComputeVertexData( Point screen_glyph_position = (screen_offset + unrounded_glyph_position + subpixel_adjustment) .Floor(); - Vector3 screen_size = basis_transform * scaled_bounds.GetSize(); - - Matrix position_to_uv = MakeRectTransform( - Rect::MakeOriginSize(screen_glyph_position, - Size(screen_size.x, screen_size.y)), - Rect::MakeOriginSize(uv_origin, Size(uv_size.x, uv_size.y))); for (const Point& point : unit_points) { + Point position; if (is_translation_scale) { - vtx.position = (screen_glyph_position + - (basis_transform * point * scaled_bounds.GetSize())) - .Round(); - vtx.uv = position_to_uv * vtx.position; + position = (screen_glyph_position + + (basis_transform * point * scaled_bounds.GetSize())) + .Round(); } else { - vtx.position = entity_transform * - (glyph_position.position + scaled_bounds.GetLeftTop() + - point * scaled_bounds.GetSize()); - vtx.uv = uv_origin + (uv_size * point); + position = entity_transform * + (glyph_position.position + scaled_bounds.GetLeftTop() + + point * scaled_bounds.GetSize()); } + vtx.uv = uv_origin + (uv_size * point); + vtx.position = position; vtx_contents[i++] = vtx; } }