Skip to content

Commit

Permalink
Revert "images: Move animation of images to cc."
Browse files Browse the repository at this point in the history 
This reverts commit cb1afec.

Reason for revert: 
This CL is the most likely reason of failures in MSAN bots on cc_unittests 
GpuImageDecodeCacheTests/GpuImageDecodeCacheTest.QualityCappedAtMedium/1
GpuImageDecodeCacheTests/GpuImageDecodeCacheTest.GetDecodedImageForDrawNegative/1
GpuImageDecodeCacheTests/GpuImageDecodeCacheTest.GetTaskForImageDifferentImage/1
(and >30 more)
on Linux and CrOS https://uberchromegw.corp.google.com/i/chromium.memory/builders/Linux%20MSan%20Tests/builds/4424


Original change's description:
> images: Move animation of images to cc.
> 
> Currently animating an image requires an invalidation and repaint, to
> record the next frame of animation in an updated DisplayItemList from
> blink to cc. This change eliminates repaints for these animations by
> ticking them in the compositor and selecting the frame index at playback
> time during raster, instead of the value stored in the recording. Doing
> this requires the following:
> 
> 1) An ImageAnimationController class is added to track the frame index
> for an image used on the active/pending tree and to schedule an
> invalidation when the animation needs to be advanced.
> 
> 2) The frame_index to use for an image is added to DrawImage for the
> image decode caches. When creating a raster task for a tile, a
> map of image_id_to_frame_index_ to use for an image is stored in the
> PlaybackImageProvider, based on the tree of this tile.
> 
> 3) The animation state for an image is persisted in the controller across
> commits. This is necessary to ensure that the animation is resumed from
> the last frame displayed, even if the image is no longer being painted.
> Subsequent changes will remove this tracking on navigations, or update
> it if explicitly reset on the main thread.
> 
> The following improvements in smoothness and thread_times result for
> https://giphy.com/search/60-fps on Android Nexus5 from this change:
> 
> BEFORE
> thread_total_all_cpu_time_per_frame: 64.023 ms
> thread_total_fast_path_cpu_time_per_frame: 24.411 ms
> thread_renderer_main_cpu_time_per_frame: 9.866 ms
> thread_renderer_compositor_cpu_time_per_frame: 5.513 ms
> thread_raster_cpu_time_per_frame: 29.376 ms
> 
> avg_surface_fps: 28.250
> mean_frame_time: 35.523 ms
> percentage_smooth: 41.495
> jank_count: 95
> max_frame_delay: 9.250
> 
> AFTER
> thread_total_all_cpu_time_per_frame: 42.568 ms
> thread_total_fast_path_cpu_time_per_frame: 21.003 ms
> thread_renderer_main_cpu_time_per_frame: 1.614 ms
> thread_renderer_compositor_cpu_time_per_frame: 4.826 ms
> thread_raster_cpu_time_per_frame: 19.695 ms
> 
> avg_surface_fps: 36.5
> mean_frame_time: 27.302 ms
> percentage_smooth: 60.045
> jank_count: 73
> max_frame_delay: 7.250
> 
> R=​ chrishtr@chromium.org, enne@chromium.org, vmpstr@chromium.org
> 
> Bug: 735741
> Cq-Include-Trybots: master.tryserver.blink:linux_trusty_blink_rel
> Change-Id: I4dc018d4fc2aa82962d05f24d88ae0e73b39f7ca
> Reviewed-on: https://chromium-review.googlesource.com/651136
> Commit-Queue: Khushal <khushalsagar@chromium.org>
> Reviewed-by: Chris Harrelson <chrishtr@chromium.org>
> Reviewed-by: vmpstr <vmpstr@chromium.org>
> Reviewed-by: enne <enne@chromium.org>
> Reviewed-by: Antoine Labour <piman@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#503660}

TBR=vmpstr@chromium.org,chrishtr@chromium.org,enne@chromium.org,khushalsagar@chromium.org,piman@chromium.org

Change-Id: I266bd9daa67937a1b5950ad2ca06d888cbe2b119
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 735741
Cq-Include-Trybots: master.tryserver.blink:linux_trusty_blink_rel
Reviewed-on: https://chromium-review.googlesource.com/678734
Reviewed-by: Vadym Doroshenko <dvadym@chromium.org>
Commit-Queue: Vadym Doroshenko <dvadym@chromium.org>
Cr-Commit-Position: refs/heads/master@{#503702}
  • Loading branch information
Vadym Doroshenko authored and Commit Bot committed Sep 22, 2017
1 parent 419d739 commit 1f14e22
Show file tree
Hide file tree
Showing 56 changed files with 251 additions and 2,204 deletions.
7 changes: 2 additions & 5 deletions cc/BUILD.gn
View file
Original file line number Diff line number Diff line change
Expand Up @@ -282,8 +282,6 @@ cc_component("cc") {
"trees/element_id.h",
"trees/frame_rate_counter.cc",
"trees/frame_rate_counter.h",
"trees/image_animation_controller.cc",
"trees/image_animation_controller.h",
"trees/latency_info_swap_promise.cc",
"trees/latency_info_swap_promise.h",
"trees/latency_info_swap_promise_monitor.cc",
Expand Down Expand Up @@ -412,8 +410,6 @@ cc_static_library("test_support") {
"test/fake_output_surface.h",
"test/fake_output_surface_client.cc",
"test/fake_output_surface_client.h",
"test/fake_paint_image_generator.cc",
"test/fake_paint_image_generator.h",
"test/fake_painted_scrollbar_layer.cc",
"test/fake_painted_scrollbar_layer.h",
"test/fake_picture_layer.cc",
Expand Down Expand Up @@ -489,6 +485,8 @@ cc_static_library("test_support") {
"test/stub_layer_tree_host_client.h",
"test/stub_layer_tree_host_single_thread_client.cc",
"test/stub_layer_tree_host_single_thread_client.h",
"test/stub_paint_image_generator.cc",
"test/stub_paint_image_generator.h",
"test/task_graph_runner_test_template.cc",
"test/task_graph_runner_test_template.h",
"test/test_context_provider.cc",
Expand Down Expand Up @@ -647,7 +645,6 @@ cc_test("cc_unittests") {
"tiles/tile_priority_unittest.cc",
"trees/blocking_task_runner_unittest.cc",
"trees/damage_tracker_unittest.cc",
"trees/image_animation_controller_unittest.cc",
"trees/layer_tree_frame_sink_unittest.cc",
"trees/layer_tree_host_common_unittest.cc",
"trees/layer_tree_host_impl_unittest.cc",
Expand Down
2 changes: 1 addition & 1 deletion cc/benchmarks/rasterize_and_record_benchmark_impl.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,7 @@ void RunBenchmark(RasterSource* raster_source,

PlaybackImageProvider image_provider(false, PaintImageIdFlatSet(), {},
image_decode_cache,
gfx::ColorSpace(), {});
gfx::ColorSpace());
RasterSource::PlaybackSettings settings;
settings.image_provider = &image_provider;

Expand Down
74 changes: 0 additions & 74 deletions cc/layers/picture_layer_impl.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@
#include "cc/debug/debug_colors.h"
#include "cc/layers/append_quads_data.h"
#include "cc/layers/solid_color_layer_impl.h"
#include "cc/paint/display_item_list.h"
#include "cc/tiles/tile_manager.h"
#include "cc/tiles/tiling_set_raster_queue_all.h"
#include "cc/trees/layer_tree_impl.h"
Expand Down Expand Up @@ -120,9 +119,6 @@ PictureLayerImpl::~PictureLayerImpl() {
if (twin_layer_)
twin_layer_->twin_layer_ = nullptr;
layer_tree_impl()->UnregisterPictureLayerImpl(this);

// Unregister for all images on the current raster source.
UnregisterAnimatedImages();
}

void PictureLayerImpl::SetLayerMaskType(Layer::LayerMaskType mask_type) {
Expand Down Expand Up @@ -602,27 +598,11 @@ void PictureLayerImpl::UpdateRasterSource(
<< " bounds " << bounds().ToString() << " pile "
<< raster_source->GetSize().ToString();

// We have an updated recording if the DisplayItemList in the new RasterSource
// is different.
const bool recording_updated =
!raster_source_ || raster_source_->GetDisplayItemList() !=
raster_source->GetDisplayItemList();

// Unregister for all images on the current raster source, if the recording
// was updated.
if (recording_updated)
UnregisterAnimatedImages();

// The |raster_source_| is initially null, so have to check for that for the
// first frame.
bool could_have_tilings = raster_source_.get() && CanHaveTilings();
raster_source_.swap(raster_source);

// Register images from the new raster source, if the recording was updated.
// TODO(khushalsagar): UMA the number of animated images in layer?
if (recording_updated)
RegisterAnimatedImages();

// The |new_invalidation| must be cleared before updating tilings since they
// access the invalidation through the PictureLayerTilingClient interface.
invalidation_.Clear();
Expand Down Expand Up @@ -766,26 +746,6 @@ gfx::Rect PictureLayerImpl::GetEnclosingRectInTargetSpace() const {
return GetScaledEnclosingRectInTargetSpace(MaximumTilingContentsScale());
}

bool PictureLayerImpl::ShouldAnimate(PaintImage::Id paint_image_id) const {
DCHECK(raster_source_);

// Only animate images for layers which HasValidTilePriorities. This check is
// important for 2 reasons:
// 1) It avoids doing additional work for layers we don't plan to rasterize
// and/or draw. The updated state will be pulled by the animation system
// if the draw properties change.
// 2) It eliminates considering layers on the recycle tree. Once the pending
// tree is activated, the layers on the recycle tree remain registered as
// animation drivers, but should not drive animations since they don't have
// updated draw properties.
//
// Additionally only animate images which are on-screen, animations are
// paused once they are not visible.
return HasValidTilePriorities() &&
raster_source_->GetRectForImage(paint_image_id)
.Intersects(visible_layer_rect());
}

gfx::Size PictureLayerImpl::CalculateTileSize(
const gfx::Size& content_bounds) const {
int max_texture_size =
Expand Down Expand Up @@ -1542,38 +1502,4 @@ void PictureLayerImpl::InvalidateRegionForImages(
"Invalidation", invalidation.ToString());
}

void PictureLayerImpl::RegisterAnimatedImages() {
if (!raster_source_ || !raster_source_->GetDisplayItemList())
return;

auto* controller = layer_tree_impl()->image_animation_controller();
if (!controller)
return;

const auto& metadata = raster_source_->GetDisplayItemList()
->discardable_image_map()
.animated_images_metadata();
for (const auto& data : metadata) {
// Only update the metadata from updated recordings received from a commit.
if (layer_tree_impl()->IsSyncTree())
controller->UpdateAnimatedImage(data);
controller->RegisterAnimationDriver(data.paint_image_id, this);
}
}

void PictureLayerImpl::UnregisterAnimatedImages() {
if (!raster_source_ || !raster_source_->GetDisplayItemList())
return;

auto* controller = layer_tree_impl()->image_animation_controller();
if (!controller)
return;

const auto& metadata = raster_source_->GetDisplayItemList()
->discardable_image_map()
.animated_images_metadata();
for (const auto& data : metadata)
controller->UnregisterAnimationDriver(data.paint_image_id, this);
}

} // namespace cc
15 changes: 2 additions & 13 deletions cc/layers/picture_layer_impl.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,18 +20,15 @@
#include "cc/tiles/picture_layer_tiling.h"
#include "cc/tiles/picture_layer_tiling_set.h"
#include "cc/tiles/tiling_set_eviction_queue.h"
#include "cc/trees/image_animation_controller.h"

namespace cc {

class AppendQuadsData;
class MicroBenchmarkImpl;
class Tile;

class CC_EXPORT PictureLayerImpl
: public LayerImpl,
public PictureLayerTilingClient,
public ImageAnimationController::AnimationDriver {
class CC_EXPORT PictureLayerImpl : public LayerImpl,
public PictureLayerTilingClient {
public:
static std::unique_ptr<PictureLayerImpl>
Create(LayerTreeImpl* tree_impl, int id, Layer::LayerMaskType mask_type) {
Expand Down Expand Up @@ -66,9 +63,6 @@ class CC_EXPORT PictureLayerImpl
bool RequiresHighResToDraw() const override;
gfx::Rect GetEnclosingRectInTargetSpace() const override;

// ImageAnimationController::AnimationDriver overrides.
bool ShouldAnimate(PaintImage::Id paint_image_id) const override;

void set_gpu_raster_max_texture_size(gfx::Size gpu_raster_max_texture_size) {
gpu_raster_max_texture_size_ = gpu_raster_max_texture_size;
}
Expand Down Expand Up @@ -113,8 +107,6 @@ class CC_EXPORT PictureLayerImpl

bool RasterSourceUsesLCDTextForTesting() const { return can_use_lcd_text_; }

const Region& InvalidationForTesting() const { return invalidation_; }

protected:
PictureLayerImpl(LayerTreeImpl* tree_impl,
int id,
Expand Down Expand Up @@ -144,9 +136,6 @@ class CC_EXPORT PictureLayerImpl
float MaximumTilingContentsScale() const;
std::unique_ptr<PictureLayerTilingSet> CreatePictureLayerTilingSet();

void RegisterAnimatedImages();
void UnregisterAnimatedImages();

PictureLayerImpl* twin_layer_;

std::unique_ptr<PictureLayerTilingSet> tilings_;
Expand Down
56 changes: 0 additions & 56 deletions cc/layers/picture_layer_impl_unittest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,6 @@
#include "cc/test/fake_recording_source.h"
#include "cc/test/geometry_test_utils.h"
#include "cc/test/layer_test_common.h"
#include "cc/test/skia_common.h"
#include "cc/test/test_layer_tree_host_base.h"
#include "cc/test/test_task_graph_runner.h"
#include "cc/test/test_web_graphics_context_3d.h"
Expand Down Expand Up @@ -85,7 +84,6 @@ class PictureLayerImplTest : public TestLayerTreeHostBase {
settings.create_low_res_tiling = true;
settings.resource_settings.buffer_to_texture_target_map =
viz::DefaultBufferToTextureTargetMapForTesting();
settings.enable_image_animations = true;
return settings;
}

Expand Down Expand Up @@ -5201,59 +5199,5 @@ TEST_F(PictureLayerImplTest, ChangeRasterTranslationNukeActiveLayerTiles) {
}
}

TEST_F(PictureLayerImplTest, AnimatedImages) {
gfx::Size layer_bounds(1000, 1000);

// Set up a raster source with 2 animated images.
auto recording_source = FakeRecordingSource::CreateRecordingSource(
gfx::Rect(layer_bounds), layer_bounds);
std::vector<FrameMetadata> frames = {
FrameMetadata(true, base::TimeDelta::FromMilliseconds(1)),
FrameMetadata(true, base::TimeDelta::FromMilliseconds(1))};
PaintImage image1 = CreateAnimatedImage(gfx::Size(200, 200), frames);
PaintImage image2 = CreateAnimatedImage(gfx::Size(200, 200), frames);
recording_source->add_draw_image(image1, gfx::Point(100, 100));
recording_source->add_draw_image(image2, gfx::Point(500, 500));
recording_source->Rerecord();
scoped_refptr<RasterSource> raster_source =
recording_source->CreateRasterSource();

// All images should be registered on the pending layer.
SetupPendingTree(raster_source, gfx::Size(), Region(gfx::Rect(layer_bounds)));
auto* controller = host_impl()->image_animation_controller();
EXPECT_EQ(controller->GetDriversForTesting(image1.stable_id())
.count(pending_layer()),
1u);
EXPECT_EQ(controller->GetDriversForTesting(image2.stable_id())
.count(pending_layer()),
1u);

// Make only the first image visible and verify that only this image is
// animated.
gfx::Rect visible_rect(0, 0, 300, 300);
pending_layer()->set_visible_layer_rect(visible_rect);
EXPECT_TRUE(pending_layer()->ShouldAnimate(image1.stable_id()));
EXPECT_FALSE(pending_layer()->ShouldAnimate(image2.stable_id()));

// Now activate and make sure the active layer is registered as well.
ActivateTree();
active_layer()->set_visible_layer_rect(visible_rect);
EXPECT_EQ(controller->GetDriversForTesting(image1.stable_id())
.count(active_layer()),
1u);
EXPECT_EQ(controller->GetDriversForTesting(image2.stable_id())
.count(active_layer()),
1u);

// Once activated, only the active layer should drive animations for these
// images. Since DrawProperties are not updated on the recycle tree, it has
// stale state for visibility of images.
ASSERT_EQ(old_pending_layer()->visible_layer_rect(), visible_rect);
EXPECT_FALSE(old_pending_layer()->ShouldAnimate(image1.stable_id()));
EXPECT_FALSE(old_pending_layer()->ShouldAnimate(image2.stable_id()));
EXPECT_TRUE(active_layer()->ShouldAnimate(image1.stable_id()));
EXPECT_FALSE(active_layer()->ShouldAnimate(image2.stable_id()));
}

} // namespace
} // namespace cc
3 changes: 1 addition & 2 deletions cc/layers/recording_source_unittest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -111,8 +111,7 @@ TEST(RecordingSourceTest, DiscardableImagesWithTransform) {
std::vector<const DrawImage*> images;
raster_source->GetDiscardableImagesInRect(gfx::Rect(130, 0, 128, 128),
&images);
DrawImage image(*images[0], scale, PaintImage::kDefaultFrameIndex,
DefaultColorSpace());
DrawImage image(*images[0], scale, DefaultColorSpace());
EXPECT_EQ(1u, images.size());
EXPECT_FLOAT_EQ(scale, image.scale().width());
EXPECT_FLOAT_EQ(scale, image.scale().height());
Expand Down
2 changes: 1 addition & 1 deletion cc/paint/discardable_image_map.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ class PaintOpBuffer;
// rect and get back a list of DrawImages in that rect.
class CC_PAINT_EXPORT DiscardableImageMap {
public:
struct CC_PAINT_EXPORT AnimatedImageMetadata {
struct AnimatedImageMetadata {
AnimatedImageMetadata(PaintImage::Id paint_image_id,
PaintImage::CompletionState completion_state,
std::vector<FrameMetadata> frames,
Expand Down
24 changes: 3 additions & 21 deletions cc/paint/discardable_image_map_unittest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@
#include <memory>

#include "base/memory/ref_counted.h"
#include "base/test/gtest_util.h"
#include "base/values.h"
#include "cc/base/region.h"
#include "cc/paint/paint_flags.h"
Expand Down Expand Up @@ -61,8 +60,7 @@ class DiscardableImageMapTest : public testing::Test {
image_map.GetDiscardableImagesInRect(rect, &draw_image_ptrs);
std::vector<DrawImage> draw_images;
for (const auto* image : draw_image_ptrs)
draw_images.push_back(DrawImage(
*image, 1.f, PaintImage::kDefaultFrameIndex, target_color_space));
draw_images.push_back(DrawImage(*image, 1.f, target_color_space));

std::vector<PositionScaleDrawImage> position_draw_images;
for (DrawImage& image : image_map.images_rtree_.Search(rect)) {
Expand Down Expand Up @@ -700,16 +698,11 @@ TEST_F(DiscardableImageMapTest, GathersAnimatedImages) {
FakeContentLayerClient content_layer_client;
content_layer_client.set_bounds(visible_rect.size());

std::vector<FrameMetadata> frames = {
FrameMetadata(true, base::TimeDelta::FromMilliseconds(2)),
FrameMetadata(true, base::TimeDelta::FromMilliseconds(3))};

gfx::Size image_size(100, 100);
PaintImage static_image = CreateDiscardablePaintImage(image_size);
PaintImage animated_loop_none =
CreateAnimatedImage(image_size, frames, kAnimationNone);
PaintImage animation_loop_infinite =
CreateAnimatedImage(image_size, frames, 1u);
CreateAnimatedImage(image_size, {FrameMetadata()}, kAnimationNone);
PaintImage animation_loop_infinite = CreateAnimatedImage(image_size);

PaintFlags flags;
content_layer_client.add_draw_image(static_image, gfx::Point(0, 0), flags);
Expand All @@ -734,17 +727,6 @@ TEST_F(DiscardableImageMapTest, GathersAnimatedImages) {
animation_loop_infinite.GetFrameMetadata());
EXPECT_EQ(animated_images_metadata[0].repetition_count,
animation_loop_infinite.repetition_count());

std::vector<const DrawImage*> images;
display_list->discardable_image_map().GetDiscardableImagesInRect(visible_rect,
&images);
ASSERT_EQ(images.size(), 3u);
EXPECT_EQ(images[0]->paint_image(), static_image);
EXPECT_DCHECK_DEATH(images[0]->frame_index());
EXPECT_EQ(images[1]->paint_image(), animated_loop_none);
EXPECT_DCHECK_DEATH(images[1]->frame_index());
EXPECT_EQ(images[2]->paint_image(), animation_loop_infinite);
EXPECT_DCHECK_DEATH(images[2]->frame_index());
}

class DiscardableImageMapColorSpaceTest
Expand Down
4 changes: 0 additions & 4 deletions cc/paint/draw_image.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -32,27 +32,23 @@ DrawImage::DrawImage(PaintImage image,
const SkIRect& src_rect,
SkFilterQuality filter_quality,
const SkMatrix& matrix,
base::Optional<size_t> frame_index,
const base::Optional<gfx::ColorSpace>& color_space)
: paint_image_(std::move(image)),
src_rect_(src_rect),
filter_quality_(filter_quality),
frame_index_(frame_index),
target_color_space_(color_space) {
matrix_is_decomposable_ = ExtractScale(matrix, &scale_);
}

DrawImage::DrawImage(const DrawImage& other,
float scale_adjustment,
size_t frame_index,
const gfx::ColorSpace& color_space)
: paint_image_(other.paint_image_),
src_rect_(other.src_rect_),
filter_quality_(other.filter_quality_),
scale_(SkSize::Make(other.scale_.width() * scale_adjustment,
other.scale_.height() * scale_adjustment)),
matrix_is_decomposable_(other.matrix_is_decomposable_),
frame_index_(frame_index),
target_color_space_(color_space) {}

DrawImage::DrawImage(const DrawImage& other) = default;
Expand Down
Loading

0 comments on commit 1f14e22

Please sign in to comment.