Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.
Merged
30 changes: 30 additions & 0 deletions impeller/aiks/aiks_unittests.cc
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
#include "impeller/aiks/testing/context_spy.h"
#include "impeller/core/capture.h"
#include "impeller/entity/contents/conical_gradient_contents.h"
#include "impeller/entity/contents/filters/gaussian_blur_filter_contents.h"
#include "impeller/entity/contents/filters/inputs/filter_input.h"
#include "impeller/entity/contents/linear_gradient_contents.h"
#include "impeller/entity/contents/radial_gradient_contents.h"
Expand Down Expand Up @@ -3795,6 +3796,7 @@ TEST_P(AiksTest, GaussianBlurAllocatesCorrectMipCountRenderTarget) {
}

TEST_P(AiksTest, GaussianBlurMipMapNestedLayer) {
fml::testing::LogCapture log_capture;
Canvas canvas;
canvas.DrawPaint({.color = Color::Wheat()});
canvas.SaveLayer({.blend_mode = BlendMode::kMultiply});
Expand All @@ -3818,6 +3820,34 @@ TEST_P(AiksTest, GaussianBlurMipMapNestedLayer) {
std::max(it->texture->GetTextureDescriptor().mip_count, max_mip_count);
}
EXPECT_EQ(max_mip_count, 1lu);
EXPECT_EQ(log_capture.str().find(GaussianBlurFilterContents::kNoMipsError),
std::string::npos);
}

TEST_P(AiksTest, GaussianBlurMipMapImageFilter) {
fml::testing::LogCapture log_capture;
Canvas canvas;
canvas.SaveLayer(
{.image_filter = ImageFilter::MakeBlur(Sigma(30), Sigma(30),
FilterContents::BlurStyle::kNormal,
Entity::TileMode::kClamp)});
canvas.DrawCircle({200, 200}, 50, {.color = Color::Chartreuse()});

Picture picture = canvas.EndRecordingAsPicture();
std::shared_ptr<RenderTargetCache> cache =
std::make_shared<RenderTargetCache>(GetContext()->GetResourceAllocator());
AiksContext aiks_context(GetContext(), nullptr, cache);
picture.ToImage(aiks_context, {1024, 768});

size_t max_mip_count = 0;
for (auto it = cache->GetTextureDataBegin(); it != cache->GetTextureDataEnd();
++it) {
max_mip_count =
std::max(it->texture->GetTextureDescriptor().mip_count, max_mip_count);
}
EXPECT_EQ(max_mip_count, 1lu);
EXPECT_EQ(log_capture.str().find(GaussianBlurFilterContents::kNoMipsError),
std::string::npos);
}

} // namespace testing
Expand Down
11 changes: 11 additions & 0 deletions impeller/aiks/canvas.cc
Original file line number Diff line number Diff line change
Expand Up @@ -190,6 +190,9 @@ void Canvas::Save(bool create_subpass,
MipCountVisitor mip_count_visitor;
backdrop_filter->Visit(mip_count_visitor);
subpass->SetRequiredMipCount(mip_count_visitor.GetRequiredMipCount());

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
subpass->SetRequiredMipCount(mip_count_visitor.GetRequiredMipCount());

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

done

current_pass_->SetRequiredMipCount(
std::max(current_pass_->GetRequiredMipCount(),
mip_count_visitor.GetRequiredMipCount()));
}
subpass->SetBlendMode(blend_mode);
current_pass_ = GetCurrentPass().AddSubpass(std::move(subpass));
Expand Down Expand Up @@ -725,6 +728,14 @@ void Canvas::SaveLayer(const Paint& paint,
auto& new_layer_pass = GetCurrentPass();
new_layer_pass.SetBoundsLimit(bounds);

if (paint.image_filter) {
MipCountVisitor mip_count_visitor;
paint.image_filter->Visit(mip_count_visitor);
new_layer_pass.SetRequiredMipCount(
std::max(mip_count_visitor.GetRequiredMipCount(),
Comment thread
gaaclarke marked this conversation as resolved.
Outdated
new_layer_pass.GetRequiredMipCount()));

@bdero bdero Jan 16, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The new layer will always have a default required mip count. The place where the maxing needs to happen is when a layer is being appended that has a backdrop filter.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The place where the maxing needs to happen is when a layer is being appended that has a backdrop filter.

That still exists from the last PR. This covers both cases, image filter and backdrop filter. Is there a case this doesn't cover, or are you suggesting there is a singular place we can put this code instead of having it in 2 locations?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Wait, you are saying the max needs to happen there as well? I'm not understanding the order of operations that is problematic. Can you lay it out for me?

@bdero bdero Jan 16, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The mip count we need to use when allocating the texture is the maximum we detect for all reads. A backdrop texture is read by both subpass backdrop filters as well as the image filter that happened to be set at the time the SaveLayer was made.

Inside of Save(), the RequiredMipCount of the new layer is being set to the backdrop filter's mip count. But the backdrop filter will read from the parent layer's backdrop, not the new layer's backdrop.

@bdero bdero Jan 16, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

So when we come across a backdrop filter, we need to max the mip count with the parent pass mip count. And when an image filter is being applied via the paint state, no maxing is necessary when setting the mip count because nothing should have set the mip count yet.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Ah yea I see, thanks. This started making the previous tests print out the "no mips" message when rendering the blur. I would like to have a way to assert that in the tests. We still have some tests that use the filter without mips (assuming they will have them at runtime) so I was a bit squishy with that. I'll see if I can find a way to assert it.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Okay fixed. I added a logs check to make sure this doesn't regress. We don't have a better way to assert this currently. I can file an issue to make it a hard assert and migrate all the tests over.

}

// Only apply opacity peephole on default blending.
if (paint.blend_mode == BlendMode::kSourceOver) {
new_layer_pass.SetDelegate(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -189,6 +189,9 @@ Rect MakeReferenceUVs(const Rect& reference, const Rect& rect) {
}
} // namespace

std::string_view GaussianBlurFilterContents::kNoMipsError =
"Applying gaussian blur without mipmap.";

GaussianBlurFilterContents::GaussianBlurFilterContents(
Scalar sigma_x,
Scalar sigma_y,
Expand Down Expand Up @@ -280,7 +283,7 @@ std::optional<Entity> GaussianBlurFilterContents::RenderFilter(

// In order to avoid shimmering in downsampling step, we should have mips.
if (input_snapshot->texture->GetMipCount() <= 1) {
FML_DLOG(ERROR) << "Applying gaussian blur without mipmap.";
FML_DLOG(ERROR) << kNoMipsError;
}
FML_DCHECK(!input_snapshot->texture->NeedsMipmapGeneration());

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,8 @@ KernelPipeline::FragmentShader::KernelSamples GenerateBlurInfo(
/// Note: This will replace `DirectionalGaussianBlurFilterContents`.
class GaussianBlurFilterContents final : public FilterContents {
public:
static std::string_view kNoMipsError;

explicit GaussianBlurFilterContents(Scalar sigma_x,
Scalar sigma_y,
Entity::TileMode tile_mode);
Expand Down
14 changes: 2 additions & 12 deletions impeller/entity/entity_pass.cc
Original file line number Diff line number Diff line change
Expand Up @@ -344,7 +344,7 @@ bool EntityPass::Render(ContentContext& renderer,
if (reads_from_onscreen_backdrop) {
EntityPassTarget offscreen_target = CreateRenderTarget(
renderer, root_render_target.GetRenderTargetSize(),
GetBackdropFilterMipCount(),
GetRequiredMipCount(),
GetClearColorOrDefault(render_target.GetRenderTargetSize()));

if (!OnRender(renderer, // renderer
Expand Down Expand Up @@ -606,7 +606,7 @@ EntityPass::EntityResult EntityPass::GetEntityForElement(
auto subpass_target = CreateRenderTarget(
renderer, // renderer
subpass_size, // size
subpass->GetBackdropFilterMipCount(),
subpass->GetRequiredMipCount(),
subpass->GetClearColorOrDefault(subpass_size)); // clear_color

if (!subpass_target.IsValid()) {
Expand Down Expand Up @@ -1191,16 +1191,6 @@ void EntityPass::SetEnableOffscreenCheckerboard(bool enabled) {
enable_offscreen_debug_checkerboard_ = enabled;
}

int32_t EntityPass::GetBackdropFilterMipCount() const {
int32_t result = 1;
for (auto& element : elements_) {
if (auto subpass = std::get_if<std::unique_ptr<EntityPass>>(&element)) {
result = std::max(result, subpass->get()->GetRequiredMipCount());
}
}
return result;
}

EntityPassClipRecorder::EntityPassClipRecorder() {}

void EntityPassClipRecorder::RecordEntity(const Entity& entity,
Expand Down
4 changes: 0 additions & 4 deletions impeller/entity/entity_pass.h
Original file line number Diff line number Diff line change
Expand Up @@ -157,10 +157,6 @@ class EntityPass {
required_mip_count_ = mip_count;
}

/// Returns the mip map count that should be required for the render target
/// receiving this EntityPass.
int32_t GetBackdropFilterMipCount() const;

//----------------------------------------------------------------------------
/// @brief Computes the coverage of a given subpass. This is used to
/// determine the texture size of a given subpass before it's rendered
Expand Down