cc: Address raster regression from PlaybackImageProvider construction.

Adding additional parameters to PlaybackImageProvider ctor causes a
regression in total raster time on some mac configs. In order to address
this, break out these settings to a separate struct which can optionally
be given to the provider. The params are modified on the Settings
object post construction.

R=enne@chromium.org

Bug: 768538, 768543
Cq-Include-Trybots: master.tryserver.blink:linux_trusty_blink_rel
Change-Id: I0e7fc0df225826e5a26f0f9d09c106a6b2d1f640
Reviewed-on: https://chromium-review.googlesource.com/691206Reviewed-by: enne <enne@chromium.org>
Reviewed-by: vmpstr <vmpstr@chromium.org>
Commit-Queue: Khushal <khushalsagar@chromium.org>
Cr-Commit-Position: refs/heads/master@{#505416}

cc/benchmarks/rasterize_and_record_benchmark_impl.cc

@@ -58,9 +58,16 @@ void RunBenchmark(RasterSource* raster_source,
                                                     content_rect.height()));
       SkCanvas canvas(bitmap);
 
-      PlaybackImageProvider image_provider(false, PaintImageIdFlatSet(), {},
-                                           image_decode_cache,
-                                           gfx::ColorSpace(), {});
+      // Pass an empty settings to make sure that the decode cache is used to
+      // replace all images.
+      base::Optional<PlaybackImageProvider::Settings> image_settings;
+      image_settings.emplace();
+      image_settings->images_to_skip = {};
+      image_settings->at_raster_images = {};
+      image_settings->image_to_current_frame_index = {};
+
+      PlaybackImageProvider image_provider(
+          image_decode_cache, gfx::ColorSpace(), std::move(image_settings));
       RasterSource::PlaybackSettings settings;
       settings.image_provider = &image_provider;
 

cc/raster/playback_image_provider.cc

@@ -18,18 +18,12 @@ void UnrefImageFromCache(DrawImage draw_image,
 }  // namespace
 
 PlaybackImageProvider::PlaybackImageProvider(
-    bool skip_all_images,
-    PaintImageIdFlatSet images_to_skip,
-    std::vector<DrawImage> at_raster_images,
     ImageDecodeCache* cache,
     const gfx::ColorSpace& target_color_space,
-    base::flat_map<PaintImage::Id, size_t> image_to_current_frame_index)
-    : skip_all_images_(skip_all_images),
-      images_to_skip_(std::move(images_to_skip)),
-      at_raster_images_(std::move(at_raster_images)),
-      cache_(cache),
+    base::Optional<Settings> settings)
+    : cache_(cache),
       target_color_space_(target_color_space),
-      image_to_current_frame_index_(std::move(image_to_current_frame_index)) {
+      settings_(std::move(settings)) {
   DCHECK(cache_);
 }
 
@@ -47,7 +41,11 @@ void PlaybackImageProvider::BeginRaster() {
   DCHECK(decoded_at_raster_.empty());
   DCHECK(!in_raster_);
   in_raster_ = true;
-  for (auto& draw_image : at_raster_images_)
+
+  if (!settings_.has_value())
+    return;
+
+  for (auto& draw_image : settings_->at_raster_images)
     decoded_at_raster_.push_back(GetDecodedDrawImage(draw_image));
 }
 
@@ -61,14 +59,14 @@ ImageProvider::ScopedDecodedDrawImage
 PlaybackImageProvider::GetDecodedDrawImage(const DrawImage& draw_image) {
   DCHECK(in_raster_);
 
-  // Return an empty decoded images if we are skipping all images during this
+  // Return an empty decoded image if we are skipping all images during this
   // raster.
-  if (skip_all_images_)
+  if (!settings_.has_value())
     return ScopedDecodedDrawImage();
 
   const PaintImage& paint_image = draw_image.paint_image();
 
-  if (images_to_skip_.count(paint_image.stable_id()) != 0) {
+  if (settings_->images_to_skip.count(paint_image.stable_id()) != 0) {
     DCHECK(paint_image.GetSkImage()->isLazyGenerated());
     return ScopedDecodedDrawImage();
   }
@@ -79,8 +77,9 @@ PlaybackImageProvider::GetDecodedDrawImage(const DrawImage& draw_image) {
                          SkSize::Make(1.f, 1.f), draw_image.filter_quality()));
   }
 
-  const auto& it = image_to_current_frame_index_.find(paint_image.stable_id());
-  size_t frame_index = it == image_to_current_frame_index_.end()
+  const auto& it =
+      settings_->image_to_current_frame_index.find(paint_image.stable_id());
+  size_t frame_index = it == settings_->image_to_current_frame_index.end()
                            ? paint_image.frame_index()
                            : it->second;
 
@@ -92,4 +91,8 @@ PlaybackImageProvider::GetDecodedDrawImage(const DrawImage& draw_image) {
       base::BindOnce(&UnrefImageFromCache, std::move(adjusted_image), cache_));
 }
 
+PlaybackImageProvider::Settings::Settings() = default;
+PlaybackImageProvider::Settings::Settings(const Settings& other) = default;
+PlaybackImageProvider::Settings::~Settings() = default;
+
 }  // namespace cc

cc/raster/playback_image_provider.h

@@ -15,20 +15,31 @@ namespace cc {
 class ImageDecodeCache;
 
 // PlaybackImageProvider is used to replace lazy generated PaintImages with
-// decoded images for raster from the ImageDecodeCache. The following settings
-// can be used to modify rasterization of these images:
-// 1) skip_all_images: Ensures that no images are decoded or rasterized.
-// 2) images_to_skip: Used to selectively skip images during raster. This should
-//    only be used for lazy generated images.
+// decoded images for raster from the ImageDecodeCache.
 class CC_EXPORT PlaybackImageProvider : public ImageProvider {
  public:
-  PlaybackImageProvider(
-      bool skip_all_images,
-      PaintImageIdFlatSet images_to_skip,
-      std::vector<DrawImage> at_raster_images,
-      ImageDecodeCache* cache,
-      const gfx::ColorSpace& taget_color_space,
-      base::flat_map<PaintImage::Id, size_t> image_to_current_frame_index);
+  struct CC_EXPORT Settings {
+    Settings();
+    Settings(const Settings& other);
+    ~Settings();
+
+    // The set of image ids to skip during raster.
+    PaintImageIdFlatSet images_to_skip;
+
+    // The set of images which must be decoded by the provider before beginning
+    // raster. The images are decoded and locked by the provider in BeginRaster
+    // and unlocked in EndRaster.
+    std::vector<DrawImage> at_raster_images;
+
+    // The frame index to use for the given image id. If no index is provided,
+    // the frame index provided in the PaintImage will be used.
+    base::flat_map<PaintImage::Id, size_t> image_to_current_frame_index;
+  };
+
+  // If no settings are provided, all images are skipped during rasterization.
+  PlaybackImageProvider(ImageDecodeCache* cache,
+                        const gfx::ColorSpace& target_color_space,
+                        base::Optional<Settings> settings);
   ~PlaybackImageProvider() override;
 
   void BeginRaster() override;
@@ -42,14 +53,12 @@ class CC_EXPORT PlaybackImageProvider : public ImageProvider {
       const DrawImage& draw_image) override;
 
  private:
-  bool skip_all_images_;
-  bool in_raster_ = false;
-  PaintImageIdFlatSet images_to_skip_;
-  std::vector<DrawImage> at_raster_images_;
-  std::vector<ImageProvider::ScopedDecodedDrawImage> decoded_at_raster_;
   ImageDecodeCache* cache_;
   gfx::ColorSpace target_color_space_;
-  base::flat_map<PaintImage::Id, size_t> image_to_current_frame_index_;
+  base::Optional<Settings> settings_;
+
+  bool in_raster_ = false;
+  std::vector<ImageProvider::ScopedDecodedDrawImage> decoded_at_raster_;
 
   DISALLOW_COPY_AND_ASSIGN(PlaybackImageProvider);
 };

cc/raster/playback_image_provider_unittest.cc

@@ -55,7 +55,7 @@ class MockDecodeCache : public StubDecodeCache {
 
 TEST(PlaybackImageProviderTest, SkipsAllImages) {
   MockDecodeCache cache;
-  PlaybackImageProvider provider(true, {}, {}, &cache, gfx::ColorSpace(), {});
+  PlaybackImageProvider provider(&cache, gfx::ColorSpace(), base::nullopt);
   provider.BeginRaster();
 
   SkIRect rect = SkIRect::MakeWH(10, 10);
@@ -79,8 +79,12 @@ TEST(PlaybackImageProviderTest, SkipsAllImages) {
 TEST(PlaybackImageProviderTest, SkipsSomeImages) {
   MockDecodeCache cache;
   PaintImage skip_image = CreateDiscardablePaintImage(gfx::Size(10, 10));
-  PlaybackImageProvider provider(false, {skip_image.stable_id()}, {}, &cache,
-                                 gfx::ColorSpace(), {});
+
+  base::Optional<PlaybackImageProvider::Settings> settings;
+  settings.emplace();
+  settings->images_to_skip = {skip_image.stable_id()};
+
+  PlaybackImageProvider provider(&cache, gfx::ColorSpace(), settings);
   provider.BeginRaster();
 
   SkIRect rect = SkIRect::MakeWH(10, 10);
@@ -93,7 +97,10 @@ TEST(PlaybackImageProviderTest, SkipsSomeImages) {
 
 TEST(PlaybackImageProviderTest, RefAndUnrefDecode) {
   MockDecodeCache cache;
-  PlaybackImageProvider provider(false, {}, {}, &cache, gfx::ColorSpace(), {});
+
+  base::Optional<PlaybackImageProvider::Settings> settings;
+  settings.emplace();
+  PlaybackImageProvider provider(&cache, gfx::ColorSpace(), settings);
   provider.BeginRaster();
 
   {
@@ -122,8 +129,11 @@ TEST(PlaybackImageProviderTest, AtRasterImages) {
   auto draw_image2 = CreateDiscardableDrawImage(
       size, nullptr, rect, kMedium_SkFilterQuality, matrix);
 
-  PlaybackImageProvider provider(false, {}, {draw_image1, draw_image2}, &cache,
-                                 gfx::ColorSpace(), {});
+  base::Optional<PlaybackImageProvider::Settings> settings;
+  settings.emplace();
+  settings->at_raster_images = {draw_image1, draw_image2};
+
+  PlaybackImageProvider provider(&cache, gfx::ColorSpace(), settings);
 
   EXPECT_EQ(cache.refed_image_count(), 0);
   provider.BeginRaster();
@@ -144,8 +154,11 @@ TEST(PlaybackImageProviderTest, SwapsGivenFrames) {
 
   base::flat_map<PaintImage::Id, size_t> image_to_frame;
   image_to_frame[image.stable_id()] = 1u;
-  PlaybackImageProvider provider(false, {}, {}, &cache, gfx::ColorSpace(),
-                                 image_to_frame);
+  base::Optional<PlaybackImageProvider::Settings> settings;
+  settings.emplace();
+  settings->image_to_current_frame_index = image_to_frame;
+
+  PlaybackImageProvider provider(&cache, gfx::ColorSpace(), settings);
   provider.BeginRaster();
 
   SkIRect rect = SkIRect::MakeWH(10, 10);

cc/tiles/tile_manager.cc

@@ -1186,10 +1186,17 @@ scoped_refptr<TileTask> TileManager::CreateRasterTask(
   const bool has_at_raster_images = !at_raster_images.empty();
   if (skip_images || has_checker_images || has_sync_decoded_images ||
       has_at_raster_images) {
-    image_provider.emplace(skip_images, std::move(images_to_skip),
-                           std::move(at_raster_images),
-                           image_controller_.cache(), color_space,
-                           std::move(image_id_to_current_frame_index));
+    base::Optional<PlaybackImageProvider::Settings> settings;
+    if (!skip_images) {
+      settings.emplace();
+      settings->images_to_skip = std::move(images_to_skip);
+      settings->at_raster_images = std::move(at_raster_images);
+      settings->image_to_current_frame_index =
+          std::move(image_id_to_current_frame_index);
+    }
+
+    image_provider.emplace(image_controller_.cache(), color_space,
+                           std::move(settings));
   }
 
   return base::MakeRefCounted<RasterTaskImpl>(