[CP-stable]Add buffer around rerasterized input to fragment shaders to maintain coordinate space when clipped

[flutteractionsbot]

This pull request is created by automatic cherry pick workflow
Please fill in the form below, and a flutter domain expert will evaluate this cherry pick request.

Issue Link:

What is the link to the issue this cherry-pick is addressing?

#181660

Impact Description:

Without this PR certain combinations of blur and custom fragment shaders don't render correctly. This is an impediment some researchers are doing for advanced fragment shaders like https://pub.dev/packages/liquid_glass_renderer

Changelog Description:

• This PR makes sure all combinations of clip+blur+fragment shader behave in a consistent manner with respect to the coordinate system the fragment shader is calculated in.
• Before this change adding a clip rect could change the coordinate space of fragment shaders
• All impeller platforms are impacted.

flutter/181660 When using clip rects with ImageFilter.combine(fragment_shader, blur) on Impeller, the coordinate space of the fragment shader would fit the clipped region, not the region being clipped.

Workaround:

Is there a workaround for this issue?

No.

Risk:

What is the risk level of this cherry-pick?

The changed code happens inside a deep branch of logic that few code executes today.

• Low
• Medium
• High

Test Coverage:

Are you confident that your fix is well-tested by automated tests?

• Yes
• No

Validation Steps:

What are the steps to validate that this fix works?

There is a reproduction in #181660

[flutteractionsbot]

@gaaclarke please fill out the PR description above, afterwards the release team will review this request.

[flutter-dashboard]

This pull request was opened from and to a release candidate branch. This should only be done as part of the official Flutter release process. If you are attempting to make a regular contribution to the Flutter project, please close this PR and follow the instructions at Tree Hygiene for detailed instructions on contributing to Flutter.

Reviewers: Use caution before merging pull requests to release branches. Ensure the proper procedure has been followed.

[gemini-code-assist]

Code Review

This pull request addresses an issue with clipped runtime effects by adding a buffer around rerasterized inputs to fragment shaders. The core change is in RuntimeEffectFilterContents, where an AnonymousContents is used to adjust the coverage of the input snapshot, effectively re-padding it to maintain the correct coordinate space. The corresponding fragment shader runtime_stage_filter_circle.frag is updated to take the origin as a uniform instead of a hardcoded value. Several tests in aiks_dl_runtime_effect_unittests.cc are updated to use this new uniform, and a new interactive test is added to validate the behavior with clipped backdrop filters.

[gemini-code-assist]

The FragUniforms struct and the logic to create uniform_data are duplicated in this test, ComposeBackdropRuntimeOuterBlurInnerSmallSigma, and ClippedComposeBackdropRuntimeOuterBlurInnerSmallSigma.

To improve maintainability and reduce code duplication, consider defining the FragUniforms struct once in a shared scope (like an anonymous namespace at the top of the file) and creating a helper function to generate the uniform_data.

For example:

namespace {
struct FragUniforms {
  Vector2 size;
  Vector2 origin;
};

std::shared_ptr<std::vector<uint8_t>> CreateUniformData(
    const FragUniforms& uniforms) {
  auto data = std::make_shared<std::vector<uint8_t>>(sizeof(FragUniforms));
  memcpy(data->data(), &uniforms, sizeof(FragUniforms));
  return data;
}
}  // namespace

// Then in your test:
auto uniform_data = CreateUniformData({.size = Vector2(1, 1), .origin = Vector2(30.f, 30.f)});

[gemini-code-assist]

This file uses Vector2, which is defined in impeller/geometry/vector.h. While <impeller/geometry/point.h> might include it transitively, it's best practice to include the headers for all symbols you use directly. This aligns with the Google C++ Style Guide, which this repository follows.

Please add #include "impeller/geometry/vector.h" for clarity and to prevent potential build breaks if transitive includes change.

References

1. The repository's style guide specifies adherence to the Google C++ Style Guide. The Google C++ Style Guide's 'Include What You Use' principle recommends directly including headers for all symbols used. ^(link)

[gemini-code-assist]

This comment is helpful for understanding the 'why' behind this change, but this part is a bit confusing:

So, we can clamp those values to the coverage's RB values.

This suggests that clamping is being done, but the code below doesn't perform any clamping. If clamping is not necessary (as the rest of the comment seems to imply), it might be clearer to rephrase this to avoid confusion for future readers. For example, you could clarify why clamping is considered but ultimately not needed.

[gemini-code-assist]

Code Review

This pull request addresses an issue where the coordinate space for fragment shaders could be altered when combined with clipping and blurring, leading to incorrect rendering. The fix involves re-rasterizing the input with an offset to preserve the coordinate space, and new tests are added to validate this behavior. My feedback focuses on improving code readability and maintainability by reducing duplication in the new tests and clarifying a complex comment in the implementation.

[gemini-code-assist]

The FragUniforms struct and the logic to populate uniform_data are duplicated across several tests. To improve maintainability and adhere to the 'Optimize for readability' principle, consider defining the struct once and creating a helper function to generate the uniform data.

Additionally, the creation of uniform_data can be simplified to a single line, avoiding separate resize and memcpy calls.

Suggested change

	auto uniform_data = std::make_shared<std::vector<uint8_t>>();
	uniform_data->resize(sizeof(Vector2));
	uniform_data->resize(sizeof(FragUniforms));
	memcpy(uniform_data->data(), &frag_uniforms, sizeof(FragUniforms));
	auto uniform_data = std::make_shared<std::vector<uint8_t>>(
	reinterpret_cast<const uint8_t*>(&frag_uniforms),
	reinterpret_cast<const uint8_t*>(&frag_uniforms) + sizeof(frag_uniforms));

References

1. Code is read more often than it is written, so it should be optimized for readability. Reducing duplication and using more concise standard library features improves readability and maintainability. ^(link)

[gemini-code-assist]

The comments explaining this logic are a bit verbose, and the long explanation within the lambda can be confusing, especially the part about clamping which isn't reflected in the code. This goes against the 'Optimize for readability' and 'Documentation should be useful' guidelines.

To improve clarity, consider replacing these comments with a single, more concise explanation of the 'why' and 'how' before the AnonymousContents is created.

Suggested change

	// Use an AnonymousContents to restore the padding around the input that
	// may have been cut out with a clip rect to maintain the correct
	// coordinates for the fragment shader to perform.
	auto anonymous_contents = AnonymousContents::Make(
	[&texture_contents](const ContentContext& renderer,
	const Entity& entity, RenderPass& pass) -> bool {
	return texture_contents.Render(renderer, entity, pass);
	},
	[maybe_input_coverage,
	entity_offset](const Entity& entity) -> std::optional<Rect> {
	Rect coverage = maybe_input_coverage.value();
	// The LT values come from the offset of the clip rect, that creates
	// the clipping effect on the content that will be rendered from
	// the fragment shader. The RB values define the region we'll be
	// synthesizing and ultimately defines the width and the height of
	// the rasterized image. The LT values can be thought of shifting
	// the window that will be rasterized. Since we are shifting from
	// the top-left corner, that is effectively pushing the the bottom
	// right corner lower, outside of the rendering space. So, we can
	// clamp those values to the coverage's RB values. This doesn't
	// cause the fragment shader's rendering to deform because the
	// magic width/height values sent to the fragment shader don't take
	// the rasterized image's size into account.
	// When a runtime effect is applied after a clip, the input texture is
	// clipped to the filter's coverage. This can alter the coordinate space
	// that the fragment shader runs in, leading to incorrect rendering.
	//
	// To fix this, we re-rasterize the input into a new texture that is
	// offset to match the clip's origin by wrapping it in an
	// AnonymousContents with adjusted coverage. This preserves the global
	// coordinate space for the fragment shader. The new coverage is defined
	// by the clip offset (entity_offset) as the top-left, and the original
	// input coverage's bottom-right.
	auto anonymous_contents = AnonymousContents::Make(
	[&texture_contents](const ContentContext& renderer,
	const Entity& entity, RenderPass& pass) -> bool {
	return texture_contents.Render(renderer, entity, pass);
	},
	[maybe_input_coverage,
	entity_offset](const Entity& entity) -> std::optional<Rect> {
	Rect coverage = maybe_input_coverage.value();

References

1. Code is read more often than it is written. The documentation should be clear and concise to aid understanding. ^(link)
2. Documentation should be useful and explain the why and the how of the code. Confusing or inaccurate comments detract from this. ^(link)

[reidbaker]

@gaaclarke does the changelog entry flutter/181660 When using clip rects with ImageFilter.combine(fragment_shader, blur) on Impeller, the coordinate space of the fragment shader would fit the clipped region, not the region being clipped. apply to all platforms or a subset?

@flar can you give this a review and let me know why it is safe to land this without a test?

[gaaclarke]

@gaaclarke does the changelog entry flutter/181660 When using clip rects with ImageFilter.combine(fragment_shader, blur) on Impeller, the coordinate space of the fragment shader would fit the clipped region, not the region being clipped. apply to all platforms or a subset?

It applies to all platforms using Impeller, yes.

@flar can you give this a review and let me know why it is safe to land this without a test?

@reidbaker , this cherry-pick includes a test.

[reidbaker]

@gaaclarke you are correct. I apologize. I scanned the folders for test and did not see the file with a test suffix.

[flar]

@flar can you give this a review and let me know why it is safe to land this without a test?

@reidbaker , this cherry-pick includes a test.

And the unit test is something I believe I reviewed before so good on all (test-related) fronts.

[gaaclarke]

@flar for process's sake I think @reidbaker is looking for someone other than the person the filed the cherry-pick to approve it. I know you reviewed it but you are verifying that what I said in the cherry-pick request is true and that there is no risk that would make it appropriate for main but not stable.

[flar]

RSLGTM