Make SkImage_Gpu share it's pixelref with the surface to prevent premature return to scratch pool.

BUG=crbug.com/263329
TEST=Surface unit test, function Test_crbug263329
R=bsalomon@google.com, reed@google.com

Author: junov@chromium.org

Review URL: https://chromiumcodereview.appspot.com/20354003

git-svn-id: http://skia.googlecode.com/svn/trunk@10378 2bbb7eff-a529-9590-31e7-b0007b416f81

include/core/SkImage.h

@@ -65,7 +65,13 @@ public:
     static SkImage* NewRasterCopy(const Info&, const void* pixels, size_t rowBytes);
     static SkImage* NewRasterData(const Info&, SkData* pixels, size_t rowBytes);
     static SkImage* NewEncodedData(SkData*);
-    static SkImage* NewTexture(GrTexture*);
+
+    /**
+     * GrTexture is a more logical parameter for this factory, but its 
+     * interactions with scratch cache still has issues, so for now we take
+     * SkBitmap instead. This will be changed in the future. skbug.com/1449
+     */
+    static SkImage* NewTexture(const SkBitmap&);
 
     int width() const { return fWidth; }
     int height() const { return fHeight; }

src/image/SkImage_Gpu.cpp

@@ -17,7 +17,7 @@ class SkImage_Gpu : public SkImage_Base {
 public:
     SK_DECLARE_INST_COUNT(SkImage_Gpu)
 
-    SkImage_Gpu(GrTexture*);
+    explicit SkImage_Gpu(const SkBitmap&);
     virtual ~SkImage_Gpu();
 
     virtual void onDraw(SkCanvas*, SkScalar x, SkScalar y, const SkPaint*) SK_OVERRIDE;
@@ -28,12 +28,9 @@ public:
         return false;
     }
 
-    GrTexture* getTexture() { return fTexture; }
-
-    void setTexture(GrTexture* texture);
+    GrTexture* getTexture() { return fBitmap.getTexture(); }
 
 private:
-    GrTexture*  fTexture;
     SkBitmap    fBitmap;
 
     typedef SkImage_Base INHERITED;
@@ -43,18 +40,13 @@ SK_DEFINE_INST_COUNT(SkImage_Gpu)
 
 ///////////////////////////////////////////////////////////////////////////////
 
-SkImage_Gpu::SkImage_Gpu(GrTexture* texture)
-    : INHERITED(texture->width(), texture->height())
-    , fTexture(texture) {
-
-    SkASSERT(NULL != fTexture);
-    fTexture->ref();
-    fBitmap.setConfig(SkBitmap::kARGB_8888_Config, fTexture->width(), fTexture->height());
-    fBitmap.setPixelRef(SkNEW_ARGS(SkGrPixelRef, (fTexture)))->unref();
+SkImage_Gpu::SkImage_Gpu(const SkBitmap& bitmap)
+    : INHERITED(bitmap.width(), bitmap.height())
+    , fBitmap(bitmap) {
+    SkASSERT(NULL != fBitmap.getTexture());
 }
 
 SkImage_Gpu::~SkImage_Gpu() {
-    SkSafeUnref(fTexture);
 }
 
 void SkImage_Gpu::onDraw(SkCanvas* canvas, SkScalar x, SkScalar y,
@@ -68,33 +60,19 @@ void SkImage_Gpu::onDrawRectToRect(SkCanvas* canvas, const SkRect* src, const Sk
 }
 
 GrTexture* SkImage_Gpu::onGetTexture() {
-    return fTexture;
-}
-
-void SkImage_Gpu::setTexture(GrTexture* texture) {
-
-    if (texture == fTexture) {
-        return;
-    }
-
-    SkRefCnt_SafeAssign(fTexture, texture);
-    fBitmap.setPixelRef(new SkGrPixelRef(texture))->unref();
+    return fBitmap.getTexture();
 }
 
 ///////////////////////////////////////////////////////////////////////////////
 
-SkImage* SkImage::NewTexture(GrTexture* texture) {
-    if (NULL == texture) {
+SkImage* SkImage::NewTexture(const SkBitmap& bitmap) {
+    if (NULL == bitmap.getTexture()) {
         return NULL;
     }
 
-    return SkNEW_ARGS(SkImage_Gpu, (texture));
+    return SkNEW_ARGS(SkImage_Gpu, (bitmap));
 }
 
 GrTexture* SkTextureImageGetTexture(SkImage* image) {
     return ((SkImage_Gpu*)image)->getTexture();
 }
-
-void SkTextureImageSetTexture(SkImage* image, GrTexture* texture) {
-    ((SkImage_Gpu*)image)->setTexture(texture);
-}

src/image/SkSurface_Gpu.cpp

@@ -72,10 +72,7 @@ SkSurface* SkSurface_Gpu::onNewSurface(const SkImage::Info& info) {
 }
 
 SkImage* SkSurface_Gpu::onNewImageSnapshot() {
-
-    GrRenderTarget* rt = fDevice->accessRenderTarget();
-
-    return SkImage::NewTexture(rt->asTexture());
+    return SkImage::NewTexture(fDevice->accessBitmap(false));
 }
 
 void SkSurface_Gpu::onDraw(SkCanvas* canvas, SkScalar x, SkScalar y,

tests/SurfaceTest.cpp

@@ -139,15 +139,48 @@ static void TestSurfaceWritableAfterSnapshotRelease(skiatest::Reporter* reporter
     // This test succeeds by not triggering an assertion.
     // The test verifies that the surface remains writable (usable) after
     // acquiring and releasing a snapshot without triggering a copy on write.
-    SkSurface* surface = createSurface(surfaceType, context);
-    SkAutoTUnref<SkSurface> aur_surface(surface);
+    SkAutoTUnref<SkSurface> surface(createSurface(surfaceType, context));
     SkCanvas* canvas = surface->getCanvas();
     canvas->clear(1);
     surface->newImageSnapshot()->unref();  // Create and destroy SkImage
-    canvas->clear(2);
+    canvas->clear(2);  // Must not assert internally
 }
 
 #if SK_SUPPORT_GPU
+static void Test_crbug263329(skiatest::Reporter* reporter,
+                             GrContext* context) {
+    // This is a regression test for crbug.com/263329
+    // Bug was caused by onCopyOnWrite releasing the old surface texture
+    // back to the scratch texture pool even though the texture is used
+    // by and active SkImage_Gpu.
+    SkAutoTUnref<SkSurface> surface1(createSurface(kGpu_SurfaceType, context));
+    SkAutoTUnref<SkSurface> surface2(createSurface(kGpu_SurfaceType, context));
+    SkCanvas* canvas1 = surface1->getCanvas();
+    SkCanvas* canvas2 = surface2->getCanvas();
+    canvas1->clear(1);
+    SkAutoTUnref<SkImage> image1(surface1->newImageSnapshot());
+    // Trigger copy on write, new backing is a scratch texture
+    canvas1->clear(2);
+    SkAutoTUnref<SkImage> image2(surface1->newImageSnapshot());
+    // Trigger copy on write, old backing should not be returned to scratch
+    // pool because it is held by image2
+    canvas1->clear(3);
+
+    canvas2->clear(4);
+    SkAutoTUnref<SkImage> image3(surface2->newImageSnapshot());
+    // Trigger copy on write on surface2. The new backing store should not
+    // be recycling a texture that is held by an existing image.
+    canvas2->clear(5);
+    SkAutoTUnref<SkImage> image4(surface2->newImageSnapshot());
+    REPORTER_ASSERT(reporter, image4->getTexture() != image3->getTexture());
+    // The following assertion checks crbug.com/263329
+    REPORTER_ASSERT(reporter, image4->getTexture() != image2->getTexture());
+    REPORTER_ASSERT(reporter, image4->getTexture() != image1->getTexture());
+    REPORTER_ASSERT(reporter, image3->getTexture() != image2->getTexture());
+    REPORTER_ASSERT(reporter, image3->getTexture() != image1->getTexture());
+    REPORTER_ASSERT(reporter, image2->getTexture() != image1->getTexture());
+}
+
 static void TestGetTexture(skiatest::Reporter* reporter,
                                  SurfaceType surfaceType,
                                  GrContext* context) {
@@ -209,6 +242,7 @@ static void TestSurface(skiatest::Reporter* reporter, GrContextFactory* factory)
     TestGetTexture(reporter, kPicture_SurfaceType, NULL);
     if (NULL != factory) {
         GrContext* context = factory->get(GrContextFactory::kNative_GLContextType);
+        Test_crbug263329(reporter, context);
         TestSurfaceCopyOnWrite(reporter, kGpu_SurfaceType, context);
         TestSurfaceWritableAfterSnapshotRelease(reporter, kGpu_SurfaceType, context);
         TestSurfaceNoCanvas(reporter, kGpu_SurfaceType, context, SkSurface::kDiscard_ContentChangeMode);