[Impeller] create a 300 es variant of all GLES shaders to support UBO binding. by jonahwilliams · Pull Request #56960 · flutter/engine

jonahwilliams · 2024-12-05T03:47:25Z

Create a GLES3 "backend" by compiling a second set of GLES shaders to 300 es. This allows the usage of UBOs and SSBOs.

flutter-dashboard · 2024-12-05T03:47:27Z

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption, contact "@test-exemption-reviewer" in the #hackers channel in Discord (don't just cc them here, they won't see it!).

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. The test exemption team is a small volunteer group, so all reviewers should feel empowered to ask for tests, without delegating that responsibility entirely to the test exemption group.

shell/common/shell.cc

chinmaygarde · 2024-12-05T19:04:22Z

impeller/renderer/backend/gles/device_buffer_gles.cc

 #include <cstring>
 #include <memory>

+#include "GLES3/gl3.h"


Like vk.h, the convention in the GL backend is to include //impeller/renderer/backend/gles/gles.h.

chinmaygarde · 2024-12-05T19:08:29Z

impeller/renderer/backend/gles/buffer_bindings_gles.cc

+  gl.GetProgramiv(program, GL_ACTIVE_UNIFORM_BLOCKS, &uniform_blocks);
+  for (GLint i = 0; i < uniform_blocks; i++) {
+    GLint name_length = 0;
+    gl.GetActiveUniformBlockiv(program, i, GL_UNIFORM_BLOCK_NAME_LENGTH,


Paranoid note: Does the name include the NULL terminator?

according to https://registry.khronos.org/OpenGL-Refpages/gl4/html/glGetActiveUniformBlock.xhtml it does

chinmaygarde · 2024-12-05T19:10:02Z

impeller/renderer/backend/gles/buffer_bindings_gles.cc

+
+    std::vector<GLchar> name;
+    name.resize(name_length);
+    GLint length;


Like name_length above, zero this out so an error in the next call doesn't leave garbage in the variable that you will later use to create a string.

chinmaygarde · 2024-12-05T19:11:19Z

impeller/renderer/backend/gles/buffer_bindings_gles.cc

+        std::make_pair(block_index, i);
+  }
+  use_ubo_ = true;
+  return ReadUniformsBindingsV2(gl, program);


Nicely done.

chinmaygarde · 2024-12-05T19:12:17Z

impeller/renderer/backend/gles/device_buffer_gles.h


  void Flush(std::optional<Range> range = std::nullopt) const override;

+  std::optional<GLuint> GetHandle() const {


chinmaygarde · 2024-12-05T19:17:33Z

impeller/renderer/backend/gles/buffer_bindings_gles.cc

+                      element_count,  // count
+                      buffer_data     // data
+        );
+        continue;


Perhaps add the default: case that logs the member type so we don't silently ignore stuff? But yeah, at this point, probably all types have shaken out.

… Android. (#159857) Required changes for flutter/engine#56960 This configures an additional shader target for Android devices.

…port UBO binding. (flutter/engine#56960)

flutter/engine@3a8204a...bbe33f2 2024-12-05 flar@google.com [DisplayList] Disable group opacity when a RuntimeEffect is in use (flutter/engine#56936) 2024-12-05 30870216+gaaclarke@users.noreply.github.com Removes ReactorGLES::Ref (flutter/engine#56981) 2024-12-05 jonahwilliams@google.com [Impeller] create a 300 es variant of all GLES shaders to support UBO binding. (flutter/engine#56960) 2024-12-05 dkwingsmt@users.noreply.github.com [Impeller] Add rounded superellipse (flutter/engine#56726) 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 bdero@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

jonahwilliams · 2024-12-06T16:38:20Z

It looks like the code size hit for this is about ~120 Kb. I don't know if this is compressed or uncompressed.

https://flutter-flutter-perf.skia.org/e/?keys=X7d49843668c5a555309193ee145055fe&selected=commit%3D43599%26name%3D%252Carch%253Darm%252Cbranch%253Dmaster%252Cconfig%253Ddefault%252Cdevice_type%253Dmokey%252Cdevice_version%253Dnone%252Chost_type%253Dmac%252Csub_result%253Dlibflutter_uncompressed_bytes%252Ctest%253Dhello_world_android__compile%252C&xbaroffset=43598

… binding. (flutter/engine#56960) Create a GLES3 "backend" by compiling a second set of GLES shaders to 300 es. This allows the usage of UBOs and SSBOs.

ubo baby.

1ad1f94

github-actions bot added platform-android e: impeller labels Dec 5, 2024

jonahwilliams commented Dec 5, 2024

View reviewed changes

shell/common/shell.cc Outdated Show resolved Hide resolved

jonahwilliams added 5 commits December 4, 2024 21:25

move to method on shell, null check ubo lookup.

4fe533c

more updates.

5f46d14

add missing override.

dd05a42

one more override.

bd1e80d

++

f2442f2

jonahwilliams changed the title ~~gles3 android backend~~ [Impeller] create a 300 es variant of all GLES shaders to support UBO binding. Dec 5, 2024

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

678fb0e

chinmaygarde approved these changes Dec 5, 2024

View reviewed changes

cleanups.

854355c

jonahwilliams mentioned this pull request Dec 5, 2024

[flutter_tools] configure shader compiler to output GLES3 shaders for Android. flutter/flutter#159857

Merged

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

auto-submit bot merged commit 2fdce10 into flutter:main Dec 5, 2024

jonahwilliams deleted the do_ubo_baby branch December 5, 2024 21:05

engine-flutter-autoroll mentioned this pull request Dec 5, 2024

Roll Flutter Engine from 3a8204a0ab41 to bbe33f2d8df2 (4 revisions) flutter/flutter#159865

Merged

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

2fdce10 [Impeller] create a 300 es variant of all GLES shaders to sup…

ffec1ec

…port UBO binding. (flutter/engine#56960)


		void Flush(std::optional<Range> range = std::nullopt) const override;

		std::optional<GLuint> GetHandle() const {

Comments

Conversation

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

Uh oh!

Uh oh!

flutter-dashboard bot commented Dec 5, 2024

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!

jonahwilliams commented Dec 6, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

jonahwilliams commented Dec 5, 2024 •

edited

Loading