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

[Impeller] make sure binding nullptr texture does not crash. #56381

Merged
auto-submit merged 1 commit into from
Nov 6, 2024
Merged

[Impeller] make sure binding nullptr texture does not crash. #56381

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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: 3 additions & 0 deletions impeller/renderer/backend/metal/render_pass_mtl.mm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -408,6 +408,9 @@ static bool Bind(PassBindingsCacheMTL& pass,
const ShaderMetadata& metadata,
std::shared_ptr<const Texture> texture,
const std::unique_ptr<const Sampler>& sampler) {
if (!texture) {
return false;
}
Comment on lines +411 to +413
Copy link
Member

@gaaclarke gaaclarke Nov 6, 2024

Choose a reason for hiding this comment

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

nit: I would prefer this is an assert instead of turning BindResource to a noop. It's harder to follow code if every procedure call is prefaced with an implicit "maybe", like "MaybeBindResource()". New people on the team complain about this pattern making the code hard to follow.

This at least communicates when nothing has happened with the returnval and matches existing patterns though so we shouldn't hold back the PR about it. How about adding [[nodiscard]] to this method to communicate at callsites that the method potentially is a noop?

Copy link
Contributor Author

@jonahwilliams jonahwilliams Nov 6, 2024

Choose a reason for hiding this comment

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

We don't want to crash the app if one of the draws is misconfigured. There is some ambiguity (due to runtime effect) as to whether this is covering up a bug in flutter or a bug in the user code.

DCHECK would be fine, except that it doesn't stop the production crash.

What I'd really like is to get those nullability annotations working correctly so we can correctly eliminate the nullptr closer to where it was generated.

return Bind(pass_bindings_, stage, slot.texture_index, sampler, *texture);
}

Expand Down
2 changes: 1 addition & 1 deletion impeller/renderer/backend/vulkan/render_pass_vk.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -602,7 +602,7 @@ bool RenderPassVK::BindResource(ShaderStage stage,
if (bound_buffer_offset_ >= kMaxBindings) {
return false;
}
if (!texture->IsValid() || !sampler) {
if (!texture || !texture->IsValid() || !sampler) {
return false;
}
const TextureVK& texture_vk = TextureVK::Cast(*texture);
Expand Down
16 changes: 15 additions & 1 deletion impeller/renderer/renderer_unittests.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,6 @@
#include "impeller/fixtures/swizzle.frag.h"
#include "impeller/fixtures/texture.frag.h"
#include "impeller/fixtures/texture.vert.h"
#include "impeller/geometry/path_builder.h"
#include "impeller/playground/playground.h"
#include "impeller/playground/playground_test.h"
#include "impeller/renderer/command_buffer.h"
Expand Down Expand Up @@ -1626,6 +1625,21 @@ TEST_P(RendererTest, CanSepiaToneThenSwizzleWithSubpasses) {
OpenPlaygroundHere(callback);
}

TEST_P(RendererTest, BindingNullTexturesDoesNotCrash) {
using FS = BoxFadeFragmentShader;

auto context = GetContext();
const std::unique_ptr<const Sampler>& sampler =
context->GetSamplerLibrary()->GetSampler({});
auto command_buffer = context->CreateCommandBuffer();

RenderTargetAllocator allocator(context->GetResourceAllocator());
RenderTarget target = allocator.CreateOffscreen(*context, {1, 1}, 1);

auto pass = command_buffer->CreateRenderPass(target);
EXPECT_FALSE(FS::BindContents2(*pass, nullptr, sampler));
}

} // namespace testing
} // namespace impeller

Expand Down