Skip to content

Commit dfd92b7

Browse files
auto-submit[bot]auto-submit[bot]
auto-submit[bot]
and
auto-submit[bot]
authored
Reverts "Fixes metal vec3 uniform padding (#181340)" (#181552)
<!-- start_original_pr_link --> Reverts: #181340 <!-- end_original_pr_link --> <!-- start_initiating_author --> Initiated by: gaaclarke <!-- end_initiating_author --> <!-- start_revert_reason --> Reason for reverting: golden failures and known logical errors that didn't have test coverage <!-- end_revert_reason --> <!-- start_original_pr_author --> Original PR Author: gaaclarke <!-- end_original_pr_author --> <!-- start_reviewers --> Reviewed By: {walley892, b-luk} <!-- end_reviewers --> <!-- start_revert_body --> This change reverts the following previous change: fixes #180873 ## 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], including [Features we expect every widget to implement]. - [x] I signed the [CLA]. - [x] I listed at least one issue that this PR fixes in the description above. - [x] I updated/added relevant documentation (doc comments with `///`). - [x] I added new tests to check the change I am making, or this PR is [test-exempt]. - [x] I followed the [breaking change policy] and added [Data Driven Fixes] where supported. - [x] All existing and new tests are passing. If you need help, consider asking for advice on the #hackers-new channel on [Discord]. **Note**: The Flutter team is currently trialing the use of [Gemini Code Assist for GitHub](https://developers.google.com/gemini-code-assist/docs/review-github-code). Comments from the `gemini-code-assist` bot should not be taken as authoritative feedback from the Flutter team. If you find its comments useful you can update your code accordingly, but if you are unsure or disagree with the feedback, please feel free to wait for a Flutter team member's review for guidance on which automated comments should be addressed. <!-- Links --> [Contributor Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#overview [Tree Hygiene]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md [test-exempt]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#tests [Flutter Style Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md [Features we expect every widget to implement]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md#features-we-expect-every-widget-to-implement [CLA]: https://cla.developers.google.com/ [flutter/tests]: https://github.com/flutter/tests [breaking change policy]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#handling-breaking-changes [Discord]: https://github.com/flutter/flutter/blob/main/docs/contributing/Chat.md [Data Driven Fixes]: https://github.com/flutter/flutter/blob/main/docs/contributing/Data-driven-Fixes.md <!-- end_revert_body --> Co-authored-by: auto-submit[bot] <flutter-engprod-team@google.com>
1 parent d7d35fd commit dfd92b7

15 files changed

Lines changed: 104 additions & 190 deletions

‎engine/src/flutter/impeller/compiler/compiler_test.cc‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -106,7 +106,6 @@ bool CompilerTestBase::CanCompileAndReflect(
106106
entry_point_name);
107107

108108
Reflector::Options reflector_options;
109-
reflector_options.target_platform = GetParam();
110109
reflector_options.header_file_name = ReflectionHeaderName(fixture_name);
111110
reflector_options.shader_name = "shader_name";
112111

‎engine/src/flutter/impeller/compiler/reflector.cc‎

Lines changed: 6 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -26,7 +26,6 @@
2626
#include "impeller/geometry/matrix.h"
2727
#include "impeller/geometry/scalar.h"
2828
#include "impeller/runtime_stage/runtime_stage.h"
29-
#include "runtime_stage_types_flatbuffers.h"
3029
#include "spirv_common.hpp"
3130

3231
namespace impeller {
@@ -373,27 +372,6 @@ std::shared_ptr<RuntimeStageData::Shader> Reflector::GenerateRuntimeStageData()
373372
uniform_description.columns = spir_type.columns;
374373
uniform_description.bit_width = spir_type.width;
375374
uniform_description.array_elements = GetArrayElements(spir_type);
376-
377-
if (TargetPlatformIsMetal(options_.target_platform) &&
378-
uniform_description.type == spirv_cross::SPIRType::BaseType::Float) {
379-
// Metal aligns float3 to 16 bytes.
380-
// Metal aligns float3x3 COLUMNS to 16 bytes.
381-
// For float3: Size 12. Padding 4. Stride 16.
382-
// For float3x3: Size 36. Padding 12 (4 per col). Stride 48.
383-
384-
if (spir_type.vecsize == 3 &&
385-
(spir_type.columns == 1 || spir_type.columns == 3)) {
386-
for (size_t c = 0; c < spir_type.columns; c++) {
387-
for (size_t v = 0; v < 3; v++) {
388-
uniform_description.padding_layout.push_back(
389-
fb::PaddingType::kFloat);
390-
}
391-
uniform_description.padding_layout.push_back(
392-
fb::PaddingType::kPadding);
393-
}
394-
}
395-
}
396-
397375
FML_CHECK(data->backend != RuntimeStageBackend::kVulkan ||
398376
spir_type.basetype ==
399377
spirv_cross::SPIRType::BaseType::SampledImage)
@@ -418,7 +396,7 @@ std::shared_ptr<RuntimeStageData::Shader> Reflector::GenerateRuntimeStageData()
418396
size_t binding =
419397
compiler_->get_decoration(ubo.id, spv::Decoration::DecorationBinding);
420398
auto members = ReadStructMembers(ubo.type_id);
421-
std::vector<fb::PaddingType> padding_layout;
399+
std::vector<uint8_t> struct_layout;
422400
size_t float_count = 0;
423401

424402
for (size_t i = 0; i < members.size(); i += 1) {
@@ -429,7 +407,7 @@ std::shared_ptr<RuntimeStageData::Shader> Reflector::GenerateRuntimeStageData()
429407
size_t padding_count =
430408
(member.size + sizeof(float) - 1) / sizeof(float);
431409
while (padding_count > 0) {
432-
padding_layout.push_back(fb::PaddingType::kPadding);
410+
struct_layout.push_back(0);
433411
padding_count--;
434412
}
435413
break;
@@ -440,18 +418,18 @@ std::shared_ptr<RuntimeStageData::Shader> Reflector::GenerateRuntimeStageData()
440418
// and 0 layout property per byte of padding
441419
for (auto i = 0; i < member.array_elements; i++) {
442420
for (auto j = 0u; j < member.size / sizeof(float); j++) {
443-
padding_layout.push_back(fb::PaddingType::kFloat);
421+
struct_layout.push_back(1);
444422
}
445423
for (auto j = 0u; j < member.element_padding / sizeof(float);
446424
j++) {
447-
padding_layout.push_back(fb::PaddingType::kPadding);
425+
struct_layout.push_back(0);
448426
}
449427
}
450428
} else {
451429
size_t member_float_count = member.byte_length / sizeof(float);
452430
float_count += member_float_count;
453431
while (member_float_count > 0) {
454-
padding_layout.push_back(fb::PaddingType::kFloat);
432+
struct_layout.push_back(1);
455433
member_float_count--;
456434
}
457435
}
@@ -468,7 +446,7 @@ std::shared_ptr<RuntimeStageData::Shader> Reflector::GenerateRuntimeStageData()
468446
.location = binding,
469447
.binding = binding,
470448
.type = spirv_cross::SPIRType::Struct,
471-
.padding_layout = std::move(padding_layout),
449+
.struct_layout = std::move(struct_layout),
472450
.struct_float_count = float_count,
473451
});
474452
}

‎engine/src/flutter/impeller/compiler/runtime_stage_data.cc‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -332,8 +332,8 @@ std::unique_ptr<fb::RuntimeStageT> RuntimeStageData::CreateStageFlatbuffer(
332332
desc->array_elements = uniform.array_elements.value();
333333
}
334334

335-
for (const auto& byte_type : uniform.padding_layout) {
336-
desc->padding_layout.push_back(static_cast<fb::PaddingType>(byte_type));
335+
for (const auto& byte_type : uniform.struct_layout) {
336+
desc->struct_layout.push_back(static_cast<fb::StructByteType>(byte_type));
337337
}
338338
desc->struct_float_count = uniform.struct_float_count;
339339

‎engine/src/flutter/impeller/compiler/types.h‎

Lines changed: 1 addition & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,6 @@
1212
#include <optional>
1313
#include <string>
1414

15-
#include "runtime_stage_types_flatbuffers.h"
1615
#include "shaderc/shaderc.hpp"
1716
#include "spirv_cross.hpp"
1817
#include "spirv_msl.hpp"
@@ -56,11 +55,7 @@ struct UniformDescription {
5655
size_t columns = 0u;
5756
size_t bit_width = 0u;
5857
std::optional<size_t> array_elements = std::nullopt;
59-
/// The layout of padding bytes in the uniform buffer.
60-
/// The format matches the values in the flatbuffer
61-
/// UniformDescription::padding_layout.
62-
/// \see RuntimeEffectContents::EmplaceUniform
63-
std::vector<fb::PaddingType> padding_layout = {};
58+
std::vector<uint8_t> struct_layout = {};
6459
size_t struct_float_count = 0u;
6560
};
6661

‎engine/src/flutter/impeller/core/runtime_types.cc‎

Lines changed: 2 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -4,35 +4,16 @@
44

55
#include "impeller/core/runtime_types.h"
66

7-
#include "flutter/fml/logging.h"
8-
97
namespace impeller {
108

11-
size_t RuntimeUniformDescription::GetDartSize() const {
12-
// Struct uniforms aren't yet supported, they only exist as Vulkan
13-
// collections.
14-
FML_DCHECK(type != kStruct);
9+
size_t RuntimeUniformDescription::GetSize() const {
1510
size_t size = dimensions.rows * dimensions.cols * bit_width / 8u;
1611
if (array_elements.value_or(0) > 0) {
1712
// Covered by check on the line above.
1813
// NOLINTNEXTLINE(bugprone-unchecked-optional-access)
1914
size *= array_elements.value();
2015
}
21-
return size;
22-
}
23-
24-
size_t RuntimeUniformDescription::GetGPUSize() const {
25-
size_t size = 0;
26-
if (padding_layout.empty()) {
27-
size = dimensions.rows * dimensions.cols * bit_width / 8u;
28-
} else {
29-
size = sizeof(float) * padding_layout.size();
30-
}
31-
if (array_elements.value_or(0) > 0) {
32-
// Covered by check on the line above.
33-
// NOLINTNEXTLINE(bugprone-unchecked-optional-access)
34-
size *= array_elements.value();
35-
}
16+
size += sizeof(float) * struct_layout.size();
3617
return size;
3718
}
3819

‎engine/src/flutter/impeller/core/runtime_types.h‎

Lines changed: 3 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -38,11 +38,6 @@ struct RuntimeUniformDimensions {
3838
size_t cols = 0;
3939
};
4040

41-
enum class RuntimePaddingType : uint8_t {
42-
kPadding = 0,
43-
kFloat = 1,
44-
};
45-
4641
struct RuntimeUniformDescription {
4742
std::string name;
4843
size_t location = 0u;
@@ -52,16 +47,11 @@ struct RuntimeUniformDescription {
5247
RuntimeUniformDimensions dimensions = {};
5348
size_t bit_width = 0u;
5449
std::optional<size_t> array_elements;
55-
std::vector<RuntimePaddingType> padding_layout = {};
50+
std::vector<uint8_t> struct_layout = {};
5651
size_t struct_float_count = 0u;
5752

58-
/// @brief Computes the total number of bytes that this uniform requires for
59-
/// representation in the Dart float buffer.
60-
size_t GetDartSize() const;
61-
62-
/// @brief Computes the total number of bytes that this uniform requires for
63-
/// representation in the GPU.
64-
size_t GetGPUSize() const;
53+
/// @brief Computes the total number of bytes that this uniform requires.
54+
size_t GetSize() const;
6555
};
6656

6757
} // namespace impeller

‎engine/src/flutter/impeller/display_list/dl_runtime_effect_impeller.cc‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -38,7 +38,7 @@ size_t DlRuntimeEffectImpeller::uniform_size() const {
3838

3939
size_t total = 0;
4040
for (const auto& uniform : runtime_stage_->GetUniforms()) {
41-
total += uniform.GetGPUSize();
41+
total += uniform.GetSize();
4242
}
4343
return total;
4444
}

‎engine/src/flutter/impeller/entity/contents/runtime_effect_contents.cc‎

Lines changed: 37 additions & 44 deletions
Original file line numberDiff line numberDiff line change
@@ -24,48 +24,35 @@
2424

2525
namespace impeller {
2626

27+
namespace {
28+
constexpr char kPaddingType = 0;
29+
constexpr char kFloatType = 1;
30+
} // namespace
31+
2732
// static
28-
BufferView RuntimeEffectContents::EmplaceUniform(
29-
const uint8_t* source_data,
33+
BufferView RuntimeEffectContents::EmplaceVulkanUniform(
34+
const std::shared_ptr<const std::vector<uint8_t>>& input_data,
3035
HostBuffer& data_host_buffer,
31-
const RuntimeUniformDescription& uniform) {
32-
size_t minimum_uniform_alignment =
33-
data_host_buffer.GetMinimumUniformAlignment();
34-
size_t alignment = std::max(uniform.bit_width / 8, minimum_uniform_alignment);
35-
36-
if (uniform.padding_layout.empty()) {
37-
return data_host_buffer.Emplace(source_data, uniform.GetGPUSize(),
38-
alignment);
36+
const RuntimeUniformDescription& uniform,
37+
size_t minimum_uniform_alignment) {
38+
// TODO(jonahwilliams): rewrite this to emplace directly into
39+
// HostBuffer.
40+
std::vector<float> uniform_buffer;
41+
uniform_buffer.reserve(uniform.struct_layout.size());
42+
size_t uniform_byte_index = 0u;
43+
for (char byte_type : uniform.struct_layout) {
44+
if (byte_type == kPaddingType) {
45+
uniform_buffer.push_back(0.f);
46+
} else {
47+
FML_DCHECK(byte_type == kFloatType);
48+
uniform_buffer.push_back(reinterpret_cast<const float*>(
49+
input_data->data())[uniform_byte_index++]);
50+
}
3951
}
4052

41-
// If the uniform has a padding layout, we need to repack the data.
42-
// We can do this by using the EmplaceProc to write directly to the
43-
// HostBuffer.
4453
return data_host_buffer.Emplace(
45-
uniform.GetGPUSize(), alignment,
46-
[&uniform, source_data](uint8_t* destination) {
47-
size_t count = uniform.array_elements.value_or(1);
48-
if (count == 0) {
49-
// Make sure to run at least once.
50-
count = 1;
51-
}
52-
size_t uniform_byte_index = 0u;
53-
size_t struct_float_index = 0u;
54-
auto* float_destination = reinterpret_cast<float*>(destination);
55-
auto* float_source = reinterpret_cast<const float*>(source_data);
56-
57-
for (size_t i = 0; i < count; i++) {
58-
for (RuntimePaddingType byte_type : uniform.padding_layout) {
59-
if (byte_type == RuntimePaddingType::kPadding) {
60-
float_destination[struct_float_index++] = 0.f;
61-
} else {
62-
FML_DCHECK(byte_type == RuntimePaddingType::kFloat);
63-
float_destination[struct_float_index++] =
64-
float_source[uniform_byte_index++];
65-
}
66-
}
67-
}
68-
});
54+
reinterpret_cast<const void*>(uniform_buffer.data()),
55+
sizeof(float) * uniform_buffer.size(), minimum_uniform_alignment);
6956
}
7057

7158
void RuntimeEffectContents::SetRuntimeStage(
@@ -297,8 +284,12 @@ bool RuntimeEffectContents::Render(const ContentContext& renderer,
297284
<< "Uniform " << uniform.name
298285
<< " had unexpected type kFloat for Vulkan backend.";
299286

300-
BufferView buffer_view = EmplaceUniform(
301-
uniform_data_->data() + buffer_offset, data_host_buffer, uniform);
287+
size_t alignment =
288+
std::max(uniform.bit_width / 8,
289+
data_host_buffer.GetMinimumUniformAlignment());
290+
BufferView buffer_view =
291+
data_host_buffer.Emplace(uniform_data_->data() + buffer_offset,
292+
uniform.GetSize(), alignment);
302293

303294
ShaderUniformSlot uniform_slot;
304295
uniform_slot.name = uniform.name.c_str();
@@ -307,7 +298,7 @@ bool RuntimeEffectContents::Render(const ContentContext& renderer,
307298
DescriptorType::kUniformBuffer, uniform_slot,
308299
std::move(metadata), std::move(buffer_view));
309300
buffer_index++;
310-
buffer_offset += uniform.GetDartSize();
301+
buffer_offset += uniform.GetSize();
311302
buffer_location++;
312303
break;
313304
}
@@ -318,10 +309,12 @@ bool RuntimeEffectContents::Render(const ContentContext& renderer,
318309
uniform_slot.binding = uniform.location;
319310
uniform_slot.name = uniform.name.c_str();
320311

321-
pass.BindResource(
322-
ShaderStage::kFragment, DescriptorType::kUniformBuffer,
323-
uniform_slot, nullptr,
324-
EmplaceUniform(uniform_data_->data(), data_host_buffer, uniform));
312+
pass.BindResource(ShaderStage::kFragment,
313+
DescriptorType::kUniformBuffer, uniform_slot,
314+
nullptr,
315+
EmplaceVulkanUniform(
316+
uniform_data_, data_host_buffer, uniform,
317+
data_host_buffer.GetMinimumUniformAlignment()));
325318
}
326319
}
327320
}

‎engine/src/flutter/impeller/entity/contents/runtime_effect_contents.h‎

Lines changed: 5 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -37,17 +37,11 @@ class RuntimeEffectContents final : public ColorSourceContents {
3737
bool BootstrapShader(const ContentContext& renderer) const;
3838

3939
// Visible for testing
40-
/// Copies the uniform data into the host buffer.
41-
///
42-
/// If the `uniform` has a `padding_layout`, it is used to repack the data.
43-
///
44-
/// @param source_data The pointer to the start of the uniform data in the
45-
/// source.
46-
/// @param host_buffer The host buffer to emplace the uniform data into.
47-
/// @param uniform The description of the uniform being emplaced.
48-
static BufferView EmplaceUniform(const uint8_t* source_data,
49-
HostBuffer& host_buffer,
50-
const RuntimeUniformDescription& uniform);
40+
static BufferView EmplaceVulkanUniform(
41+
const std::shared_ptr<const std::vector<uint8_t>>& input_data,
42+
HostBuffer& host_buffer,
43+
const RuntimeUniformDescription& uniform,
44+
size_t minimum_uniform_alignment);
5145

5246
private:
5347
bool RegisterShader(const ContentContext& renderer) const;

‎engine/src/flutter/impeller/entity/entity_unittests.cc‎

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1922,9 +1922,12 @@ TEST_P(EntityTest, RuntimeEffectSetsRightSizeWhenUniformIsStruct) {
19221922
uniform_data->resize(sizeof(FragUniforms));
19231923
memcpy(uniform_data->data(), &frag_uniforms, sizeof(FragUniforms));
19241924

1925-
auto buffer_view = RuntimeEffectContents::EmplaceUniform(
1926-
uniform_data->data(), GetContentContext()->GetTransientsDataBuffer(),
1927-
runtime_stage->GetUniforms()[0]);
1925+
auto buffer_view = RuntimeEffectContents::EmplaceVulkanUniform(
1926+
uniform_data, GetContentContext()->GetTransientsDataBuffer(),
1927+
runtime_stage->GetUniforms()[0],
1928+
GetContentContext()
1929+
->GetTransientsDataBuffer()
1930+
.GetMinimumUniformAlignment());
19281931

19291932
// 16 bytes:
19301933
// 8 bytes for iResolution

0 commit comments

Comments
 (0)