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] attempt to get validation errors from CI unittests. #51341

Merged
merged 41 commits into from
Mar 13, 2024
Merged
Show file tree
Hide file tree
Changes from 40 commits
Commits
Show all changes
41 commits
Select commit Hold shift + click to select a range
f86860c
testing.
jonahwilliams Mar 11, 2024
e06c9c9
make fatal
jonahwilliams Mar 11, 2024
dcd1c5e
Add validation layers to deps?
jonahwilliams Mar 11, 2024
29c7edc
more validation
jonahwilliams Mar 12, 2024
408e7f5
more cleanups
jonahwilliams Mar 12, 2024
be3271b
start with test skips.
jonahwilliams Mar 12, 2024
43e202a
++
jonahwilliams Mar 12, 2024
1c05893
++
jonahwilliams Mar 12, 2024
1cecd26
add back gles flag.
jonahwilliams Mar 12, 2024
d72221b
add more skips.
jonahwilliams Mar 12, 2024
fd68de3
++
jonahwilliams Mar 12, 2024
a453f7f
++
jonahwilliams Mar 12, 2024
ab9a072
more skips.
jonahwilliams Mar 12, 2024
26dc17f
++
jonahwilliams Mar 12, 2024
4e2eeac
++
jonahwilliams Mar 12, 2024
2cf6407
++
jonahwilliams Mar 12, 2024
79231ec
++
jonahwilliams Mar 12, 2024
9e378b1
++
jonahwilliams Mar 12, 2024
b8bcfcb
++
jonahwilliams Mar 12, 2024
f3ee710
++
jonahwilliams Mar 12, 2024
eea1bce
more fixes
jonahwilliams Mar 12, 2024
7917811
point field fixes.
jonahwilliams Mar 12, 2024
fb6e3ee
++
jonahwilliams Mar 12, 2024
b7031ba
make skips actually work.
jonahwilliams Mar 12, 2024
a38397f
fix gaussians.
jonahwilliams Mar 12, 2024
de0fa35
fix validation disable
jonahwilliams Mar 12, 2024
2d6fdfb
more skips.
jonahwilliams Mar 12, 2024
c7f4b80
++
jonahwilliams Mar 12, 2024
dfa2b37
update linux builders.
jonahwilliams Mar 12, 2024
dfd121f
add vvl config to impeller unittests for linux.
jonahwilliams Mar 12, 2024
a8f9e41
++
jonahwilliams Mar 12, 2024
1a2b88d
++
jonahwilliams Mar 12, 2024
7b6514c
cleanups.
jonahwilliams Mar 12, 2024
bf9d430
++
jonahwilliams Mar 12, 2024
50fcdde
merge dictionaries.
jonahwilliams Mar 13, 2024
412912b
++
jonahwilliams Mar 13, 2024
2cec37c
remove unused build_dir.
jonahwilliams Mar 13, 2024
34ce5b6
++
jonahwilliams Mar 13, 2024
c53ff77
remove unused gn flag.
jonahwilliams Mar 13, 2024
50cfa9d
Merge branch 'main' into turn_on_validation
jonahwilliams Mar 13, 2024
3d105f0
more adjustments.
jonahwilliams Mar 13, 2024
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
3 changes: 2 additions & 1 deletion ci/builders/linux_unopt.json
Original file line number Diff line number Diff line change
Expand Up @@ -121,7 +121,8 @@
"--target-dir",
"host_debug_unopt_impeller_tests",
"--rbe",
"--no-goma"
"--no-goma",
"--use-glfw-swiftshader"
],
"name": "host_debug_unopt_impeller_tests",
"ninja": {
Expand Down
5 changes: 3 additions & 2 deletions ci/builders/mac_unopt.json
Original file line number Diff line number Diff line change
Expand Up @@ -135,7 +135,8 @@
"arm64",
"--rbe",
"--no-goma",
"--xcode-symlinks"
"--xcode-symlinks",
"--use-glfw-swiftshader"
],
"name": "host_debug_unopt_arm64",
"ninja": {
Expand All @@ -157,7 +158,7 @@
"--variant",
"host_debug_unopt_arm64",
"--type",
"dart,dart-host,engine",
"dart,dart-host,engine,impeller-golden",
"--engine-capture-core-dump"
]
}
Expand Down
4 changes: 0 additions & 4 deletions impeller/BUILD.gn
Original file line number Diff line number Diff line change
Expand Up @@ -34,10 +34,6 @@ config("impeller_public_config") {
defines += [ "IMPELLER_ENABLE_VULKAN=1" ]
}

if (impeller_enable_vulkan_playgrounds) {
defines += [ "IMPELLER_ENABLE_VULKAN_PLAYGROUNDS=1" ]
}

if (impeller_trace_all_gl_calls) {
defines += [ "IMPELLER_TRACE_ALL_GL_CALLS" ]
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -72,11 +72,15 @@ fml::StatusOr<Scalar> CalculateSigmaForBlurRadius(

class GaussianBlurFilterContentsTest : public EntityPlayground {
public:
std::shared_ptr<Texture> MakeTexture(const TextureDescriptor& desc) {
return GetContentContext()
->GetContext()
->GetResourceAllocator()
->CreateTexture(desc);
/// Create a texture that has been cleared to transparent black.
std::shared_ptr<Texture> MakeTexture(ISize size) {
auto render_target = GetContentContext()->MakeSubpass(
"Clear Subpass", size,
[](const ContentContext&, RenderPass&) { return true; });
if (render_target.ok()) {
return render_target.value().GetRenderTargetTexture();
}
return nullptr;
}
};
INSTANTIATE_PLAYGROUND_SUITE(GaussianBlurFilterContentsTest);
Expand Down Expand Up @@ -109,6 +113,7 @@ TEST(GaussianBlurFilterContentsTest, CoverageSimple) {
Entity entity;
std::optional<Rect> coverage =
contents.GetFilterCoverage(inputs, entity, /*effect_transform=*/Matrix());

ASSERT_EQ(coverage, Rect::MakeLTRB(10, 10, 110, 110));
}

Expand All @@ -125,45 +130,35 @@ TEST(GaussianBlurFilterContentsTest, CoverageWithSigma) {
Entity entity;
std::optional<Rect> coverage =
contents.GetFilterCoverage(inputs, entity, /*effect_transform=*/Matrix());

EXPECT_TRUE(coverage.has_value());
if (coverage.has_value()) {
EXPECT_RECT_NEAR(coverage.value(), Rect::MakeLTRB(99, 99, 201, 201));
}
}

TEST_P(GaussianBlurFilterContentsTest, CoverageWithTexture) {
TextureDescriptor desc = {
.storage_mode = StorageMode::kDevicePrivate,
.format = PixelFormat::kB8G8R8A8UNormInt,
.size = ISize(100, 100),
};
fml::StatusOr<Scalar> sigma_radius_1 =
CalculateSigmaForBlurRadius(1.0, Matrix());
ASSERT_TRUE(sigma_radius_1.ok());
GaussianBlurFilterContents contents(
/*sigma_X=*/sigma_radius_1.value(),
/*sigma_y=*/sigma_radius_1.value(), Entity::TileMode::kDecal,
FilterContents::BlurStyle::kNormal, /*mask_geometry=*/nullptr);
std::shared_ptr<Texture> texture =
GetContentContext()->GetContext()->GetResourceAllocator()->CreateTexture(
desc);
std::shared_ptr<Texture> texture = MakeTexture(ISize(100, 100));
FilterInput::Vector inputs = {FilterInput::Make(texture)};
Entity entity;
entity.SetTransform(Matrix::MakeTranslation({100, 100, 0}));
std::optional<Rect> coverage =
contents.GetFilterCoverage(inputs, entity, /*effect_transform=*/Matrix());

EXPECT_TRUE(coverage.has_value());
if (coverage.has_value()) {
EXPECT_RECT_NEAR(coverage.value(), Rect::MakeLTRB(99, 99, 201, 201));
}
}

TEST_P(GaussianBlurFilterContentsTest, CoverageWithEffectTransform) {
TextureDescriptor desc = {
.storage_mode = StorageMode::kDevicePrivate,
.format = PixelFormat::kB8G8R8A8UNormInt,
.size = ISize(100, 100),
};
Matrix effect_transform = Matrix::MakeScale({2.0, 2.0, 1.0});
fml::StatusOr<Scalar> sigma_radius_1 =
CalculateSigmaForBlurRadius(1.0, effect_transform);
Expand All @@ -172,9 +167,7 @@ TEST_P(GaussianBlurFilterContentsTest, CoverageWithEffectTransform) {
/*sigma_x=*/sigma_radius_1.value(),
/*sigma_y=*/sigma_radius_1.value(), Entity::TileMode::kDecal,
FilterContents::BlurStyle::kNormal, /*mask_geometry=*/nullptr);
std::shared_ptr<Texture> texture =
GetContentContext()->GetContext()->GetResourceAllocator()->CreateTexture(
desc);
std::shared_ptr<Texture> texture = MakeTexture(ISize(100, 100));
FilterInput::Vector inputs = {FilterInput::Make(texture)};
Entity entity;
entity.SetTransform(Matrix::MakeTranslation({100, 100, 0}));
Expand Down Expand Up @@ -218,12 +211,7 @@ TEST(GaussianBlurFilterContentsTest, CalculateSigmaValues) {
}

TEST_P(GaussianBlurFilterContentsTest, RenderCoverageMatchesGetCoverage) {
TextureDescriptor desc = {
.storage_mode = StorageMode::kDevicePrivate,
.format = PixelFormat::kB8G8R8A8UNormInt,
.size = ISize(100, 100),
};
std::shared_ptr<Texture> texture = MakeTexture(desc);
std::shared_ptr<Texture> texture = MakeTexture(ISize(100, 100));
fml::StatusOr<Scalar> sigma_radius_1 =
CalculateSigmaForBlurRadius(1.0, Matrix());
ASSERT_TRUE(sigma_radius_1.ok());
Expand Down Expand Up @@ -254,12 +242,7 @@ TEST_P(GaussianBlurFilterContentsTest, RenderCoverageMatchesGetCoverage) {

TEST_P(GaussianBlurFilterContentsTest,
RenderCoverageMatchesGetCoverageTranslate) {
TextureDescriptor desc = {
.storage_mode = StorageMode::kDevicePrivate,
.format = PixelFormat::kB8G8R8A8UNormInt,
.size = ISize(100, 100),
};
std::shared_ptr<Texture> texture = MakeTexture(desc);
std::shared_ptr<Texture> texture = MakeTexture(ISize(100, 100));
fml::StatusOr<Scalar> sigma_radius_1 =
CalculateSigmaForBlurRadius(1.0, Matrix());
ASSERT_TRUE(sigma_radius_1.ok());
Expand Down Expand Up @@ -292,12 +275,7 @@ TEST_P(GaussianBlurFilterContentsTest,

TEST_P(GaussianBlurFilterContentsTest,
RenderCoverageMatchesGetCoverageRotated) {
TextureDescriptor desc = {
.storage_mode = StorageMode::kDevicePrivate,
.format = PixelFormat::kB8G8R8A8UNormInt,
.size = ISize(400, 300),
};
std::shared_ptr<Texture> texture = MakeTexture(desc);
std::shared_ptr<Texture> texture = MakeTexture(ISize(400, 300));
fml::StatusOr<Scalar> sigma_radius_1 =
CalculateSigmaForBlurRadius(1.0, Matrix());
auto contents = std::make_unique<GaussianBlurFilterContents>(
Expand Down Expand Up @@ -329,12 +307,7 @@ TEST_P(GaussianBlurFilterContentsTest,
}

TEST_P(GaussianBlurFilterContentsTest, CalculateUVsSimple) {
TextureDescriptor desc = {
.storage_mode = StorageMode::kDevicePrivate,
.format = PixelFormat::kB8G8R8A8UNormInt,
.size = ISize(100, 100),
};
std::shared_ptr<Texture> texture = MakeTexture(desc);
std::shared_ptr<Texture> texture = MakeTexture(ISize(100, 100));
auto filter_input = FilterInput::Make(texture);
Entity entity;
Quad uvs = GaussianBlurFilterContents::CalculateUVs(
Expand All @@ -347,13 +320,7 @@ TEST_P(GaussianBlurFilterContentsTest, CalculateUVsSimple) {
}

TEST_P(GaussianBlurFilterContentsTest, TextureContentsWithDestinationRect) {
TextureDescriptor desc = {
.storage_mode = StorageMode::kDevicePrivate,
.format = PixelFormat::kB8G8R8A8UNormInt,
.size = ISize(100, 100),
};

std::shared_ptr<Texture> texture = MakeTexture(desc);
std::shared_ptr<Texture> texture = MakeTexture(ISize(100, 100));
auto texture_contents = std::make_shared<TextureContents>();
texture_contents->SetSourceRect(Rect::MakeSize(texture->GetSize()));
texture_contents->SetTexture(texture);
Expand Down Expand Up @@ -388,13 +355,7 @@ TEST_P(GaussianBlurFilterContentsTest, TextureContentsWithDestinationRect) {

TEST_P(GaussianBlurFilterContentsTest,
TextureContentsWithDestinationRectScaled) {
TextureDescriptor desc = {
.storage_mode = StorageMode::kDevicePrivate,
.format = PixelFormat::kB8G8R8A8UNormInt,
.size = ISize(100, 100),
};

std::shared_ptr<Texture> texture = MakeTexture(desc);
std::shared_ptr<Texture> texture = MakeTexture(ISize(100, 100));
auto texture_contents = std::make_shared<TextureContents>();
texture_contents->SetSourceRect(Rect::MakeSize(texture->GetSize()));
texture_contents->SetTexture(texture);
Expand Down Expand Up @@ -429,14 +390,8 @@ TEST_P(GaussianBlurFilterContentsTest,
}

TEST_P(GaussianBlurFilterContentsTest, TextureContentsWithEffectTransform) {
TextureDescriptor desc = {
.storage_mode = StorageMode::kDevicePrivate,
.format = PixelFormat::kB8G8R8A8UNormInt,
.size = ISize(100, 100),
};

Matrix effect_transform = Matrix::MakeScale({2.0, 2.0, 1.0});
std::shared_ptr<Texture> texture = MakeTexture(desc);
std::shared_ptr<Texture> texture = MakeTexture(ISize(100, 100));
auto texture_contents = std::make_shared<TextureContents>();
texture_contents->SetSourceRect(Rect::MakeSize(texture->GetSize()));
texture_contents->SetTexture(texture);
Expand Down
23 changes: 12 additions & 11 deletions impeller/entity/entity_unittests.cc
Original file line number Diff line number Diff line change
Expand Up @@ -2167,7 +2167,6 @@ TEST_P(EntityTest, RuntimeEffect) {

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

contents->SetRuntimeStage(runtime_stage);

struct FragUniforms {
Expand Down Expand Up @@ -2643,13 +2642,15 @@ TEST_P(EntityTest, AdvancedBlendCoverageHintIsNotResetByEntityPass) {
}

TEST_P(EntityTest, SpecializationConstantsAreAppliedToVariants) {
auto content_context =
ContentContext(GetContext(), TypographerContextSkia::Make());
auto content_context = GetContentContext();

auto default_color_burn = content_context.GetBlendColorBurnPipeline(
{.has_depth_stencil_attachments = false});
auto alt_color_burn = content_context.GetBlendColorBurnPipeline(
{.has_depth_stencil_attachments = true});
auto default_color_burn = content_context->GetBlendColorBurnPipeline({
.color_attachment_pixel_format = PixelFormat::kR8G8B8A8UNormInt,
.has_depth_stencil_attachments = false,
});
auto alt_color_burn = content_context->GetBlendColorBurnPipeline(
{.color_attachment_pixel_format = PixelFormat::kR8G8B8A8UNormInt,
.has_depth_stencil_attachments = true});

ASSERT_NE(default_color_burn, alt_color_burn);
ASSERT_EQ(default_color_burn->GetDescriptor().GetSpecializationConstants(),
Expand All @@ -2663,10 +2664,10 @@ TEST_P(EntityTest, SpecializationConstantsAreAppliedToVariants) {
}

TEST_P(EntityTest, DecalSpecializationAppliedToMorphologyFilter) {
auto content_context =
ContentContext(GetContext(), TypographerContextSkia::Make());

auto default_color_burn = content_context.GetMorphologyFilterPipeline({});
auto content_context = GetContentContext();
auto default_color_burn = content_context->GetMorphologyFilterPipeline({
.color_attachment_pixel_format = PixelFormat::kR8G8B8A8UNormInt,
});

auto decal_supported = static_cast<Scalar>(
GetContext()->GetCapabilities()->SupportsDecalSamplerAddressMode());
Expand Down
9 changes: 6 additions & 3 deletions impeller/entity/geometry/point_field_geometry.cc
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@

#include "impeller/entity/geometry/point_field_geometry.h"

#include "impeller/geometry/color.h"
#include "impeller/renderer/command_buffer.h"

namespace impeller {
Expand Down Expand Up @@ -157,7 +158,8 @@ GeometryResult PointFieldGeometry::GetPositionBufferGPU(
DefaultUniformAlignment());

BufferView geometry_buffer =
host_buffer.Emplace(nullptr, total * sizeof(Point), alignof(Point));
host_buffer.Emplace(nullptr, total * sizeof(Point),
std::max(DefaultUniformAlignment(), alignof(Point)));

BufferView output;
{
Expand Down Expand Up @@ -185,8 +187,9 @@ GeometryResult PointFieldGeometry::GetPositionBufferGPU(
}

if (texture_coverage.has_value() && effect_transform.has_value()) {
BufferView geometry_uv_buffer =
host_buffer.Emplace(nullptr, total * sizeof(Vector4), alignof(Vector4));
BufferView geometry_uv_buffer = host_buffer.Emplace(
nullptr, total * sizeof(Vector4),
std::max(DefaultUniformAlignment(), alignof(Vector4)));

using UV = UvComputeShader;

Expand Down
1 change: 0 additions & 1 deletion impeller/golden_tests/golden_playground_test.h
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@

#include <memory>

#include "flutter/fml/macros.h"
#include "flutter/impeller/aiks/aiks_context.h"
#include "flutter/impeller/playground/playground.h"
#include "flutter/impeller/renderer/render_target.h"
Expand Down
2 changes: 0 additions & 2 deletions impeller/golden_tests/golden_playground_test_mac.cc
Original file line number Diff line number Diff line change
Expand Up @@ -69,8 +69,6 @@ static const std::vector<std::string> kSkipTests = {
"impeller_Play_AiksTest_CanRenderClippedRuntimeEffects_Vulkan",
};

static const std::vector<std::string> kVulkanDenyValidationTests = {};

namespace {
std::string GetTestName() {
std::string suite_name =
Expand Down
1 change: 1 addition & 0 deletions impeller/playground/BUILD.gn
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,7 @@ impeller_component("playground") {
"image:image_skia_backend",
"imgui:imgui_impeller_backend",
"//flutter/fml",
"//flutter/testing:testing_lib",
"//flutter/third_party/glfw",
"//flutter/third_party/imgui:imgui_glfw",
]
Expand Down
29 changes: 17 additions & 12 deletions impeller/playground/backend/vulkan/playground_impl_vk.cc
Original file line number Diff line number Diff line change
Expand Up @@ -60,18 +60,7 @@ void PlaygroundImplVK::DestroyWindowHandle(WindowHandle handle) {

PlaygroundImplVK::PlaygroundImplVK(PlaygroundSwitches switches)
Copy link
Member

Choose a reason for hiding this comment

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

Ideally this would be a static function that returns StatusOr<std::unique_ptr<PlaygroundImplVK>> instead of short circuiting a constructor, leaving a half initialized object.

https://google.github.io/styleguide/cppguide.html#Doing_Work_in_Constructors

Copy link
Member Author

Choose a reason for hiding this comment

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

I think we can just lift the check out of the constructor, since its a static method. I made this an FML_CHECK

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

: PlaygroundImpl(switches), handle_(nullptr, &DestroyWindowHandle) {
if (!::glfwVulkanSupported()) {
#ifdef TARGET_OS_MAC
VALIDATION_LOG << "Attempted to initialize a Vulkan playground on macOS "
"where Vulkan cannot be found. It can be installed via "
"MoltenVK and make sure to install it globally so "
"dlopen can find it.";
#else
VALIDATION_LOG << "Attempted to initialize a Vulkan playground on a system "
"that does not support Vulkan.";
#endif
return;
}
FML_CHECK(IsVulkanDriverPresent());

InitGlobalVulkanInstance();

Expand Down Expand Up @@ -224,4 +213,20 @@ fml::Status PlaygroundImplVK::SetCapabilities(
"PlaygroundImplVK doesn't support setting the capabilities.");
}

bool PlaygroundImplVK::IsVulkanDriverPresent() {
if (::glfwVulkanSupported()) {
return true;
}
#ifdef TARGET_OS_MAC
FML_LOG(ERROR) << "Attempting to initialize a Vulkan playground on macOS "
"where Vulkan cannot be found. It can be installed via "
"MoltenVK and make sure to install it globally so "
"dlopen can find it.";
#else // TARGET_OS_MAC
FML_LOG(ERROR) << "Attempting to initialize a Vulkan playground on a system "
"that does not support Vulkan.";
#endif // TARGET_OS_MAC
return false;
}

} // namespace impeller
3 changes: 2 additions & 1 deletion impeller/playground/backend/vulkan/playground_impl_vk.h
Original file line number Diff line number Diff line change
Expand Up @@ -5,14 +5,15 @@
#ifndef FLUTTER_IMPELLER_PLAYGROUND_BACKEND_VULKAN_PLAYGROUND_IMPL_VK_H_
#define FLUTTER_IMPELLER_PLAYGROUND_BACKEND_VULKAN_PLAYGROUND_IMPL_VK_H_

#include "flutter/fml/macros.h"
#include "impeller/playground/playground_impl.h"
#include "impeller/renderer/backend/vulkan/vk.h"

namespace impeller {

class PlaygroundImplVK final : public PlaygroundImpl {
public:
static bool IsVulkanDriverPresent();

explicit PlaygroundImplVK(PlaygroundSwitches switches);

~PlaygroundImplVK();
Expand Down
Loading