[Impeller] Support copy from buffer to texture for GLES blitpass, use blit pass to set contents in glyph atlas. #52510

jonahwilliams · 2024-05-02T03:30:13Z

Works around flutter/flutter#144498 for the glyph atlas.

Adds BlitPass::AddCopy implementation for GLES, which is mostly a copy of Texture::SetContents. Updates the glyph atlas to use a blit pass and device private textures instead of set contents (which is unsafely on Metal).

This also removes DeviceBuffer::AsTexture, which isn't actually used anywhere - and creates a linear texture on iOS (and fails with an unsupported API on simulators).

Note that in general, we don't actually have support for hostVisible textures on GLES or Vulkan. Instead, VMA is falling back to device private textures. We may want to, separately, remove the concept of host visible from textures - and similarly remove the concept of transient from buffers.

lib/ui/painting/image_decoder_impeller.h

impeller/core/device_buffer.cc

jonahwilliams · 2024-05-04T21:05:19Z

impeller/entity/contents/content_context.cc

@@ -263,7 +263,17 @@ ContentContext::ContentContext(
    desc.size = ISize{1, 1};
    empty_texture_ = GetContext()->GetResourceAllocator()->CreateTexture(desc);
    auto data = Color::BlackTransparent().ToR8G8B8A8();
-    if (!empty_texture_->SetContents(data.data(), 4)) {
+    auto cmd_buffer = GetContext()->CreateCommandBuffer();


Wanted to change a few places to use the blitpass to make sure it works. We should eventually remove Texture::SetContents.

jonahwilliams · 2024-05-04T21:05:43Z

impeller/entity/contents/filters/blend_filter_contents.cc

@@ -864,6 +853,29 @@ std::optional<Entity> BlendFilterContents::CreateFramebufferAdvancedBlend(

  std::shared_ptr<CommandBuffer> cmd_buffer =
      renderer.GetContext()->CreateCommandBuffer();
+
+  // Generate a 1x1 texture to implement foreground color blending.
+  if (foreground_color.has_value()) {


This is better because we don't need to create an extra cmd buffer or host texture for the upload.

jonahwilliams · 2024-05-04T21:06:24Z

impeller/typographer/backends/skia/typographer_context_skia.cc

@@ -450,6 +473,21 @@ std::shared_ptr<GlyphAtlas> TypographerContextSkia::CreateGlyphAtlas(
  }
  atlas_context_skia.UpdateBitmap(bitmap);

+  // If the new atlas size is the same size as the previous texture, reuse the


This was an optimization that was reverted because TextureMTL::SetContents is unsafe.

jonahwilliams · 2024-05-07T18:10:53Z

@chinmaygarde PTAL

chinmaygarde · 2024-05-07T18:25:55Z

Whoops. Missing this. Looking.

impeller/core/device_buffer.cc

chinmaygarde · 2024-05-07T21:45:51Z

impeller/core/device_buffer.h

@@ -31,11 +30,6 @@ class DeviceBuffer {
  /// @brief Create a buffer view of this entire buffer.
  static BufferView AsBufferView(std::shared_ptr<DeviceBuffer> buffer);

-  virtual std::shared_ptr<Texture> AsTexture(


lol, I didn't know this API existed.

chinmaygarde · 2024-05-07T21:46:52Z

impeller/core/texture.h

@@ -20,11 +20,13 @@ class Texture {

  virtual void SetLabel(std::string_view label) = 0;

+  // Deprecated: use BlitPass::AddCopy instead.


An issue to followup on cleaning this up?

flutter/flutter#147945

chinmaygarde · 2024-05-07T21:48:36Z

impeller/renderer/backend/gles/blit_command_gles.cc

+      case PixelFormat::kR16G16B16A16Float:
+        internal_format = GL_RGBA;
+        external_format = GL_RGBA;
+        type = GL_HALF_FLOAT;


I don't think these are supported by GL ES 2.0 but we will probably put these behind a capability check anyway I suppose.

chinmaygarde · 2024-05-07T21:49:27Z

impeller/renderer/backend/gles/blit_command_gles.cc

+  if (!gl_handle.has_value()) {
+    VALIDATION_LOG
+        << "Texture was collected before it could be uploaded to the GPU.";
+    return true;


Return false here right?

chinmaygarde · 2024-05-07T21:51:56Z

impeller/renderer/backend/vulkan/blit_command_vk.cc

+  {
+    BarrierVK barrier;
+    barrier.cmd_buffer = cmd_buffer;
+    barrier.src_access = vk::AccessFlagBits::eColorAttachmentWrite |


I am not sure why you need the ColorAttachmentWrite src access. Only the transfer write should be sufficient.

chinmaygarde

Other than the return false; in the GLES blit, I think everything else lgtm with minor nits. I didn't look super closely at the mostly mechanical replacements of the SetContents call but I trust those are fine. Thanks!

…tpass, use blit pass to set contents in glyph atlas. (flutter/engine#52510)

…147955) flutter/engine@1c52274...8b0ab93 2024-05-08 [email protected] [Impeller] Support copy from buffer to texture for GLES blitpass, use blit pass to set contents in glyph atlas. (flutter/engine#52510) 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 [email protected],[email protected] 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://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

… of origin for buffer to texture copies. (#52555) Based on #52510 Work towards flutter/flutter#138798 Change IPoint destination_origin to IRect destination_region, which allows us to specify an area smaller than the entire texture to replace. This will eventually allow us to only upload individual glyphs. This fixes the cubemap issue I previously hit: each face needs to track initialization separately.

[WIP][Impeller] Make BlitPass responsible for setting Texture contents.

6ebde28

github-actions bot added the e: impeller label May 2, 2024

jonahwilliams commented May 2, 2024

View reviewed changes

lib/ui/painting/image_decoder_impeller.h Outdated Show resolved Hide resolved

jonahwilliams added 6 commits May 1, 2024 21:35

++

6d5f1c9

more fixes.

70aa446

revert migration

d689668

revert texture.

3bbfcf1

more reverts.

eac8881

++

fce8d81

jonahwilliams commented May 3, 2024

View reviewed changes

impeller/core/device_buffer.cc Show resolved Hide resolved

jonahwilliams added 3 commits May 2, 2024 19:58

++

4a4f8b9

Merge branch 'main' of github.com:flutter/engine into make_image_safe

a552fe9

delete mock test.

dd7a2c2

jonahwilliams changed the title ~~[WIP][Impeller] Make BlitPass responsible for setting Texture contents.~~ [WIP][Impeller] Support copy from buffer to texture for GLES blitpass, use blit pass to set contents in glyph atlas. May 3, 2024

jonahwilliams added 4 commits May 2, 2024 21:21

++

8feea36

++

e0e859e

remove excluded files.

f72a16e

recycle same sized texture.

a553fe2

jonahwilliams changed the title ~~[WIP][Impeller] Support copy from buffer to texture for GLES blitpass, use blit pass to set contents in glyph atlas.~~ [Impeller] Support copy from buffer to texture for GLES blitpass, use blit pass to set contents in glyph atlas. May 3, 2024

jonahwilliams marked this pull request as ready for review May 3, 2024 19:11

jonahwilliams added 2 commits May 3, 2024 13:36

just use public API.

3dc0c83

remove unique

32c2d82

jonahwilliams mentioned this pull request May 4, 2024

[Impeller] Update BlitPass::AddCopy to use destination_region instead of origin for buffer to texture copies. #52555

Merged

jonahwilliams commented May 4, 2024

View reviewed changes

jonahwilliams requested a review from chinmaygarde May 4, 2024 21:06

chinmaygarde reviewed May 7, 2024

View reviewed changes

chinmaygarde approved these changes May 7, 2024

View reviewed changes

jonahwilliams mentioned this pull request May 7, 2024

[Impeller] Deprecate and remove Texture::SetContents in favor of BlitPass. flutter/flutter#147945

Open

Merge branch 'main' of github.com:flutter/engine into make_image_safe

536746d

jonahwilliams added the autosubmit Merge PR when tree becomes green via auto submit App label May 7, 2024

auto-submit bot merged commit 8b0ab93 into flutter:main May 8, 2024
33 checks passed

jonahwilliams deleted the make_image_safe branch May 8, 2024 00:03

engine-flutter-autoroll mentioned this pull request May 8, 2024

Roll Flutter Engine from 1c52274e1118 to 8b0ab9366715 (1 revision) flutter/flutter#147955

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request May 8, 2024

8b0ab9366 [Impeller] Support copy from buffer to texture for GLES bli…

1a39e3b

…tpass, use blit pass to set contents in glyph atlas. (flutter/engine#52510)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Impeller] Support copy from buffer to texture for GLES blitpass, use blit pass to set contents in glyph atlas. #52510

[Impeller] Support copy from buffer to texture for GLES blitpass, use blit pass to set contents in glyph atlas. #52510

jonahwilliams commented May 2, 2024 •

edited

Loading

jonahwilliams May 4, 2024

jonahwilliams May 4, 2024

jonahwilliams May 4, 2024

jonahwilliams commented May 7, 2024

chinmaygarde commented May 7, 2024

chinmaygarde May 7, 2024

chinmaygarde May 7, 2024

jonahwilliams May 7, 2024

chinmaygarde May 7, 2024

chinmaygarde May 7, 2024

jonahwilliams May 7, 2024

chinmaygarde May 7, 2024

jonahwilliams May 7, 2024

chinmaygarde left a comment

		@@ -20,11 +20,13 @@ class Texture {

		virtual void SetLabel(std::string_view label) = 0;

		// Deprecated: use BlitPass::AddCopy instead.

[Impeller] Support copy from buffer to texture for GLES blitpass, use blit pass to set contents in glyph atlas. #52510

[Impeller] Support copy from buffer to texture for GLES blitpass, use blit pass to set contents in glyph atlas. #52510

Conversation

jonahwilliams commented May 2, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonahwilliams commented May 7, 2024

chinmaygarde commented May 7, 2024

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

chinmaygarde left a comment

Choose a reason for hiding this comment

jonahwilliams commented May 2, 2024 •

edited

Loading