[Impeller] Store Buffer/Texture bindings in vector instead of map. #48719

jonahwilliams · 2023-12-06T06:48:48Z

Places the binding data in a vector, since they key was only meaningful on metal but not used anywhere. I don't think that we need to specificially handle the case where our own contents bind the same contents multiple times, but interested to discuss if folks disagree.

jonahwilliams · 2023-12-06T17:03:18Z

impeller/compiler/code_gen_template.h

+  static constexpr auto kResource{{camel_case(sampled_image.name)}} = ShaderUniformSlot { // {{sampled_image.name}}
    "{{sampled_image.name}}",      // name
-    {{sampled_image.ext_res_0}}u,  // texture
-    {{sampled_image.ext_res_1}}u,  // sampler


This was unused

gaaclarke

LGTM, this is a step backwards in readability where people used to use texture_index and are now using ext_res_0. It would be nice if we could come up with a better name there.

gaaclarke · 2023-12-06T17:05:49Z

impeller/renderer/backend/vulkan/binding_helpers_vk.cc

                       std::vector<vk::DescriptorImageInfo>& images,
                       std::vector<vk::WriteDescriptorSet>& writes) {
-  for (const auto& [index, data] : bindings.sampled_images) {
+  for (const auto& data : bindings.sampled_images) {


remove auto please

gaaclarke · 2023-12-06T17:05:58Z

impeller/renderer/backend/vulkan/compute_pass_vk.cc

  barrier.new_layout = vk::ImageLayout::eShaderReadOnlyOptimal;

-  for (const auto& [_, data] : bindings.sampled_images) {
+  for (const auto& data : bindings.sampled_images) {


gaaclarke · 2023-12-06T17:06:12Z

impeller/renderer/backend/vulkan/render_pass_vk.cc

  barrier.new_layout = vk::ImageLayout::eShaderReadOnlyOptimal;

-  for (const auto& [_, data] : bindings.sampled_images) {
+  for (const auto& data : bindings.sampled_images) {


gaaclarke · 2023-12-06T17:09:32Z

impeller/entity/contents/test/contents_test_helpers.h

 template <typename T>
 typename T::FragInfo* GetFragInfo(const Command& command) {
-  auto resource = command.fragment_bindings.buffers.find(0u);
+  auto resource = std::find_if(command.fragment_bindings.buffers.begin(),


So we are getting linear search here hopefully? This should be faster for the small sizes we have.

This is just for testing, so performance doesnt matter as much

We dont ever have to look up particular bindings, we just iterate through the set of bindings and bind each one.

gaaclarke · 2023-12-06T17:11:28Z

impeller/compiler/code_gen_template.h

-    {{sampled_image.ext_res_0}}u,  // texture
-    {{sampled_image.ext_res_1}}u,  // sampler
-    {{sampled_image.binding}}u,    // binding
+    {{sampled_image.ext_res_0}}u,  // ext_res_0


Can we have a better name for this? This represents which texture unit this will be bound to right? ext_res_0 doesn't mean anything to me and seems to be a Metal-ism?

Back to texture_index, with a note that this is ext_res_0

jonahwilliams · 2023-12-06T17:21:33Z

RE: ext_res_0 being a terrible name, I agree. I could call it metal_binding instead?

gaaclarke

Okay, I looked through this a bit more. I don't like us removing SampledImageSlot and using ShaderUniformSlot instead, it's weird and the names don't match up. If we wanted to unify them we should create something that has more generic names.

What I do like in this change though is switching to a vector for storage and removing the unused field. I would prefer we keep those changes but also keep SampledImageSlot.

jonahwilliams · 2023-12-06T17:35:49Z

SG, will do that

jonahwilliams · 2023-12-06T17:38:15Z

My only problem was the previous names was that texture_index implied that was a meaningul value on Vulkan/ GLES whereas it is only valid on Metal.

gaaclarke · 2023-12-06T17:39:07Z

My only problem was the previous names was that texture_index implied that was a meaningul value on Vulkan/ GLES whereas it is only valid on Metal.

I mean if we wanted to be super efficient this could be a uniform since a backend will only ever use one of them.

jonahwilliams · 2023-12-06T17:43:22Z

The index/binding are the values we set to configure the uniforms. It is technically a constant per backend, but since we support multiple backends on the same platform we'd need to content with some kind of runtime look up for the value.

The current approach solves the problem by making the shader metadata just be the union of all backend specific binding values, which works OK for now.

jonahwilliams · 2023-12-06T17:43:41Z

By the current approach I don't mean this PR, just the overall design.

gaaclarke · 2023-12-06T17:52:12Z

Sorry, i said uniform but I meant union. This is what I was thinking.

struct VulkanSampledImageIndices {
  size_t texture_and_sampler_index;
  size_t descriptor_set_index;
};

struct SampledImageSlot {
  const char* name;

  union {
    size_t metal_texture_index;
    VulkanSampledImageIndices vulkan_indices;
  }
};

BTW, it looks like HasTexture doesn't even work on vulkan.

jonahwilliams · 2023-12-06T17:53:42Z

Right, that is more or less what this data is. the caveat is that the command code doesn't know what backend its running from.

Also for a bonus caveat, the GLES binding number has to be looked up at runtime, and requires us to use the separate ShaderMetaData struct + name.

gaaclarke

LGTM!

jonahwilliams · 2023-12-06T18:02:59Z

@gaaclarke maybe next year when I'm feeling more ambitious we can sit down and figure out how to make the binding code ... better 😆

flutter-dashboard · 2023-12-06T18:05:41Z

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie or stuartmorgan on the #hackers channel in Chat (don't just cc them here, they won't see it! Use Discord!).

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.

Hixie · 2023-12-06T18:53:20Z

test-exempt: code refactor with no semantic change

…of map. (flutter/engine#48719)

…139716) flutter/engine@347f506...82de334 2023-12-07 jason-simmons@users.noreply.github.com Revert "Replace use of Fontmgr::RefDefault with explicit creation calls" (flutter/engine#48755) 2023-12-07 skia-flutter-autoroll@skia.org Roll Skia from 2abb01e18ab3 to 8ebf43ba1c09 (1 revision) (flutter/engine#48761) 2023-12-07 skia-flutter-autoroll@skia.org Roll Dart SDK from dbfe4e3f867e to be8a95b6717d (1 revision) (flutter/engine#48757) 2023-12-06 godofredoc@google.com Remove obsolete properties. (flutter/engine#48753) 2023-12-06 skia-flutter-autoroll@skia.org Roll Skia from 7ff0103760d0 to 2abb01e18ab3 (1 revision) (flutter/engine#48751) 2023-12-06 skia-flutter-autoroll@skia.org Roll Skia from 570103e08087 to 7ff0103760d0 (1 revision) (flutter/engine#48748) 2023-12-06 skia-flutter-autoroll@skia.org Roll Skia from 326bdc97ac40 to 570103e08087 (1 revision) (flutter/engine#48746) 2023-12-06 skia-flutter-autoroll@skia.org Roll Dart SDK from 6eb13603c212 to dbfe4e3f867e (1 revision) (flutter/engine#48745) 2023-12-06 jonahwilliams@google.com [Impeller] Store Buffer/Texture bindings in vector instead of map. (flutter/engine#48719) 2023-12-06 skia-flutter-autoroll@skia.org Roll Skia from 33cba437bf70 to 326bdc97ac40 (2 revisions) (flutter/engine#48743) 2023-12-06 jason-simmons@users.noreply.github.com [Impeller] Provide the clear color to an advanced blend if it was optimized out (flutter/engine#48646) 2023-12-06 737941+loic-sharma@users.noreply.github.com [Windows] Set swap interval on raster thread after startup (flutter/engine#47787) 2023-12-06 skia-flutter-autoroll@skia.org Roll Skia from 384d14063dc1 to 33cba437bf70 (3 revisions) (flutter/engine#48740) 2023-12-06 737941+loic-sharma@users.noreply.github.com [Windows] Refactor the GLES proc table (flutter/engine#48688) 2023-12-06 kjlubick@users.noreply.github.com Replace use of Fontmgr::RefDefault with explicit creation calls (flutter/engine#48571) 2023-12-06 jonahwilliams@google.com [Impeller] disable entity culling by default. (flutter/engine#48717) 2023-12-06 skia-flutter-autoroll@skia.org Roll Skia from 23e1cb20a6b5 to 384d14063dc1 (1 revision) (flutter/engine#48738) 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 chinmaygarde@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://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

[Impeller] simplify binding logic further.

181e64c

github-actions bot added the e: impeller label Dec 6, 2023

jonahwilliams marked this pull request as ready for review December 6, 2023 17:02

jonahwilliams requested review from bdero, gaaclarke and matanlurey December 6, 2023 17:02

jonahwilliams commented Dec 6, 2023

View reviewed changes

gaaclarke reviewed Dec 6, 2023

View reviewed changes

jonahwilliams added 2 commits December 6, 2023 09:58

Add back sampled image slot.

3d2c44d

missed one.

1f7dfdc

gaaclarke approved these changes Dec 6, 2023

View reviewed changes

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

jonahwilliams removed the autosubmit Merge PR when tree becomes green via auto submit App label Dec 6, 2023

jonahwilliams added 2 commits December 6, 2023 10:06

revert template changes.

298c2b5

++

4e5a814

jonahwilliams changed the title ~~[Impeller] simplify binding logic further.~~ [Impeller] Store Buffer/Texture bindings in vector instead of map. Dec 6, 2023

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

auto-submit bot merged commit 077e3b5 into flutter:main Dec 6, 2023

engine-flutter-autoroll mentioned this pull request Dec 6, 2023

Roll Flutter Engine from 347f506a341f to 077e3b50ce72 (9 revisions) flutter/flutter#139684

Closed

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

077e3b50c [Impeller] Store Buffer/Texture bindings in vector instead …

1f58f0d

…of map. (flutter/engine#48719)

engine-flutter-autoroll mentioned this pull request Dec 6, 2023

Roll Flutter Engine from 347f506a341f to 64df738fdba0 (11 revisions) flutter/flutter#139686

Closed

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

077e3b50c [Impeller] Store Buffer/Texture bindings in vector instead …

26f9bf4

…of map. (flutter/engine#48719)

engine-flutter-autoroll mentioned this pull request Dec 6, 2023

Roll Flutter Engine from 347f506a341f to 0e63da711f3f (12 revisions) flutter/flutter#139691

Closed

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

077e3b50c [Impeller] Store Buffer/Texture bindings in vector instead …

019dace

…of map. (flutter/engine#48719)

engine-flutter-autoroll mentioned this pull request Dec 6, 2023

Roll Flutter Engine from 347f506a341f to a0d0515aa939 (13 revisions) flutter/flutter#139695

Closed

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

077e3b50c [Impeller] Store Buffer/Texture bindings in vector instead …

846dbe7

…of map. (flutter/engine#48719)

engine-flutter-autoroll mentioned this pull request Dec 6, 2023

Roll Flutter Engine from 347f506a341f to 6f5b6347d472 (14 revisions) flutter/flutter#139701

Closed

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

077e3b50c [Impeller] Store Buffer/Texture bindings in vector instead …

950be46

…of map. (flutter/engine#48719)

engine-flutter-autoroll mentioned this pull request Dec 7, 2023

Roll Flutter Engine from 347f506a341f to d16704bc9b10 (15 revisions) flutter/flutter#139711

Closed

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

077e3b50c [Impeller] Store Buffer/Texture bindings in vector instead …

ef06e39

…of map. (flutter/engine#48719)

engine-flutter-autoroll mentioned this pull request Dec 7, 2023

Roll Flutter Engine from 347f506a341f to 82de3347ccd7 (17 revisions) flutter/flutter#139716

Merged

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

077e3b50c [Impeller] Store Buffer/Texture bindings in vector instead …

98a188a

…of map. (flutter/engine#48719)

jonahwilliams deleted the simplify_binding_more branch December 7, 2023 19:28

[Impeller] Store Buffer/Texture bindings in vector instead of map. #48719

[Impeller] Store Buffer/Texture bindings in vector instead of map. #48719

Uh oh!

Conversation

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

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gaaclarke 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!

Choose a reason for hiding this comment

Uh oh!

jonahwilliams commented Dec 6, 2023

Uh oh!

gaaclarke left a comment

Choose a reason for hiding this comment

Uh oh!

jonahwilliams commented Dec 6, 2023

Uh oh!

jonahwilliams commented Dec 6, 2023

Uh oh!

gaaclarke commented Dec 6, 2023

Uh oh!

jonahwilliams commented Dec 6, 2023

Uh oh!

jonahwilliams commented Dec 6, 2023

Uh oh!

gaaclarke commented Dec 6, 2023

Uh oh!

jonahwilliams commented Dec 6, 2023

Uh oh!

gaaclarke left a comment

Choose a reason for hiding this comment

Uh oh!

jonahwilliams commented Dec 6, 2023

Uh oh!

flutter-dashboard bot commented Dec 6, 2023

Uh oh!

Hixie commented Dec 6, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

jonahwilliams commented Dec 6, 2023 •

edited

Loading