Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

[Impeller] Support user defined structs in buffers, clean up compute tests #37084

Merged
merged 2 commits into from
Oct 27, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 23 additions & 0 deletions impeller/compiler/reflector.cc
Original file line number Diff line number Diff line change
Expand Up @@ -603,6 +603,29 @@ std::vector<StructMember> Reflector::ReadStructMembers(

FML_CHECK(current_byte_offset == struct_member_offset);

// A user defined struct.
if (member.basetype == spirv_cross::SPIRType::BaseType::Struct) {
const size_t size =
GetReflectedStructSize(ReadStructMembers(member.self));
uint32_t stride = GetArrayStride<0>(struct_type, member, i);
if (stride == 0) {
stride = size;
}
uint32_t element_padding = stride - size;
result.emplace_back(StructMember{
compiler_->get_name(member.self), // type
BaseTypeToString(member.basetype), // basetype
GetMemberNameAtIndex(struct_type, i), // name
struct_member_offset, // offset
size, // size
stride * array_elements.value_or(1), // byte_length
array_elements, // array_elements
element_padding, // element_padding
});
current_byte_offset += stride * array_elements.value_or(1);
continue;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this adds support for nested structs across all backend and stage types? Might be worth adding a small raster test that uses a nested struct (and make sure the GLES metadata doesn't need to be updated for this).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Proooobably?

I'll give it a try.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm going to punt on this fornow - I'm still trying to track down what to do about the buffer to texture issue and want to keep making progress on compute right now.


// Tightly packed 4x4 Matrix is special cased as we know how to work with
// those.
if (member.basetype == spirv_cross::SPIRType::BaseType::Float && //
Expand Down
12 changes: 9 additions & 3 deletions impeller/fixtures/sample.comp
Original file line number Diff line number Diff line change
@@ -1,6 +1,11 @@
layout(local_size_x = 128) in;
layout(std430) buffer;

struct SomeStruct {
vec2 vf;
uint i;
};

layout(binding = 0) writeonly buffer Output {
vec4 elements[];
} output_data;
Expand All @@ -12,6 +17,7 @@ layout(binding = 1) readonly buffer Input0 {
} input_data0;

layout(binding = 2) readonly buffer Input1 {
SomeStruct some_struct;
uvec2 fixed_array[4];
vec4 elements[];
} input_data1;
Expand All @@ -30,7 +36,7 @@ void main()
}

output_data.elements[ident] = input_data0.elements[ident] * input_data1.elements[ident];
output_data.elements[ident].x += input_data0.fixed_array[1].x;
output_data.elements[ident].y += input_data1.fixed_array[0].y;
output_data.elements[ident].z += input_data0.some_int;
output_data.elements[ident].x += input_data0.fixed_array[1].x + input_data1.some_struct.i;
output_data.elements[ident].y += input_data1.fixed_array[0].y + input_data1.some_struct.vf.x;
output_data.elements[ident].z += input_data0.some_int + input_data1.some_struct.vf.y;
}
2 changes: 2 additions & 0 deletions impeller/playground/BUILD.gn
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,8 @@ impeller_component("playground_test") {
testonly = true

sources = [
"compute_playground_test.cc",
"compute_playground_test.h",
"playground_test.cc",
"playground_test.h",
]
Expand Down
70 changes: 70 additions & 0 deletions impeller/playground/compute_playground_test.cc
Original file line number Diff line number Diff line change
@@ -0,0 +1,70 @@
// Copyright 2013 The Flutter Authors. All rights reserved.
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.

#include "flutter/fml/time/time_point.h"

#include "impeller/playground/compute_playground_test.h"

namespace impeller {

ComputePlaygroundTest::ComputePlaygroundTest() = default;

ComputePlaygroundTest::~ComputePlaygroundTest() = default;

void ComputePlaygroundTest::SetUp() {
if (!Playground::SupportsBackend(GetParam())) {
GTEST_SKIP_("Playground doesn't support this backend type.");
return;
}

if (!Playground::ShouldOpenNewPlaygrounds()) {
GTEST_SKIP_("Skipping due to user action.");
return;
}

SetupContext(GetParam());

start_time_ = fml::TimePoint::Now().ToEpochDelta();
}

void ComputePlaygroundTest::TearDown() {
TeardownWindow();
}

// |Playground|
std::unique_ptr<fml::Mapping> ComputePlaygroundTest::OpenAssetAsMapping(
std::string asset_name) const {
return flutter::testing::OpenFixtureAsMapping(asset_name);
}

std::shared_ptr<RuntimeStage> ComputePlaygroundTest::OpenAssetAsRuntimeStage(
const char* asset_name) const {
auto fixture = flutter::testing::OpenFixtureAsMapping(asset_name);
if (!fixture || fixture->GetSize() == 0) {
return nullptr;
}
auto stage = std::make_unique<RuntimeStage>(std::move(fixture));
if (!stage->IsValid()) {
return nullptr;
}
return stage;
}

static std::string FormatWindowTitle(const std::string& test_name) {
std::stringstream stream;
stream << "Impeller Playground for '" << test_name
<< "' (Press ESC or 'q' to quit)";
return stream.str();
}

// |Playground|
std::string ComputePlaygroundTest::GetWindowTitle() const {
return FormatWindowTitle(flutter::testing::GetCurrentTestName());
}

Scalar ComputePlaygroundTest::GetSecondsElapsed() const {
return (fml::TimePoint::Now().ToEpochDelta() - start_time_).ToSecondsF();
}

} // namespace impeller
55 changes: 55 additions & 0 deletions impeller/playground/compute_playground_test.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,55 @@
// Copyright 2013 The Flutter Authors. All rights reserved.
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.

#pragma once

#include <memory>

#include "flutter/fml/macros.h"
#include "flutter/fml/time/time_delta.h"
#include "flutter/testing/testing.h"
#include "impeller/geometry/scalar.h"
#include "impeller/playground/playground.h"

namespace impeller {

class ComputePlaygroundTest
: public Playground,
public ::testing::TestWithParam<PlaygroundBackend> {
public:
ComputePlaygroundTest();

virtual ~ComputePlaygroundTest();

void SetUp() override;

void TearDown() override;

// |Playground|
std::unique_ptr<fml::Mapping> OpenAssetAsMapping(
std::string asset_name) const override;

std::shared_ptr<RuntimeStage> OpenAssetAsRuntimeStage(
const char* asset_name) const;

// |Playground|
std::string GetWindowTitle() const override;

/// @brief Get the amount of time elapsed from the start of the playground
/// test's execution.
Scalar GetSecondsElapsed() const;

private:
fml::TimeDelta start_time_;

FML_DISALLOW_COPY_AND_ASSIGN(ComputePlaygroundTest);
};

#define INSTANTIATE_COMPUTE_SUITE(playground) \
INSTANTIATE_TEST_SUITE_P( \
Compute, playground, ::testing::Values(PlaygroundBackend::kMetal), \
[](const ::testing::TestParamInfo<ComputePlaygroundTest::ParamType>& \
info) { return PlaygroundBackendToString(info.param); });

} // namespace impeller
17 changes: 12 additions & 5 deletions impeller/playground/playground.cc
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,7 @@ Playground::Playground()
Playground::~Playground() = default;

std::shared_ptr<Context> Playground::GetContext() const {
return renderer_ ? renderer_->GetContext() : nullptr;
return context_;
}

bool Playground::SupportsBackend(PlaygroundBackend backend) {
Expand All @@ -104,25 +104,32 @@ bool Playground::SupportsBackend(PlaygroundBackend backend) {
FML_UNREACHABLE();
}

void Playground::SetupWindow(PlaygroundBackend backend) {
void Playground::SetupContext(PlaygroundBackend backend) {
FML_CHECK(SupportsBackend(backend));

impl_ = PlaygroundImpl::Create(backend);
if (!impl_) {
return;
}
auto context = impl_->GetContext();
if (!context) {

context_ = impl_->GetContext();
}

void Playground::SetupWindow() {
if (!context_) {
FML_LOG(WARNING)
<< "Asked to setup a window with no context (call SetupContext first).";
return;
}
auto renderer = std::make_unique<Renderer>(std::move(context));
auto renderer = std::make_unique<Renderer>(context_);
if (!renderer->IsValid()) {
return;
}
renderer_ = std::move(renderer);
}

void Playground::TeardownWindow() {
context_.reset();
renderer_.reset();
impl_.reset();
}
Expand Down
5 changes: 4 additions & 1 deletion impeller/playground/playground.h
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,9 @@ class Playground {

static bool ShouldOpenNewPlaygrounds();

void SetupWindow(PlaygroundBackend backend);
void SetupContext(PlaygroundBackend backend);

void SetupWindow();

void TeardownWindow();

Expand Down Expand Up @@ -81,6 +83,7 @@ class Playground {
struct GLFWInitializer;
std::unique_ptr<GLFWInitializer> glfw_initializer_;
std::unique_ptr<PlaygroundImpl> impl_;
std::shared_ptr<Context> context_;
std::unique_ptr<Renderer> renderer_;
Point cursor_position_;
ISize window_size_ = ISize{1024, 768};
Expand Down
3 changes: 2 additions & 1 deletion impeller/playground/playground_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,8 @@ void PlaygroundTest::SetUp() {
return;
}

SetupWindow(GetParam());
SetupContext(GetParam());
SetupWindow();

start_time_ = fml::TimePoint::Now().ToEpochDelta();
}
Expand Down
20 changes: 8 additions & 12 deletions impeller/renderer/compute_unittests.cc
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
#include "flutter/testing/testing.h"
#include "impeller/base/strings.h"
#include "impeller/fixtures/sample.comp.h"
#include "impeller/playground/playground_test.h"
#include "impeller/playground/compute_playground_test.h"
#include "impeller/renderer/command_buffer.h"
#include "impeller/renderer/compute_command.h"
#include "impeller/renderer/compute_pipeline_builder.h"
Expand All @@ -17,17 +17,10 @@
namespace impeller {
namespace testing {

using ComputeTest = PlaygroundTest;
INSTANTIATE_PLAYGROUND_SUITE(ComputeTest);
using ComputeTest = ComputePlaygroundTest;
INSTANTIATE_COMPUTE_SUITE(ComputeTest);

TEST_P(ComputeTest, CanCreateComputePass) {
if (GetParam() == PlaygroundBackend::kOpenGLES) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you mean to remove these skips?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes - this now gets skipped in the macro for setting up a compute playground. We can add Vulkan and GLES when/if we start supporting them.

GTEST_SKIP_("Compute is not supported on GL.");
}
if (GetParam() == PlaygroundBackend::kVulkan) {
GTEST_SKIP_("Compute is not supported on Vulkan yet.");
}

using CS = SampleComputeShader;
auto context = GetContext();
ASSERT_TRUE(context);
Expand Down Expand Up @@ -63,6 +56,7 @@ TEST_P(ComputeTest, CanCreateComputePass) {
input_0.fixed_array[1] = IPoint32(2, 2);
input_1.fixed_array[0] = UintPoint32(3, 3);
input_0.some_int = 5;
input_1.some_struct = CS::SomeStruct{.vf = Point(3, 4), .i = 42};

DeviceBufferDescriptor buffer_desc;
buffer_desc.storage_mode = StorageMode::kHostVisible;
Expand Down Expand Up @@ -97,8 +91,10 @@ TEST_P(ComputeTest, CanCreateComputePass) {
for (size_t i = 0; i < kCount; i++) {
Vector4 vector = output->elements[i];
Vector4 computed = input_0.elements[i] * input_1.elements[i];
EXPECT_EQ(vector, Vector4(computed.x + 2, computed.y + 3,
computed.z + 5, computed.w));
EXPECT_EQ(vector, Vector4(computed.x + 2 + input_1.some_struct.i,
computed.y + 3 + input_1.some_struct.vf.x,
computed.z + 5 + input_1.some_struct.vf.y,
computed.w));
}
latch.Signal();
}));
Expand Down