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

Commit cce3017

Browse files
committed
Avoid unnecessary transform resets for pixel snapping
1 parent eac124e commit cce3017

File tree

7 files changed

+137
-52
lines changed

7 files changed

+137
-52
lines changed

flow/diff_context.cc

Lines changed: 10 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -61,11 +61,17 @@ void DiffContext::MakeCurrentTransformIntegral() {
6161
// TODO(knopp): This is duplicated from LayerStack. Maybe should be part of
6262
// clip tracker?
6363
if (clip_tracker_.using_4x4_matrix()) {
64-
clip_tracker_.setTransform(
65-
RasterCacheUtil::GetIntegralTransCTM(clip_tracker_.matrix_4x4()));
64+
SkM44 integral;
65+
if (RasterCacheUtil::ComputeIntegralTransCTM(clip_tracker_.matrix_4x4(),
66+
&integral)) {
67+
clip_tracker_.setTransform(integral);
68+
}
6669
} else {
67-
clip_tracker_.setTransform(
68-
RasterCacheUtil::GetIntegralTransCTM(clip_tracker_.matrix_3x3()));
70+
SkMatrix integral;
71+
if (RasterCacheUtil::ComputeIntegralTransCTM(clip_tracker_.matrix_3x3(),
72+
&integral)) {
73+
clip_tracker_.setTransform(integral);
74+
}
6975
}
7076
}
7177

flow/layers/display_list_layer_unittests.cc

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -249,7 +249,7 @@ TEST_F(DisplayListLayerTest, CachedIncompatibleDisplayListOpacityInheritance) {
249249
&paint_context(), false);
250250

251251
int opacity_alpha = 0x7F;
252-
SkPoint opacity_offset = SkPoint::Make(10, 10);
252+
SkPoint opacity_offset = SkPoint::Make(10.2, 10.2);
253253
auto opacity_layer =
254254
std::make_shared<OpacityLayer>(opacity_alpha, opacity_offset);
255255
opacity_layer->Add(display_list_layer);

flow/layers/image_filter_layer_unittests.cc

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -454,9 +454,8 @@ TEST_F(ImageFilterLayerTest, CacheChildren) {
454454
expected_builder.Save();
455455
{
456456
expected_builder.Translate(offset.fX, offset.fY);
457-
// snap translation components to pixels due to using raster cache
458-
expected_builder.TransformReset();
459-
expected_builder.Transform(snapped_matrix);
457+
// translation components already snapped to pixels, intent to
458+
// use raster cache won't change them
460459
DlPaint dl_paint;
461460
dl_paint.setImageFilter(transformed_filter.get());
462461
raster_cache()->Draw(cacheable_image_filter_item->GetId().value(),

flow/layers/layer_state_stack.cc

Lines changed: 14 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -114,8 +114,10 @@ class DlCanvasDelegate : public LayerStateStack::Delegate {
114114
canvas_->Transform(matrix);
115115
}
116116
void integralTransform() override {
117-
SkM44 matrix = RasterCacheUtil::GetIntegralTransCTM(matrix_4x4());
118-
canvas_->SetTransform(matrix);
117+
SkM44 integral;
118+
if (RasterCacheUtil::ComputeIntegralTransCTM(matrix_4x4(), &integral)) {
119+
canvas_->SetTransform(integral);
120+
}
119121
}
120122

121123
void clipRect(const SkRect& rect, ClipOp op, bool is_aa) override {
@@ -168,11 +170,17 @@ class PrerollDelegate : public LayerStateStack::Delegate {
168170
}
169171
void integralTransform() override {
170172
if (tracker_.using_4x4_matrix()) {
171-
tracker_.setTransform(
172-
RasterCacheUtil::GetIntegralTransCTM(tracker_.matrix_4x4()));
173+
SkM44 integral;
174+
if (RasterCacheUtil::ComputeIntegralTransCTM(tracker_.matrix_4x4(),
175+
&integral)) {
176+
tracker_.setTransform(integral);
177+
}
173178
} else {
174-
tracker_.setTransform(
175-
RasterCacheUtil::GetIntegralTransCTM(tracker_.matrix_3x3()));
179+
SkMatrix integral;
180+
if (RasterCacheUtil::ComputeIntegralTransCTM(tracker_.matrix_3x3(),
181+
&integral)) {
182+
tracker_.setTransform(integral);
183+
}
176184
}
177185
}
178186

flow/layers/shader_mask_layer_unittests.cc

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -443,10 +443,10 @@ TEST_F(ShaderMaskLayerTest, SimpleFilterWithRasterCacheLayerNotCached) {
443443
/* (ShaderMask)layer::Paint */ {
444444
expected_builder.Save();
445445
{
446-
expected_builder.TransformReset();
447-
// The layer will perform this Identity transform operation by default,
448-
// but it should be ignored both here and in the layer paint
449-
expected_builder.Transform(SkMatrix());
446+
// The layer will notice that the CTM is already an integer matrix
447+
// and will not perform an Integral CTM operation.
448+
// expected_builder.TransformReset();
449+
// expected_builder.Transform(SkMatrix());
450450
expected_builder.SaveLayer(&child_bounds);
451451
{
452452
/* mock_layer::Paint */ {

flow/raster_cache_util.cc

Lines changed: 64 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4,4 +4,67 @@
44

55
#include "flutter/flow/raster_cache_util.h"
66

7-
namespace flutter {}
7+
namespace flutter {
8+
9+
bool RasterCacheUtil::ComputeIntegralTransCTM(const SkMatrix& in,
10+
SkMatrix* out) {
11+
// Avoid integral snapping if the matrix has complex transformation to avoid
12+
// the artifact observed in https://github.com/flutter/flutter/issues/41654.
13+
if (!in.isScaleTranslate()) {
14+
return false;
15+
}
16+
17+
SkScalar in_tx = in.getTranslateX();
18+
SkScalar in_ty = in.getTranslateY();
19+
SkScalar out_tx = SkScalarRoundToScalar(in_tx);
20+
SkScalar out_ty = SkScalarRoundToScalar(in_ty);
21+
if (out_tx != in_tx || out_ty != in_ty) {
22+
// As a side effect of those tests we also know that neither translation
23+
// component was a NaN
24+
*out = in;
25+
(*out)[SkMatrix::kMTransX] = out_tx;
26+
(*out)[SkMatrix::kMTransY] = out_ty;
27+
return true;
28+
}
29+
30+
return false;
31+
}
32+
33+
bool RasterCacheUtil::ComputeIntegralTransCTM(const SkM44& in, SkM44* out) {
34+
// Avoid integral snapping if the matrix has complex transformation to avoid
35+
// the artifact observed in https://github.com/flutter/flutter/issues/41654.
36+
if (in.rc(0, 1) != 0 || in.rc(0, 2) != 0) {
37+
// X multiplied by either Y or Z
38+
return false;
39+
}
40+
if (in.rc(1, 0) != 0 || in.rc(1, 2) != 0) {
41+
// Y multiplied by either X or Z
42+
return false;
43+
}
44+
// We do not need to worry about the Z row unless the W row
45+
// has perspective entries...
46+
if (in.rc(3, 0) != 0 || in.rc(3, 1) != 0 || in.rc(3, 2) != 0 ||
47+
in.rc(3, 3) != 1) {
48+
// W not identity row, therefore perspective is applied
49+
return false;
50+
}
51+
52+
SkScalar in_tx = in.rc(0, 3);
53+
SkScalar in_ty = in.rc(1, 3);
54+
SkScalar out_tx = SkScalarRoundToScalar(in_tx);
55+
SkScalar out_ty = SkScalarRoundToScalar(in_ty);
56+
if (out_tx != in_tx || out_ty != in_ty) {
57+
// As a side effect of those tests we also know that neither translation
58+
// component was a NaN
59+
*out = in;
60+
out->setRC(0, 3, out_tx);
61+
out->setRC(1, 3, out_ty);
62+
// No need to worry about Z translation because it has no effect
63+
// without perspective entries...
64+
return true;
65+
}
66+
67+
return false;
68+
}
69+
70+
} // namespace flutter

flow/raster_cache_util.h

Lines changed: 42 additions & 33 deletions
Original file line numberDiff line numberDiff line change
@@ -78,17 +78,29 @@ struct RasterCacheUtil {
7878
* @return SkMatrix the snapped transformation matrix.
7979
*/
8080
static SkMatrix GetIntegralTransCTM(const SkMatrix& ctm) {
81-
// Avoid integral snapping if the matrix has complex transformation to avoid
82-
// the artifact observed in https://github.com/flutter/flutter/issues/41654.
83-
if (!ctm.isScaleTranslate()) {
84-
return ctm;
85-
}
86-
SkMatrix result = ctm;
87-
result[SkMatrix::kMTransX] = SkScalarRoundToScalar(ctm.getTranslateX());
88-
result[SkMatrix::kMTransY] = SkScalarRoundToScalar(ctm.getTranslateY());
89-
return result;
81+
SkMatrix integral;
82+
return ComputeIntegralTransCTM(ctm, &integral) ? integral : ctm;
9083
}
9184

85+
/**
86+
* @brief Snap the translation components of the |in| matrix to integers
87+
* and store the snapped matrix in |out|.
88+
*
89+
* The snapping will only happen if the matrix only has scale and translation
90+
* transformations. This is used, along with GetRoundedOutDeviceBounds, to
91+
* ensure that the textures drawn by the raster cache are exactly aligned to
92+
* physical pixels. Any layers that participate in raster caching must align
93+
* themselves to physical pixels even when not cached to prevent a change in
94+
* apparent location if caching is later applied.
95+
*
96+
* The |out| matrix will not be modified if this method returns false.
97+
*
98+
* @param in the current transformation matrix.
99+
* @param out the storage for the snapped matrix.
100+
* @return true if the integral modification was needed, false otherwise.
101+
*/
102+
static bool ComputeIntegralTransCTM(const SkMatrix& in, SkMatrix* out);
103+
92104
/**
93105
* @brief Snap the translation components of the matrix to integers.
94106
*
@@ -103,31 +115,28 @@ struct RasterCacheUtil {
103115
* @return SkM44 the snapped transformation matrix.
104116
*/
105117
static SkM44 GetIntegralTransCTM(const SkM44& ctm) {
106-
// Avoid integral snapping if the matrix has complex transformation to avoid
107-
// the artifact observed in https://github.com/flutter/flutter/issues/41654.
108-
if (ctm.rc(0, 1) != 0 || ctm.rc(0, 2) != 0) {
109-
// X multiplied by either Y or Z
110-
return ctm;
111-
}
112-
if (ctm.rc(1, 0) != 0 || ctm.rc(1, 2) != 0) {
113-
// Y multiplied by either X or Z
114-
return ctm;
115-
}
116-
// We do not need to worry about the Z row unless the W row
117-
// has perspective entries...
118-
if (ctm.rc(3, 0) != 0 || ctm.rc(3, 1) != 0 || ctm.rc(3, 2) != 0 ||
119-
ctm.rc(3, 3) != 1) {
120-
// W not identity row, therefore perspective is applied
121-
return ctm;
122-
}
123-
124-
SkM44 result = ctm;
125-
result.setRC(0, 3, SkScalarRoundToScalar(ctm.rc(0, 3)));
126-
result.setRC(1, 3, SkScalarRoundToScalar(ctm.rc(1, 3)));
127-
// No need to worry about Z translation because it has no effect
128-
// without perspective entries...
129-
return result;
118+
SkM44 integral;
119+
return ComputeIntegralTransCTM(ctm, &integral) ? integral : ctm;
130120
}
121+
122+
/**
123+
* @brief Snap the translation components of the |in| matrix to integers
124+
* and store the snapped matrix in |out|.
125+
*
126+
* The snapping will only happen if the matrix only has scale and translation
127+
* transformations. This is used, along with GetRoundedOutDeviceBounds, to
128+
* ensure that the textures drawn by the raster cache are exactly aligned to
129+
* physical pixels. Any layers that participate in raster caching must align
130+
* themselves to physical pixels even when not cached to prevent a change in
131+
* apparent location if caching is later applied.
132+
*
133+
* The |out| matrix will not be modified if this method returns false.
134+
*
135+
* @param in the current transformation matrix.
136+
* @param out the storage for the snapped matrix.
137+
* @return true if the integral modification was needed, false otherwise.
138+
*/
139+
static bool ComputeIntegralTransCTM(const SkM44& in, SkM44* out);
131140
};
132141

133142
} // namespace flutter

0 commit comments

Comments
 (0)