[Impeller] SDF text rendering #36171

dnfield · 2022-09-15T16:23:31Z

No description provided.

jonahwilliams · 2022-09-16T18:25:18Z

impeller/typographer/glyph.h

+  //------------------------------------------------------------------------------
+  /// @brief      Whether the glyph is a path or a bitmap.
+  ///
+  Type type = Type::kPath;


This should be incorporated into equality/hash right?

I... guess? This is specific to the glyph ID though.

yeah....I dunno. As long as we never have glyph collisions its fine

jonahwilliams · 2022-09-16T18:26:13Z

impeller/typographer/glyph_atlas.h

+  /// @brief      Describes how the glyphs are represented in the texture.
+  enum class Type {
+    //--------------------------------------------------------------------------
+    /// The glyphs are represented at a fixed size in an 8-bit grayscale texture


FWIW there are SDF algorithms that use all of the color channels for fewer artifacts.

Yes, but in talking with Chinmay about this he's concerned they're slow and more meant for offline processing.

Maybe we could find a compute based algorithm that would be fast... in which case we'd update this.

I think we'd probably add another enumeration for that too, something like kMultiChannelSignedDistanceField

jonahwilliams · 2022-09-16T18:58:06Z

impeller/typographer/text_run.cc


 bool TextRun::AddGlyph(Glyph glyph, Point position) {
  glyphs_.emplace_back(GlyphPosition{glyph, position});
+  has_color_ |= glyph.type == Glyph::Type::kBitmap;


Its not that they're always bitmaps though. The COLR fonts can be drawn as a path too

Should I rename this to something like is_bitmap or something then?

the underlying is_color is being used to detect both bitmaps and COLR fonts right now though

there was a skia parameter that returned whether or not something was a bitmap but I couldn't figure out how it was initialized, and seemed to always be false even for emoji

dnfield · 2022-09-20T20:53:39Z

This definitely has some issues still, but I think it's good enough to review and land to star tmaking incremental improvements.

Current issues:

We're not picking a strategy for text rendering at this point. [Impeller] Come up with a heuristic/strategy for when to use different types of text rendering strategies. flutter#111640.
The SDF atlas is currently being rendered at the full point size/scale of the glyphs without SDF. Need to figure out a point size to actually use and how to pick that point size back out when we want to use it.
The SDF atlas might not be spacing things out correctly - I don't know that this is causing problems yet but it seems like it might.
SDF text is not getting anti-aliasing. @bdero seems to have ideas about how to make this work.
The SDF algorithm would be really nice to do as a compute algorithm, and in its current state there's enough for me to try to pick that back up and see what I can do with it.

impeller/entity/shaders/glyph_atlas_sdf.frag

jonahwilliams · 2022-09-20T20:57:40Z

impeller/entity/shaders/glyph_atlas_sdf.frag

+in vec2 v_unit_vertex;
+in vec2 v_atlas_position;
+in vec2 v_atlas_glyph_size;
+in float v_color_glyph;


This one isn't used. I think we can't render full color glyphs or bitmaps with SDF anyway so I'd drop it

I think I'd rather just leave this to avoid replicating a bunch of code on the C++ side for now.

Co-authored-by: Jonah Williams <jonahwilliams@google.com>

chinmaygarde · 2022-09-20T21:10:12Z

testing/testing.gni

 # Unit tests targets are only enabled for host machines and Fuchsia right now
 declare_args() {
-  enable_unittests = current_toolchain == host_toolchain || is_fuchsia
+  enable_unittests = current_toolchain == host_toolchain || is_fuchsia || true


Did you mean to stage this?

chinmaygarde · 2022-09-20T21:21:00Z

impeller/entity/contents/text_contents.cc

-
-  if (auto atlas = std::get_if<std::shared_ptr<GlyphAtlas>>(&atlas_)) {
-    return *atlas;
+  FML_DCHECK(lazy_atlas_);


Having the assertion as well as the conditional is redundant. I suggest removing the assert.

The conditional is defensive - the assert is so that we don't get here in a test and return null, which would fail to render text.

chinmaygarde · 2022-09-20T21:23:36Z

impeller/entity/shaders/glyph_atlas_sdf.vert

+  mat4 mvp;
+} frame_info;
+
+in vec2 unit_vertex;


This seems to be shared with the non-SDF glyph atlas implementation. You can put the common stuff in its own header and #include that here and in the other spot too.

TBH, I am surprised so much is shared between the two strategies.

This is very similar. We might be able to simplify it if we move SDF to using a fixed glyph size, but it's probably not worth it. The differences are really in the fragment shader.

Moved this to transforms.glsl

chinmaygarde · 2022-09-20T21:24:34Z

impeller/typographer/backends/skia/text_render_context_skia.cc

  while (auto frame = frame_iterator()) {
    for (const auto& run : frame->GetRuns()) {
      auto font = run.GetFont();
+      // switch (type) {


Isn't a consistent font size desirable for an SDF based strategy?

Oh, I saw your clarifying remark. Let's cross reference a bug here instead of leaving the code commented out.

flutter/flutter#112016, adding TODO

chinmaygarde · 2022-09-20T21:27:29Z

impeller/typographer/backends/skia/text_render_context_skia.cc

+/// Compute signed-distance field for an 8-bpp grayscale image (values greater
+/// than 127 are considered "on") For details of this algorithm, see "The 'dead
+/// reckoning' signed distance transform" [Grevera 2004]
+static void ConvertBitmapToSignedDistanceField(uint8_t* pixels,


Its going to take me a bit to walk through this (and we should totally not block landing this because of it). But, can we fixup the style to match the style guide?

Do you mean the comment bit? I tried to fix up the variable names.

Perhaps you mean the in-out pointer parameter? Doing this in-place avoids an extra allocation the size of the texture though. Maybe there's a more style-guide friendly way of doing that?

chinmaygarde · 2022-09-20T21:33:54Z

impeller/typographer/backends/skia/text_frame_skia.cc

  for (SkTextBlobRunIterator run(blob.get()); !run.done(); run.next()) {
    TextRun text_run(ToFont(run.font(), scale));
+
+    // TODO(jonahwilliams): ask Skia for a public API to look this up.


Can we file a bug for this and cross reference it here? We can followup.

flutter/flutter#112005

Added bug to comment.

chinmaygarde

Some nits. Let's file issues for the remaining action items for followup. This is a great start and let's land iterate on it ASAP.

bdero · 2022-09-20T21:17:06Z

impeller/entity/contents/text_contents.cc

-  cmd.pipeline =
-      renderer.GetGlyphAtlasPipeline(OptionsFromPassAndEntity(pass, entity));
-  cmd.stencil_reference = entity.GetStencilDepth();
+template <class VS_, class FS_>


The template type param can just be the pipeline type itself rather than its shaders (see the advanced blends in BlendFilter for an example of this).

bdero · 2022-09-20T21:18:53Z

impeller/entity/shaders/glyph_atlas_sdf.frag

+  float sample_distance = texture(glyph_atlas_sampler, v_unit_vertex * scale_perspective + offset).a;
+  // // Use local automatic gradients to find anti-aliased anisotropic edge width, cf. Gustavson 2012
+  float edge_width = length(vec2(dFdx(sample_distance), dFdy(sample_distance)));
+  // // Smooth the glyph edge by interpolating across the boundary in a band with the width determined above


Nit: Double double slashes.

bdero · 2022-09-20T21:20:05Z

impeller/entity/shaders/glyph_atlas_sdf.frag

+  // // Smooth the glyph edge by interpolating across the boundary in a band with the width determined above
+  float insideness = smoothstep(edge_distance - edge_width, edge_distance + edge_width, sample_distance);
+  frag_color = frag_info.text_color * insideness;
+}


Nit: IIRC, I think you're using the same VScode plugins as me for shaders. Mind running this through the formatter with Cmd + Alt + L?

I can't seem to get the formatter working :\

bdero · 2022-09-20T21:27:40Z

impeller/entity/shaders/glyph_atlas_sdf.frag

+  // Sample the signed-distance field to find distance from this fragment to the glyph outline
+  float sample_distance = texture(glyph_atlas_sampler, v_unit_vertex * scale_perspective + offset).a;
+  // // Use local automatic gradients to find anti-aliased anisotropic edge width, cf. Gustavson 2012
+  float edge_width = length(vec2(dFdx(sample_distance), dFdy(sample_distance)));


Nice trick -- derivatives work the same whether or not the render pass does multisample resolve (considering MSAA changes nothing about how the fragment stage runs), right?

I.. think so. I heavily borrowed from the Metal by Example shader for SDF text rendering.

bdero · 2022-09-20T21:33:20Z

impeller/entity/shaders/glyph_atlas_sdf.frag

+  // // Use local automatic gradients to find anti-aliased anisotropic edge width, cf. Gustavson 2012
+  float edge_width = length(vec2(dFdx(sample_distance), dFdy(sample_distance)));
+  // // Smooth the glyph edge by interpolating across the boundary in a band with the width determined above
+  float insideness = smoothstep(edge_distance - edge_width, edge_distance + edge_width, sample_distance);


How are you planning to address non-antialiased drawing? Given the template solution, It seems like binding an extra float uniform for all atlas implementations wouldn't be so bad even if unused in some of them.

Yeah. I'm not married to the template solution, but it saved me from copy/pasting a bunch of code that was nearly identical. There's already a branch in there based on the type of glyph atlas, we could add another branch for SDF if needed.

Ahh except that branch doesn't work lol.

Ok, fixed the branching using std::is_same, and we should be able to do that for other parameters we need here ... although at some point it may make sense to stop using templates and just replicate some code or create additional inline helper functions or something.

The actual metal by example drawing seemed to handling some AA here in a way that didn't quite work, it'd probably be worth revisiting.

dnfield · 2022-09-20T23:59:46Z

Marking this for submission, realizing it's not quite complete and will need some further work.

This reverts commit 38fef05.

dnfield added 3 commits September 14, 2022 23:07

baby steps

774b55d

missing callsite

e36a584

merge

e8d48fd

jonahwilliams reviewed Sep 16, 2022

View reviewed changes

chinmaygarde added the e: impeller label Sep 16, 2022

dnfield force-pushed the sdf_text branch from 9f3e4fd to e76e236 Compare September 17, 2022 04:14

A little closer

9d89a4e

dnfield force-pushed the sdf_text branch from e76e236 to 9d89a4e Compare September 17, 2022 04:45

some more

9081484

chinmaygarde assigned dnfield Sep 19, 2022

dnfield added 2 commits September 20, 2022 09:35

Merge remote-tracking branch 'upstream/main' into sdf_text

26ab1f1

eh, close enough

4cc547a

dnfield marked this pull request as ready for review September 20, 2022 20:53

jonahwilliams reviewed Sep 20, 2022

View reviewed changes

impeller/entity/shaders/glyph_atlas_sdf.frag Outdated Show resolved Hide resolved

jonahwilliams reviewed Sep 20, 2022

View reviewed changes

dnfield and others added 2 commits September 20, 2022 13:58

Update impeller/entity/shaders/glyph_atlas_sdf.frag

8d2ac04

Co-authored-by: Jonah Williams <jonahwilliams@google.com>

color_glyph

85633ba

dnfield requested review from bdero, chinmaygarde and jonahwilliams September 20, 2022 21:05

chinmaygarde reviewed Sep 20, 2022

View reviewed changes

Revert testing.gni

54b4f90

chinmaygarde reviewed Sep 20, 2022

View reviewed changes

chinmaygarde approved these changes Sep 20, 2022

View reviewed changes

bdero reviewed Sep 20, 2022

View reviewed changes

review, fix branching

2ac5cf5

dnfield added the autosubmit Merge PR when tree becomes green via auto submit App label Sep 20, 2022

is_same_v

dc7f88a

auto-submit bot merged commit 38fef05 into flutter:main Sep 21, 2022

engine-flutter-autoroll mentioned this pull request Sep 21, 2022

Roll Flutter Engine from 4395fd0a04e0 to 38fef0572fa0 (2 revisions) flutter/flutter#112032

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Sep 21, 2022

38fef05 [Impeller] SDF text rendering (flutter/engine#36171)

6946569

yjbanov mentioned this pull request Sep 21, 2022

[impeller] fullscreen_textfield_perf_impeller_ios__e2e_summary benchmark regressed flutter/flutter#112107

Closed

dnfield deleted the sdf_text branch September 21, 2022 21:00

dnfield added a commit that referenced this pull request Sep 21, 2022

Revert "[Impeller] SDF text rendering (#36171)"

9d6c03b

This reverts commit 38fef05.

dnfield mentioned this pull request Sep 21, 2022

Revert "[Impeller] SDF text rendering " #36330

Merged

Oleh-Sv pushed a commit to Oleh-Sv/engine that referenced this pull request Sep 28, 2022

[Impeller] SDF text rendering (flutter#36171)

c99ae14

[Impeller] SDF text rendering #36171

[Impeller] SDF text rendering #36171

Uh oh!

Conversation

dnfield commented Sep 15, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dnfield commented Sep 20, 2022

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chinmaygarde left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!