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] Vulkan framebuffer fetch via VK_ARM_RASTERIZATION_ORDER_ATTACHMENT_ACCESS #48458

Merged
Merged
Show file tree
Hide file tree
Changes from 17 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
9 changes: 8 additions & 1 deletion impeller/compiler/code_gen_template.h
Original file line number Diff line number Diff line change
Expand Up @@ -173,7 +173,14 @@ std::move({{ arg.argument_name }}){% if not loop.is_last %}, {% endif %}
// ===========================================================================
// Metadata for Vulkan =======================================================
// ===========================================================================
static constexpr std::array<DescriptorSetLayout,{{length(buffers)+length(sampled_images)}}> kDescriptorSetLayouts{
static constexpr std::array<DescriptorSetLayout,{{length(buffers)+length(sampled_images)+length(subpass_inputs)}}> kDescriptorSetLayouts{
{% for subpass_input in subpass_inputs %}
DescriptorSetLayout{
{{subpass_input.binding}}, // binding = {{subpass_input.binding}}
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 its a bit of a mistake to assume that all bindings are in the set index zero. We should fix that separately as that affects all instances of DescriptorSetLayout in the generated header. For example, if we were to manually specify layout(set=2, binding=n), we'd keel over.

In the same vein, I think its a mistake to assume that we can only have one input attachment and at index zero. I'd also put the input_attachment_index in the descriptor set layout. The previous attempt at this patch did the same.

You should be able to reproduce this by manually specifying the input_attachment_index to anything other than zero and checking the validity of the setup.

For the descriptor set index, we can file a separate followup issue.

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 practically speaking we can't really do bindings any other way. The actual render_pass_vk logic can only handle descriptors at index set zero.

Perhaps it would be possible with some refactoring, but it seems really in the weeds of what will yield a performance improvement.

{{subpass_input.descriptor_type}}, // descriptor_type = {{subpass_input.descriptor_type}}
{{to_shader_stage(shader_stage)}}, // shader_stage = {{to_shader_stage(shader_stage)}}
},
{% endfor %}
{% for buffer in buffers %}
DescriptorSetLayout{
{{buffer.binding}}, // binding = {{buffer.binding}}
Expand Down
15 changes: 15 additions & 0 deletions impeller/compiler/reflector.cc
Original file line number Diff line number Diff line change
Expand Up @@ -200,6 +200,21 @@ std::optional<nlohmann::json> Reflector::GenerateTemplateArguments() const {

const auto shader_resources = compiler_->get_shader_resources();

// Subpass Inputs.
{
auto& subpass_inputs = root["subpass_inputs"] = nlohmann::json::array_t{};
if (auto subpass_inputs_json =
ReflectResources(shader_resources.subpass_inputs);
subpass_inputs_json.has_value()) {
for (auto subpass_input : subpass_inputs_json.value()) {
subpass_input["descriptor_type"] = "DescriptorType::kInputAttachment";
subpass_inputs.emplace_back(std::move(subpass_input));
}
} else {
return std::nullopt;
}
}

// Uniform and storage buffers.
{
auto& buffers = root["buffers"] = nlohmann::json::array_t{};
Expand Down
1 change: 1 addition & 0 deletions impeller/core/shader_types.h
Original file line number Diff line number Diff line change
Expand Up @@ -145,6 +145,7 @@ enum class DescriptorType {
kSampledImage,
kImage,
kSampler,
kInputAttachment,
};

struct DescriptorSetLayout {
Expand Down
45 changes: 30 additions & 15 deletions impeller/entity/contents/content_context.cc
Original file line number Diff line number Diff line change
Expand Up @@ -221,49 +221,64 @@ ContentContext::ContentContext(
if (context_->GetCapabilities()->SupportsFramebufferFetch()) {
framebuffer_blend_color_pipelines_.CreateDefault(
*context_, options_trianglestrip,
{static_cast<int32_t>(BlendSelectValues::kColor), supports_decal});
{static_cast<int32_t>(BlendSelectValues::kColor), supports_decal},
UseSubpassInput::kYes);
framebuffer_blend_colorburn_pipelines_.CreateDefault(
*context_, options_trianglestrip,
{static_cast<int32_t>(BlendSelectValues::kColorBurn), supports_decal});
{static_cast<int32_t>(BlendSelectValues::kColorBurn), supports_decal},
UseSubpassInput::kYes);
framebuffer_blend_colordodge_pipelines_.CreateDefault(
*context_, options_trianglestrip,
{static_cast<int32_t>(BlendSelectValues::kColorDodge), supports_decal});
{static_cast<int32_t>(BlendSelectValues::kColorDodge), supports_decal},
UseSubpassInput::kYes);
framebuffer_blend_darken_pipelines_.CreateDefault(
*context_, options_trianglestrip,
{static_cast<int32_t>(BlendSelectValues::kDarken), supports_decal});
{static_cast<int32_t>(BlendSelectValues::kDarken), supports_decal},
UseSubpassInput::kYes);
framebuffer_blend_difference_pipelines_.CreateDefault(
*context_, options_trianglestrip,
{static_cast<int32_t>(BlendSelectValues::kDifference), supports_decal});
{static_cast<int32_t>(BlendSelectValues::kDifference), supports_decal},
UseSubpassInput::kYes);
framebuffer_blend_exclusion_pipelines_.CreateDefault(
*context_, options_trianglestrip,
{static_cast<int32_t>(BlendSelectValues::kExclusion), supports_decal});
{static_cast<int32_t>(BlendSelectValues::kExclusion), supports_decal},
UseSubpassInput::kYes);
framebuffer_blend_hardlight_pipelines_.CreateDefault(
*context_, options_trianglestrip,
{static_cast<int32_t>(BlendSelectValues::kHardLight), supports_decal});
{static_cast<int32_t>(BlendSelectValues::kHardLight), supports_decal},
UseSubpassInput::kYes);
framebuffer_blend_hue_pipelines_.CreateDefault(
*context_, options_trianglestrip,
{static_cast<int32_t>(BlendSelectValues::kHue), supports_decal});
{static_cast<int32_t>(BlendSelectValues::kHue), supports_decal},
UseSubpassInput::kYes);
framebuffer_blend_lighten_pipelines_.CreateDefault(
*context_, options_trianglestrip,
{static_cast<int32_t>(BlendSelectValues::kLighten), supports_decal});
{static_cast<int32_t>(BlendSelectValues::kLighten), supports_decal},
UseSubpassInput::kYes);
framebuffer_blend_luminosity_pipelines_.CreateDefault(
*context_, options_trianglestrip,
{static_cast<int32_t>(BlendSelectValues::kLuminosity), supports_decal});
{static_cast<int32_t>(BlendSelectValues::kLuminosity), supports_decal},
UseSubpassInput::kYes);
framebuffer_blend_multiply_pipelines_.CreateDefault(
*context_, options_trianglestrip,
{static_cast<int32_t>(BlendSelectValues::kMultiply), supports_decal});
{static_cast<int32_t>(BlendSelectValues::kMultiply), supports_decal},
UseSubpassInput::kYes);
framebuffer_blend_overlay_pipelines_.CreateDefault(
*context_, options_trianglestrip,
{static_cast<int32_t>(BlendSelectValues::kOverlay), supports_decal});
{static_cast<int32_t>(BlendSelectValues::kOverlay), supports_decal},
UseSubpassInput::kYes);
framebuffer_blend_saturation_pipelines_.CreateDefault(
*context_, options_trianglestrip,
{static_cast<int32_t>(BlendSelectValues::kSaturation), supports_decal});
{static_cast<int32_t>(BlendSelectValues::kSaturation), supports_decal},
UseSubpassInput::kYes);
framebuffer_blend_screen_pipelines_.CreateDefault(
*context_, options_trianglestrip,
{static_cast<int32_t>(BlendSelectValues::kScreen), supports_decal});
{static_cast<int32_t>(BlendSelectValues::kScreen), supports_decal},
UseSubpassInput::kYes);
framebuffer_blend_softlight_pipelines_.CreateDefault(
*context_, options_trianglestrip,
{static_cast<int32_t>(BlendSelectValues::kSoftLight), supports_decal});
{static_cast<int32_t>(BlendSelectValues::kSoftLight), supports_decal},
UseSubpassInput::kYes);
}

blend_color_pipelines_.CreateDefault(
Expand Down
5 changes: 4 additions & 1 deletion impeller/entity/contents/content_context.h
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
#include "impeller/entity/entity.h"
#include "impeller/renderer/capabilities.h"
#include "impeller/renderer/pipeline.h"
#include "impeller/renderer/pipeline_descriptor.h"
#include "impeller/renderer/render_target.h"
#include "impeller/typographer/typographer_context.h"

Expand Down Expand Up @@ -713,13 +714,15 @@ class ContentContext {

void CreateDefault(const Context& context,
const ContentContextOptions& options,
const std::initializer_list<int32_t>& constants = {}) {
const std::initializer_list<int32_t>& constants = {},
UseSubpassInput subpass_input = UseSubpassInput::kNo) {
auto desc =
PipelineT::Builder::MakeDefaultPipelineDescriptor(context, constants);
if (!desc.has_value()) {
VALIDATION_LOG << "Failed to create default pipeline.";
return;
}
desc->SetUseSubpassInput(subpass_input);
options.ApplyToPipelineDescriptor(*desc);
SetDefault(options, std::make_unique<PipelineT>(context, desc));
}
Expand Down
35 changes: 35 additions & 0 deletions impeller/entity/entity_unittests.cc
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@

#include "fml/logging.h"
#include "gtest/gtest.h"
#include "impeller/core/formats.h"
#include "impeller/core/texture_descriptor.h"
#include "impeller/entity/contents/atlas_contents.h"
#include "impeller/entity/contents/clip_contents.h"
Expand Down Expand Up @@ -41,6 +42,7 @@
#include "impeller/playground/playground.h"
#include "impeller/playground/widgets.h"
#include "impeller/renderer/command.h"
#include "impeller/renderer/pipeline_descriptor.h"
#include "impeller/renderer/render_pass.h"
#include "impeller/renderer/vertex_buffer_builder.h"
#include "impeller/typographer/backends/skia/text_frame_skia.h"
Expand Down Expand Up @@ -2527,6 +2529,39 @@ TEST_P(EntityTest, DecalSpecializationAppliedToMorphologyFilter) {
expected_constants);
}

TEST_P(EntityTest, FramebufferFetchPipelinesDeclareUsage) {
auto content_context =
ContentContext(GetContext(), TypographerContextSkia::Make());
if (!content_context.GetDeviceCapabilities().SupportsFramebufferFetch()) {
GTEST_SKIP() << "Framebuffer fetch not supported.";
}

ContentContextOptions options;
options.color_attachment_pixel_format = PixelFormat::kR8G8B8A8UNormInt;
auto color_burn =
content_context.GetFramebufferBlendColorBurnPipeline(options);

EXPECT_TRUE(color_burn->GetDescriptor().UsesSubpassInput());
}

TEST_P(EntityTest, PipelineDescriptorEqAndHash) {
auto desc_1 = std::make_shared<PipelineDescriptor>();
auto desc_2 = std::make_shared<PipelineDescriptor>();

EXPECT_TRUE(desc_1->IsEqual(*desc_2));
EXPECT_EQ(desc_1->GetHash(), desc_2->GetHash());

desc_1->SetUseSubpassInput(UseSubpassInput::kYes);

EXPECT_FALSE(desc_1->IsEqual(*desc_2));
EXPECT_NE(desc_1->GetHash(), desc_2->GetHash());

desc_2->SetUseSubpassInput(UseSubpassInput::kYes);

EXPECT_TRUE(desc_1->IsEqual(*desc_2));
EXPECT_EQ(desc_1->GetHash(), desc_2->GetHash());
}

} // namespace testing
} // namespace impeller

Expand Down
15 changes: 15 additions & 0 deletions impeller/entity/shaders/blending/framebuffer_blend.frag
Original file line number Diff line number Diff line change
Expand Up @@ -10,16 +10,31 @@
#include <impeller/types.glsl>
#include "blend_select.glsl"

// Warning: if any of the constant values or layouts are changed in this
// file, then the hard-coded constant value in
// impeller/renderer/backend/vulkan/binding_helpers_vk.cc
layout(constant_id = 0) const int blend_type = 0;
layout(constant_id = 1) const int supports_decal = 1;

#ifdef IMPELLER_TARGET_VULKAN
layout(set = 0,
binding = 0,
input_attachment_index = 0) uniform subpassInputMS uSub;

vec4 ReadDestination() {
return (subpassLoad(uSub, 0) + subpassLoad(uSub, 1) + subpassLoad(uSub, 2) +
subpassLoad(uSub, 3)) /
vec4(4.0);
}
#else
layout(set = 0,
binding = 0,
input_attachment_index = 0) uniform subpassInput uSub;

vec4 ReadDestination() {
return subpassLoad(uSub);
}
#endif // IMPELLER_TARGET_VULKAN

uniform sampler2D texture_sampler_src;

Expand Down
3 changes: 3 additions & 0 deletions impeller/entity/shaders/blending/framebuffer_blend.vert
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,9 @@
#include <impeller/conversions.glsl>
#include <impeller/types.glsl>

// Warning: if any of the constant values or layouts are changed in this
// file, then the hard-coded constant value in
// impeller/renderer/backend/vulkan/binding_helpers_vk.cc
Copy link
Member

Choose a reason for hiding this comment

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

Thinking out aloud on how to address this issue (in a separate patch later of course): We should be able to import a single header file from both a C++ TU and a glsl header. The same macro expansion capabilities as the C pre-processor are available in impellerc as well. As the IMPELLER_DEVICE macro is set in ImpellerC so we should be able to set it. So we can use that to ifdef away C++/GLSL-isms from the other TU.

Copy link
Member

Choose a reason for hiding this comment

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

I believe the default impellerc shaders GN rule set also add the header include paths during the impellerc invocation.

Copy link
Member

Choose a reason for hiding this comment

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

Just a nit; But I am just nervous about "please keep these in sync manually" type comments.

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 that won't work in this case, because its not that we're setting to binding 64 or w/e, that is just the first available binding in the fragment stage. So adding a new binding to the vertex stage might also change that? Not sure.

What I would really like is to have separate GLLS, Vulkan, and Metal headers. As this would also solve the problem of conditionally using float16.

uniform FrameInfo {
mat4 mvp;
float src_y_coord_scale;
Expand Down
4 changes: 4 additions & 0 deletions impeller/playground/backend/vulkan/playground_impl_vk.cc
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
#include "flutter/fml/logging.h"
#include "flutter/fml/mapping.h"
#include "impeller/entity/vk/entity_shaders_vk.h"
#include "impeller/entity/vk/framebuffer_blend_shaders_vk.h"
#include "impeller/entity/vk/modern_shaders_vk.h"
#include "impeller/fixtures/vk/fixtures_shaders_vk.h"
#include "impeller/playground/imgui/vk/imgui_shaders_vk.h"
Expand All @@ -33,6 +34,9 @@ ShaderLibraryMappingsForPlayground() {
impeller_entity_shaders_vk_length),
std::make_shared<fml::NonOwnedMapping>(impeller_modern_shaders_vk_data,
impeller_modern_shaders_vk_length),
std::make_shared<fml::NonOwnedMapping>(
impeller_framebuffer_blend_shaders_vk_data,
impeller_framebuffer_blend_shaders_vk_length),
std::make_shared<fml::NonOwnedMapping>(
impeller_fixtures_shaders_vk_data,
impeller_fixtures_shaders_vk_length),
Expand Down
20 changes: 14 additions & 6 deletions impeller/renderer/backend/vulkan/allocator_vk.cc
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
#include "impeller/renderer/backend/vulkan/device_buffer_vk.h"
#include "impeller/renderer/backend/vulkan/formats_vk.h"
#include "impeller/renderer/backend/vulkan/texture_vk.h"
#include "vulkan/vulkan_enums.hpp"

namespace impeller {

Expand Down Expand Up @@ -148,6 +149,7 @@ AllocatorVK::AllocatorVK(std::weak_ptr<Context> context,
allocator_.reset(allocator);
supports_memoryless_textures_ =
capabilities.SupportsDeviceTransientTextures();
supports_framebuffer_fetch_ = capabilities.SupportsFramebufferFetch();
is_valid_ = true;
}

Expand All @@ -167,7 +169,8 @@ static constexpr vk::ImageUsageFlags ToVKImageUsageFlags(
PixelFormat format,
TextureUsageMask usage,
StorageMode mode,
bool supports_memoryless_textures) {
bool supports_memoryless_textures,
bool supports_framebuffer_fetch) {
vk::ImageUsageFlags vk_usage;

switch (mode) {
Expand All @@ -187,6 +190,9 @@ static constexpr vk::ImageUsageFlags ToVKImageUsageFlags(
} else {
vk_usage |= vk::ImageUsageFlagBits::eColorAttachment;
}
if (supports_framebuffer_fetch) {
vk_usage |= vk::ImageUsageFlagBits::eInputAttachment;
}
}

if (usage & static_cast<TextureUsageMask>(TextureUsage::kShaderRead)) {
Expand Down Expand Up @@ -263,7 +269,8 @@ class AllocatedTextureSourceVK final : public TextureSourceVK {
const TextureDescriptor& desc,
VmaAllocator allocator,
vk::Device device,
bool supports_memoryless_textures)
bool supports_memoryless_textures,
bool supports_framebuffer_fetch)
: TextureSourceVK(desc), resource_(std::move(resource_manager)) {
FML_DCHECK(desc.format != PixelFormat::kUnknown);
TRACE_EVENT0("impeller", "CreateDeviceTexture");
Expand All @@ -281,9 +288,9 @@ class AllocatedTextureSourceVK final : public TextureSourceVK {
image_info.arrayLayers = ToArrayLayerCount(desc.type);
image_info.tiling = vk::ImageTiling::eOptimal;
image_info.initialLayout = vk::ImageLayout::eUndefined;
image_info.usage =
ToVKImageUsageFlags(desc.format, desc.usage, desc.storage_mode,
supports_memoryless_textures);
image_info.usage = ToVKImageUsageFlags(
desc.format, desc.usage, desc.storage_mode,
supports_memoryless_textures, supports_framebuffer_fetch);
image_info.sharingMode = vk::SharingMode::eExclusive;

VmaAllocationCreateInfo alloc_nfo = {};
Expand Down Expand Up @@ -412,7 +419,8 @@ std::shared_ptr<Texture> AllocatorVK::OnCreateTexture(
desc, //
allocator_.get(), //
device_holder->GetDevice(), //
supports_memoryless_textures_ //
supports_memoryless_textures_, //
supports_framebuffer_fetch_ //
);
if (!source->IsValid()) {
return nullptr;
Expand Down
1 change: 1 addition & 0 deletions impeller/renderer/backend/vulkan/allocator_vk.h
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ class AllocatorVK final : public Allocator {
ISize max_texture_size_;
bool is_valid_ = false;
bool supports_memoryless_textures_ = false;
bool supports_framebuffer_fetch_ = false;
// TODO(jonahwilliams): figure out why CI can't create these buffer pools.
bool created_buffer_pool_ = true;
uint32_t frame_count_ = 0;
Expand Down