Skip to content

Commit

Permalink
Revert "Try rasterizing images and layers only once , even when their…
Browse files Browse the repository at this point in the history
… rasterization fails. Further enforce the same access threshold on layers as on Pictures. Previously layers would always be cached. The latter is a semantic change. (flutter#16545)" (flutter#16889)

This caused regression in several benchmarks, including:
animated_placeholder_perf. Regression tracked in
flutter/flutter#51776.

This reverts commit 01a52b9.
  • Loading branch information
cbracken authored Mar 2, 2020
1 parent 5073cc7 commit df94213
Show file tree
Hide file tree
Showing 3 changed files with 34 additions and 64 deletions.
39 changes: 18 additions & 21 deletions flow/raster_cache.cc
Original file line number Diff line number Diff line change
Expand Up @@ -119,15 +119,24 @@ static RasterCacheResult Rasterize(
return {surface->makeImageSnapshot(), logical_rect};
}

bool RasterCache::Prepare(PrerollContext* context,
RasterCacheResult RasterizePicture(SkPicture* picture,
GrContext* context,
const SkMatrix& ctm,
SkColorSpace* dst_color_space,
bool checkerboard) {
return Rasterize(context, ctm, dst_color_space, checkerboard,
picture->cullRect(),
[=](SkCanvas* canvas) { canvas->drawPicture(picture); });
}

void RasterCache::Prepare(PrerollContext* context,
Layer* layer,
const SkMatrix& ctm) {
LayerRasterCacheKey cache_key(layer->unique_id(), ctm);

Entry& entry = layer_cache_[cache_key];

if (!entry.did_rasterize && !entry.image.is_valid() &&
entry.access_count >= access_threshold_) {
entry.access_count++;
entry.used_this_frame = true;
if (!entry.image.is_valid()) {
entry.image = Rasterize(
context->gr_context, ctm, context->dst_color_space,
checkerboard_images_, layer->paint_bounds(),
Expand All @@ -152,12 +161,7 @@ bool RasterCache::Prepare(PrerollContext* context,
layer->Paint(paintContext);
}
});

entry.did_rasterize = true;

return true;
}
return false;
}

bool RasterCache::Prepare(GrContext* context,
Expand Down Expand Up @@ -196,19 +200,12 @@ bool RasterCache::Prepare(GrContext* context,
return false;
}

// Don't try to rasterize pictures that were already attempted to be
// rasterized even if the image is invalid.
if (!entry.did_rasterize && !entry.image.is_valid()) {
entry.image =
Rasterize(context, transformation_matrix, dst_color_space,
checkerboard_images_, picture->cullRect(),
[=](SkCanvas* canvas) { canvas->drawPicture(picture); });

entry.did_rasterize = true;
if (!entry.image.is_valid()) {
entry.image = RasterizePicture(picture, context, transformation_matrix,
dst_color_space, checkerboard_images_);
picture_cached_this_frame_++;
return true;
}
return false;
return true;
}

RasterCacheResult RasterCache::Get(const SkPicture& picture,
Expand Down
3 changes: 1 addition & 2 deletions flow/raster_cache.h
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,7 @@ class RasterCache {
bool is_complex,
bool will_change);

bool Prepare(PrerollContext* context, Layer* layer, const SkMatrix& ctm);
void Prepare(PrerollContext* context, Layer* layer, const SkMatrix& ctm);

RasterCacheResult Get(const SkPicture& picture, const SkMatrix& ctm) const;

Expand All @@ -101,7 +101,6 @@ class RasterCache {
private:
struct Entry {
bool used_this_frame = false;
bool did_rasterize = false;
size_t access_count = 0;
RasterCacheResult image;
};
Expand Down
56 changes: 15 additions & 41 deletions flow/raster_cache_unittests.cc
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@

#include "flutter/flow/raster_cache.h"

#include "flutter/flow/layers/container_layer.h"
#include "gtest/gtest.h"
#include "third_party/skia/include/core/SkPicture.h"
#include "third_party/skia/include/core/SkPictureRecorder.h"
Expand All @@ -30,14 +29,16 @@ TEST(RasterCache, SimpleInitialization) {
ASSERT_TRUE(true);
}

TEST(RasterCache, ThresholdIsRespectedForPictures) {
TEST(RasterCache, ThresholdIsRespected) {
size_t threshold = 2;
flutter::RasterCache cache(threshold);

SkMatrix matrix = SkMatrix::I();

auto picture = GetSamplePicture();

sk_sp<SkImage> image;

sk_sp<SkColorSpace> srgb = SkColorSpace::MakeSRGB();
ASSERT_FALSE(
cache.Prepare(NULL, picture.get(), matrix, srgb.get(), true, false));
Expand All @@ -60,28 +61,21 @@ TEST(RasterCache, ThresholdIsRespectedForPictures) {
ASSERT_TRUE(cache.Get(*picture, matrix).is_valid());
}

TEST(RasterCache, ThresholdIsRespectedForLayers) {
size_t threshold = 2;
TEST(RasterCache, AccessThresholdOfZeroDisablesCaching) {
size_t threshold = 0;
flutter::RasterCache cache(threshold);

SkMatrix matrix = SkMatrix::I();

ContainerLayer layer;

sk_sp<SkColorSpace> srgb = SkColorSpace::MakeSRGB();
ASSERT_FALSE(cache.Prepare(nullptr, &layer, matrix));
ASSERT_FALSE(cache.Prepare(nullptr, &layer, matrix));
ASSERT_FALSE(cache.Prepare(nullptr, &layer, matrix));

// 1st access.
ASSERT_FALSE(cache.Get(&layer, matrix).is_valid());
auto picture = GetSamplePicture();

ASSERT_FALSE(cache.Prepare(nullptr, &layer, matrix));
sk_sp<SkImage> image;

// 2st access.
ASSERT_FALSE(cache.Get(&layer, matrix).is_valid());
sk_sp<SkColorSpace> srgb = SkColorSpace::MakeSRGB();
ASSERT_FALSE(
cache.Prepare(NULL, picture.get(), matrix, srgb.get(), true, false));

// Calling Prepare now would crash due to the nullptr.
ASSERT_FALSE(cache.Get(*picture, matrix).is_valid());
}

TEST(RasterCache, PictureCacheLimitPerFrameIsRespectedWhenZero) {
Expand All @@ -92,6 +86,8 @@ TEST(RasterCache, PictureCacheLimitPerFrameIsRespectedWhenZero) {

auto picture = GetSamplePicture();

sk_sp<SkImage> image;

sk_sp<SkColorSpace> srgb = SkColorSpace::MakeSRGB();
ASSERT_FALSE(
cache.Prepare(NULL, picture.get(), matrix, srgb.get(), true, false));
Expand All @@ -107,6 +103,8 @@ TEST(RasterCache, SweepsRemoveUnusedFrames) {

auto picture = GetSamplePicture();

sk_sp<SkImage> image;

sk_sp<SkColorSpace> srgb = SkColorSpace::MakeSRGB();
ASSERT_FALSE(cache.Prepare(NULL, picture.get(), matrix, srgb.get(), true,
false)); // 1
Expand All @@ -124,29 +122,5 @@ TEST(RasterCache, SweepsRemoveUnusedFrames) {
ASSERT_FALSE(cache.Get(*picture, matrix).is_valid());
}

TEST(RasterCache, TryRasterizngOnlyOnce) {
size_t threshold = 1;
flutter::RasterCache cache(threshold);

SkMatrix matrix = SkMatrix::I();
// Test picture too large to successfully rasterize.
auto picture = SkPicture::MakePlaceholder(SkRect::MakeWH(2e12, 2e12));

sk_sp<SkColorSpace> srgb = SkColorSpace::MakeSRGB();
ASSERT_FALSE(cache.Prepare(nullptr, picture.get(), matrix, srgb.get(), true,
false)); // 1
ASSERT_FALSE(cache.Get(*picture, matrix).is_valid());

// Rasterization ran, though Get() below returns an invalid image.
ASSERT_TRUE(cache.Prepare(nullptr, picture.get(), matrix, srgb.get(), true,
false)); // 2
ASSERT_FALSE(cache.Get(*picture, matrix).is_valid());

// This time we should not try again to rasterize.
ASSERT_FALSE(cache.Prepare(nullptr, picture.get(), matrix, srgb.get(), true,
false)); // 2
ASSERT_FALSE(cache.Get(*picture, matrix).is_valid());
}

} // namespace testing
} // namespace flutter

0 comments on commit df94213

Please sign in to comment.