Skip to content

Commit

Permalink
Revert of cc: Build Image to Region map during image meta-data genera…
Browse files Browse the repository at this point in the history 
…tion. (patchset #6 id:100001 of https://codereview.chromium.org/2496283004/ )

Reason for revert:
Computing these image regions adds significant time to the critical path on image heavy content.  Let's re-land after addressing the performance.

BUG=670926

Original issue's description:
> cc: Build Image to Region map during image meta-data generation.
>
> During the discardable image meta-data generation phase, we build an
> Rtree to pull the set of images given a rect. This change adds a
> reverse lookup map to be built at the same time for querying the region
> for an image in a DisplayItemList.
>
> BUG=665266
> CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
>
> Committed: https://crrev.com/fc85c1aa0881047c4410cbedc1e4543bcf789a1f
> Cr-Commit-Position: refs/heads/master@{#432841}

TBR=vmpstr@chromium.org,khushalsagar@chromium.org
BUG=665266

Review-Url: https://codereview.chromium.org/2551583003
Cr-Commit-Position: refs/heads/master@{#436174}
(cherry picked from commit 654417a)

Review URL: https://codereview.chromium.org/2552903002 .

Cr-Commit-Position: refs/branch-heads/2924@{#347}
Cr-Branched-From: 3a87aec-refs/heads/master@{#433059}
  • Loading branch information
Victor Miura committed Dec 6, 2016
1 parent c42fb96 commit c006456
Show file tree
Hide file tree
Showing 5 changed files with 3 additions and 82 deletions.
19 changes: 3 additions & 16 deletions cc/playback/discardable_image_map.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -60,11 +60,9 @@ class DiscardableImagesMetadataCanvas : public SkNWayCanvas {
DiscardableImagesMetadataCanvas(
int width,
int height,
std::vector<std::pair<DrawImage, gfx::Rect>>* image_set,
DiscardableImageMap::ImageToRegionMap* image_to_region)
std::vector<std::pair<DrawImage, gfx::Rect>>* image_set)
: SkNWayCanvas(width, height),
image_set_(image_set),
image_id_to_region_(image_to_region),
canvas_bounds_(SkRect::MakeIWH(width, height)),
canvas_size_(width, height) {}

Expand Down Expand Up @@ -174,18 +172,12 @@ class DiscardableImagesMetadataCanvas : public SkNWayCanvas {

SkIRect src_irect;
src_rect.roundOut(&src_irect);
gfx::Rect image_rect = SafeClampPaintRectToSize(paint_rect, canvas_size_);

Region& region = (*image_id_to_region_)[image->uniqueID()];
region.Union(image_rect);

image_set_->push_back(std::make_pair(
DrawImage(std::move(image), src_irect, filter_quality, matrix),
image_rect));
SafeClampPaintRectToSize(paint_rect, canvas_size_)));
}

std::vector<std::pair<DrawImage, gfx::Rect>>* image_set_;
DiscardableImageMap::ImageToRegionMap* image_id_to_region_;
const SkRect canvas_bounds_;
const gfx::Size canvas_size_;
std::vector<SkPaint> saved_paints_;
Expand All @@ -201,7 +193,7 @@ sk_sp<SkCanvas> DiscardableImageMap::BeginGeneratingMetadata(
const gfx::Size& bounds) {
DCHECK(all_images_.empty());
return sk_make_sp<DiscardableImagesMetadataCanvas>(
bounds.width(), bounds.height(), &all_images_, &image_id_to_region_);
bounds.width(), bounds.height(), &all_images_);
}

void DiscardableImageMap::EndGeneratingMetadata() {
Expand All @@ -221,11 +213,6 @@ void DiscardableImageMap::GetDiscardableImagesInRect(
images->push_back(all_images_[index].first.ApplyScale(raster_scales));
}

Region DiscardableImageMap::GetRegionForImage(uint32_t image_id) const {
const auto& it = image_id_to_region_.find(image_id);
return it == image_id_to_region_.end() ? Region() : it->second;
}

DiscardableImageMap::ScopedMetadataGenerator::ScopedMetadataGenerator(
DiscardableImageMap* image_map,
const gfx::Size& bounds)
Expand Down
8 changes: 0 additions & 8 deletions cc/playback/discardable_image_map.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,12 +5,10 @@
#ifndef CC_PLAYBACK_DISCARDABLE_IMAGE_MAP_H_
#define CC_PLAYBACK_DISCARDABLE_IMAGE_MAP_H_

#include <unordered_map>
#include <utility>
#include <vector>

#include "cc/base/cc_export.h"
#include "cc/base/region.h"
#include "cc/base/rtree.h"
#include "cc/playback/draw_image.h"
#include "third_party/skia/include/core/SkCanvas.h"
Expand All @@ -28,9 +26,6 @@ SkRect MapRect(const SkMatrix& matrix, const SkRect& src);
// rect and get back a list of DrawImages in that rect.
class CC_EXPORT DiscardableImageMap {
public:
// A map of SkImage id to the region for this image.
using ImageToRegionMap = std::unordered_map<uint32_t, Region>;

class CC_EXPORT ScopedMetadataGenerator {
public:
ScopedMetadataGenerator(DiscardableImageMap* image_map,
Expand All @@ -51,7 +46,6 @@ class CC_EXPORT DiscardableImageMap {
void GetDiscardableImagesInRect(const gfx::Rect& rect,
const gfx::SizeF& raster_scales,
std::vector<DrawImage>* images) const;
Region GetRegionForImage(uint32_t image_id) const;

private:
friend class ScopedMetadataGenerator;
Expand All @@ -61,8 +55,6 @@ class CC_EXPORT DiscardableImageMap {
void EndGeneratingMetadata();

std::vector<std::pair<DrawImage, gfx::Rect>> all_images_;
ImageToRegionMap image_id_to_region_;

RTree images_rtree_;
};

Expand Down
53 changes: 0 additions & 53 deletions cc/playback/discardable_image_map_unittest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -108,8 +108,6 @@ TEST_F(DiscardableImageMapTest, GetDiscardableImagesInRectTest) {
<< y;
EXPECT_EQ(gfx::Rect(x * 512 + 6, y * 512 + 6, 500, 500),
images[0].image_rect);
EXPECT_EQ(Region(images[0].image_rect),
image_map.GetRegionForImage(images[0].image->uniqueID()));
} else {
EXPECT_EQ(0u, images.size()) << x << " " << y;
}
Expand All @@ -120,28 +118,16 @@ TEST_F(DiscardableImageMapTest, GetDiscardableImagesInRectTest) {
std::vector<PositionDrawImage> images =
GetDiscardableImagesInRect(image_map, gfx::Rect(512, 512, 2048, 2048));
EXPECT_EQ(4u, images.size());

EXPECT_TRUE(images[0].image == discardable_image[1][2]);
EXPECT_EQ(gfx::Rect(2 * 512 + 6, 512 + 6, 500, 500), images[0].image_rect);
EXPECT_EQ(Region(images[0].image_rect),
image_map.GetRegionForImage(images[0].image->uniqueID()));

EXPECT_TRUE(images[1].image == discardable_image[2][1]);
EXPECT_EQ(gfx::Rect(512 + 6, 2 * 512 + 6, 500, 500), images[1].image_rect);
EXPECT_EQ(Region(images[1].image_rect),
image_map.GetRegionForImage(images[1].image->uniqueID()));

EXPECT_TRUE(images[2].image == discardable_image[2][3]);
EXPECT_EQ(gfx::Rect(3 * 512 + 6, 2 * 512 + 6, 500, 500),
images[2].image_rect);
EXPECT_EQ(Region(images[2].image_rect),
image_map.GetRegionForImage(images[2].image->uniqueID()));

EXPECT_TRUE(images[3].image == discardable_image[3][2]);
EXPECT_EQ(gfx::Rect(2 * 512 + 6, 3 * 512 + 6, 500, 500),
images[3].image_rect);
EXPECT_EQ(Region(images[3].image_rect),
image_map.GetRegionForImage(images[3].image->uniqueID()));
}

TEST_F(DiscardableImageMapTest, GetDiscardableImagesInRectNonZeroLayer) {
Expand Down Expand Up @@ -195,8 +181,6 @@ TEST_F(DiscardableImageMapTest, GetDiscardableImagesInRectNonZeroLayer) {
<< y;
EXPECT_EQ(gfx::Rect(1024 + x * 512 + 6, y * 512 + 6, 500, 500),
images[0].image_rect);
EXPECT_EQ(Region(images[0].image_rect),
image_map.GetRegionForImage(images[0].image->uniqueID()));
} else {
EXPECT_EQ(0u, images.size()) << x << " " << y;
}
Expand All @@ -207,30 +191,18 @@ TEST_F(DiscardableImageMapTest, GetDiscardableImagesInRectNonZeroLayer) {
std::vector<PositionDrawImage> images = GetDiscardableImagesInRect(
image_map, gfx::Rect(1024 + 512, 512, 2048, 2048));
EXPECT_EQ(4u, images.size());

EXPECT_TRUE(images[0].image == discardable_image[1][2]);
EXPECT_EQ(gfx::Rect(1024 + 2 * 512 + 6, 512 + 6, 500, 500),
images[0].image_rect);
EXPECT_EQ(Region(images[0].image_rect),
image_map.GetRegionForImage(images[0].image->uniqueID()));

EXPECT_TRUE(images[1].image == discardable_image[2][1]);
EXPECT_EQ(gfx::Rect(1024 + 512 + 6, 2 * 512 + 6, 500, 500),
images[1].image_rect);
EXPECT_EQ(Region(images[1].image_rect),
image_map.GetRegionForImage(images[1].image->uniqueID()));

EXPECT_TRUE(images[2].image == discardable_image[2][3]);
EXPECT_EQ(gfx::Rect(1024 + 3 * 512 + 6, 2 * 512 + 6, 500, 500),
images[2].image_rect);
EXPECT_EQ(Region(images[2].image_rect),
image_map.GetRegionForImage(images[2].image->uniqueID()));

EXPECT_TRUE(images[3].image == discardable_image[3][2]);
EXPECT_EQ(gfx::Rect(1024 + 2 * 512 + 6, 3 * 512 + 6, 500, 500),
images[3].image_rect);
EXPECT_EQ(Region(images[3].image_rect),
image_map.GetRegionForImage(images[3].image->uniqueID()));
}

// Non intersecting rects
Expand All @@ -254,12 +226,6 @@ TEST_F(DiscardableImageMapTest, GetDiscardableImagesInRectNonZeroLayer) {
image_map, gfx::Rect(3500, 1100, 1000, 1000));
EXPECT_EQ(0u, images.size());
}

// Image not present in the list.
{
sk_sp<SkImage> image = CreateDiscardableImage(gfx::Size(500, 500));
EXPECT_EQ(Region(), image_map.GetRegionForImage(image->uniqueID()));
}
}

TEST_F(DiscardableImageMapTest, GetDiscardableImagesInRectOnePixelQuery) {
Expand Down Expand Up @@ -311,8 +277,6 @@ TEST_F(DiscardableImageMapTest, GetDiscardableImagesInRectOnePixelQuery) {
<< y;
EXPECT_EQ(gfx::Rect(x * 512 + 6, y * 512 + 6, 500, 500),
images[0].image_rect);
EXPECT_EQ(Region(images[0].image_rect),
image_map.GetRegionForImage(images[0].image->uniqueID()));
} else {
EXPECT_EQ(0u, images.size()) << x << " " << y;
}
Expand Down Expand Up @@ -346,8 +310,6 @@ TEST_F(DiscardableImageMapTest, GetDiscardableImagesInRectMassiveImage) {
EXPECT_EQ(1u, images.size());
EXPECT_TRUE(images[0].image == discardable_image);
EXPECT_EQ(gfx::Rect(0, 0, 2048, 2048), images[0].image_rect);
EXPECT_EQ(Region(images[0].image_rect),
image_map.GetRegionForImage(images[0].image->uniqueID()));
}

TEST_F(DiscardableImageMapTest, PaintDestroyedWhileImageIsDrawn) {
Expand Down Expand Up @@ -403,8 +365,6 @@ TEST_F(DiscardableImageMapTest, GetDiscardableImagesInRectMaxImage) {
EXPECT_EQ(1u, images.size());
EXPECT_TRUE(images[0].image == discardable_image);
EXPECT_EQ(gfx::Rect(42, 42, 2006, 2006), images[0].image_rect);
EXPECT_EQ(Region(images[0].image_rect),
image_map.GetRegionForImage(images[0].image->uniqueID()));
}

TEST_F(DiscardableImageMapTest, GetDiscardableImagesInRectMaxImageMaxLayer) {
Expand Down Expand Up @@ -439,10 +399,6 @@ TEST_F(DiscardableImageMapTest, GetDiscardableImagesInRectMaxImageMaxLayer) {
visible_rect.size());
display_list->Raster(generator.canvas(), nullptr, visible_rect, 1.f);
}

EXPECT_EQ(gfx::Rect(0, 0, dimension, dimension),
image_map.GetRegionForImage(discardable_image->uniqueID()));

std::vector<PositionDrawImage> images =
GetDiscardableImagesInRect(image_map, gfx::Rect(0, 0, 1, 1));
EXPECT_EQ(1u, images.size());
Expand Down Expand Up @@ -506,15 +462,6 @@ TEST_F(DiscardableImageMapTest, GetDiscardableImagesRectInBounds) {
images = GetDiscardableImagesInRect(image_map, gfx::Rect(0, 500, 1, 1));
EXPECT_EQ(1u, images.size());
EXPECT_EQ(gfx::Rect(0, 500, 1000, 100), images[0].image_rect);

Region discardable_image_region;
discardable_image_region.Union(gfx::Rect(0, 0, 90, 89));
discardable_image_region.Union(gfx::Rect(950, 951, 50, 49));
EXPECT_EQ(discardable_image_region,
image_map.GetRegionForImage(discardable_image->uniqueID()));

EXPECT_EQ(Region(gfx::Rect(0, 500, 1000, 100)),
image_map.GetRegionForImage(long_discardable_image->uniqueID()));
}

} // namespace cc
4 changes: 0 additions & 4 deletions cc/playback/display_item_list.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -271,8 +271,4 @@ void DisplayItemList::GetDiscardableImagesInRect(
image_map_.GetDiscardableImagesInRect(rect, raster_scales, images);
}

Region DisplayItemList::GetRegionForImage(uint32_t image_id) {
return image_map_.GetRegionForImage(image_id);
}

} // namespace cc
1 change: 0 additions & 1 deletion cc/playback/display_item_list.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -155,7 +155,6 @@ class CC_EXPORT DisplayItemList
void GetDiscardableImagesInRect(const gfx::Rect& rect,
const gfx::SizeF& raster_scales,
std::vector<DrawImage>* images);
Region GetRegionForImage(uint32_t image_id);

void SetRetainVisualRectsForTesting(bool retain) {
retain_visual_rects_ = retain;
Expand Down

0 comments on commit c006456

Please sign in to comment.