[Impeller] Rendering difference using ImageFilter.matrix #147807
Assignees
Labels
c: rendering
UI glitches reported at the engine/skia rendering level
e: impeller
Impeller rendering backend issues and features requests
engine
flutter/engine repository. See also e: labels.
found in release: 3.19
Found to occur in 3.19
found in release: 3.22
Found to occur in 3.22
has reproducible steps
The issue has been confirmed reproducible and is ready to work on
P1
High-priority issues at the top of the work list
slimpeller
Engine binary size reduction. go/slimpeller
team-engine
Owned by Engine team
triaged-engine
Triaged by Engine team
Comments
|
Reproducible using the code sample provided above. flutter doctor -v |
danagbemava-nc
added
engine
|
The impeller backdrop filter is being offset incorrectly. This is an image matrix filter. This is not partial repaint related since it reproduces on Android. So maybe we're just missing an offset somewhere? or missing a transform on the offset. |
jonahwilliams
added
P1
|
Here is the display list for bad draw: DisplayList {
save();
{
transform2DAffine([ 2, 0, 0 ], [ 0, 2, 0 ], );
drawDisplayList(ID : 233, bounds
: SkRect(left : 0, top : 0, right : 376, bottom : 680),
opacity : 1) {
DisplayList {
setAntiAlias(true);
setColor(DlColor(fffef7ff));
drawRect(SkRect(left : 0, top : 0, right : 375, bottom : 667));
setColor(DlColor(ffff9800));
drawRect(SkRect(left : 0, top : 0, right : 187.5, bottom : 333.5));
setColor(DlColor(ff9c27b0));
drawRect(SkRect(left : 187.5, top : 0, right : 375, bottom : 333.5));
setColor(DlColor(ff4caf50));
drawRect(SkRect(left : 0, top : 333.5, right : 187.5, bottom : 667));
setColor(DlColor(fff44336));
drawRect(SkRect(left : 187.5, top : 333.5, right : 375, bottom : 667));
save();
{
transform2DAffine(
[ 1, 2.449293705170336e-16, -1.70530256582424e-13 ],
[ -2.449293705170336e-16, 1, 1.13686837721616e-13 ], );
drawShadow(SkPath(bounds
: SkRect(left : 303, top : 595, right : 359,
bottom : 651)),
DlColor(ff000000), 6, false, 2);
setColor(DlColor(ffebddff));
drawPath(SkPath(
bounds
: SkRect(left : 303, top : 595, right : 359, bottom : 651)));
save();
{
translate(303, 595);
clipRect(SkRect(left : 0, top : 0, right : 56, bottom : 56),
ClipOp::kIntersect, isaa
: true);
save();
{
clipRRect(
SkRRect(SkRect(left : 0, top : 0, right : 56, bottom : 56), ul
: (16, 16), ur
: (16, 16), lr
: (16, 16), ll
: (16, 16)),
ClipOp::kIntersect, isaa
: true);
setColor(DlColor(6230f46));
drawCircle(SkPoint(21.55413055419922, 25.02498245239258),
12.15186500549316);
}
restore();
save();
{
clipRRect(
SkRRect(SkRect(left : 0, top : 0, right : 56, bottom : 56), ul
: (16, 16), ur
: (16, 16), lr
: (16, 16), ll
: (16, 16)),
ClipOp::kIntersect, isaa
: true);
setColor(DlColor(9bcbcbc));
drawRect(SkRect(left : 0, top : 0, right : 56, bottom : 56));
}
restore();
}
restore();
setColor(DlColor(ff230f46));
drawTextFrame(&TextFrame(0, -24 = > 24, 0), 319, 635);
save();
{ translate(303, 595); }
restore();
}
restore();
}
}
save();
{
clipRRect(
SkRRect(SkRect(left : 201.25, top : 10, right : 361.25, bottom : 170),
ul
: (80, 80), ur
: (80, 80), lr
: (80, 80), ll
: (80, 80)),
ClipOp::kIntersect, isaa
: true);
saveLayer(
SkRect(left : 198.75, top : 7.5, right : 363.75, bottom : 172.5),
SaveLayerOptions(can_distribute_opacity
: false, renders_with_attributes
: false),
backdrop
: &DlMatrixImageFilter(
SkMatrix([ 3, 0, -299.25 ], [ 0, 3, -949 ], [ 0, 0, 1 ]),
LinearSampling));
{
drawDisplayList(
ID : 234, bounds
: SkRect(left : 198.75, top : 7.5, right : 363.75, bottom : 172.5),
opacity : 1) {
DisplayList {
save();
{
translate(201.25, 10);
setAntiAlias(true);
setColor(DlColor(ff2196f3));
setStyle(Style::kStroke);
setStrokeWidth(5);
drawCircle(SkPoint(80, 80), 80);
setColor(DlColor(fff44336));
setStrokeWidth(1.666666626930237);
drawRect(SkRect(left : 70, top : 70, right : 90, bottom : 90));
setColor(DlColor(ff000000));
drawLine(SkPoint(75.19999694824219, 80),
SkPoint(84.80000305175781, 80));
drawLine(SkPoint(80, 75.19999694824219),
SkPoint(80, 84.80000305175781));
}
restore();
}
}
}
restore();
}
restore();
drawDisplayList(ID : 9, bounds
: SkRect(left : 301.6132202148438, top
: -25.30353164672852, right : 400.3035278320312,
bottom : 73.38679504394531),
opacity : 1) {
DisplayList {
save();
{
translate(375, 0);
rotate(45);
setAntiAlias(true);
setColor(DlColor(7f000000));
setMaskFilter(DlMaskFilter(BlurStyle::kNormal, 3.964099884033203));
drawRect(SkRect(left : -40, top : 28, right : 40, bottom : 40));
setColor(DlColor(a0b71c1c));
setMaskFilter(no MaskFilter);
drawRect(SkRect(left : -40, top : 28, right : 40, bottom : 40));
setColor(DlColor(ffffffff));
drawTextFrame(&TextFrame(-1, -9 = > 38.87286376953125, 2),
-18.88345336914062, 37);
}
restore();
}
}
}
restore();
} |
|
This mostly renders correctly if I apply the change to With that change the magnifier tracks the pointer. But the magnified backdrop image is truncated if you move the pointer toward the right edge of the screen. |
|
I verified that brandon's change still fixes the issue, here it is updated for the current repo: diff --git a/impeller/entity/contents/filters/matrix_filter_contents.cc b/impeller/entity/contents/filters/matrix_filter_contents.cc
index 9681e8a6d6..634fea3db9 100644
--- a/impeller/entity/contents/filters/matrix_filter_contents.cc
+++ b/impeller/entity/contents/filters/matrix_filter_contents.cc
@@ -40,29 +40,45 @@ std::optional<Entity> MatrixFilterContents::RenderFilter(
return std::nullopt;
}
- // The filter's matrix needs to be applied within the space defined by the
- // scene's current transform matrix (CTM). For example: If the CTM is
- // scaled up, then translations applied by the matrix should be magnified
- // accordingly.
- //
- // To accomplish this, we sandwich the filter's matrix within the CTM in both
- // cases. But notice that for the subpass backdrop filter case, we use the
- // "effect transform" instead of the Entity's transform!
- //
- // That's because in the subpass backdrop filter case, the Entity's transform
- // isn't actually the captured CTM of the scene like it usually is; instead,
- // it's just a screen space translation that offsets the backdrop texture (as
- // mentioned above). And so we sneak the subpass's captured CTM in through the
- // effect transform.
-
- auto transform = rendering_mode_ == Entity::RenderingMode::kSubpass
- ? effect_transform
- : entity.GetTransform();
- snapshot->transform = transform * //
- matrix_ * //
- transform.Invert() * //
- snapshot->transform;
-
+ if (rendering_mode_ == Entity::RenderingMode::kSubpass) {
+ // There are two special quirks with how Matrix filters behave when used as
+ // subpass backdrop filters:
+ //
+ // 1. For subpass backdrop filters, the snapshot transform is always just a
+ // translation that positions the parent pass texture correctly relative
+ // to the subpass texture. However, this translation always needs to be
+ // applied in screen space.
+ //
+ // Since we know the snapshot transform will always have an identity
+ // basis in this case, we safely reverse the order and apply the filter's
+ // matrix within the snapshot transform space.
+ //
+ // 2. The filter's matrix needs to be applied within the space defined by
+ // the scene's current transformation matrix (CTM). For example: If the
+ // CTM is scaled up, then translations applied by the matrix should be
+ // magnified accordingly.
+ //
+ // To accomplish this, we sandwitch the filter's matrix within the CTM in
+ // both cases. But notice that for the subpass backdrop filter case, we
+ // use the "effect transform" instead of the Entity's transform!
+ // use the "effect transform" instead of the Entity's transform!
+ //
+ // That's because in the subpass backdrop filter case, the Entity's
+ // transform isn't actually the captured CTM of the scene like it usually
+ // is; instead, it's just a screen space translation that offsets the
+ // backdrop texture (as mentioned above). And so we sneak the subpass's
+ // captured CTM in through the effect transform.
+ //
+
+ snapshot->transform = snapshot->transform * //
+ effect_transform * //
+ matrix_ * //
+ effect_transform.Invert();
+ } else {
+ snapshot->transform = entity.GetTransform() * //
+ matrix_ * //
+ entity.GetTransform().Invert() * //
+ snapshot->transform;
+ }
snapshot->sampler_descriptor = sampler_descriptor_;
if (!snapshot.has_value()) {
return std::nullopt;Here is a test for it: TEST_P(DlGoldenTest, Bug147807) {
Point content_scale = GetContentScale();
auto draw = [content_scale](DlCanvas* canvas,
const std::vector<sk_sp<DlImage>>& images) {
canvas->Transform2DAffine(content_scale.x, 0, 0, 0, content_scale.y, 0);
DlPaint paint;
paint.setColor(DlColor(0xfffef7ff));
canvas->DrawRect(SkRect::MakeLTRB(0, 0, 375, 667), paint);
paint.setColor(DlColor(0xffff9800));
canvas->DrawRect(SkRect::MakeLTRB(0, 0, 187.5, 333.5), paint);
paint.setColor(DlColor(0xff9c27b0));
canvas->DrawRect(SkRect::MakeLTRB(187.5, 0, 375, 333.5), paint);
paint.setColor(DlColor(0xff4caf50));
canvas->DrawRect(SkRect::MakeLTRB(0, 333.5, 187.5, 667), paint);
paint.setColor(DlColor(0xfff44336));
canvas->DrawRect(SkRect::MakeLTRB(187.5, 333.5, 375, 667), paint);
canvas->Save();
{
canvas->ClipRRect(
SkRRect::MakeOval(SkRect::MakeLTRB(201.25, 10, 361.25, 170)),
DlCanvas::ClipOp::kIntersect, true);
SkRect save_layer_bounds = SkRect::MakeLTRB(201.25, 10, 361.25, 170);
DlMatrixImageFilter backdrop(SkMatrix::MakeAll(3, 0, -299.25, //
0, 3, -949, //
0, 0, 1),
DlImageSampling::kLinear);
canvas->SaveLayer(&save_layer_bounds, /*paint=*/nullptr, &backdrop);
{
canvas->Translate(201.25, 10);
auto paint = DlPaint()
.setAntiAlias(true)
.setColor(DlColor(0xff2196f3))
.setStrokeWidth(5)
.setDrawStyle(DlDrawStyle::kStroke);
canvas->DrawCircle(SkPoint::Make(80, 80), 80, paint);
}
canvas->Restore();
}
canvas->Restore();
};
DisplayListBuilder builder;
std::vector<sk_sp<DlImage>> images;
draw(&builder, images);
ASSERT_TRUE(OpenPlaygroundHere(builder.Build()));
} |
|
The problem with truncation is probably happening because the coverage hint is wrong. I wonder if we tried backing out the coverage hint or setting it to max size. |
|
Just to clarify the truncation problem I'm seeing means that at a certain finger position at the right of the screen the zoomed in view will just disappear (as opposed to drawing a segment of what it should be rendering). So it seems like a coverage issue, not a rendering issue as Jonah suspects. |
diff --git a/impeller/entity/contents/filters/filter_contents.cc b/impeller/entity/contents/filters/filter_contents.cc
index 46cfab8c87..b303c07b15 100644
--- a/impeller/entity/contents/filters/filter_contents.cc
+++ b/impeller/entity/contents/filters/filter_contents.cc
@@ -151,10 +151,10 @@ bool FilterContents::Render(const ContentContext& renderer,
std::optional<Rect> FilterContents::GetLocalCoverage(
const Entity& local_entity) const {
auto coverage = GetFilterCoverage(inputs_, local_entity, effect_transform_);
- auto coverage_hint = GetCoverageHint();
- if (coverage_hint.has_value() && coverage.has_value()) {
- coverage = coverage->Intersection(coverage_hint.value());
- }
+// auto coverage_hint = GetCoverageHint();
+// if (coverage_hint.has_value() && coverage.has_value()) {
+// coverage = coverage->Intersection(coverage_hint.value());
+// }
return coverage;
} |
|
Would it be appropriate to allow filters to scale their coverage hint? So in this problem case, the coverage hint is |
|
FYI @bdero I thought the intention was the coverage at this point was all in screen space, though it might be offset. IF we're getting the coverage hint wrong because of an offset in the filter transform, maybe that is the cause - so it makes sense to me. That said we need to avoid double transforming things. |
|
Here is the problem numbers for consideration:
The matrix filter is scaling by 3 and translating. Looking at these numbers it seems that transforming the coverage hint could get them to overlap. However it seems the filter coverage isn't in the appropriate coordinate space, maybe it's missing the offset of the subpass? |
8 tasks
|
I've identified a case where a rendered entity's coverage does not match the result of the The test that fails: TEST_P(MatrixFilterContentsTest, RenderCoverageMatchesGetCoverageSubpassScale) {
std::shared_ptr<Texture> texture = MakeTexture(ISize(100, 100));
MatrixFilterContents contents;
contents.SetInputs({FilterInput::Make(texture)});
contents.SetMatrix(Matrix::MakeScale({3, 3, 1}));
contents.SetEffectTransform(Matrix::MakeScale({2, 2, 1}));
contents.SetRenderingMode(Entity::RenderingMode::kSubpass);
Entity entity;
entity.SetTransform(Matrix::MakeTranslation({100, 200, 0}));
std::shared_ptr<ContentContext> renderer = GetContentContext();
std::optional<Entity> result =
contents.GetEntity(*renderer, entity, /*coverage_hint=*/{});
expectRenderCoverageEqual(result, contents.GetCoverage(entity),
Rect::MakeXYWH(300, 600, 300, 300));
}
|
|
I was able to verify that the failing test in #147807 (comment) is a proper reproduction of the "truncation problem". I have it fixed up and I'm cleaning it up for review. |
8 tasks
auto-submit bot
added a commit
to flutter/engine
that referenced
this issue
May 18, 2024
) Reverts: #52880 Initiated by: jonahwilliams Reason for reverting: unexpected framework golden change Original PR Author: gaaclarke Reviewed By: {bdero} This change reverts the following previous change: fixes: flutter/flutter#147807 relands #43943 (with fixes that hopefully avoid it being reverted again) [C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style
danagbemava-nc
added
the
r: fixed
|
The fix was reverted for a failure in a framework golden test: flutter/engine#52880 (comment) |
gaaclarke
added a commit
to gaaclarke/engine
that referenced
this issue
May 20, 2024
fixes: flutter/flutter#147807 relands flutter#43943 (with fixes that hopefully avoid it being reverted again) [C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style
8 tasks
danagbemava-nc
removed
the
r: fixed
auto-submit bot
pushed a commit
to flutter/engine
that referenced
this issue
May 21, 2024
fixes flutter/flutter#147807 relands #52880 relands #43943 This was previously reverted because of the golden test failure `widgets.widgets.magnifier.styled`. This fixes that problem by instead of focusing on ImageFilter and BackdropFilter subpasses, instead focuses on wether a subpass is clipped or not and by extension how the math should be handled. `widgets.widgets.magnifier.styled` after diff:  [C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Steps to reproduce
Expected results
skia and impeller act the same
Expect to see all 4 colors within magnified section if center of the app is long pressed
Actual results
A different area will become magnified when using impeller
Code sample
https://github.com/garrettApproachableGeek/impeller_bug
Screenshots or Video
Screenshots
Impeller Enabled
Impeller Disabled
Logs
Logs
flutter run -d <device_id> --enable-impellerflutter run -d <device_id> --no-enable-impellerFlutter Doctor output
Doctor output
The text was updated successfully, but these errors were encountered: