cc: Gather and lock/unlock SkDiscardablePixelRefs instead of skia::LazyPixelRefs.

cc: Gather and lock/unlock SkDiscardablePixelRefs instead of skia::LazyPixelRefs.

This removes the use of skia::LazyPixelRef from the compositor,
which allows us to gather and lock/unlock SkDiscardablePixelRefs
in the instead.

skia::LazyPixelRef will be removed completely when no longer in use
by blink.

BUG=329094
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=243660

[reveman, 2013-12-20 16:23:33]

This makes the compositor gather discardable pixel refs instead of lazy pixel
refs. Follow up patches will refactor the raster task system to lock and unlock
pixel refs using separate tasks.

Please take a look.

[vmpstr, 2013-12-20 17:41:06]

Do you want to add some sort of a comment to lazy_pixel_ref.h indicating that
it's deprecated and shouldn't be used? Or do you think the follow-up to remove
it will be soon after this?

https://codereview.chromium.org/111143005/diff/1/cc/resources/raster_worker_p...
File cc/resources/raster_worker_pool.cc (left):

https://codereview.chromium.org/111143005/diff/1/cc/resources/raster_worker_p...
cc/resources/raster_worker_pool.cc:13: #include "skia/ext/lazy_pixel_ref.h"
include third_party/skia/include/core/SkPixelRef.h instead please (it's not
clear what include is bringing that in right now)

https://codereview.chromium.org/111143005/diff/1/cc/resources/raster_worker_p...
File cc/resources/raster_worker_pool.cc (right):

https://codereview.chromium.org/111143005/diff/1/cc/resources/raster_worker_p...
cc/resources/raster_worker_pool.cc:270: pixel_ref_->lockPixels();
This will still keep it in some sort of a cache, right? That is, unlockPixels
doesn't _force_ the pixel ref to go away from the cache right?

https://codereview.chromium.org/111143005/diff/1/cc/resources/raster_worker_p...
File cc/resources/raster_worker_pool.h (right):

https://codereview.chromium.org/111143005/diff/1/cc/resources/raster_worker_p...
cc/resources/raster_worker_pool.h:203: SkPixelRef* pixel_ref,
Forward declare this? I can't tell which include is bringing this in.

https://codereview.chromium.org/111143005/diff/1/cc/resources/tile_manager.cc
File cc/resources/tile_manager.cc (right):

https://codereview.chromium.org/111143005/diff/1/cc/resources/tile_manager.cc...
cc/resources/tile_manager.cc:932: image_decode_tasks_.find(layer_id);
I assume we'll also be removing this decode-once thing when we lock/unlock in
separate tasks?

https://codereview.chromium.org/111143005/diff/1/skia/ext/discardable_pixel_r...
File skia/ext/discardable_pixel_ref_utils.h (right):

https://codereview.chromium.org/111143005/diff/1/skia/ext/discardable_pixel_r...
skia/ext/discardable_pixel_ref_utils.h:15: class SK_API DiscardablePixelRefUtils
{
What do you think of naming this PixelRefUtils and having GatherPixelRefs be
renamed to GatherDiscardablePixelRefs?

I think that would better reflect what we're doing? And in the future, if we
rename discardable to whatever else, the class name can stay the same :)

The only draw back is that "PixelRefUtils" might be a bit too generic, and
possibly already used in skia or somewhere else.

[reveman, 2013-12-20 18:22:19]

https://codereview.chromium.org/111143005/diff/1/cc/resources/raster_worker_p...
File cc/resources/raster_worker_pool.cc (left):

https://codereview.chromium.org/111143005/diff/1/cc/resources/raster_worker_p...
cc/resources/raster_worker_pool.cc:13: #include "skia/ext/lazy_pixel_ref.h"
On 2013/12/20 17:41:06, vmpstr wrote:
> include third_party/skia/include/core/SkPixelRef.h instead please (it's not
> clear what include is bringing that in right now)

Done. And required now that I removed the include from picture.h.

https://codereview.chromium.org/111143005/diff/1/cc/resources/raster_worker_p...
File cc/resources/raster_worker_pool.cc (right):

https://codereview.chromium.org/111143005/diff/1/cc/resources/raster_worker_p...
cc/resources/raster_worker_pool.cc:270: pixel_ref_->lockPixels();
On 2013/12/20 17:41:06, vmpstr wrote:
> This will still keep it in some sort of a cache, right? That is, unlockPixels
> doesn't _force_ the pixel ref to go away from the cache right?

This is exactly the same thing that LazyPixelRef::Decode() does. unlockPixels
doesn't force it out of the cache. It just allows it to be purged if necessary.

https://codereview.chromium.org/111143005/diff/1/cc/resources/raster_worker_p...
File cc/resources/raster_worker_pool.h (right):

https://codereview.chromium.org/111143005/diff/1/cc/resources/raster_worker_p...
cc/resources/raster_worker_pool.h:203: SkPixelRef* pixel_ref,
On 2013/12/20 17:41:06, vmpstr wrote:
> Forward declare this? I can't tell which include is bringing this in.

Done. picture.h pulled in SkPixelRef.h when it shouldn't.

https://codereview.chromium.org/111143005/diff/1/cc/resources/tile_manager.cc
File cc/resources/tile_manager.cc (right):

https://codereview.chromium.org/111143005/diff/1/cc/resources/tile_manager.cc...
cc/resources/tile_manager.cc:932: image_decode_tasks_.find(layer_id);
On 2013/12/20 17:41:06, vmpstr wrote:
> I assume we'll also be removing this decode-once thing when we lock/unlock in
> separate tasks?

Yes, correct.

https://codereview.chromium.org/111143005/diff/1/skia/ext/discardable_pixel_r...
File skia/ext/discardable_pixel_ref_utils.h (right):

https://codereview.chromium.org/111143005/diff/1/skia/ext/discardable_pixel_r...
skia/ext/discardable_pixel_ref_utils.h:15: class SK_API DiscardablePixelRefUtils
{
On 2013/12/20 17:41:06, vmpstr wrote:
> What do you think of naming this PixelRefUtils and having GatherPixelRefs be
> renamed to GatherDiscardablePixelRefs?
> 
> I think that would better reflect what we're doing? And in the future, if we
> rename discardable to whatever else, the class name can stay the same :)
> 
> The only draw back is that "PixelRefUtils" might be a bit too generic, and
> possibly already used in skia or somewhere else.

Good idea. Done.

[enne (OOO), 2013-12-20 18:22:24]

lgtm % vmpstr's approval.  I don't have anything else to add; thanks for taking
care of this.

[vmpstr, 2013-12-20 18:31:06]

lgtm2. This is pretty great (and I can't wait for the follow-up :P)

[reveman, 2013-12-20 19:12:33]

+tomhudson for skia/ review

[reveman, 2014-01-07 21:54:57]

+qinmin who wrote the LazyPixelRef code

[qinmin, 2014-01-07 22:37:44]

lgtm

[tomhudson, 2014-01-08 11:01:33]

Don't know how I missed this last time around; skia renamings LGTM.

[commit-bot: I haz the power, 2014-01-08 16:31:41]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/reveman@chromium.org/111143005/90001

[commit-bot: I haz the power, 2014-01-08 16:31:53]

Failed to apply patch for skia/ext/pixel_ref_utils_unittest.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  A         skia/ext/pixel_ref_utils_unittest.cc
  Copied skia/ext/lazy_pixel_ref_utils_unittest.cc ->
skia/ext/pixel_ref_utils_unittest.cc
  patching file skia/ext/pixel_ref_utils_unittest.cc
  Hunk #2 FAILED at 39.
  Hunk #3 succeeded at 55 (offset -15 lines).
  Hunk #4 succeeded at 68 (offset -15 lines).
  Hunk #5 FAILED at 106.
  Hunk #6 succeeded at 135 (offset -27 lines).
  Hunk #7 succeeded at 162 (offset -27 lines).
  Hunk #8 succeeded at 174 (offset -27 lines).
  Hunk #9 succeeded at 215 (offset -27 lines).
  Hunk #10 succeeded at 227 (offset -27 lines).
  Hunk #11 succeeded at 261 (offset -27 lines).
  Hunk #12 succeeded at 273 (offset -27 lines).
  Hunk #13 succeeded at 312 (offset -27 lines).
  Hunk #14 succeeded at 324 (offset -27 lines).
  Hunk #15 succeeded at 362 (offset -27 lines).
  Hunk #16 succeeded at 374 (offset -27 lines).
  Hunk #17 succeeded at 406 (offset -27 lines).
  Hunk #18 succeeded at 416 (offset -27 lines).
  Hunk #19 succeeded at 460 (offset -27 lines).
  Hunk #20 succeeded at 477 (offset -27 lines).
  Hunk #21 succeeded at 514 (offset -27 lines).
  Hunk #22 succeeded at 528 (offset -27 lines).
  Hunk #23 succeeded at 567 (offset -27 lines).
  Hunk #24 succeeded at 577 (offset -27 lines).
  Hunk #25 succeeded at 595 (offset -27 lines).
  Hunk #26 succeeded at 622 (offset -27 lines).
  Hunk #27 succeeded at 695 (offset -27 lines).
  2 out of 27 hunks FAILED -- saving rejects to file
skia/ext/pixel_ref_utils_unittest.cc.rej

Patch:   NR 
skia/ext/lazy_pixel_ref_utils_unittest.cc->skia/ext/pixel_ref_utils_unittest.cc
Index: skia/ext/pixel_ref_utils_unittest.cc
diff --git a/skia/ext/lazy_pixel_ref_utils_unittest.cc
b/skia/ext/pixel_ref_utils_unittest.cc
similarity index 76%
rename from skia/ext/lazy_pixel_ref_utils_unittest.cc
rename to skia/ext/pixel_ref_utils_unittest.cc
index
054613341fd13b5e09faa65d2f0a1d478abd064f..ea40b1ebbb3ab4ea5169d8bb761c5e1931336013
100644
--- a/skia/ext/lazy_pixel_ref_utils_unittest.cc
+++ b/skia/ext/pixel_ref_utils_unittest.cc
@@ -5,13 +5,13 @@
 #include "base/compiler_specific.h"
 #include "base/memory/scoped_ptr.h"
 #include "cc/test/geometry_test_utils.h"
-#include "skia/ext/lazy_pixel_ref.h"
-#include "skia/ext/lazy_pixel_ref_utils.h"
+#include "skia/ext/pixel_ref_utils.h"
 #include "skia/ext/refptr.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #include "third_party/skia/include/core/SkBitmap.h"
 #include "third_party/skia/include/core/SkCanvas.h"
 #include "third_party/skia/include/core/SkFlattenableBuffers.h"
+#include "third_party/skia/include/core/SkPixelRef.h"
 #include "third_party/skia/include/core/SkPoint.h"
 #include "third_party/skia/include/core/SkShader.h"
 #include "third_party/skia/src/core/SkOrderedReadBuffer.h"
@@ -39,29 +39,13 @@ class TestPixelRef : public SkPixelRef {
   scoped_ptr<char[]> pixels_;
 };
 
-class TestLazyPixelRef : public skia::LazyPixelRef {
+class TestDiscardableShader : public SkShader {
  public:
-  TestLazyPixelRef(int width, int height);
-  virtual ~TestLazyPixelRef();
-
-  virtual SkFlattenable::Factory getFactory() const OVERRIDE;
-  virtual void* onLockPixels(SkColorTable** color_table) OVERRIDE;
-  virtual void onUnlockPixels() OVERRIDE {}
-  virtual bool PrepareToDecode(const PrepareParams& params) OVERRIDE;
-  virtual bool MaybeDecoded() OVERRIDE;
-  virtual SkPixelRef* deepCopy(SkBitmap::Config config, const SkIRect* subset)
-      OVERRIDE;
-  virtual void Decode() OVERRIDE {}
-
- private:
-  scoped_ptr<char[]> pixels_;
-};
-
-class TestLazyShader : public SkShader {
- public:
-  TestLazyShader() { CreateBitmap(gfx::Size(50, 50), "lazy", &bitmap_); }
+  TestDiscardableShader() {
+    CreateBitmap(gfx::Size(50, 50), "discardable", &bitmap_);
+  }
 
-  TestLazyShader(SkFlattenableReadBuffer& flattenable_buffer) {
+  TestDiscardableShader(SkFlattenableReadBuffer& flattenable_buffer) {
     SkOrderedReadBuffer& buffer =
         static_cast<SkOrderedReadBuffer&>(flattenable_buffer);
     SkReader32* reader = buffer.getReader32();
@@ -70,7 +54,7 @@ class TestLazyShader : public SkShader {
     uint32_t toSkip = reader->readU32();
     reader->skip(toSkip);
 
-    CreateBitmap(gfx::Size(50, 50), "lazy", &bitmap_);
+    CreateBitmap(gfx::Size(50, 50), "discardable", &bitmap_);
   }
 
   virtual SkShader::BitmapType asABitmap(SkBitmap* bitmap,
@@ -83,7 +67,7 @@ class TestLazyShader : public SkShader {
 
   // Pure virtual implementaiton.
   virtual void shadeSpan(int x, int y, SkPMColor[], int count) OVERRIDE {}
-  SK_DECLARE_PUBLIC_FLATTENABLE_DESERIALIZATION_PROCS(TestLazyShader);
+  SK_DECLARE_PUBLIC_FLATTENABLE_DESERIALIZATION_PROCS(TestDiscardableShader);
 
  private:
   SkBitmap bitmap_;
@@ -106,38 +90,13 @@ SkPixelRef* TestPixelRef::deepCopy(SkBitmap::Config config,
   return this;
 }
 
-TestLazyPixelRef::TestLazyPixelRef(int width, int height)
-    : pixels_(new char[4 * width * height]) {}
-
-TestLazyPixelRef::~TestLazyPixelRef() {}
-
-SkFlattenable::Factory TestLazyPixelRef::getFactory() const { return NULL; }
-
-void* TestLazyPixelRef::onLockPixels(SkColorTable** color_table) {
-  return pixels_.get();
-}
-
-bool TestLazyPixelRef::PrepareToDecode(const PrepareParams& params) {
-  return true;
-}
-
-bool TestLazyPixelRef::MaybeDecoded() {
-  return true;
-}
-
-SkPixelRef* TestLazyPixelRef::deepCopy(SkBitmap::Config config,
-                                       const SkIRect* subset) {
-  this->ref();
-  return this;
-}
-
 void CreateBitmap(gfx::Size size, const char* uri, SkBitmap* bitmap) {
-  skia::RefPtr<TestLazyPixelRef> lazy_pixel_ref =
-      skia::AdoptRef(new TestLazyPixelRef(size.width(), size.height()));
-  lazy_pixel_ref->setURI(uri);
+  skia::RefPtr<TestPixelRef> pixel_ref =
+      skia::AdoptRef(new TestPixelRef(size.width(), size.height()));
+  pixel_ref->setURI(uri);
 
   bitmap->setConfig(SkBitmap::kARGB_8888_Config, size.width(), size.height());
-  bitmap->setPixelRef(lazy_pixel_ref.get());
+  bitmap->setPixelRef(pixel_ref.get());
 }
 
 SkCanvas* StartRecording(SkPicture* picture, gfx::Rect layer_rect) {
@@ -162,21 +121,21 @@ void StopRecording(SkPicture* picture, SkCanvas* canvas) {
 
 }  // namespace
 
-TEST(LazyPixelRefUtilsTest, DrawPaint) {
+TEST(PixelRefUtilsTest, DrawPaint) {
   gfx::Rect layer_rect(0, 0, 256, 256);
 
   skia::RefPtr<SkPicture> picture = skia::AdoptRef(new SkPicture);
   SkCanvas* canvas = StartRecording(picture.get(), layer_rect);
 
-  TestLazyShader first_shader;
+  TestDiscardableShader first_shader;
   SkPaint first_paint;
   first_paint.setShader(&first_shader);
 
-  TestLazyShader second_shader;
+  TestDiscardableShader second_shader;
   SkPaint second_paint;
   second_paint.setShader(&second_shader);
 
-  TestLazyShader third_shader;
+  TestDiscardableShader third_shader;
   SkPaint third_paint;
   third_paint.setShader(&third_shader);
 
@@ -189,8 +148,8 @@ TEST(LazyPixelRefUtilsTest, DrawPaint) {
 
   StopRecording(picture.get(), canvas);
 
-  std::vector<skia::LazyPixelRefUtils::PositionLazyPixelRef> pixel_refs;
-  skia::LazyPixelRefUtils::GatherPixelRefs(picture.get(), &pixel_refs);
+  std::vector<skia::PixelRefUtils::PositionPixelRef> pixel_refs;
+  skia::PixelRefUtils::GatherDiscardablePixelRefs(picture.get(), &pixel_refs);
 
   EXPECT_EQ(3u, pixel_refs.size());
   EXPECT_FLOAT_RECT_EQ(gfx::RectF(0, 0, 256, 256),
@@ -201,21 +160,21 @@ TEST(LazyPixelRefUtilsTest, DrawPaint) {
                        gfx::SkRectToRectF(pixel_refs[2].pixel_ref_rect));
 }
 
-TEST(LazyPixelRefUtilsTest, DrawPoints) {
+TEST(PixelRefUtilsTest, DrawPoints) {
   gfx::Rect layer_rect(0, 0, 256, 256);
 
   skia::RefPtr<SkPicture> picture = skia::AdoptRef(new SkPicture);
   SkCanvas* canvas = StartRecording(picture.get(), layer_rect);
 
-  TestLazyShader first_shader;
+  TestDiscardableShader first_shader;
   SkPaint first_paint;
   first_paint.setShader(&first_shader);
 
-  TestLazyShader second_shader;
+  TestDiscardableShader second_shader;
   SkPaint second_paint;
   second_paint.setShader(&second_shader);
 
-  TestLazyShader third_shader;
+  TestDiscardableShader third_shader;
   SkPaint third_paint;
   third_paint.setShader(&third_shader);
 
@@ -242,8 +201,8 @@ TEST(LazyPixelRefUtilsTest, DrawPoints) {
 
   StopRecording(picture.get(), canvas);
 
-  std::vector<skia::LazyPixelRefUtils::PositionLazyPixelRef> pixel_refs;
-  skia::LazyPixelRefUtils::GatherPixelRefs(picture.get(), &pixel_refs);
+  std::vector<skia::PixelRefUtils::PositionPixelRef> pixel_refs;
+  skia::PixelRefUtils::GatherDiscardablePixelRefs(picture.get(), &pixel_refs);
 
   EXPECT_EQ(3u, pixel_refs.size());
   EXPECT_FLOAT_RECT_EQ(gfx::RectF(10, 10, 90, 90),
@@ -254,21 +213,21 @@ TEST(LazyPixelRefUtilsTest, DrawPoints) {
                        gfx::SkRectToRectF(pixel_refs[2].pixel_ref_rect));
 }
 
-TEST(LazyPixelRefUtilsTest, DrawRect) {
+TEST(PixelRefUtilsTest, DrawRect) {
   gfx::Rect layer_rect(0, 0, 256, 256);
 
   skia::RefPtr<SkPicture> picture = skia::AdoptRef(new SkPicture);
   SkCanvas* canvas = StartRecording(picture.get(), layer_rect);
 
-  TestLazyShader first_shader;
+  TestDiscardableShader first_shader;
   SkPaint first_paint;
   first_paint.setShader(&first_shader);
 
-  TestLazyShader second_shader;
+  TestDiscardableShader second_shader;
   SkPaint second_paint;
   second_paint.setShader(&second_shader);
 
-  TestLazyShader third_shader;
+  TestDiscardableShader third_shader;
   SkPaint third_paint;
   third_paint.setShader(&third_shader);
 
@@ -288,8 +247,8 @@ TEST(LazyPixelRefUtilsTest, DrawRect) {
   // (50, 50, 50, 50)
   canvas->drawRect(SkRect::MakeXYWH(0, 0, 100, 100), third_paint);
 
-  std::vector<skia::LazyPixelRefUtils::PositionLazyPixelRef> pixel_refs;
-  skia::LazyPixelRefUtils::GatherPixelRefs(picture.get(), &pixel_refs);
+  std::vector<skia::PixelRefUtils::PositionPixelRef> pixel_refs;
+  skia::PixelRefUtils::…
(message too large)

[commit-bot: I haz the power, 2014-01-08 16:54:25]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/reveman@chromium.org/111143005/210001

[commit-bot: I haz the power, 2014-01-08 17:14:42]

Retried try job too often on linux_rel for step(s) base_unittests,
browser_tests, interactive_ui_tests, net_unittests, unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2014-01-08 17:51:35]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/reveman@chromium.org/111143005/460001

[commit-bot: I haz the power, 2014-01-08 18:30:46]

Retried try job too often on linux_rel for step(s) cc_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2014-01-08 19:11:55]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/reveman@chromium.org/111143005/690001

[Alpha Left Google, 2014-01-08 20:42:17]

Thanks. LGTM. Don't know why I missed this.

[commit-bot: I haz the power, 2014-01-08 21:57:14]

Change committed as 243660