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

[Impeller] new blur: limit uvs to blur region #49299

Merged
merged 4 commits into from
Jan 3, 2024
Merged

[Impeller] new blur: limit uvs to blur region #49299

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
43d6539
[Impeller] limit blur uvs to the clip region
gaaclarke Dec 20, 2023
8d92328
added assert
gaaclarke Jan 2, 2024
4a9b2af
brandon feedback
gaaclarke Jan 2, 2024
a809f2d
updated comment
gaaclarke Jan 3, 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
28 changes: 27 additions & 1 deletion impeller/aiks/aiks_unittests.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4003,6 +4003,30 @@ TEST_P(AiksTest, GaussianBlurRotatedAndClipped) {
ASSERT_TRUE(OpenPlaygroundHere(canvas.EndRecordingAsPicture()));
}

TEST_P(AiksTest, GaussianBlurScaledAndClipped) {
Canvas canvas;
std::shared_ptr<Texture> boston = CreateTextureForFixture("boston.jpg");
Rect bounds =
Rect::MakeXYWH(0, 0, boston->GetSize().width, boston->GetSize().height);
Vector2 image_center = Vector2(bounds.GetSize() / 2);
Paint paint = {.image_filter =
ImageFilter::MakeBlur(Sigma(20.0), Sigma(20.0),
FilterContents::BlurStyle::kNormal,
Entity::TileMode::kDecal)};
Vector2 clip_size = {150, 75};
Vector2 center = Vector2(1024, 768) / 2;
canvas.Scale(GetContentScale());
canvas.ClipRect(
Rect::MakeLTRB(center.x, center.y, center.x, center.y).Expand(clip_size));
canvas.Translate({center.x, center.y, 0});
canvas.Scale({0.6, 0.6, 1});

canvas.DrawImageRect(std::make_shared<Image>(boston), /*source=*/bounds,
/*dest=*/bounds.Shift(-image_center), paint);

ASSERT_TRUE(OpenPlaygroundHere(canvas.EndRecordingAsPicture()));
}

TEST_P(AiksTest, GaussianBlurRotatedAndClippedInteractive) {
std::shared_ptr<Texture> boston = CreateTextureForFixture("boston.jpg");

Expand All @@ -4013,11 +4037,13 @@ TEST_P(AiksTest, GaussianBlurRotatedAndClippedInteractive) {
Entity::TileMode::kMirror, Entity::TileMode::kDecal};

static float rotation = 0;
static float scale = 0.6;
static int selected_tile_mode = 3;

ImGui::Begin("Controls", nullptr, ImGuiWindowFlags_AlwaysAutoResize);
{
ImGui::SliderFloat("Rotation (degrees)", &rotation, -180, 180);
ImGui::SliderFloat("Scale", &scale, 0, 2.0);
ImGui::Combo("Tile mode", &selected_tile_mode, tile_mode_names,
sizeof(tile_mode_names) / sizeof(char*));
}
Expand All @@ -4038,7 +4064,7 @@ TEST_P(AiksTest, GaussianBlurRotatedAndClippedInteractive) {
canvas.ClipRect(
Rect::MakeLTRB(handle_a.x, handle_a.y, handle_b.x, handle_b.y));
canvas.Translate({center.x, center.y, 0});
canvas.Scale({0.6, 0.6, 1});
canvas.Scale({scale, scale, 1});
canvas.Rotate(Degrees(rotation));

canvas.DrawImageRect(std::make_shared<Image>(boston), /*source=*/bounds,
Expand Down
46 changes: 39 additions & 7 deletions impeller/entity/contents/filters/gaussian_blur_filter_contents.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -121,7 +121,8 @@ fml::StatusOr<RenderTarget> MakeBlurSubpass(
const SamplerDescriptor& sampler_descriptor,
Entity::TileMode tile_mode,
const GaussianBlurFragmentShader::BlurInfo& blur_info,
std::optional<RenderTarget> destination_target) {
std::optional<RenderTarget> destination_target,
const Quad& blur_uvs) {
if (blur_info.blur_sigma < kEhCloseEnough) {
return input_pass;
}
Expand Down Expand Up @@ -153,10 +154,10 @@ fml::StatusOr<RenderTarget> MakeBlurSubpass(

BindVertices<GaussianBlurVertexShader>(cmd, host_buffer,
{
{Point(0, 0), Point(0, 0)},
{Point(1, 0), Point(1, 0)},
{Point(0, 1), Point(0, 1)},
{Point(1, 1), Point(1, 1)},
{blur_uvs[0], blur_uvs[0]},
{blur_uvs[1], blur_uvs[1]},
{blur_uvs[2], blur_uvs[2]},
{blur_uvs[3], blur_uvs[3]},
});

SamplerDescriptor linear_sampler_descriptor = sampler_descriptor;
Expand All @@ -183,6 +184,14 @@ fml::StatusOr<RenderTarget> MakeBlurSubpass(
}
}

/// Returns `rect` relative to `reference`, where Rect::MakeXYWH(0,0,1,1) will
/// be returned when `rect` == `reference`.
Rect MakeReferenceUVs(const Rect& reference, const Rect& rect) {
Rect result = Rect::MakeOriginSize(rect.GetOrigin() - reference.GetOrigin(),
rect.GetSize());
return result.Scale(1.0f / Vector2(reference.GetSize()));
}

} // namespace

GaussianBlurFilterContents::GaussianBlurFilterContents(
Expand Down Expand Up @@ -311,6 +320,29 @@ std::optional<Entity> GaussianBlurFilterContents::RenderFilter(
Vector2 pass1_pixel_size =
1.0 / Vector2(pass1_out.value().GetRenderTargetTexture()->GetSize());

std::optional<Rect> input_snapshot_coverage = input_snapshot->GetCoverage();
Quad blur_uvs = {Point(0, 0), Point(1, 0), Point(0, 1), Point(1, 1)};
if (expanded_coverage_hint.has_value() &&
input_snapshot_coverage.has_value() &&
// TODO(https://github.com/flutter/flutter/issues/140890): Remove this
// condition. There is some flaw in coverage stopping us from using this
// today. I attempted to use source coordinates to calculate the uvs,
// but that didn't work either.
input_snapshot.has_value() &&
input_snapshot.value().transform.IsTranslationScaleOnly()) {
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 this needs to:

  • check the entity.GetTransform() (rather than the input_snapshot's transform)
  • check if the transform is translation-only (rather than scale-translation-only)

If either one of these isn't true, then I think coverage_hint and input_snapshot_coverage could end up being in difference spaces in some situations, which will cause a variation of the UV mapping bug.

Copy link
Member

Choose a reason for hiding this comment

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

If you're up for it, one way you could smoke test this issue is by adding a scale param to the interactive Aiks toy.

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 added the parameter to the interactive test and added an automated test. Let me know if you are thinking of some other case where this may break.

Copy link
Member Author

Choose a reason for hiding this comment

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

To be clear it's necessarily that I don't believe there can be a problem; I just don't want to shrink the domain of this optimization without proof that it doesn't work everywhere via a test. That test can serve as the basis for expanding the domain in the future.

Copy link
Member

Choose a reason for hiding this comment

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

Oh yeah, I get you. Also I think you should land this at will, we can patch other issues as we find em. ;)

It would also be good to test a case where the transform+coverage rect is absorbed by the snapshot rather than forwarded. For example: A blur applied to a gradient circle (can't be a solid color since that'll fall into the rrect fast path).

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 just tried the interactive clip when drawing a rect with a gradient on a rrect. I wasn't able to see a problem. I'm going to leave the existing test as is since it doesn't seem to demonstrate anything different.

Copy link
Member

Choose a reason for hiding this comment

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

SGTM

// Only process the uvs where the blur is happening, not the whole texture.
std::optional<Rect> uvs = MakeReferenceUVs(input_snapshot_coverage.value(),
expanded_coverage_hint.value())
.Intersection(Rect::MakeSize(Size(1, 1)));
FML_DCHECK(uvs.has_value());
if (uvs.has_value()) {
blur_uvs[0] = uvs->GetLeftTop();
blur_uvs[1] = uvs->GetRightTop();
blur_uvs[2] = uvs->GetLeftBottom();
blur_uvs[3] = uvs->GetRightBottom();
}
}
Copy link
Member

Choose a reason for hiding this comment

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

As you pointed out, this doesn't work when the basis of the entity transform isn't identity. Video of the issue:

Screen.Recording.2023-12-20.at.7.19.06.PM.mov

Just so we're on the same page: Snapshots are an instruction for how to render a texture consisting of sampling state, a transform, and the texture itself.

The issue in detail

This issue happens because the coverage_hint is given in screen/pass space and texture-based filter inputs (like TiledTextureContents or TextureContents) create snapshots that are in local space. So the Entity's transform will simply be forwarded through to the Snapshot::transform field. So the act of snapshotting incurs zero rendering overhead by deferring the transform via the snapshot protocol.

Filters (and everything else that consumes snapshots) must contend with this behavior. And that's a very good thing! Because absorbing the snapshot would mean burdening the blur workload with a very expensive 4th RenderPass that does nothing but sample the entire original image and print it to a possibly oversized texture with a bunch of unused fully transparent area. And that unused transparent area would be a very annoying problem to deal with in the blur, because we'd need to carefully work around its presence when generating the outer sampling region in the second pass. I wouldn't want to be on the hook for that UV mess! Same goes for optimizing the two subsequent blur passes.

For other kinds of inputs that aren't already a rasterized image (like, say, a SolidColorContents that draws a circle), we need a RenderPass to snapshot anyhow, and so we get to absorb the full transform when snapshotting with no loss to performance. For these cases, the Snapshot::transform will just be a translation to position the resulting texture correctly on-screen and nothing else, regardless of whatever nutty Entity transform was given.

A quick fix

So your solution as written works well when the Entity transform is specifically translation-only. This is because under those circumstances, the input_snapshot_coverage accurately shrinkwraps the texture. And both coverage_hint and input_snapshot_coverage are in the same space.

Well, backdrop filters and subpass textures are actually rendered by EntityPass in translated screen/pass space, and so the Entity transform for all backdrop filters already qualifies as translation-only. Backdrop filters are the major pain point that spawned the need for the coverage hint in the first place, so an effective short-term fix would be to just turn this optimization on only when the Entity transform is translation-only.

A comprehensive fix (make it work for ui.image texture inputs too)

The "quick fix" is actually good setup for the more comprehensive fix, so check that out first.

Right now, we take our Entity transform and throw the whole thing at the snapshot input at once. But we could just.... pass a transform containing only the scale of the basis vectors into the snapshot (the reason we'd pass the scale here is that we need to allow non-texture inputs to consume the scale so that e.g. scaled up paths won't end up having a source image that's too pixellated), which will guarantee that we always get a translation-scale-only result back from the snapshot.

Now we can form the full "deferred" transform by normalizing the basis vectors of the original Entity transform and multiplying it with the snapshot's returned transform on the RHS. This "deferred" transform should get multiplied it into the filter's returned Entity transform.

Caution

Since we're deferring the input snapshot's transform, the snapshot's transform should be changed to an identity matrix before calling Snapshot::GetCoverage to compute input_snapshot_coverage. Otherwise the input_snapshot_coverage will be defined in the wrong space for some texture-based filter inputs.

This means that we get to operate in an axis-aligned space that's very close to local space, which allows us to avoid all of the extra UV mapping wackiness that would otherwise be required of us. Deferring most of the entity transform also means that the blur effect will get transformed correctly as well, and even work the same way that Skia does for non-affine/perspective transforms.

So now for the coverage hint. input_snapshot_coverage is no longer in screen space, but our close-to-local space, so we need to convert the coverage_hint (and by extension, any derivatives of the coverage_hint) to conservative bounds in our local-space. This can be done by applying the inverse deferred transform to the rectangle and re-bounding it. And again, the "deferred" transform is the same transform that we deferred to the returned Entity described above. Rect::TransformBounds is intended for precisely this kind of situation:

auto local_coverage_hint = coverage_hint.TransformBounds(deferred_transform.Invert());

From here, the local_coverage_hint can be used everywhere in place of the coverage_hint, and the blurred texture will trim it's sides to hug the coverage hint regardless of the Entity transform, but never cross into it.

This whole thing is a slightly more detailed explanation of the solution I proposed in flutter/flutter#139165 (comment).

Copy link
Member

Choose a reason for hiding this comment

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

@gaaclarke lemme know if you'd like to dig into this in detail at some point. Reading it back, it's a bit dense... But the quick fix should be super simple.

Copy link
Member

Choose a reason for hiding this comment

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

Ah actually looking at the code, it looks like the guard for the quick fix is partially in place.

Copy link
Member Author

Choose a reason for hiding this comment

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

Okay, I created a new issue to address the rotation case: flutter/flutter#140890. I'm going to land this and deprioritize that while we get other optimizations in and try to address shimmering.


fml::StatusOr<RenderTarget> pass2_out =
MakeBlurSubpass(renderer, /*input_pass=*/pass1_out.value(),
input_snapshot->sampler_descriptor, tile_mode_,
Expand All @@ -320,7 +352,7 @@ std::optional<Entity> GaussianBlurFilterContents::RenderFilter(
.blur_radius = blur_radius.y * effective_scalar.y,
.step_size = 1.0,
},
/*destination_target=*/std::nullopt);
/*destination_target=*/std::nullopt, blur_uvs);

if (!pass2_out.ok()) {
return std::nullopt;
Expand All @@ -341,7 +373,7 @@ std::optional<Entity> GaussianBlurFilterContents::RenderFilter(
.blur_radius = blur_radius.x * effective_scalar.x,
.step_size = 1.0,
},
pass3_destination);
pass3_destination, blur_uvs);

if (!pass3_out.ok()) {
return std::nullopt;
Expand Down