[Impeller] Make interleaved layout (more) explicit in generated headers. #42628

jonahwilliams · 2023-06-07T17:59:33Z

Working on #42415 , I found that it was difficult to customize the buffer layout as the interleaved layout is implicitly confiured in the backends. Rather than bake another built in layout, I've pulled (most) information about buffer layout into the generated headers so it is explicitly confiured, which should allow easier customization as the backend has fewer choices to make.

TBD whether or not we need to do something weird for GLES since stride has a different meaning there...

Work towards flutter/flutter#116168

jonahwilliams · 2023-06-07T18:00:02Z

impeller/renderer/backend/gles/buffer_bindings_gles.cc

-    const std::vector<ShaderStageIOSlot>& p_inputs) {
-  // Attrib locations have to be iterated over in order of location because we
-  // will be calculating offsets later.
-  auto inputs = p_inputs;


don't need to sort, offsets must already be computed

jonahwilliams · 2023-06-07T18:00:13Z

impeller/renderer/backend/metal/vertex_descriptor_mtl.h


  MTLVertexDescriptor* GetMTLVertexDescriptor() const;

 private:
-  struct StageInput {


don't need to sort, offsets must already be computed

jonahwilliams · 2023-06-07T18:00:50Z

impeller/renderer/pipeline_builder.h

-        return false;
-      }
-
+      vertex_descriptor->SetStageInputs(VertexShader::kAllShaderStageInputs,


There methods were unconditionally returning true so I've removed the fake validation. To do actual validation we need the backends

jonahwilliams · 2023-06-07T18:01:14Z

impeller/renderer/vertex_descriptor.cc

  return seed;
 }

 // |Comparable<VertexDescriptor>|
 bool VertexDescriptor::IsEqual(const VertexDescriptor& other) const {
-  return inputs_ == other.inputs_;
+  return inputs_ == other.inputs_ && layouts_ == other.layouts_;


This doesn't actually seem like the right comparison but unsure if its actually used.

jonahwilliams · 2023-06-07T18:02:28Z

impeller/compiler/code_gen_template.h

@@ -86,7 +86,8 @@ struct {{camel_case(shader_name)}}{{camel_case(shader_stage)}}Shader {
    {{stage_input.type.type_name}},     // type
    {{stage_input.type.bit_width}}u,    // bit width of type
    {{stage_input.type.vec_size}}u,     // vec size
-    {{stage_input.type.columns}}u       // number of columns
+    {{stage_input.type.columns}}u,      // number of columns
+    {{stage_input.offset}}u,            // offset for interleaved layout


Essentially the addtions here are:

ShaderStageIO must have an offset

A secondary array that has binding+stride info

We may need more info but this is good enough for now. TBD weirdness about stride in GLES

flutter-dashboard · 2023-06-07T18:54:59Z

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 #42628 at sha d5acf20

jonahwilliams · 2023-06-07T18:56:06Z

I appear to have broken some things 🥲

jonahwilliams · 2023-06-07T18:56:45Z

actually just text thankfully

Fixes flutter/flutter#128283 Why doesn't this happen elsewhere? might depend on how spirv compiles shaders or the order they run in. Long term fix is #42628 which computes all offset data in the compiler ![flutter_04](https://github.com/flutter/engine/assets/8975114/645c5db6-4f2f-42f7-af8f-6230bba72b2b)

…t_stride_compiler

jonahwilliams · 2023-06-13T20:29:06Z

@dnfield @bdero PTAL, this should be working now. Doesn't fully allow customizing vertex layout, but completely moves offset computation offline and removes the need to sort. Should be a better base to build #42415 on

jonahwilliams · 2023-06-13T20:30:39Z

impeller/renderer/vertex_descriptor.cc

  for (size_t i = 0; i < count; i++) {
    inputs_.emplace_back(*stage_inputs[i]);
  }
-  // TODO(jonahwilliams): vulkan shader stage needed sorting too. Remove once


Remove, offsets are already computed so no sorting needs to happen.

dnfield · 2023-06-14T19:41:40Z

impeller/compiler/reflector.cc

+    const auto type = compiler_->get_type(resource.type_id);
+    auto location = compiler_->get_decoration(
+        resource.id, spv::Decoration::DecorationLocation);
+    // Malformed shader, will be caught later on.


Why not have some kind of VALIDATION_LOG here?

So this happens with invalid inputs, there is already a validation log that fires and exit that happens, so we'd either need to perform all of that work twice (since this code only runs on vertex shaders), or we do what I did here and let the later stage catch it.

…ated headers. (flutter/engine#42628)

…128959) flutter/engine@9934c0d...48e0b4e 2023-06-15 [email protected] Roll Skia from 0b66c6928dcf to 2d531d020c26 (3 revisions) (flutter/engine#42883) 2023-06-15 [email protected] Raster cache should preserve RTree for overlay layers (flutter/engine#42552) 2023-06-15 [email protected] Roll Skia from c0c74b433117 to 0b66c6928dcf (1 revision) (flutter/engine#42879) 2023-06-15 [email protected] [Linux] Allow BasicMessageChannel sending and responding to null message (flutter/engine#42808) 2023-06-15 [email protected] Roll Skia from 12375fb6f3c8 to c0c74b433117 (1 revision) (flutter/engine#42876) 2023-06-15 [email protected] Roll Skia from d62221bd33a6 to 12375fb6f3c8 (1 revision) (flutter/engine#42873) 2023-06-15 [email protected] Roll Skia from e2e0256d4c6a to d62221bd33a6 (1 revision) (flutter/engine#42871) 2023-06-15 [email protected] Roll Skia from c3abd540c7f9 to e2e0256d4c6a (1 revision) (flutter/engine#42869) 2023-06-15 [email protected] Roll Fuchsia Linux SDK from uvmDF7KM34dWGdsuK... to 53EjCyuRu91oFTBf2... (flutter/engine#42868) 2023-06-15 [email protected] Roll Fuchsia Mac SDK from h3-8RUVrC889UXou7... to P7QA6bfO_Ij5dre7B... (flutter/engine#42867) 2023-06-15 [email protected] Roll Skia from 2718866006d2 to c3abd540c7f9 (1 revision) (flutter/engine#42866) 2023-06-15 [email protected] Roll Skia from 19051bc5fc90 to 2718866006d2 (33 revisions) (flutter/engine#42864) 2023-06-15 [email protected] Roll Dart SDK from 922c315b2c34 to 8eaed3382237 (1 revision) (flutter/engine#42862) 2023-06-15 [email protected] Add missing artifact to the android_arm64_profile config. (flutter/engine#42858) 2023-06-14 [email protected] Build skia with expat (flutter/engine#42859) 2023-06-14 [email protected] [ios] use interfaceOrientation orientation on iOS 13 and above (flutter/engine#42846) 2023-06-14 [email protected] Manual roll Dart SDK from f1387834bfd9 to 922c315b2c34 (4 revisions) (flutter/engine#42855) 2023-06-14 [email protected] [Impeller] Make interleaved layout (more) explicit in generated headers. (flutter/engine#42628) 2023-06-14 [email protected] Renamed validation layers build (flutter/engine#42826) 2023-06-14 [email protected] [ios] view controller based status bar (flutter/engine#42643) 2023-06-14 [email protected] Roll Skia from 6d5dc31d88e2 to 19051bc5fc90 (25 revisions) (flutter/engine#42828) 2023-06-14 [email protected] Roll Fuchsia Linux SDK from Xi3c5nti2LKnEOqYt... to uvmDF7KM34dWGdsuK... (flutter/engine#42842) 2023-06-14 [email protected] Fix generateLockfiles running directory for documentation (flutter/engine#42734) 2023-06-14 [email protected] Roll Fuchsia Mac SDK from Cld7-rm6ZmCOO8j-K... to h3-8RUVrC889UXou7... (flutter/engine#42839) 2023-06-14 [email protected] Roll ANGLE from 7e075469ff02 to 3a3a3c655a96 (8 revisions) (flutter/engine#42834) 2023-06-14 [email protected] Roll Dart SDK from c4e9794df8af to f1387834bfd9 (1 revision) (flutter/engine#42833) Also rolling transitive DEPS: fuchsia/sdk/core/linux-amd64 from Xi3c5nti2LKn to 53EjCyuRu91o fuchsia/sdk/core/mac-amd64 from Cld7-rm6ZmCO to P7QA6bfO_Ij5 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],[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://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

[Impeller] make interleaved layout (more) explicit in generated headers

d5acf20

jonahwilliams commented Jun 7, 2023

View reviewed changes

jonahwilliams requested a review from bdero June 7, 2023 18:01

jonahwilliams commented Jun 7, 2023

View reviewed changes

jonahwilliams requested a review from dnfield June 7, 2023 18:13

flutter-dashboard bot added the will affect goldens label Jun 7, 2023

++

bce747d

jonahwilliams added the Work in progress (WIP) Not ready (yet) for review! label Jun 7, 2023

jonahwilliams mentioned this pull request Jun 8, 2023

[Impeller] sort all vertex inputs by location. #42664

Merged

jonahwilliams added 2 commits June 13, 2023 09:53

Merge branch 'master' of github.com:flutter/engine into compute_offse…

3237c3c

…t_stride_compiler

check for size

7cd9792

github-actions bot added the e: impeller label Jun 13, 2023

++

f5b6627

jonahwilliams removed Work in progress (WIP) Not ready (yet) for review! will affect goldens labels Jun 13, 2023

jonahwilliams added 3 commits June 13, 2023 11:03

++

0e251f7

dont assume locations are valid

9cc0bc5

+

154b63b

Merge branch 'main' into compute_offset_stride_compiler

4235ecf

jonahwilliams commented Jun 13, 2023

View reviewed changes

chinmaygarde changed the title ~~[Impeller] make interleaved layout (more) explicit in generated headers~~ [Impeller] Make interleaved layout (more) explicit in generated headers. Jun 13, 2023

chinmaygarde assigned jonahwilliams Jun 13, 2023

dnfield reviewed Jun 14, 2023

View reviewed changes

dnfield approved these changes Jun 14, 2023

View reviewed changes

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

auto-submit bot merged commit 9988f20 into flutter:main Jun 14, 2023

engine-flutter-autoroll mentioned this pull request Jun 15, 2023

Roll Flutter Engine from 9934c0de738c to 48e0b4e66422 (26 revisions) flutter/flutter#128959

Merged

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

9988f206a [Impeller] Make interleaved layout (more) explicit in gener…

ca72a17

…ated headers. (flutter/engine#42628)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[Impeller] Make interleaved layout (more) explicit in generated headers. #42628

[Impeller] Make interleaved layout (more) explicit in generated headers. #42628

Uh oh!

jonahwilliams commented Jun 7, 2023

Uh oh!

jonahwilliams Jun 7, 2023

Uh oh!

jonahwilliams Jun 7, 2023

Uh oh!

jonahwilliams Jun 7, 2023

Uh oh!

jonahwilliams Jun 7, 2023

Uh oh!

jonahwilliams Jun 7, 2023

Uh oh!

flutter-dashboard bot commented Jun 7, 2023

Uh oh!

jonahwilliams commented Jun 7, 2023

Uh oh!

jonahwilliams commented Jun 7, 2023

Uh oh!

jonahwilliams commented Jun 13, 2023

Uh oh!

jonahwilliams Jun 13, 2023

Uh oh!

dnfield Jun 14, 2023

Uh oh!

jonahwilliams Jun 14, 2023

Uh oh!

Uh oh!

[Impeller] Make interleaved layout (more) explicit in generated headers. #42628

[Impeller] Make interleaved layout (more) explicit in generated headers. #42628

Uh oh!

Conversation

jonahwilliams commented Jun 7, 2023

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!

flutter-dashboard bot commented Jun 7, 2023

Uh oh!

jonahwilliams commented Jun 7, 2023

Uh oh!

jonahwilliams commented Jun 7, 2023

Uh oh!

jonahwilliams commented Jun 13, 2023

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!