New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[Impeller] Add STB text backend. #44887

Merged

bdero merged 12 commits into flutter:main from bdero:bdero/add-stb-text-backend

Aug 23, 2023

Member

bdero commented Aug 21, 2023 •

edited

Loading

Pull the STB text backend into the engine from impeller-cmake.

Depends on: #44884
Requires buildroot bump: flutter/buildroot#754

Originally authored by @johnoneil in bdero/impeller-cmake#12

Diff: f39f4e9

Upgrade past recent text refactors.
Engine-fy the style.
Switch the rect packer to our new skyline rect packer.
Add a TextFrame shaper factory that uses STB's metrics/kerning. Behaves similarly to Skia text blob conversion.
Add an Aiks golden.

bdero added the e: impeller label

bdero requested review from chinmaygarde and zanderso

August 21, 2023 04:55

bdero self-assigned this

bdero mentioned this pull request

[Impeller] Add testonly STB config flutter/buildroot#754

Merged

Member Author

bdero commented Aug 21, 2023 •

edited

Loading

The golden reproduces the slight alignment issues that @johnoneil showed us, and so we should be able to get it nailed down pretty easily from here.

One thing that's gone is the crunchiness that looked like we were skipping over pixel columns when sampling, but I suspect that's thanks to #40912.

It's hard to see in the screenshot, but some characters have a pixel cut off on right and/or bottom (see the o for example), and sometimes characters are jumping 1 pixel above the baseline.

The cutoff issue appears to be an issue while the glyphs themselves are being rendered to the atlas, since the cutoff is also visible in the atlas. Not sure if the bouncing around the baseline is an issue with the text shaping routine I added (https://github.com/flutter/engine/pull/44887/files#diff-9074fefbe9c66b53a5be2a73f54e7f92ddf6f579889a63df5c8f3fadde62bec6R11-R59) or something else.

bdero force-pushed the bdero/add-stb-text-backend branch from cc8b996 to 077d879 Compare

August 21, 2023 16:40

flutter-dashboard bot commented Aug 21, 2023

Golden file changes have been found for this pull request. Click here to view and triage (e.g. because this is an intentional change).

If you are still iterating on this change and are not ready to resolve the images on the Flutter Gold dashboard, consider marking this PR as a draft pull request above. You will still be able to view image results on the dashboard, commenting will be silenced, and the check will not try to resolve itself until marked ready for review.

Changes reported for pull request #44887 at sha 077d879

flutter-dashboard bot added the will affect goldens label

chinmaygarde reviewed

View reviewed changes

impeller/golden_tests/golden_playground_test.h Outdated

-                bool OpenPlaygroundHere(const Picture& picture);
+                bool OpenPlaygroundHere(const Picture& picture,
+                                        std::unique_ptr<TextRenderContext>
+                                            text_render_context_override = nullptr);

Member

chinmaygarde Aug 21, 2023

Same comment as the other patch about making these be shared pointers to text render contexts and having a setter on the fixture itself.

Member Author

bdero Aug 22, 2023

Done.

impeller/typographer/backends/stb/text_render_context_stb.h Outdated


		namespace impeller {

		class TextRenderContextSTB : public TextRenderContext {

Member

chinmaygarde Aug 21, 2023

Can we call these TypographerContexts? Like AiksContext, RendererContext, etc.. That is, the component name is the prefix with Context.

Member Author

bdero Aug 22, 2023

Sure, done.

impeller/typographer/backends/stb/text_render_context_stb.cc Outdated

+                size_t GetSize() const { return width_ * height_ * bytes_per_pixel_; }
+               private:
+                size_t width_;

Member

chinmaygarde Aug 21, 2023

Here and elsewhere, zero these out?

Member Author

bdero Aug 22, 2023

Done.

impeller/typographer/backends/stb/text_render_context_stb.cc Outdated

+                size_t width_;
+                size_t height_;
+                size_t bytes_per_pixel_;
+                std::unique_ptr<uint8_t[]> pixels_;

Member

chinmaygarde Aug 21, 2023

Why not just std::vector<uint8_t>?

Member Author

bdero Aug 22, 2023

Done.

impeller/typographer/backends/stb/text_render_context_stb.cc Outdated

+              // Analogous to the bitmaps (one for each type) stored in each Atlas context.
+              static auto alpha_bitmap =
+                  std::make_shared<STBBitmap>(MIN_GLYPH_ATLAS_SIZE, MIN_GLYPH_ATLAS_SIZE, 1);

Member

chinmaygarde Aug 21, 2023

Here and elsewhere, constants follow the kMinGlyphAtlasSize convention.

Member Author

bdero Aug 22, 2023

Removed these macros.

impeller/typographer/backends/stb/text_render_context_stb.cc Outdated

+                                                                     MIN_GLYPH_ATLAS_SIZE,
+                                                                     COLOR_FONT_BPP);
+              static std::shared_ptr<STBBitmap> get_atlas_bitmap(

Member

chinmaygarde Aug 21, 2023

Nit: PascalCase per style gyide.

Member

chinmaygarde Aug 21, 2023

Here and elsewhere.

Member Author

bdero Aug 22, 2023

Ended up removing these along with the globals they're accessing.

impeller/typographer/backends/stb/text_render_context_stb.cc Outdated

+              // Analogous to the bitmaps (one for each type) stored in each Atlas context.
+              static auto alpha_bitmap =
+                  std::make_shared<STBBitmap>(MIN_GLYPH_ATLAS_SIZE, MIN_GLYPH_ATLAS_SIZE, 1);
+              static auto color_bitmap = std::make_shared<STBBitmap>(MIN_GLYPH_ATLAS_SIZE,

Member

chinmaygarde Aug 21, 2023

Nit: gColorBitmap per convention.

Member

chinmaygarde Aug 21, 2023

On second thought, I don't think we have static globals elsewhere for objects as large as these. These look like leaks and on engine teardown and are hard to track down. Can we do this API right™?

Member Author

bdero Aug 22, 2023

Done. Yeah, it turns out the GlyphAtlasContext still had an SkBitmap. I suspect that's the reason these were globals.

I reworked it to make backend-specific versions and got rid of the static stuff, so now both backends are storing their bitmap data in the atlas context.

impeller/typographer/backends/stb/text_render_context_stb.cc Outdated

+              // Function returns the count of "remaining pairs" not packed into rect of given
+              // size.
+              static size_t PairsFitInAtlasOfSize(

Member

chinmaygarde Aug 21, 2023

We really need a generic rect packer ready to go in //impeller/core. Can we file an issue for this?

Member Author

bdero Aug 22, 2023

flutter/flutter#133028

impeller/typographer/backends/stb/text_render_context_stb.cc Outdated

+                return true;
+              }
+              namespace {

Member

chinmaygarde Aug 21, 2023

static instead of anonymous namespace

Member Author

bdero Aug 22, 2023

Done.

impeller/typographer/backends/stb/text_render_context_stb.cc Outdated

+                return texture;
+              }
+              std::shared_ptr<GlyphAtlas> TextRenderContextSTB::CreateGlyphAtlas(

Member

chinmaygarde Aug 21, 2023

In a later patch, we can dry this up with the Skia typographer context. Can we file a followup issue?

Member Author

bdero Aug 22, 2023

True, especially now that these globals aren't floating around. flutter/flutter#133029

bdero added a commit to flutter/buildroot that referenced this pull request


          [Impeller] Add testonly STB config (#754)

f91786b

For flutter/engine#44887.

bdero force-pushed the bdero/add-stb-text-backend branch from 077d879 to bb9ebf7 Compare

August 22, 2023 05:00

bdero mentioned this pull request

[Impeller] Move the skyline rect packer in typographer to core. flutter/flutter#133028

Open

bdero requested a review from chinmaygarde

August 22, 2023 06:53

bdero added 10 commits

August 22, 2023 10:38


          [Impeller] Add STB text backend.

adc2cca


          Engine-fy, upgrade to head, add TextFrame shaper factory, and add Aik…

bb24c8d

…s test


          Licenses

49ad67b


          Buildroot bump

fc4058f


          Remove todo

cd10c10


          Licenses 2

01d0122


          Rebase

bd1ee8b


          Rename TextRenderContext to TypographerContext

8bf1bc5


          Address comments


          Update buildroot to HEAD

c6d7d87

bdero force-pushed the bdero/add-stb-text-backend branch from 2e510ec to c6d7d87 Compare

August 22, 2023 17:39

Member Author

bdero commented Aug 22, 2023

Need to work out a new segfault happening in the tests.


          Fix segv

8351bf0

Member Author

bdero commented Aug 23, 2023

Fixed. :)

chinmaygarde approved these changes

View reviewed changes

Member

chinmaygarde left a comment

Not familiar with stb but did my best. Some nits we can address later if need be. But LGTM.

impeller/typographer/backends/stb/typeface_stb.cc

+                    is_valid_(false) {
+                // We need an "offset" into the ttf file
+                auto offset = stbtt_GetFontOffsetForIndex(typeface_mapping_->GetMapping(), 0);
+                if (stbtt_InitFont(font_info_.get(), typeface_mapping_->GetMapping(),

Member

chinmaygarde Aug 23, 2023

Is there a de-init font? Is this a leak?

Member Author

bdero Aug 23, 2023

Nah, it just initializes the stbtt_fontinfo (first param).

Docstring:

// Given an offset into the file that defines a font, this function builds
// the necessary cached info for the rest of the system. You must allocate
// the stbtt_fontinfo yourself, and stbtt_InitFont will fill it out. You don't
// need to do anything special to free it, because the contents are pure
// value data with no additional data structures. Returns 0 on failure.

impeller/typographer/backends/stb/typeface_stb.cc Outdated


		namespace impeller {

		TypefaceSTB::~TypefaceSTB() = default;

Member

chinmaygarde Aug 23, 2023

dtor after ctor.

Member Author

bdero Aug 23, 2023

Done.

impeller/typographer/backends/stb/typeface_stb.h Outdated

+                // This assumes a constant pixels per em.
+                static constexpr float kPointsToPixels = 96.0 / 72.0;
+                TypefaceSTB() = delete;

Member

chinmaygarde Aug 23, 2023

ool this dtor.

Member Author

bdero Aug 23, 2023

Removed this because it's already implicitly deleted.


          Address comments

dad7d3d

bdero added the autosubmit label

bdero merged commit d3172a0 into flutter:main

27 checks passed

engine-flutter-autoroll mentioned this pull request

Roll Flutter Engine from 58dc868c26cb to be7c280b0e12 (13 revisions) flutter/flutter#133205

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request


          d3172a072 [Impeller] Add STB text backend. (flutter/engine#44887)

0dd46e5

engine-flutter-autoroll mentioned this pull request

Roll Flutter Engine from 58dc868c26cb to 27d75f6221d2 (14 revisions) flutter/flutter#133211

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request


          d3172a072 [Impeller] Add STB text backend. (flutter/engine#44887)

auto-submit bot pushed a commit to flutter/flutter that referenced this pull request


          Roll Flutter Engine from 58dc868c26cb to 27d75f6221d2 (14 revisions) (#…

cfe41a6

…133211)

flutter/engine@58dc868...27d75f6

2023-08-23 skia-flutter-autoroll@skia.org Roll Skia from 76898dad9fda to a631fefdba37 (2 revisions) (flutter/engine#45027)
2023-08-23 zanderso@users.noreply.github.com Revert "FontVariation.lerp, custom FontVariation constructors, and more documentation" (flutter/engine#45023)
2023-08-23 bdero@google.com [Impeller] Dat rvalue reference (fix engine head) (flutter/engine#45024)
2023-08-23 bdero@google.com Revert "Enable clang-tidy for pre-push (opt-out), exclude `performance-unnecessary-value-param`" (flutter/engine#45020)
2023-08-23 skia-flutter-autoroll@skia.org Roll Skia from 4e42b51cfe27 to 76898dad9fda (1 revision) (flutter/engine#45019)
2023-08-23 bdero@google.com [Impeller] Add STB text backend. (flutter/engine#44887)
2023-08-23 reidbaker@google.com Followup to flutter/engine#44982 (flutter/engine#45018)
2023-08-23 skia-flutter-autoroll@skia.org Roll Skia from 5428f147e632 to 4e42b51cfe27 (4 revisions) (flutter/engine#45016)
2023-08-23 reidbaker@google.com Eliminate android test log spam (flutter/engine#44982)
2023-08-23 mdebbar@google.com [web] Remove some unused functions (flutter/engine#44505)
2023-08-23 lhkbob@gmail.com Use decal TileMode in blur image_filter_test.dart (flutter/engine#45004)
2023-08-23 ian@hixie.ch FontVariation.lerp, custom FontVariation constructors, and more documentation (flutter/engine#44996)
2023-08-23 jonahwilliams@google.com [impeller] combine sampler and texture maps. (flutter/engine#44990)
2023-08-23 bdero@google.com [Impeller] Flutter GPU: Add HostBuffer. (flutter/engine#44696)

If this roll has caused a breakage, revert this CL and stop the roller
using the controls here:
https://autoroll.skia.org/r/flutter-engine-flutter-autoroll
Please CC jimgraham@google.com,rmistry@google.com,zra@google.com on the revert to ensure that a human
is aware of the problem.

To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose

To report a problem with the AutoRoller itself, please file a bug:
https://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

reidbaker mentioned this pull request

Update to very new gradle version #45124

Closed

8 tasks

gaaclarke pushed a commit to gaaclarke/engine that referenced this pull request


          [Impeller] Add STB text backend. (flutter#44887)

Pull the STB text backend into the engine from impeller-cmake.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

autosubmit e: impeller will affect goldens