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
diff --git a/include/core/SkImage.h b/include/core/SkImage.h
index c85a2bf..678133d 100644
--- a/include/core/SkImage.h
+++ b/include/core/SkImage.h
@@ -65,7 +65,13 @@
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; }
diff --git a/src/image/SkImage_Gpu.cpp b/src/image/SkImage_Gpu.cpp
index d19d210..036e45b 100644
--- a/src/image/SkImage_Gpu.cpp
+++ b/src/image/SkImage_Gpu.cpp
@@ -17,7 +17,7 @@
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 @@
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 @@
///////////////////////////////////////////////////////////////////////////////
-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 @@
}
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);
-}
diff --git a/src/image/SkSurface_Gpu.cpp b/src/image/SkSurface_Gpu.cpp
index 75c3890..e5b7bd4 100644
--- a/src/image/SkSurface_Gpu.cpp
+++ b/src/image/SkSurface_Gpu.cpp
@@ -72,10 +72,7 @@
}
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,
diff --git a/tests/SurfaceTest.cpp b/tests/SurfaceTest.cpp
index 07322f9..049f250 100644
--- a/tests/SurfaceTest.cpp
+++ b/tests/SurfaceTest.cpp
@@ -139,15 +139,48 @@
// 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 @@
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);