Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Impeller] fix remaining Validation errors. #51692

Merged
merged 2 commits into from
Mar 27, 2024
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
15 changes: 8 additions & 7 deletions impeller/entity/entity_unittests.cc
Original file line number Diff line number Diff line change
Expand Up @@ -2200,7 +2200,7 @@ TEST_P(EntityTest, RuntimeEffect) {
ASSERT_TRUE(runtime_stage->IsDirty());

bool expect_dirty = true;
Pipeline<PipelineDescriptor>* first_pipeline = nullptr;
Pipeline<PipelineDescriptor>* first_pipeline;
auto callback = [&](ContentContext& context, RenderPass& pass) -> bool {
EXPECT_EQ(runtime_stage->IsDirty(), expect_dirty);

Expand Down Expand Up @@ -2238,7 +2238,10 @@ TEST_P(EntityTest, RuntimeEffect) {
// Simulate some renders and hot reloading of the shader.
auto content_context = GetContentContext();
{
RenderTarget target;
RenderTarget target =
content_context->GetRenderTargetCache()->CreateOffscreen(
*content_context->GetContext(), {1, 1}, 1u);

testing::MockRenderPass mock_pass(GetContext(), target);
callback(*content_context, mock_pass);
callback(*content_context, mock_pass);
Expand All @@ -2252,10 +2255,7 @@ TEST_P(EntityTest, RuntimeEffect) {
expect_dirty = true;

callback(*content_context, mock_pass);
callback(*content_context, mock_pass);
}

ASSERT_TRUE(OpenPlaygroundHere(callback));
}

TEST_P(EntityTest, RuntimeEffectCanSuccessfullyRender) {
Expand Down Expand Up @@ -2319,7 +2319,6 @@ TEST_P(EntityTest, RuntimeEffectSetsRightSizeWhenUniformIsStruct) {

auto contents = std::make_shared<RuntimeEffectContents>();
contents->SetGeometry(Geometry::MakeCover());

contents->SetRuntimeStage(runtime_stage);

struct FragUniforms {
Expand All @@ -2338,7 +2337,9 @@ TEST_P(EntityTest, RuntimeEffectSetsRightSizeWhenUniformIsStruct) {
entity.SetContents(contents);

auto context = GetContentContext();
RenderTarget target;
RenderTarget target = context->GetRenderTargetCache()->CreateOffscreen(
*context->GetContext(), {1, 1}, 1u);

testing::MockRenderPass pass(GetContext(), target);
ASSERT_TRUE(contents->Render(*context, entity, pass));
ASSERT_EQ(pass.GetCommands().size(), 1u);
Expand Down
10 changes: 6 additions & 4 deletions impeller/fixtures/runtime_stage_example.frag
Original file line number Diff line number Diff line change
Expand Up @@ -2,13 +2,15 @@
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.

layout(location = 0) uniform vec2 iResolution;
layout(location = 1) uniform float iTime;
#include <flutter/runtime_effect.glsl>

layout(location = 0) out vec4 fragColor;
uniform vec2 iResolution;
uniform float iTime;

out vec4 fragColor;

void main() {
vec2 uv = gl_FragCoord.xy / iResolution;
vec2 uv = FlutterFragCoord() / iResolution;
float t = 4 * iTime;
vec3 col = 0.5 + 0.5 * cos(t + uv.xyx + vec3(0, 1, 4));
fragColor = vec4(col, 1.0);
Expand Down
13 changes: 3 additions & 10 deletions impeller/golden_tests/golden_playground_test_mac.cc
Original file line number Diff line number Diff line change
Expand Up @@ -104,12 +104,6 @@ bool SaveScreenshot(std::unique_ptr<testing::Screenshot> screenshot) {
return true;
}

bool ShouldTestHaveVulkanValidations() {
std::string test_name = GetTestName();
return std::find(kVulkanDenyValidationTests.begin(),
kVulkanDenyValidationTests.end(),
test_name) == kVulkanDenyValidationTests.end();
}
} // namespace

struct GoldenPlaygroundTest::GoldenPlaygroundTestImpl {
Expand Down Expand Up @@ -144,14 +138,13 @@ void GoldenPlaygroundTest::SetUp() {
std::filesystem::path icd_path = target_path / "vk_swiftshader_icd.json";
setenv("VK_ICD_FILENAMES", icd_path.c_str(), 1);

bool enable_vulkan_validations = ShouldTestHaveVulkanValidations();
switch (GetParam()) {
case PlaygroundBackend::kMetal:
pimpl_->screenshotter = std::make_unique<testing::MetalScreenshotter>();
break;
case PlaygroundBackend::kVulkan: {
const std::unique_ptr<PlaygroundImpl>& playground =
GetSharedVulkanPlayground(enable_vulkan_validations);
GetSharedVulkanPlayground(/*enable_validations=*/true);
pimpl_->screenshotter =
std::make_unique<testing::VulkanScreenshotter>(playground);
break;
Expand All @@ -171,7 +164,7 @@ void GoldenPlaygroundTest::SetUp() {
pimpl_->screenshotter = std::make_unique<testing::MetalScreenshotter>();
} else if (GetParam() == PlaygroundBackend::kVulkan) {
const std::unique_ptr<PlaygroundImpl>& playground =
GetSharedVulkanPlayground(enable_vulkan_validations);
GetSharedVulkanPlayground(/*enable_validations=*/true);
pimpl_->screenshotter =
std::make_unique<testing::VulkanScreenshotter>(playground);
}
Expand Down Expand Up @@ -257,7 +250,7 @@ std::shared_ptr<Context> GoldenPlaygroundTest::MakeContext() const {
/// On Metal we create a context for each test.
return GetContext();
} else if (GetParam() == PlaygroundBackend::kVulkan) {
bool enable_vulkan_validations = ShouldTestHaveVulkanValidations();
bool enable_vulkan_validations = true;
FML_CHECK(!pimpl_->test_vulkan_playground)
<< "We don't support creating multiple contexts for one test";
pimpl_->test_vulkan_playground =
Expand Down
16 changes: 0 additions & 16 deletions impeller/playground/playground.h
Original file line number Diff line number Diff line change
Expand Up @@ -32,22 +32,6 @@ enum class PlaygroundBackend {
kVulkan,
};

// TODO(https://github.com/flutter/flutter/issues/145039)
// clang-format off
static const std::vector<std::string> kVulkanDenyValidationTests = {
"impeller_Play_SceneTest_FlutterLogo_Vulkan",
"impeller_Play_SceneTest_CuboidUnlit_Vulkan",
"impeller_Play_RuntimeStageTest_CanCreatePipelineFromRuntimeStage_Vulkan",
"impeller_Play_RuntimeEffectSetsRightSizeWhenUniformIsStruct_Vulkan",
"impeller_Play_RuntimeEffectCanSuccessfullyRender_Vulkan",
"impeller_Play_RuntimeEffect_Vulkan",
"impeller_Play_EntityTest_RuntimeStageTest_CanCreatePipelineFromRuntimeStage_Vulkan",
"impeller_Play_EntityTest_RuntimeEffectSetsRightSizeWhenUniformIsStruct_Vulkan",
"impeller_Play_EntityTest_RuntimeEffectCanSuccessfullyRender_Vulkan",
"impeller_Play_EntityTest_RuntimeEffect_Vulkan",
};
// clang-format on

constexpr inline RuntimeStageBackend PlaygroundBackendToRuntimeStageBackend(
PlaygroundBackend backend) {
switch (backend) {
Expand Down
25 changes: 1 addition & 24 deletions impeller/playground/playground_impl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -20,29 +20,6 @@
#include "impeller/playground/backend/vulkan/playground_impl_vk.h"
#endif // IMPELLER_ENABLE_VULKAN

namespace {

std::string GetTestName() {
std::string suite_name =
::testing::UnitTest::GetInstance()->current_test_suite()->name();
std::string test_name =
::testing::UnitTest::GetInstance()->current_test_info()->name();
std::stringstream ss;
ss << "impeller_" << suite_name << "_" << test_name;
std::string result = ss.str();
// Make sure there are no slashes in the test name.
std::replace(result.begin(), result.end(), '/', '_');
return result;
}

bool ShouldTestHaveVulkanValidations() {
std::string test_name = GetTestName();
return std::find(impeller::kVulkanDenyValidationTests.begin(),
impeller::kVulkanDenyValidationTests.end(),
test_name) == impeller::kVulkanDenyValidationTests.end();
}
} // namespace

namespace impeller {

std::unique_ptr<PlaygroundImpl> PlaygroundImpl::Create(
Expand All @@ -64,7 +41,7 @@ std::unique_ptr<PlaygroundImpl> PlaygroundImpl::Create(
"isn't available or was disabled on this platform: "
<< PlaygroundBackendToString(backend);
}
switches.enable_vulkan_validation = ShouldTestHaveVulkanValidations();
switches.enable_vulkan_validation = true;
return std::make_unique<PlaygroundImplVK>(switches);
#endif // IMPELLER_ENABLE_VULKAN
default:
Expand Down
11 changes: 10 additions & 1 deletion impeller/runtime_stage/runtime_stage_unittests.cc
Original file line number Diff line number Diff line change
Expand Up @@ -277,7 +277,16 @@ TEST_P(RuntimeStageTest, CanCreatePipelineFromRuntimeStage) {
auto vertex_descriptor = std::make_shared<VertexDescriptor>();
vertex_descriptor->SetStageInputs(VS::kAllShaderStageInputs,
VS::kInterleavedBufferLayout);
vertex_descriptor->RegisterDescriptorSetLayouts(VS::kDescriptorSetLayouts);

std::array<DescriptorSetLayout, 2> descriptor_set_layouts = {
VS::kDescriptorSetLayouts[0],
DescriptorSetLayout{
.binding = 64u,
.descriptor_type = DescriptorType::kUniformBuffer,
.shader_stage = ShaderStage::kFragment,
},
};
vertex_descriptor->RegisterDescriptorSetLayouts(descriptor_set_layouts);

desc.SetVertexDescriptor(std::move(vertex_descriptor));
ColorAttachmentDescriptor color0;
Expand Down
7 changes: 6 additions & 1 deletion impeller/scene/shaders/unlit.frag
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,13 @@ in vec4 v_color;

out vec4 frag_color;

float consume_unused() {
Copy link
Member Author

Choose a reason for hiding this comment

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

This makes the shader inputs technically consumed until we figure out how to handle this in general.

return (v_position.x + v_tangent_space[0][0]) * 0.001;
}

void main() {
vec4 vertex_color = mix(vec4(1), v_color, frag_info.vertex_color_weight);
frag_color = texture(base_color_texture, v_texture_coords) * vertex_color *
frag_info.color;
frag_info.color +
consume_unused();
}