[impeller] enable framebuffer blit when available #56596

gaaclarke · 2024-11-14T22:23:15Z

depends on #56573
fixes flutter/flutter#158523

Pre-launch Checklist

I read the Contributor Guide and followed the process outlined there for submitting PRs.
I read the Tree Hygiene wiki page, which explains my responsibilities.
I read and followed the Flutter Style Guide and the C++, Objective-C, Java style guides.
I listed at least one issue that this PR fixes in the description above.
I added new tests to check the change I am making or feature I am adding, or the PR is test-exempt. See testing the engine for instructions on writing and running engine tests.
I updated/added relevant documentation (doc comments with ///).
I signed the CLA.
All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

gaaclarke · 2024-11-14T22:39:30Z

In the current state, images that are resized are rendered upside down.

jonahwilliams · 2024-11-14T23:23:43Z

We need to set the upload intent differently

jonahwilliams · 2024-11-14T23:25:42Z

Try changing the coordinate system on the destination texture in the resize.

  texture->SetCoordinateSystem(
      impeller::TextureCoordinateSystem::kUploadFromHost);

gaaclarke · 2024-11-14T23:34:41Z

Try changing the coordinate system on the destination texture in the resize.

Ugh, that works. I don't like it. It seems wrong editing the destination when blitting to it. Looks like we are doing this elsewhere though.

impeller/renderer/backend/gles/blit_command_gles.cc

jonahwilliams · 2024-11-15T00:50:11Z

shell/platform/android/android_context_gl_impeller.cc

@@ -92,7 +92,7 @@ AndroidContextGLImpeller::AndroidContextGLImpeller(
  }

  impeller::egl::ConfigDescriptor desc;
-  desc.api = impeller::egl::API::kOpenGLES2;
+  desc.api = impeller::egl::API::kOpenGLES3;


Is this valid to do? Defiintely outside of my wheelhouse. - we do a lot of GLES 2 stuff, do we need to worry about GLES 3 having different semantics?

@chinmaygarde

Nope. That's how you'd do it. ES3 is backwards compatible with the same sematics.

gaaclarke · 2024-11-15T17:50:54Z

There appears to be tests for the software path in ImageDecoderFixtureTest.CanDecodeWithResizes, BlitPassTest.CanResizeTextures for the blit path.

gaaclarke · 2024-11-15T18:53:43Z

shell/platform/android/android_context_gl_impeller_unittests.cc

@@ -104,6 +104,15 @@ TEST_F(AndroidContextGLImpellerTest, FallbackForEmulator) {
  EXPECT_CALL(
      *display,
      ChooseConfig(Matcher<ConfigDescriptor>(AllOf(
+          Field(&ConfigDescriptor::api, impeller::egl::API::kOpenGLES3),


@jonahwilliams looks like we could have used these tests for asserting we are asking for a depth buffer. It had been a while since I looked at these.

I thought I did that by adding EGL_DEPTH_SIZE 24 above?

🤷‍♂️

jonahwilliams

LGTM

chinmaygarde · 2024-11-15T19:19:37Z

impeller/renderer/backend/gles/capabilities_gles.cc

@@ -109,6 +109,10 @@ CapabilitiesGLES::CapabilitiesGLES(const ProcTableGLES& gl) {
    default_glyph_atlas_format_ = PixelFormat::kR8UNormInt;
  }

+  if (desc->GetGlVersion().major_version >= 3) {


Completely off topic, but we should do a GL version check in ProcTableGLES and reset or not bother to resolve FOR_EACH_IMPELLER_GLES3_PROC methods when we less than ES 3. On older versions of Android, I remember seeing instances where we'd get a function pointer for ES3 functions even on ES2 contexts but those wouldn't work if called.

When I first wrote that bit, I didn't think we'd ever resolve ES3 procs. So its a blind spot.

Yea, absolutely agree. I can file an issue.

flutter/flutter#158995

gaaclarke · 2024-11-15T21:28:33Z

Drat, the added test is a playground test, but isn't wired up to be a golden test. I'd like to get that addressed so the test is automated.

flutter-dashboard · 2024-11-16T01:45:49Z

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 #56596 at sha d2fb626

test: exists in #56596 fixes: flutter/flutter#158995 [C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style

…engine#56596)

…159093) flutter/engine@9686de1...766370b 2024-11-18 30870216+gaaclarke@users.noreply.github.com Started only loading gles3 functions if we have a gles3 context (flutter/engine#56636) 2024-11-18 30870216+gaaclarke@users.noreply.github.com [impeller] enable framebuffer blit when available (flutter/engine#56596) 2024-11-18 skia-flutter-autoroll@skia.org Roll Skia from 7594233ff914 to 0b74d5c3eb4f (4 revisions) (flutter/engine#56678) 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,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://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

…engine#56596)

…159097) flutter/engine@9686de1...878f593 2024-11-18 chris@bracken.jp EmbedderTest: Extract backend-specific user_data (flutter/engine#56642) 2024-11-18 skia-flutter-autoroll@skia.org Roll Dart SDK from ae5111067032 to 625e0a9cb67a (1 revision) (flutter/engine#56679) 2024-11-18 30870216+gaaclarke@users.noreply.github.com Started only loading gles3 functions if we have a gles3 context (flutter/engine#56636) 2024-11-18 30870216+gaaclarke@users.noreply.github.com [impeller] enable framebuffer blit when available (flutter/engine#56596) 2024-11-18 skia-flutter-autoroll@skia.org Roll Skia from 7594233ff914 to 0b74d5c3eb4f (4 revisions) (flutter/engine#56678) 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,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://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

…engine#56596)

…159097) flutter/engine@9686de1...878f593 2024-11-18 chris@bracken.jp EmbedderTest: Extract backend-specific user_data (flutter/engine#56642) 2024-11-18 skia-flutter-autoroll@skia.org Roll Dart SDK from ae5111067032 to 625e0a9cb67a (1 revision) (flutter/engine#56679) 2024-11-18 30870216+gaaclarke@users.noreply.github.com Started only loading gles3 functions if we have a gles3 context (flutter/engine#56636) 2024-11-18 30870216+gaaclarke@users.noreply.github.com [impeller] enable framebuffer blit when available (flutter/engine#56596) 2024-11-18 skia-flutter-autoroll@skia.org Roll Skia from 7594233ff914 to 0b74d5c3eb4f (4 revisions) (flutter/engine#56678) 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,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://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

depends on flutter/engine#56573 fixes flutter#158523 ## Pre-launch Checklist - [x] I read the [Contributor Guide] and followed the process outlined there for submitting PRs. - [x] I read the [Tree Hygiene] wiki page, which explains my responsibilities. - [x] I read and followed the [Flutter Style Guide] and the [C++, Objective-C, Java style guides]. - [x] I listed at least one issue that this PR fixes in the description above. - [x] I added new tests to check the change I am making or feature I am adding, or the PR is [test-exempt]. See [testing the engine] for instructions on writing and running engine tests. - [x] I updated/added relevant documentation (doc comments with `///`). - [x] I signed the [CLA]. - [x] All existing and new tests are passing. If you need help, consider asking for advice on the #hackers-new channel on [Discord].  [Contributor Guide]: https://github.com/flutter/flutter/wiki/Tree-hygiene#overview [Tree Hygiene]: https://github.com/flutter/flutter/wiki/Tree-hygiene [test-exempt]: https://github.com/flutter/flutter/wiki/Tree-hygiene#tests [Flutter Style Guide]: https://github.com/flutter/flutter/wiki/Style-guide-for-Flutter-repo [C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style [testing the engine]: https://github.com/flutter/flutter/wiki/Testing-the-engine [CLA]: https://cla.developers.google.com/ [flutter/tests]: https://github.com/flutter/tests [breaking change policy]: https://github.com/flutter/flutter/wiki/Tree-hygiene#handling-breaking-changes [Discord]: https://github.com/flutter/flutter/wiki/Chat

…ter/engine#56636) test: exists in flutter/engine#56596 fixes: flutter#158995 [C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style

github-actions bot added platform-android e: impeller labels Nov 14, 2024

gaaclarke changed the title ~~Blitopengles3~~ [impeller] enable framebuffer blit when available Nov 14, 2024

jonahwilliams reviewed Nov 14, 2024

View reviewed changes

impeller/renderer/backend/gles/blit_command_gles.cc Outdated Show resolved Hide resolved

jonahwilliams reviewed Nov 15, 2024

View reviewed changes

gaaclarke added 3 commits November 15, 2024 09:06

Enabled opengles3 blits

7b2ac98

flipped the images back

49fd421

++

4b3781a

gaaclarke force-pushed the blitopengles3 branch from 1ca589b to 4b3781a Compare November 15, 2024 17:06

gaaclarke added 3 commits November 15, 2024 09:52

updated test

0f91725

added at golden test

2bf0e96

updated shell test

671c3b5

gaaclarke marked this pull request as ready for review November 15, 2024 18:52

gaaclarke requested a review from jonahwilliams November 15, 2024 18:52

gaaclarke commented Nov 15, 2024

View reviewed changes

gaaclarke requested a review from chinmaygarde November 15, 2024 18:58

jonahwilliams approved these changes Nov 15, 2024

View reviewed changes

chinmaygarde approved these changes Nov 15, 2024

View reviewed changes

fixed tidy

bd255e4

This was referenced Nov 15, 2024

[impeller] don't grab symbols for gles3 symbols with gles2 contexts flutter/flutter#158995

Closed

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

Merged

gaaclarke added 3 commits November 15, 2024 13:46

registered the test as a golden test

e84ce6d

added testing golden

b6adb29

oops

d2fb626

gaaclarke mentioned this pull request Nov 15, 2024

Started only loading gles3 functions if we have a gles3 context #56636

Merged

8 tasks

flutter-dashboard bot added the will affect goldens label Nov 16, 2024

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

gaaclarke merged commit 554786c into flutter:main Nov 18, 2024
37 checks passed

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

Roll Flutter Engine from 9686de154114 to 766370b72f7d (3 revisions) flutter/flutter#159093

Closed

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

554786c04 [impeller] enable framebuffer blit when available (flutter/…

2187584

…engine#56596)

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

Roll Flutter Engine from 9686de154114 to 878f593802e1 (5 revisions) flutter/flutter#159097

Merged

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

554786c04 [impeller] enable framebuffer blit when available (flutter/…

13be47c

…engine#56596)

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

Roll Flutter Engine from 9686de154114 to b79353d8c03d (6 revisions) flutter/flutter#159100

Closed

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

554786c04 [impeller] enable framebuffer blit when available (flutter/…

0bb3245

…engine#56596)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[impeller] enable framebuffer blit when available #56596

[impeller] enable framebuffer blit when available #56596

gaaclarke commented Nov 14, 2024 •

edited

Loading

gaaclarke commented Nov 14, 2024

jonahwilliams commented Nov 14, 2024

jonahwilliams commented Nov 14, 2024

gaaclarke commented Nov 14, 2024

jonahwilliams Nov 15, 2024

chinmaygarde Nov 15, 2024

jonahwilliams Nov 15, 2024

gaaclarke commented Nov 15, 2024

gaaclarke Nov 15, 2024

jonahwilliams Nov 15, 2024

jonahwilliams Nov 15, 2024

jonahwilliams left a comment

chinmaygarde Nov 15, 2024

gaaclarke Nov 15, 2024

gaaclarke Nov 15, 2024

gaaclarke commented Nov 15, 2024

flutter-dashboard bot commented Nov 16, 2024

[impeller] enable framebuffer blit when available #56596

[impeller] enable framebuffer blit when available #56596

Conversation

gaaclarke commented Nov 14, 2024 • edited Loading

Pre-launch Checklist

gaaclarke commented Nov 14, 2024

jonahwilliams commented Nov 14, 2024

jonahwilliams commented Nov 14, 2024

gaaclarke commented Nov 14, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gaaclarke commented Nov 15, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonahwilliams 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

gaaclarke commented Nov 15, 2024

flutter-dashboard bot commented Nov 16, 2024

gaaclarke commented Nov 14, 2024 •

edited

Loading