[Impeller] use sync fence for image uploads. #56609

jonahwilliams · 2024-11-15T04:44:31Z

If the GLES version is at least 3, then we can attach a sync fence to the texture gles object. If this operation succeeds, then we can use gl.Flush instad of gl.Finish. Then, when binding the texture - if a sync fence is present we wait and then remove the fence.

…sync

jason-simmons

Overall design LGTM

jason-simmons · 2024-11-15T19:57:10Z

impeller/renderer/backend/gles/reactor_gles.cc

      return handle;
+    case HandleType::kFence:
+      return GLHandle{.sync = gl.FenceSync(GL_SYNC_GPU_COMMANDS_COMPLETE, 0)};
+      break;


The break can be deleted

jason-simmons · 2024-11-15T20:00:41Z

impeller/renderer/backend/gles/reactor_gles.cc

@@ -117,18 +123,44 @@ const ProcTableGLES& ReactorGLES::GetProcTable() const {
 }

 std::optional<GLuint> ReactorGLES::GetGLHandle(const HandleGLES& handle) const {
+  if (handle.type == HandleType::kFence) {


The locking and the lookup in the handles_ map can be extracted out into a separate method in order to reduce the duplication between ReactorGLES::GetGLHandle and GetGLFence.

jason-simmons · 2024-11-15T20:06:05Z

lib/ui/painting/image_decoder_impeller.cc

@@ -378,6 +377,8 @@ ImageDecoderImpeller::UnsafeUploadTextureToPrivate(
  }
  blit_pass->EncodeCommands(context->GetResourceAllocator());

+  bool did_add_fence = context->AddTrackingFence(result_texture);


Create the fence after the call to CommandQueue::Submit.

glFenceSync should be called after issuing all of the GL commands that write to the texture. In theory that may not happen until after Submit returns successfully.

(In practice the ReactorGLES::AddOperation calls done during encoding will probably be able to call React, which will call the GL APIs)

jason-simmons · 2024-11-15T20:22:33Z

impeller/renderer/backend/gles/proc_table_gles.cc

@@ -346,6 +348,8 @@ static bool ResourceIsLive(const ProcTableGLES& gl,
      return gl.IsRenderbuffer(name);
    case DebugResourceType::kFrameBuffer:
      return gl.IsFramebuffer(name);
+    case DebugResourceType::kFence:
+      return true;


GLES3 has a glIsSync API that can be used here

This is only used for debug labeling which I don't believe can be done to the sync fence APIs

jason-simmons · 2024-11-15T20:26:22Z

impeller/renderer/backend/gles/texture_gles.h

@@ -121,10 +122,22 @@ class TextureGLES final : public Texture,

  bool IsSliceInitialized(size_t slice) const;

+  //----------------------------------------------------------------------------
+  /// @brief      Attach a sync fence to this texture that will be waited on
+  ///             before encoding a rendering operatio that references it.


typo: "operation"

gaaclarke

Stating the obvious; this should land after #56596 to get the gles3 context.

This is looking good. The test is looking good, the approach is looking right. I don't think we should introduce GLHandle when we have HandleGLES, it seems like we can merge those concepts, use HandleGLES when you need to potentially represent a fence.

impeller/renderer/backend/gles/reactor_gles.h

gaaclarke

lgtm modulo any remaining of jason's feedback!

gaaclarke · 2024-11-18T19:27:39Z

@jonahwilliams my PR that brings in an opengles3 context landed. This should be good to merge as far as I know.

…e#56609)

…159118) flutter/engine@878f593...c1b0e18 2024-11-19 [email protected] [Impeller] use sync fence for image uploads. (flutter/engine#56609) 2024-11-18 [email protected] Update emulator definitions version to latest available from chrome infra (flutter/engine#56313) 2024-11-18 [email protected] Roll Dart SDK from 625e0a9cb67a to 05d58364e92f (1 revision) (flutter/engine#56688) 2024-11-18 [email protected] iOS: Eliminate unguarded-availability opt-out (flutter/engine#56689) 2024-11-18 [email protected] [skwasm] Use `displayWidth`/`displayHeight` instead of `codedWidth`/`codedHeight` (flutter/engine#56686) 2024-11-18 [email protected] Extract TestGLContext to separate translation unit (flutter/engine#56647) 2024-11-18 [email protected] Roll Skia from b79e71223284 to 492e8347d7a4 (2 revisions) (flutter/engine#56687) 2024-11-18 [email protected] Update the Skia build scripts for a refactoring of the Fontconfig font manager (flutter/engine#56684) 2024-11-18 [email protected] iOS,macOS: Enable ARC in flutter_cflags_objc[c] (flutter/engine#56685) 2024-11-18 [email protected] Re-land "Remove `android_jit_release_x86`." (flutter/engine#56681) 2024-11-18 [email protected] Roll Skia from 0d24bd3268ef to b79e71223284 (1 revision) (flutter/engine#56683) 2024-11-18 [email protected] Flutter views can gain focus (flutter/engine#54985) 2024-11-18 [email protected] Roll Skia from 0b74d5c3eb4f to 0d24bd3268ef (1 revision) (flutter/engine#56680) 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

Fixes flutter#158963 If the GLES version is at least 3, then we can attach a sync fence to the texture gles object. If this operation succeeds, then we can use gl.Flush instad of gl.Finish. Then, when binding the texture - if a sync fence is present we wait and then remove the fence.

[Impeller] use sync fence for image uploads.

491e166

github-actions bot added the e: impeller label Nov 15, 2024

jonahwilliams added 5 commits November 15, 2024 08:52

Merge branch 'main' of github.com:flutter/engine into implement-wait-…

639fe30

…sync

++

75e37d6

fix importss.

c67779a

remove one more include.

6b199ea

add unit test.

6232416

jonahwilliams marked this pull request as ready for review November 15, 2024 18:11

jonahwilliams added 2 commits November 15, 2024 10:17

fix texture format.

b6bf09e

clang tidy.

1d29ad2

jason-simmons reviewed Nov 15, 2024

View reviewed changes

jason review.

e0d15ad

gaaclarke reviewed Nov 15, 2024

View reviewed changes

impeller/renderer/backend/gles/reactor_gles.h Outdated Show resolved Hide resolved

rename to GLStorage, make private.

7ef2efd

gaaclarke approved these changes Nov 15, 2024

View reviewed changes

jason-simmons approved these changes Nov 15, 2024

View reviewed changes

Merge branch 'main' into implement-wait-sync

377a926

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

auto-submit bot merged commit c1b0e18 into flutter:main Nov 19, 2024
34 checks passed

auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Nov 19, 2024

engine-flutter-autoroll mentioned this pull request Nov 19, 2024

Roll Flutter Engine from 878f593802e1 to c1b0e18a70b3 (13 revisions) flutter/flutter#159118

Merged

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

c1b0e18a7 [Impeller] use sync fence for image uploads. (flutter/engin…

0762465

…e#56609)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[Impeller] use sync fence for image uploads. #56609

[Impeller] use sync fence for image uploads. #56609

Uh oh!

jonahwilliams commented Nov 15, 2024 •

edited

Loading

Uh oh!

jason-simmons left a comment

Uh oh!

jason-simmons Nov 15, 2024

Uh oh!

jonahwilliams Nov 15, 2024

Uh oh!

jason-simmons Nov 15, 2024

Uh oh!

jonahwilliams Nov 15, 2024

Uh oh!

jason-simmons Nov 15, 2024

Uh oh!

jonahwilliams Nov 15, 2024

Uh oh!

jason-simmons Nov 15, 2024

Uh oh!

jonahwilliams Nov 15, 2024

Uh oh!

jason-simmons Nov 15, 2024

Uh oh!

jonahwilliams Nov 15, 2024

Uh oh!

gaaclarke left a comment

Uh oh!

Uh oh!

gaaclarke left a comment

Uh oh!

gaaclarke commented Nov 18, 2024

Uh oh!

Uh oh!

Uh oh!

[Impeller] use sync fence for image uploads. #56609

[Impeller] use sync fence for image uploads. #56609

Uh oh!

Conversation

jonahwilliams commented Nov 15, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jason-simmons 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!

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!

gaaclarke left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

gaaclarke left a comment

Choose a reason for hiding this comment

Uh oh!

gaaclarke commented Nov 18, 2024

Uh oh!

Uh oh!

Uh oh!

jonahwilliams commented Nov 15, 2024 •

edited

Loading