cc: Keep PaintImage in DrawImage.

cc/paint/discardable_image_store.cc

 // Copyright 2017 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "cc/paint/discardable_image_store.h"
 
 #include "base/containers/adapters.h"
 #include "base/memory/ptr_util.h"
 #include "cc/paint/display_item_list.h"
 #include "third_party/skia/include/utils/SkNoDrawCanvas.h"
@@ skipping 54 matching lines @@
   }
 
  private:
   std::vector<SkPaint> saved_paints_;
 };
 
 DiscardableImageStore::DiscardableImageStore(
     int width,
     int height,
     std::vector<std::pair<DrawImage, gfx::Rect>>* image_set,
-    base::flat_map<ImageId, gfx::Rect>* image_id_to_rect)
+    base::flat_map<PaintImage::Id, gfx::Rect>* image_id_to_rect)
     : canvas_(base::MakeUnique<PaintTrackingCanvas>(width, height)),
       image_set_(image_set),
-      image_id_to_rect_(image_id_to_rect),
-      unknown_stable_id_(PaintImage::GetNextId()) {}
+      image_id_to_rect_(image_id_to_rect) {}
 
 DiscardableImageStore::~DiscardableImageStore() = default;
 
 SkNoDrawCanvas* DiscardableImageStore::GetNoDrawCanvas() {
   return canvas_.get();
 }
 
 void DiscardableImageStore::GatherDiscardableImages(
     const PaintOpBuffer* buffer) {
   if (!buffer->HasDiscardableImages())
@@ skipping 86 matching lines @@
 // embedded in SkPaints. Other embedded image cases, such as SkPictures,
 // are not yet handled.
 void DiscardableImageStore::AddImageFromFlags(const SkRect& rect,
                                               const PaintFlags& flags) {
   SkShader* shader = flags.getShader();
   if (shader) {
     SkMatrix matrix;
     SkShader::TileMode xy[2];
     SkImage* image = shader->isAImage(&matrix, xy);
     if (image) {
-      PaintImage paint_image(unknown_stable_id_, sk_ref_sp(image),
+      // We currently use the wrong id for images that come from shaders. We
+      // don't know what the stable id is, but since the completion and
+      // animation states are both unknown, this value doesn't matter as it
+      // won't be used in checker imaging anyway. Keep this value the same to
+      // avoid id churn.
+      // TODO(vmpstr): Remove this when we can add paint images into shaders
+      // directly.
+      PaintImage paint_image(PaintImage::kWrongStableId, sk_ref_sp(image),

[vmpstr, 2017/05/16 17:43:27]

Maybe call it kUnknownStableId? If it's wrong, then why are you using it? :)

[Khushal, 2017/05/16 21:29:12]

Done.
I thought calling it wrong will give me a push to eliminate these cases sooner.
:P

                              PaintImage::AnimationType::UNKNOWN,
                              PaintImage::CompletionState::UNKNOWN);
       // TODO(ericrk): Handle cases where we only need a sub-rect from the
       // image. crbug.com/671821
-      AddImage(paint_image, SkRect::MakeFromIRect(image->bounds()), rect,
-               &matrix, flags);
+      AddImage(std::move(paint_image), SkRect::MakeFromIRect(image->bounds()),
+               rect, &matrix, flags);
     }
   }
 }
 
-void DiscardableImageStore::AddImage(const PaintImage& paint_image,
+void DiscardableImageStore::AddImage(PaintImage paint_image,
                                      const SkRect& src_rect,
                                      const SkRect& rect,
                                      const SkMatrix* local_matrix,
                                      const PaintFlags& flags) {
-  sk_sp<const SkImage> sk_image = paint_image.sk_image();
-  if (!sk_image->isLazyGenerated())
+  if (!paint_image.sk_image()->isLazyGenerated())
     return;
 
   const SkRect& clip_rect = SkRect::Make(canvas_->getDeviceClipBounds());
   const SkMatrix& ctm = canvas_->getTotalMatrix();
 
   SkRect paint_rect = MapRect(ctm, rect);
   bool computed_paint_bounds =
       canvas_->ComputePaintBounds(paint_rect, ToSkPaint(&flags), &paint_rect);
   if (!computed_paint_bounds) {
     // TODO(vmpstr): UMA this case.
@@ skipping 23 matching lines @@
   image_rect.Inset(-1, -1);
 
   // The true target color space will be assigned when it is known, in
   // GetDiscardableImagesInRect.
   gfx::ColorSpace target_color_space;
 
   SkMatrix matrix = ctm;
   if (local_matrix)
     matrix.postConcat(*local_matrix);
 
-  // TODO(khushalsagar): Keep PaintImage in DrawImage.
-  (*image_id_to_rect_)[sk_image->uniqueID()].Union(image_rect);
+  (*image_id_to_rect_)[paint_image.stable_id()].Union(image_rect);

[vmpstr, 2017/05/16 17:43:27]

I don't think it is, but is it possible for us to have two different uniqueIDs
in the same raster source with the same stable id?

[Khushal, 2017/05/16 21:29:12]

It shouldn't be. The uniqueID essentially represents the current frame for a
blink::Image and all elements using that image are dirtied and repainted
together if the image changes.

   image_set_->push_back(
-      std::make_pair(DrawImage(std::move(sk_image), src_irect, filter_quality,
-                               matrix, target_color_space),
+      std::make_pair(DrawImage(std::move(paint_image), src_irect,
+                               filter_quality, matrix, target_color_space),
                      image_rect));
 }
 
 }  // namespace cc