[Impeller] Allow binding multiple vertex buffer views. #55856

bdero · 2024-10-13T02:33:13Z

Resolves flutter/flutter#116168.
(Continuation of #49670)

Makes it possible for us to use arbitrary vertex layouts, including SoA layouts with attributes stored in different DeviceBuffers. CanRenderPerspectiveCube was converted to an SoA attribute layout with two separate buffers as an example.

Works on all the backends!

OpenGLES:

Vulkan:

Metal:

jonahwilliams · 2024-10-14T15:47:22Z

impeller/core/vertex_buffer.h


 struct VertexBuffer {
-  BufferView vertex_buffer;
+  std::variant<BufferView, std::vector<BufferView>> vertex_buffers;


This is a bit of a code smell to me. Pretend the VertexBuffer struct doesn't exist. The render pass API would probably use overloads, something like:

pass.setVertexBuffer(BufferView vertex_buffer); pass.setVertexBuffer(BufferView index_buffer, IndexType type, BufferView vertex_buffer); pass.setVertexBuffer(BufferView index_buffer, IndexType type, BufferView[] vertex_buffers)

what I think this change is doing is essentially moving the overload inside of the struct with the variant. Arguably this struct isn't even necessary.

Binding multiple vertex buffers should be done through a different API that accepts N vertex buffers instead of updating the VertexBuffer struct. That API could also pass them via iterator as to avoid more heap allocations for Vulkan/Metal which can immediately encode them.

If you set a reasonable limit to the number of bound vertex buffers too, then you can have the various render pass structs hold them without doing a heap allocation per draw.

That's a good point. For SoA layouts, we'd need one BufferView per vertex attribute. Looking around, maybe a reasonable limit would be 16.

For Metal, Apple GPUs are limited to 31 vertex attributes (Maximum number of vertex attributes, per vertex
descriptor)

GL implementations expose the attribute limit through GL_MAX_VERTEX_ATTRIBS, and according to this: "This will almost certainly be 16."

Vulkan implementations expose the attribute limit through vkGetPhysicalDeviceProperties (VkPhysicalDeviceLimits.maxVertexInputBindings). wgpu hardcodes their vertex buffer limit to 16.

The consensus seems to be that virtually everything supports 16 and more modern hardware supports ~32. 16 isn't even enough for glTFs with morph targets, but it's a bit ridiculous that those aren't interleaved anyway IMO...

I think the ideal layout varies per target platform as well. my limited understanding is that for tilers its advantageous to have position data in a separate buffer and all other varyings interleaved.

I changed this to add an API for setting the VertexBuffer and IndexBuffer config directly on RenderPass.

I think we should dissolve VertexBuffer completely, but we have a pattern across the codebase where we pass around GeometryResult structs with vertex buffers. I think the right move here would be to switch stuff like GetPositionBuffer to BindPositionBuffer and immediately bind results rather than deferring the bind to the parent scope.

That's a bigger change. I could do it as an immediate follow-up, but I can also do that now if you'd prefer.

I agree with removing VertexBuffer eventually. I think I would leave VertexBuffer as is, and instead change flutter_gpu to not use it.

impeller/renderer/backend/vulkan/render_pass_vk.cc

…cations

bdero · 2024-10-14T20:57:11Z

impeller/renderer/backend/vulkan/render_pass_vk.cc

-                   << " for vertex buffer view";
-    return false;
+  vk::Buffer buffers[kMaxVertexBuffers];
+  vk::DeviceSize vertex_buffer_offsets[kMaxVertexBuffers];


I could add a RenderPassVK override for the single buffer case to avoid most of this stack allocation, but I think it's basically free because we only ever initialize what we use here.

I think this is good! if we find that the array allocation is slower than expected then it should be easy to go back and add a specialization

jonahwilliams · 2024-10-14T22:35:29Z

impeller/renderer/backend/vulkan/render_pass_vk.cc

+  vk::Buffer buffers[kMaxVertexBuffers];
+  vk::DeviceSize vertex_buffer_offsets[kMaxVertexBuffers];
+  for (size_t i = 0; i < vertex_buffer_count; i++) {
+    buffers[i] = DeviceBufferVK::Cast(*vertex_buffers[i].buffer).GetBuffer();


We also need to track each buffer (at least until I refactor us to not need the auto tracking)

Whoops, nice catch. Done.

jonahwilliams · 2024-10-14T22:36:08Z

impeller/core/vertex_buffer.h

  /// packed in the index buffer.
  ///
-  IndexType index_type = IndexType::kUnknown;
+  IndexType index_type = IndexType::kNone;


Why change this one?

It seems like a better default to assume there is no index buffer until one is set with pass.SetIndexBuffer. Otherwise stuff that doesn't want to use an index buffer will need to call pass.SetIndexBuffer({}, IndexBuffer.kNone)

Although I guess in practice, stuff that doesn't use an index buffer will need to call pass.SetIndexBuffer({}, IndexBuffer.kNone) anyway, since it would otherwise be making assumptions about the previous draw call.

Oh in flutter_gpu? I think you should actually use an entirely different struct. Lets maybe pretend this one doesn't exist since its completely tied to how all the existing contents/geometries work.

Nah I meant in Impeller in general. I'm actually thinking we should remove bool BindVertices(VertexBuffer buffer) entirely and disallow setting the vertex/index buffers using this struct everywhere. I changed this back to kUnknown for now.

Yeah see comment above. i agree with removing it (later) for now I would not further change the struct and instead update flutter_gpu to use the other render pass APIs instead.

jonahwilliams · 2024-10-14T22:36:17Z

impeller/core/vertex_buffer.h

+
 struct VertexBuffer {
-  std::variant<BufferView, std::vector<BufferView>> vertex_buffers;
+  std::array<BufferView, kMaxVertexBuffers> vertex_buffers;


I think the VertexBuffer struct can keep the single buffer view slot, right?

GLES relies on this struct in the command for the list of bindings. But I could potentially remove the usage of this from impeller::Command and put the properties inline.

I'll try to make this change now so we can avoid changing the VertexBuffer struct.

Alright, I've changed this so that impeller::Command no longer stores a VertexBuffer and changed VertexBuffer back to its original state.

jonahwilliams · 2024-10-14T22:37:11Z

impeller/renderer/render_pass.h

+  bool SetVertexBuffer(BufferView vertex_buffer, size_t vertex_count);
+
+  //----------------------------------------------------------------------------
+  /// @brief      Specify a set of vertex buffers to use for this command.


Nit: document the limit as 16 buffers here and below.

…fer.

jonahwilliams

LGTM!

…tter/engine#55856)

…156794) flutter/engine@d4b85da...0e143d7 2024-10-15 [email protected] [Impeller] Allow binding multiple vertex buffer views. (flutter/engine#55856) 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

…e#55856) Resolves flutter#116168. (Continuation of flutter/engine#49670) Makes it possible for us to use arbitrary vertex layouts, including SoA layouts with attributes stored in different DeviceBuffers. CanRenderPerspectiveCube was converted to an SoA attribute layout with two separate buffers as an example. Works on all the backends! OpenGLES: <img width="1136" alt="image" src="https://github.com/user-attachments/assets/e2398fde-532f-402d-899a-39aaa556f24f"> Vulkan: <img width="1136" alt="image" src="https://github.com/user-attachments/assets/1c1bf664-bec1-43cb-ab2e-eb2a74718bfd"> Metal: <img width="1136" alt="image" src="https://github.com/user-attachments/assets/bf59da17-cf52-4913-88e4-ab6f0bd6fc96">

bdero added the e: impeller label Oct 13, 2024

bdero force-pushed the bdero/multiple-vertex-buffers2 branch from 2292898 to 6495a98 Compare October 13, 2024 02:40

[Impeller] Allow binding multiple vertex buffer views.

12d9579

bdero force-pushed the bdero/multiple-vertex-buffers2 branch from 6495a98 to 12d9579 Compare October 13, 2024 02:45

bdero added 2 commits October 13, 2024 20:41

Fix descriptor sets in test

e3e246d

Fix GLES oopsie

ee212d0

bdero marked this pull request as ready for review October 14, 2024 04:00

bdero requested review from chinmaygarde and jonahwilliams October 14, 2024 04:00

jonahwilliams suggested changes Oct 14, 2024

View reviewed changes

bdero added 2 commits October 14, 2024 13:07

Set vertex/index buffers directly on the RenderPass & avoid heap allo…

479eac6

…cations

Temp adjustment of VertexBuffer usage.

e0164e6

bdero commented Oct 14, 2024

View reviewed changes

bdero requested a review from jonahwilliams October 14, 2024 21:06

jonahwilliams reviewed Oct 14, 2024

View reviewed changes

bdero added 4 commits October 14, 2024 15:59

Jonah review

aad4a1e

Place command properties inline for impeller::Command/GLES

3c21d50

Change VertexBuffer back to how it was before.

65a03dc

Fix ownership issue in test and add ownership warning to SetVertexBuf…

9115154

…fer.

bdero requested a review from jonahwilliams October 15, 2024 00:01

Remove useless import

6cfe443

jonahwilliams approved these changes Oct 15, 2024

View reviewed changes

bdero added the autosubmit Merge PR when tree becomes green via auto submit App label Oct 15, 2024

auto-submit bot merged commit 0e143d7 into flutter:main Oct 15, 2024

engine-flutter-autoroll mentioned this pull request Oct 15, 2024

Roll Flutter Engine from d4b85da14b8e to 0e143d71dbfe (1 revision) flutter/flutter#156794

Merged

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

0e143d71d [Impeller] Allow binding multiple vertex buffer views. (flu…

fcfedf5

…tter/engine#55856)

bdero mentioned this pull request Oct 17, 2024

[Flutter GPU] Support configurable vertex layouts. flutter/flutter#145013

Open

[Impeller] Allow binding multiple vertex buffer views. #55856

[Impeller] Allow binding multiple vertex buffer views. #55856

Uh oh!

Conversation

bdero commented Oct 13, 2024 • 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!

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!

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!

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 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

bdero commented Oct 13, 2024 •

edited

Loading