Repository navigation
Revert: Remove BorderMaskBlur - #194567
flutteractionsbot wants to merge 1 commit into
Conversation
This reverts commit 34540e9.
github-actions
Bot
added
engine
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new BorderMaskBlur filter in Impeller, adding BorderMaskBlurFilterContents, a corresponding fragment shader border_mask_blur.frag, and updating pipeline configurations, Paint, and Canvas to support it. Unit tests are also added to verify coverage and correctness. The review feedback suggests adding null checks for inputs[0] in BorderMaskBlurFilterContents and input in Paint::WithMaskBlur to prevent potential crashes, as well as removing an unused alpha field from the FragInfo uniform block in the new shader.
| if (inputs.empty()) { | ||
| return std::nullopt; | ||
| } |
There was a problem hiding this comment.
If inputs[0] is null, calling inputs[0]->GetSnapshot will result in a null pointer dereference and crash. Add a check to ensure inputs[0] is not null.
if (inputs.empty() || !inputs[0]) {\n return std::nullopt;\n }| if (inputs.empty()) { | ||
| return std::nullopt; | ||
| } |
There was a problem hiding this comment.
If inputs[0] is null, dereferencing it on the following lines will cause a crash. We should defensively check if inputs[0] is null before proceeding.
if (inputs.empty() || !inputs[0]) {\n return std::nullopt;\n }| std::shared_ptr<Contents> Paint::WithMaskBlur(std::shared_ptr<Contents> input, | ||
| bool is_solid_color, | ||
| const Matrix& ctm) const { | ||
| if (mask_blur_descriptor.has_value()) { | ||
| input = mask_blur_descriptor->CreateMaskBlur(FilterInput::Make(input), | ||
| is_solid_color, ctm); | ||
| } | ||
| return input; | ||
| } |
There was a problem hiding this comment.
If input is null, there is no content to apply a mask blur to. We should return nullptr early to avoid unnecessary processing and potential null pointer issues down the line.
std::shared_ptr<Contents> Paint::WithMaskBlur(std::shared_ptr<Contents> input,\n bool is_solid_color,\n const Matrix& ctm) const {\n if (!input) {\n return nullptr;\n }\n if (mask_blur_descriptor.has_value()) {\n input = mask_blur_descriptor->CreateMaskBlur(FilterInput::Make(input),\n is_solid_color, ctm);\n }\n return input;\n}| uniform FragInfo { | ||
| float16_t src_factor; | ||
| float16_t inner_blur_factor; | ||
| float16_t outer_blur_factor; | ||
| float16_t alpha; | ||
|
|
||
| f16vec2 sigma_uv; | ||
| } |
There was a problem hiding this comment.
The alpha field in the FragInfo uniform block is declared but never used in the shader. Removing it simplifies the uniform block and avoids sending unused data to the GPU.
uniform FragInfo {\n float16_t src_factor;\n float16_t inner_blur_factor;\n float16_t outer_blur_factor;\n\n f16vec2 sigma_uv;\n}
References
- Suggest simplification and refactoring: Assess whether the code can be made simpler or refactored to enhance readability and maintainability. (link)
|
This PR did not introduce the problem. Fix for actual problem in #194540 |
Reverts: Remove BorderMaskBlur
Initiated by: @eyebrowsoffire
Reason for reverting: tree is broken. Example failure: https://ci.chromium.org/ui/p/flutter/builders/prod/Linux%20linux_unopt/20632/overview
Original PR Author: @walley892
Reviewed By: @gaaclarke
The original PR description is provided below:
Removes the BorderMaskBlur pipeline and associated shaders.
This is created only in
CreateMaskBlurwhenis_solid_color=false. The three argumentCreateMaskBluris only called in canvas.cc, and hardcodesis_solid_color=true.flutter/engine/src/flutter/impeller/display_list/canvas.cc
Line 2225 in 669265b
This shader looks like it was used in the olden days, but has been deadcode since 2024.
Part of #194511
Pre-launch Checklist
///).