[Impeller] Use a device buffer for SkBitmap allocation, use Linear texture on Metal backend. #41374

jonahwilliams · 2023-04-20T19:39:54Z

This reduces the cost of uploading the glyph atlas, as we can exploit metal's capabilies to create a texture from a device buffer and share the underlying memory with a linear texture. This also means that subsequent updates don't require uploads or copies either.

From scrolling through wonderous, this shaves off about 0.2 ms (0.6 ms -> 0.4 ms) of fresh atlas construction.

…hared_texture

jonahwilliams · 2023-04-20T19:43:35Z

impeller/typographer/backends/skia/text_render_context_skia.h

+class FontImpellerAllocator : public SkBitmap::Allocator {
+ public:
+  explicit FontImpellerAllocator(
+      std::shared_ptr<impeller::Allocator> allocator);
+
+  ~FontImpellerAllocator() = default;
+
+  // |Allocator|
+  bool allocPixelRef(SkBitmap* bitmap) override;
+
+  std::optional<std::shared_ptr<DeviceBuffer>> GetDeviceBuffer() const;
+
+ private:
+  std::shared_ptr<impeller::Allocator> allocator_;
+  std::optional<std::shared_ptr<DeviceBuffer>> buffer_;
+};
+


This is an exact copy of the implementation used in image decoding.

I'd like to put it somewhere shared but I'm not quite sure where that would be.

ignoring the skia dependency, it would just go in core. But with the skia dep I have no idea. I could create a new impeller-skia interop target?

I don't think we should add a dependency on skia to core but we could. I think impeller/image is probably the right spot for this.

jonahwilliams · 2023-04-20T21:08:21Z

For non metal platforms this may still be faster as it will use the device allocator instead of malloc, but IIRC it should have the same number of copies and uploads.

…/engine into upload_with_shared_texture

…hared_texture

gaaclarke · 2023-04-21T17:07:32Z

impeller/renderer/backend/vulkan/capabilities_vk.cc

 bool CapabilitiesVK::SupportsDecalTileMode() const {
  return true;
 }

+// |Capabilities|
+bool CapabilitiesVK::SupportsSharedDeviceBufferTextureMemory() const {
+  return false;


This should support it eventually, right? Probably worth adding a TODO.

I did some research but I think is metal only

gaaclarke · 2023-04-21T17:38:19Z

impeller/typographer/typographer_unittests.cc

-                                TextFrameFromTextBlob(blob));
+  auto atlas = context->CreateGlyphAtlas(
+      GlyphAtlas::Type::kAlphaBitmap, atlas_context,
+      CapabilitiesBuilder().Build(), TextFrameFromTextBlob(blob));


Shouldn't we have a test where SupportsSharedDeviceBufferTextureMemory() == true? We should get test coverage from our metal playground tests?

Added a test that only runs if the backend supports this feature and validates the texture is valid for a single character atlas.

gaaclarke · 2023-04-21T17:44:33Z

impeller/typographer/glyph_atlas.h

@@ -153,7 +154,8 @@ class GlyphAtlasContext {

  //----------------------------------------------------------------------------
  /// @brief      Retrieve the previous (if any) SkBitmap instance.
-  std::shared_ptr<SkBitmap> GetBitmap() const;
+  std::pair<std::shared_ptr<SkBitmap>, std::shared_ptr<DeviceBuffer>>
+  GetBitmap() const;


Looks like people want one or the other, right? Not both. Might be worth just keeping this the same and adding a device buffer accessor when/if we need it.

gaaclarke · 2023-04-21T17:51:40Z

impeller/typographer/backends/skia/text_render_context_skia.h

+class DeviceBuffer;
+class Allocator;
+
+class FontImpellerAllocator : public SkBitmap::Allocator {


Please add docstring. The important thing to note is having access to the DeviceBuffer.

still working on this, want to combine it with the identical class in image_decode_impeller but haven't thought of the right target yet.

Added a doc comment here. Haven't found a home yet but I think this can still be landed

gaaclarke · 2023-04-21T17:51:59Z

impeller/typographer/backends/skia/text_render_context_skia.cc

    // ---------------------------------------------------------------------------
-    if (!UpdateGlyphTextureAtlas(bitmap, last_atlas->GetTexture())) {
+    if (!capabilities->SupportsSharedDeviceBufferTextureMemory() &&


Here's the payoff, right? LGTM.

Technically this and below where we construct a texture from the device buffer, that required a second copy before but now its "free" on iOS/macOS

gaaclarke

lgtm

gaaclarke · 2023-04-21T20:05:32Z

impeller/typographer/backends/skia/text_render_context_skia.cc

@@ -552,11 +559,12 @@ std::shared_ptr<GlyphAtlas> TextRenderContextSkia::CreateGlyphAtlas(
  // ---------------------------------------------------------------------------
  // Step 7: Draw font-glyph pairs in the correct spot in the atlas.
  // ---------------------------------------------------------------------------
-  auto bitmap = CreateAtlasBitmap(*glyph_atlas, atlas_size);
+  auto [bitmap, device_buffer] = CreateAtlasBitmap(


These are shadowing the declarations in 509, right?

No, those are in a different block

…hared_texture

dnfield · 2023-04-26T03:48:33Z

impeller/typographer/backends/skia/text_render_context_skia.cc

+  auto texture = device_buffer->AsTexture(allocator, texture_descriptor,
+                                          texture_descriptor.GetBytesPerRow());


Does this punish non-metal implementations right now?

I think it doesn't because before we did the copy explicitly in this code, and now it gets done implicitly if needed there, right?

impeller/typographer/backends/skia/text_render_context_skia.h

dnfield · 2023-04-26T03:54:15Z

impeller/typographer/backends/skia/text_render_context_skia.h

+///        image_decode_impeller.cc due to the lack of a reasonable library
+///        that could be shared.


Right now we have display_list:skia_conversions, aiks, and image and this that know about Skia.

It seems like this could probably go into image, and we could add a dep to impeller/image to both lib/ui and impeller/typographer. Doesn't have to be done in this patch but would be good as a follow up if not done here.

dnfield

LGTM with nits

Co-authored-by: Dan Field <dfield@gmail.com>

…hared_texture

… Linear texture on Metal backend. (flutter/engine#41374)

…125583) flutter/engine@cf97541...fddd5ad 2023-04-26 jonahwilliams@google.com [Impeller] Use a device buffer for SkBitmap allocation, use Linear texture on Metal backend. (flutter/engine#41374) 2023-04-26 zanderso@users.noreply.github.com Manual clang roll to 5344d8e10bb7d8672d4bfae8adb010465470d51b (flutter/engine#41520) 2023-04-26 zanderso@users.noreply.github.com Download and use the goma client from cipd (flutter/engine#41488) 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 jsimmons@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

…inear texture on Metal backend. (#41374)" This reverts commit fddd5ad.

…inear texture on Metal backend." (#41533) Reverts #41374 Breaks on Simulators!

… Linear texture on Metal backend. (flutter/engine#41374)

…125593) flutter/engine@cf97541...d4ca524 2023-04-27 skia-flutter-autoroll@skia.org Roll Skia from 20a1c61c5597 to d315ab065af3 (5 revisions) (flutter/engine#41535) 2023-04-26 jonahwilliams@google.com Revert "[Impeller] Use a device buffer for SkBitmap allocation, use Linear texture on Metal backend." (flutter/engine#41533) 2023-04-26 xilaizhang@google.com [codesign] Add pinned xcode version as property to mac android aot engine (flutter/engine#41518) 2023-04-26 bdero@google.com [Impeller] Coerce opaque ColorSourceContents to Source (flutter/engine#41525) 2023-04-26 skia-flutter-autoroll@skia.org Roll Skia from 3fea88565a83 to 20a1c61c5597 (3 revisions) (flutter/engine#41530) 2023-04-26 jonahwilliams@google.com [Impeller] partial repaint for Impeller/iOS. (flutter/engine#40959) 2023-04-26 30870216+gaaclarke@users.noreply.github.com Updated todo with github issue link (flutter/engine#41517) 2023-04-26 skia-flutter-autoroll@skia.org Roll Clang from 20d06c833d83 to 5344d8e10bb7 (flutter/engine#41524) 2023-04-26 jonahwilliams@google.com [Impeller] Use a device buffer for SkBitmap allocation, use Linear texture on Metal backend. (flutter/engine#41374) 2023-04-26 zanderso@users.noreply.github.com Manual clang roll to 5344d8e10bb7d8672d4bfae8adb010465470d51b (flutter/engine#41520) 2023-04-26 zanderso@users.noreply.github.com Download and use the goma client from cipd (flutter/engine#41488) 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 jsimmons@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

…inear texture on Metal backend. (#41538) Original PR: #41374 This was reverted because it broke on simulators as they do not support linear textures. To fix this, I've ifdef'd out the DeviceBufferMTL implementation of AsTexture so that it falls back to the slow path copy. Also updated the capabilities check so that the glyph atlas updates the texture contents when it changes.

jonahwilliams added 5 commits April 19, 2023 16:38

[impeller] convert src over to src for solid color

cff1248

[Impeller] use device buffer to SkBitmap allocation

b5eb0c2

++

82ff8ad

Merge branch 'master' of github.com:flutter/engine into upload_with_s…

d8ca1a1

…hared_texture

++

0511d65

jonahwilliams commented Apr 20, 2023

View reviewed changes

jonahwilliams added 3 commits April 20, 2023 12:53

++

725ea87

++

4c5cf04

Merge branch 'main' into upload_with_shared_texture

0df598c

++

706386a

jonahwilliams changed the title ~~[Impeller] use a device buffer for SkBitmap allocation, avoid upload on metal~~ [Impeller] use a device buffer for SkBitmap allocation, use LinearTexture on metal backend Apr 21, 2023

jonahwilliams changed the title ~~[Impeller] use a device buffer for SkBitmap allocation, use LinearTexture on metal backend~~ [Impeller] use a device buffer for SkBitmap allocation, use Linear texture on metal backend Apr 21, 2023

jonahwilliams added 3 commits April 20, 2023 17:15

Merge branch 'upload_with_shared_texture' of github.com:jonahwilliams…

e9aa30e

…/engine into upload_with_shared_texture

correct check

7e7d217

++

81ad980

jonahwilliams marked this pull request as ready for review April 21, 2023 16:51

jonahwilliams added 3 commits April 21, 2023 09:53

Merge branch 'master' of github.com:flutter/engine into upload_with_s…

1ede1d0

…hared_texture

++

69ec6b0

++

dfb3f3c

chinmaygarde changed the title ~~[Impeller] use a device buffer for SkBitmap allocation, use Linear texture on metal backend~~ [Impeller] Use a device buffer for SkBitmap allocation, use Linear texture on Metal backend. Apr 21, 2023

chinmaygarde assigned jonahwilliams Apr 21, 2023

chinmaygarde added the e: impeller label Apr 21, 2023

gaaclarke reviewed Apr 21, 2023

View reviewed changes

++

f832c97

gaaclarke approved these changes Apr 21, 2023

View reviewed changes

jonahwilliams added 3 commits April 24, 2023 10:32

Merge branch 'master' of github.com:flutter/engine into upload_with_s…

9ac1722

…hared_texture

add doc comment

ffe0d88

Merge branch 'main' into upload_with_shared_texture

70597bd

jonahwilliams requested a review from dnfield April 25, 2023 18:38

dnfield reviewed Apr 26, 2023

View reviewed changes

impeller/typographer/backends/skia/text_render_context_skia.h Outdated Show resolved Hide resolved

dnfield reviewed Apr 26, 2023

View reviewed changes

dnfield approved these changes Apr 26, 2023

View reviewed changes

jonahwilliams and others added 3 commits April 26, 2023 10:17

Update impeller/typographer/backends/skia/text_render_context_skia.h

6b3d886

Co-authored-by: Dan Field <dfield@gmail.com>

Merge branch 'master' of github.com:flutter/engine into upload_with_s…

07e12f2

…hared_texture

++

78fddf7

jonahwilliams requested a review from dnfield April 26, 2023 19:16

dnfield approved these changes Apr 26, 2023

View reviewed changes

jonahwilliams added the autosubmit Merge PR when tree becomes green via auto submit App label Apr 26, 2023

auto-submit bot merged commit fddd5ad into flutter:main Apr 26, 2023

jonahwilliams deleted the upload_with_shared_texture branch April 26, 2023 19:47

jonahwilliams mentioned this pull request Apr 26, 2023

[Impeller] Failure in glyph atlas recycling unit test (host_profile, arm64 macOS) flutter/flutter#122839

Closed

engine-flutter-autoroll mentioned this pull request Apr 26, 2023

Roll Flutter Engine from cf975417ce40 to fddd5add8b62 (3 revisions) flutter/flutter#125583

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Apr 26, 2023

fddd5add8 [Impeller] Use a device buffer for SkBitmap allocation, use…

0b2de43

… Linear texture on Metal backend. (flutter/engine#41374)

jonahwilliams added a commit that referenced this pull request Apr 26, 2023

Revert "[Impeller] Use a device buffer for SkBitmap allocation, use L…

56f1452

…inear texture on Metal backend. (#41374)" This reverts commit fddd5ad.

jonahwilliams mentioned this pull request Apr 26, 2023

Revert "[Impeller] Use a device buffer for SkBitmap allocation, use Linear texture on Metal backend." #41533

Merged

auto-submit bot pushed a commit that referenced this pull request Apr 26, 2023

Revert "[Impeller] Use a device buffer for SkBitmap allocation, use L…

464abc7

…inear texture on Metal backend." (#41533) Reverts #41374 Breaks on Simulators!

engine-flutter-autoroll mentioned this pull request Apr 26, 2023

Roll Flutter Engine from cf975417ce40 to 464abc748021 (10 revisions) flutter/flutter#125592

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Apr 26, 2023

fddd5add8 [Impeller] Use a device buffer for SkBitmap allocation, use…

826a769

… Linear texture on Metal backend. (flutter/engine#41374)

jonahwilliams mentioned this pull request Apr 26, 2023

Reland: [Impeller] Use a device buffer for SkBitmap allocation, use Linear texture on Metal backend. #41538

Merged

engine-flutter-autoroll mentioned this pull request Apr 27, 2023

Roll Flutter Engine from cf975417ce40 to d4ca5240da52 (11 revisions) flutter/flutter#125593

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Apr 27, 2023

fddd5add8 [Impeller] Use a device buffer for SkBitmap allocation, use…

f3d26b8

… Linear texture on Metal backend. (flutter/engine#41374)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Impeller] Use a device buffer for SkBitmap allocation, use Linear texture on Metal backend. #41374

[Impeller] Use a device buffer for SkBitmap allocation, use Linear texture on Metal backend. #41374

jonahwilliams commented Apr 20, 2023 •

edited

Loading

jonahwilliams Apr 20, 2023

jonahwilliams Apr 20, 2023

jonahwilliams Apr 21, 2023

dnfield Apr 26, 2023

jonahwilliams commented Apr 20, 2023

gaaclarke Apr 21, 2023

jonahwilliams Apr 21, 2023

gaaclarke Apr 21, 2023

jonahwilliams Apr 21, 2023

gaaclarke Apr 21, 2023

gaaclarke Apr 21, 2023

jonahwilliams Apr 21, 2023

jonahwilliams Apr 24, 2023

gaaclarke Apr 21, 2023

jonahwilliams Apr 21, 2023

gaaclarke left a comment

gaaclarke Apr 21, 2023

jonahwilliams Apr 24, 2023

dnfield Apr 26, 2023

jonahwilliams Apr 26, 2023

dnfield Apr 26, 2023

dnfield left a comment

		auto texture = device_buffer->AsTexture(allocator, texture_descriptor,
		texture_descriptor.GetBytesPerRow());

		/// image_decode_impeller.cc due to the lack of a reasonable library
		/// that could be shared.

[Impeller] Use a device buffer for SkBitmap allocation, use Linear texture on Metal backend. #41374

[Impeller] Use a device buffer for SkBitmap allocation, use Linear texture on Metal backend. #41374

Conversation

jonahwilliams commented Apr 20, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonahwilliams commented Apr 20, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gaaclarke left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dnfield left a comment

Choose a reason for hiding this comment

jonahwilliams commented Apr 20, 2023 •

edited

Loading