Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove layer integral offset snapping #17712

Merged
merged 3 commits into from
Apr 16, 2020
Merged

Remove layer integral offset snapping #17712

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
704efa2
Remove picture layer integer offset snapping
liyuqian Apr 14, 2020
0e88f9b
Remove GetIntegralTransCTM
liyuqian Apr 14, 2020
c3dd836
ci/format.sh
liyuqian Apr 15, 2020
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
9 changes: 0 additions & 9 deletions flow/layers/image_filter_layer.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,9 +31,6 @@ void ImageFilterLayer::Preroll(PrerollContext* context,
if (!context->has_platform_view && context->raster_cache &&
SkRect::Intersects(context->cull_rect, paint_bounds())) {
Copy link
Contributor

Choose a reason for hiding this comment

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

How many of these if statements have identical tests? Could this be added as a boilerplate method either in the base Layer or in the PrerollContext?

Copy link
Contributor Author

@liyuqian liyuqian Apr 17, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

  1. Done in Reland "Remove layer integral offset snapping" #17915

SkMatrix ctm = matrix;
#ifndef SUPPORT_FRACTIONAL_TRANSLATION
ctm = RasterCache::GetIntegralTransCTM(ctm);
#endif
context->raster_cache->Prepare(context, this, ctm);
Copy link
Contributor

Choose a reason for hiding this comment

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

For cases like these (and I'm guessing there are a large number of them, the ctm local variable is unnecessary and should be deleted.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in #17915

}
}
Expand All @@ -42,12 +39,6 @@ void ImageFilterLayer::Paint(PaintContext& context) const {
TRACE_EVENT0("flutter", "ImageFilterLayer::Paint");
FML_DCHECK(needs_painting());

#ifndef SUPPORT_FRACTIONAL_TRANSLATION
SkAutoCanvasRestore save(context.leaf_nodes_canvas, true);
context.leaf_nodes_canvas->setMatrix(RasterCache::GetIntegralTransCTM(
context.leaf_nodes_canvas->getTotalMatrix()));
#endif

if (context.raster_cache) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Similarly, is this all boilerplate for many layers?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in #17791

const SkMatrix& ctm = context.leaf_nodes_canvas->getTotalMatrix();
RasterCacheResult layer_cache =
Expand Down
68 changes: 25 additions & 43 deletions flow/layers/image_filter_layer_unittests.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,14 +60,11 @@ TEST_F(ImageFilterLayerTest, EmptyFilter) {
layer->Paint(paint_context());
EXPECT_EQ(mock_canvas().draw_calls(),
std::vector({
MockCanvas::DrawCall{0, MockCanvas::SaveData{1}},
MockCanvas::DrawCall{1, MockCanvas::SetMatrixData{SkMatrix()}},
MockCanvas::DrawCall{
1, MockCanvas::SaveLayerData{child_bounds, filter_paint,
nullptr, 2}},
0, MockCanvas::SaveLayerData{child_bounds, filter_paint,
nullptr, 1}},
MockCanvas::DrawCall{
2, MockCanvas::DrawPathData{child_path, child_paint}},
MockCanvas::DrawCall{2, MockCanvas::RestoreData{1}},
1, MockCanvas::DrawPathData{child_path, child_paint}},
MockCanvas::DrawCall{1, MockCanvas::RestoreData{0}},
}));
}
Expand Down Expand Up @@ -96,14 +93,11 @@ TEST_F(ImageFilterLayerTest, SimpleFilter) {
layer->Paint(paint_context());
EXPECT_EQ(mock_canvas().draw_calls(),
std::vector({
MockCanvas::DrawCall{0, MockCanvas::SaveData{1}},
MockCanvas::DrawCall{1, MockCanvas::SetMatrixData{SkMatrix()}},
MockCanvas::DrawCall{
1, MockCanvas::SaveLayerData{child_bounds, filter_paint,
nullptr, 2}},
0, MockCanvas::SaveLayerData{child_bounds, filter_paint,
nullptr, 1}},
MockCanvas::DrawCall{
2, MockCanvas::DrawPathData{child_path, child_paint}},
MockCanvas::DrawCall{2, MockCanvas::RestoreData{1}},
1, MockCanvas::DrawPathData{child_path, child_paint}},
MockCanvas::DrawCall{1, MockCanvas::RestoreData{0}},
}));
}
Expand Down Expand Up @@ -132,14 +126,11 @@ TEST_F(ImageFilterLayerTest, SimpleFilterBounds) {
layer->Paint(paint_context());
EXPECT_EQ(mock_canvas().draw_calls(),
std::vector({
MockCanvas::DrawCall{0, MockCanvas::SaveData{1}},
MockCanvas::DrawCall{1, MockCanvas::SetMatrixData{SkMatrix()}},
MockCanvas::DrawCall{
1, MockCanvas::SaveLayerData{child_bounds, filter_paint,
nullptr, 2}},
0, MockCanvas::SaveLayerData{child_bounds, filter_paint,
nullptr, 1}},
MockCanvas::DrawCall{
2, MockCanvas::DrawPathData{child_path, child_paint}},
MockCanvas::DrawCall{2, MockCanvas::RestoreData{1}},
1, MockCanvas::DrawPathData{child_path, child_paint}},
MockCanvas::DrawCall{1, MockCanvas::RestoreData{0}},
}));
}
Expand Down Expand Up @@ -177,19 +168,16 @@ TEST_F(ImageFilterLayerTest, MultipleChildren) {
SkPaint filter_paint;
filter_paint.setImageFilter(layer_filter);
layer->Paint(paint_context());
EXPECT_EQ(mock_canvas().draw_calls(),
std::vector(
{MockCanvas::DrawCall{0, MockCanvas::SaveData{1}},
MockCanvas::DrawCall{1, MockCanvas::SetMatrixData{SkMatrix()}},
MockCanvas::DrawCall{
1, MockCanvas::SaveLayerData{children_bounds, filter_paint,
nullptr, 2}},
MockCanvas::DrawCall{
2, MockCanvas::DrawPathData{child_path1, child_paint1}},
MockCanvas::DrawCall{
2, MockCanvas::DrawPathData{child_path2, child_paint2}},
MockCanvas::DrawCall{2, MockCanvas::RestoreData{1}},
MockCanvas::DrawCall{1, MockCanvas::RestoreData{0}}}));
EXPECT_EQ(
mock_canvas().draw_calls(),
std::vector({MockCanvas::DrawCall{
0, MockCanvas::SaveLayerData{children_bounds,
filter_paint, nullptr, 1}},
MockCanvas::DrawCall{
1, MockCanvas::DrawPathData{child_path1, child_paint1}},
MockCanvas::DrawCall{
1, MockCanvas::DrawPathData{child_path2, child_paint2}},
MockCanvas::DrawCall{1, MockCanvas::RestoreData{0}}}));
}

TEST_F(ImageFilterLayerTest, Nested) {
Expand Down Expand Up @@ -237,22 +225,16 @@ TEST_F(ImageFilterLayerTest, Nested) {
layer1->Paint(paint_context());
EXPECT_EQ(mock_canvas().draw_calls(),
std::vector({
MockCanvas::DrawCall{0, MockCanvas::SaveData{1}},
MockCanvas::DrawCall{1, MockCanvas::SetMatrixData{SkMatrix()}},
MockCanvas::DrawCall{
1, MockCanvas::SaveLayerData{children_bounds, filter_paint1,
nullptr, 2}},
0, MockCanvas::SaveLayerData{children_bounds, filter_paint1,
nullptr, 1}},
MockCanvas::DrawCall{
2, MockCanvas::DrawPathData{child_path1, child_paint1}},
MockCanvas::DrawCall{2, MockCanvas::SaveData{3}},
MockCanvas::DrawCall{3, MockCanvas::SetMatrixData{SkMatrix()}},
1, MockCanvas::DrawPathData{child_path1, child_paint1}},
MockCanvas::DrawCall{
3, MockCanvas::SaveLayerData{child_path2.getBounds(),
filter_paint2, nullptr, 4}},
1, MockCanvas::SaveLayerData{child_path2.getBounds(),
filter_paint2, nullptr, 2}},
MockCanvas::DrawCall{
4, MockCanvas::DrawPathData{child_path2, child_paint2}},
MockCanvas::DrawCall{4, MockCanvas::RestoreData{3}},
MockCanvas::DrawCall{3, MockCanvas::RestoreData{2}},
2, MockCanvas::DrawPathData{child_path2, child_paint2}},
MockCanvas::DrawCall{2, MockCanvas::RestoreData{1}},
MockCanvas::DrawCall{1, MockCanvas::RestoreData{0}},
}));
Expand Down
11 changes: 1 addition & 10 deletions flow/layers/opacity_layer.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,9 +51,6 @@ void OpacityLayer::Preroll(PrerollContext* context, const SkMatrix& matrix) {
if (!context->has_platform_view && context->raster_cache &&
SkRect::Intersects(context->cull_rect, paint_bounds())) {
SkMatrix ctm = child_matrix;
Copy link
Contributor

Choose a reason for hiding this comment

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

ctm not needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in #17915

#ifndef SUPPORT_FRACTIONAL_TRANSLATION
ctm = RasterCache::GetIntegralTransCTM(ctm);
#endif
context->raster_cache->Prepare(context, container, ctm);
}
}
Expand All @@ -69,11 +66,6 @@ void OpacityLayer::Paint(PaintContext& context) const {
SkAutoCanvasRestore save(context.internal_nodes_canvas, true);
context.internal_nodes_canvas->translate(offset_.fX, offset_.fY);

#ifndef SUPPORT_FRACTIONAL_TRANSLATION
context.internal_nodes_canvas->setMatrix(RasterCache::GetIntegralTransCTM(
context.leaf_nodes_canvas->getTotalMatrix()));
#endif

if (context.raster_cache) {
ContainerLayer* container = GetChildContainer();
const SkMatrix& ctm = context.leaf_nodes_canvas->getTotalMatrix();
Expand All @@ -87,8 +79,7 @@ void OpacityLayer::Paint(PaintContext& context) const {
// Skia may clip the content with saveLayerBounds (although it's not a
// guaranteed clip). So we have to provide a big enough saveLayerBounds. To do
// so, we first remove the offset from paint bounds since it's already in the
// matrix. Then we round out the bounds because of our
// RasterCache::GetIntegralTransCTM optimization.
// matrix. Then we round out the bounds.
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps we introduced the offset into the CTM prematurely above? If we hadn't already added it, we wouldn't have to adjust for it here...?

Copy link
Contributor

Choose a reason for hiding this comment

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

(aka line 70 in this method)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We need the offset in ctm to draw the raster cache correctly. Meanwhile, the offset is subtracted from the paint_bounds(). We have to add the offset to paint_bounds() because it will be used by its parent layer.

//
// Note that the following lines are only accessible when the raster cache is
// not available (e.g., when we're using the software backend in golden
Expand Down
39 changes: 0 additions & 39 deletions flow/layers/opacity_layer_unittests.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,10 +58,6 @@ TEST_F(OpacityLayerTest, FullyOpaque) {
const SkMatrix initial_transform = SkMatrix::MakeTrans(0.5f, 0.5f);
const SkMatrix layer_transform =
SkMatrix::MakeTrans(layer_offset.fX, layer_offset.fY);
#ifndef SUPPORT_FRACTIONAL_TRANSLATION
const SkMatrix integral_layer_transform = RasterCache::GetIntegralTransCTM(
SkMatrix::Concat(initial_transform, layer_transform));
#endif
const SkPaint child_paint = SkPaint(SkColors::kGreen);
const SkRect expected_layer_bounds =
layer_transform.mapRect(child_path.getBounds());
Expand All @@ -86,10 +82,6 @@ TEST_F(OpacityLayerTest, FullyOpaque) {
auto expected_draw_calls = std::vector(
{MockCanvas::DrawCall{0, MockCanvas::SaveData{1}},
MockCanvas::DrawCall{1, MockCanvas::ConcatMatrixData{layer_transform}},
#ifndef SUPPORT_FRACTIONAL_TRANSLATION
MockCanvas::DrawCall{
1, MockCanvas::SetMatrixData{integral_layer_transform}},
#endif
MockCanvas::DrawCall{
1, MockCanvas::SaveLayerData{opacity_bounds, opacity_paint, nullptr,
2}},
Expand All @@ -107,10 +99,6 @@ TEST_F(OpacityLayerTest, FullyTransparent) {
const SkMatrix initial_transform = SkMatrix::MakeTrans(0.5f, 0.5f);
const SkMatrix layer_transform =
SkMatrix::MakeTrans(layer_offset.fX, layer_offset.fY);
#ifndef SUPPORT_FRACTIONAL_TRANSLATION
const SkMatrix integral_layer_transform = RasterCache::GetIntegralTransCTM(
SkMatrix::Concat(initial_transform, layer_transform));
#endif
const SkPaint child_paint = SkPaint(SkColors::kGreen);
const SkRect expected_layer_bounds =
layer_transform.mapRect(child_path.getBounds());
Expand All @@ -133,10 +121,6 @@ TEST_F(OpacityLayerTest, FullyTransparent) {
auto expected_draw_calls = std::vector(
{MockCanvas::DrawCall{0, MockCanvas::SaveData{1}},
MockCanvas::DrawCall{1, MockCanvas::ConcatMatrixData{layer_transform}},
#ifndef SUPPORT_FRACTIONAL_TRANSLATION
MockCanvas::DrawCall{
1, MockCanvas::SetMatrixData{integral_layer_transform}},
#endif
MockCanvas::DrawCall{1, MockCanvas::SaveData{2}},
MockCanvas::DrawCall{
2, MockCanvas::ClipRectData{kEmptyRect, SkClipOp::kIntersect,
Expand All @@ -155,10 +139,6 @@ TEST_F(OpacityLayerTest, HalfTransparent) {
const SkMatrix initial_transform = SkMatrix::MakeTrans(0.5f, 0.5f);
const SkMatrix layer_transform =
SkMatrix::MakeTrans(layer_offset.fX, layer_offset.fY);
#ifndef SUPPORT_FRACTIONAL_TRANSLATION
const SkMatrix integral_layer_transform = RasterCache::GetIntegralTransCTM(
SkMatrix::Concat(initial_transform, layer_transform));
#endif
const SkPaint child_paint = SkPaint(SkColors::kGreen);
const SkRect expected_layer_bounds =
layer_transform.mapRect(child_path.getBounds());
Expand All @@ -185,10 +165,6 @@ TEST_F(OpacityLayerTest, HalfTransparent) {
auto expected_draw_calls = std::vector(
{MockCanvas::DrawCall{0, MockCanvas::SaveData{1}},
MockCanvas::DrawCall{1, MockCanvas::ConcatMatrixData{layer_transform}},
#ifndef SUPPORT_FRACTIONAL_TRANSLATION
MockCanvas::DrawCall{
1, MockCanvas::SetMatrixData{integral_layer_transform}},
#endif
MockCanvas::DrawCall{
1, MockCanvas::SaveLayerData{opacity_bounds, opacity_paint, nullptr,
2}},
Expand All @@ -211,13 +187,6 @@ TEST_F(OpacityLayerTest, Nested) {
SkMatrix::MakeTrans(layer1_offset.fX, layer1_offset.fY);
const SkMatrix layer2_transform =
SkMatrix::MakeTrans(layer2_offset.fX, layer2_offset.fY);
#ifndef SUPPORT_FRACTIONAL_TRANSLATION
const SkMatrix integral_layer1_transform = RasterCache::GetIntegralTransCTM(
SkMatrix::Concat(initial_transform, layer1_transform));
const SkMatrix integral_layer2_transform = RasterCache::GetIntegralTransCTM(
SkMatrix::Concat(SkMatrix::Concat(initial_transform, layer1_transform),
layer2_transform));
#endif
const SkPaint child1_paint = SkPaint(SkColors::kRed);
const SkPaint child2_paint = SkPaint(SkColors::kBlue);
const SkPaint child3_paint = SkPaint(SkColors::kGreen);
Expand Down Expand Up @@ -278,21 +247,13 @@ TEST_F(OpacityLayerTest, Nested) {
auto expected_draw_calls = std::vector(
{MockCanvas::DrawCall{0, MockCanvas::SaveData{1}},
MockCanvas::DrawCall{1, MockCanvas::ConcatMatrixData{layer1_transform}},
#ifndef SUPPORT_FRACTIONAL_TRANSLATION
MockCanvas::DrawCall{
1, MockCanvas::SetMatrixData{integral_layer1_transform}},
#endif
MockCanvas::DrawCall{
1, MockCanvas::SaveLayerData{opacity1_bounds, opacity1_paint,
nullptr, 2}},
MockCanvas::DrawCall{
2, MockCanvas::DrawPathData{child1_path, child1_paint}},
MockCanvas::DrawCall{2, MockCanvas::SaveData{3}},
MockCanvas::DrawCall{3, MockCanvas::ConcatMatrixData{layer2_transform}},
#ifndef SUPPORT_FRACTIONAL_TRANSLATION
MockCanvas::DrawCall{
3, MockCanvas::SetMatrixData{integral_layer2_transform}},
#endif
MockCanvas::DrawCall{
3, MockCanvas::SaveLayerData{opacity2_bounds, opacity2_paint,
nullptr, 4}},
Expand Down
7 changes: 0 additions & 7 deletions flow/layers/picture_layer.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,9 +26,6 @@ void PictureLayer::Preroll(PrerollContext* context, const SkMatrix& matrix) {

SkMatrix ctm = matrix;
ctm.postTranslate(offset_.x(), offset_.y());
Copy link
Contributor

Choose a reason for hiding this comment

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

So, this is a counter-example for getting rid of the ctm local (that doesn't mean we shouldn't get rid of it in other methods where it really is superficial).

Is there something different about the way that picture_layer handles the offset that might benefit the other uses of the raster cache?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The difference seems to be that OpacityLayer is using MutatorsStack to track the offset due to possible PlatformView children.

#ifndef SUPPORT_FRACTIONAL_TRANSLATION
ctm = RasterCache::GetIntegralTransCTM(ctm);
#endif
cache->Prepare(context->gr_context, sk_picture, ctm,
context->dst_color_space, is_complex_, will_change_);
}
Expand All @@ -44,10 +41,6 @@ void PictureLayer::Paint(PaintContext& context) const {

SkAutoCanvasRestore save(context.leaf_nodes_canvas, true);
context.leaf_nodes_canvas->translate(offset_.x(), offset_.y());
#ifndef SUPPORT_FRACTIONAL_TRANSLATION
context.leaf_nodes_canvas->setMatrix(RasterCache::GetIntegralTransCTM(
context.leaf_nodes_canvas->getTotalMatrix()));
#endif

if (context.raster_cache) {
const SkMatrix& ctm = context.leaf_nodes_canvas->getTotalMatrix();
Expand Down
19 changes: 5 additions & 14 deletions flow/layers/picture_layer_unittests.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,10 +11,6 @@
#include "flutter/testing/mock_canvas.h"
#include "third_party/skia/include/core/SkPicture.h"

#ifndef SUPPORT_FRACTIONAL_TRANSLATION
#include "flutter/flow/raster_cache.h"
#endif

namespace flutter {
namespace testing {

Expand Down Expand Up @@ -85,16 +81,11 @@ TEST_F(PictureLayerTest, SimplePicture) {
EXPECT_FALSE(layer->needs_system_composite());

layer->Paint(paint_context());
auto expected_draw_calls = std::vector(
{MockCanvas::DrawCall{0, MockCanvas::SaveData{1}},
MockCanvas::DrawCall{1,
MockCanvas::ConcatMatrixData{layer_offset_matrix}},
#ifndef SUPPORT_FRACTIONAL_TRANSLATION
MockCanvas::DrawCall{
1, MockCanvas::SetMatrixData{RasterCache::GetIntegralTransCTM(
layer_offset_matrix)}},
#endif
MockCanvas::DrawCall{1, MockCanvas::RestoreData{0}}});
auto expected_draw_calls =
std::vector({MockCanvas::DrawCall{0, MockCanvas::SaveData{1}},
MockCanvas::DrawCall{
1, MockCanvas::ConcatMatrixData{layer_offset_matrix}},
MockCanvas::DrawCall{1, MockCanvas::RestoreData{0}}});
EXPECT_EQ(mock_canvas().draw_calls(), expected_draw_calls);
}

Expand Down
7 changes: 0 additions & 7 deletions flow/raster_cache.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,13 +62,6 @@ class RasterCache {
return bounds;
}

static SkMatrix GetIntegralTransCTM(const SkMatrix& ctm) {
SkMatrix result = ctm;
result[SkMatrix::kMTransX] = SkScalarRoundToScalar(ctm.getTranslateX());
result[SkMatrix::kMTransY] = SkScalarRoundToScalar(ctm.getTranslateY());
return result;
}

// Return true if the cache is generated.
//
// We may return false and not generate the cache if
Expand Down
7 changes: 2 additions & 5 deletions flow/raster_cache_key.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,11 +15,8 @@ template <typename ID>
class RasterCacheKey {
public:
RasterCacheKey(ID id, const SkMatrix& ctm) : id_(id), matrix_(ctm) {
matrix_[SkMatrix::kMTransX] = SkScalarFraction(ctm.getTranslateX());
matrix_[SkMatrix::kMTransY] = SkScalarFraction(ctm.getTranslateY());
#ifndef SUPPORT_FRACTIONAL_TRANSLATION
FML_DCHECK(matrix_.getTranslateX() == 0 && matrix_.getTranslateY() == 0);
#endif
matrix_[SkMatrix::kMTransX] = 0;
matrix_[SkMatrix::kMTransY] = 0;
}

ID id() const { return id_; }
Expand Down