Fixed bug with SkImage leaving canvas backing store in an immutable state after destroy.

Fixed bug with SkImage leaving canvas backing store in an immutable state after destroy.
Added unit test that verifies that surface backing is writable after creating and destroying an image.
Committed: https://code.google.com/p/skia/source/detail?r=8512

[reed1, 2013-03-28 21:35:09]

errrrrrr, lets discuss to see if we can avoid this sort of trickery

[Justin Novosad, 2013-04-03 14:07:58]

New patch. Got rid of the trickery.
I reduced this patch to the minimum that is required for moving forward for now,
which is to ensure that surfaces remain writable. The matter of globally
enforceable read locks is not a requirement for now and can be resolved at a
later time, pending further discussion.

reed OOO -> making bsalomon reviewer.

[Justin Novosad, 2013-04-03 14:10:54]

https://codereview.chromium.org/13226002/diff/10001/src/image/SkImage_Raster.cpp
File src/image/SkImage_Raster.cpp (left):

https://codereview.chromium.org/13226002/diff/10001/src/image/SkImage_Raster....
src/image/SkImage_Raster.cpp:103: fBitmap.setImmutable();
Problem here is that the pixel ref is shared with the surface's device. If the
SkImage is released before a copy on write occurs, the surface will be left in
an unwritable state.

[bsalomon, 2013-04-03 14:13:43]

lgtm

https://codereview.chromium.org/13226002/diff/10001/tests/SurfaceTest.cpp
File tests/SurfaceTest.cpp (right):

https://codereview.chromium.org/13226002/diff/10001/tests/SurfaceTest.cpp#new...
tests/SurfaceTest.cpp:150: static void TestSurface(skiatest::Reporter* reporter)
{
Just noting that TestSurface and TestSurfaceGpu could be combined as:

GrContext* context = factory->get(GrContextFactory::kNative_GLContextType);
TestSurfaceCopyOnWrite(reporter, kRaster_SurfaceType, NULL);
TestSurfaceCopyOnWrite(reporter, kPicture_SurfaceType, NULL);
TestSurfaceCopyOnWrite(reporter, kPicture_SurfaceType, NULL);
TestSurfaceCopyOnWrite(reporter, kGpu_SurfaceType, context);
TestSurfaceWritableAfterSnapshotRelease(reporter, kRaster_SurfaceType, NULL);
TestSurfaceWritableAfterSnapshotRelease(reporter, kPicture_SurfaceType, NULL);
TestSurfaceWritableAfterSnapshotRelease(reporter, kGpu_SurfaceType, context);

[Justin Novosad, 2013-04-03 14:17:16]

Are you implying to get rid of DEFINE_TESTCLASS, and just have
DEFINE_GPUTESTCLASS?

We would no longer be running the cpu tests on non-gpu platforms.

[bsalomon, 2013-04-03 14:22:40]

On 2013/04/03 14:17:16, junov wrote:
> Are you implying to get rid of DEFINE_TESTCLASS, and just have
> DEFINE_GPUTESTCLASS?
> 
> We would no longer be running the cpu tests on non-gpu platforms.

Yes, that's what I meant.

Units tests don't have the same notion of configs as bench and gm. 

When compiled with SK_SUPPORT_GPU=0 the tests created by DEFINE_GPUTESTCLASS are
still run, they just always get NULL passed as the GetGrContextFactory param.

In any event, that means there should be a NULL check in TestSurfaceGpu.

[Justin Novosad, 2013-04-03 14:26:49]

On 2013/04/03 14:22:40, bsalomon wrote:
> On 2013/04/03 14:17:16, junov wrote:
> > Are you implying to get rid of DEFINE_TESTCLASS, and just have
> > DEFINE_GPUTESTCLASS?
> > 
> > We would no longer be running the cpu tests on non-gpu platforms.
> 
> Yes, that's what I meant.
> 
> Units tests don't have the same notion of configs as bench and gm. 
> 
> When compiled with SK_SUPPORT_GPU=0 the tests created by DEFINE_GPUTESTCLASS
are
> still run, they just always get NULL passed as the GetGrContextFactory param.
> 
> In any event, that means there should be a NULL check in TestSurfaceGpu.

Ah, thanks for educating me on this.  The NULL check will be a lot cleaner than
the current #if SK_SUPPORT_GPU.

[Justin Novosad, 2013-04-03 14:31:26]

Patch.

https://codereview.chromium.org/13226002/diff/17001/tests/SurfaceTest.cpp
File tests/SurfaceTest.cpp (right):

https://codereview.chromium.org/13226002/diff/17001/tests/SurfaceTest.cpp#new...
tests/SurfaceTest.cpp:9: #include "GrContextFactory.h"
This is OK, even when SK_SUPPORT_GPU == 0, right?

[bsalomon, 2013-04-03 14:35:47]

https://codereview.chromium.org/13226002/diff/17001/tests/SurfaceTest.cpp
File tests/SurfaceTest.cpp (right):

https://codereview.chromium.org/13226002/diff/17001/tests/SurfaceTest.cpp#new...
tests/SurfaceTest.cpp:9: #include "GrContextFactory.h"
On 2013/04/03 14:31:26, junov wrote:
> This is OK, even when SK_SUPPORT_GPU == 0, right?

Actually, no, the #include needs to be guarded. We could consider changing the
header to simply forward declare the class when SK_SUPPORT_GPU == 0.

https://codereview.chromium.org/13226002/diff/17001/tests/SurfaceTest.cpp#new...
tests/SurfaceTest.cpp:141: static void TestSurface(skiatest::Reporter* reporter)
{
Do we need this function anymore?

[Justin Novosad, 2013-04-03 14:58:59]

> 
> Actually, no, the #include needs to be guarded. We could consider changing the
> header to simply forward declare the class when SK_SUPPORT_GPU == 0.

Then, unfortunately I still need #if in the code because this line needs
GrContextFactory to be defined: GrContext* context =
factory->get(GrContextFactory::kNative_GLContextType);

[Justin Novosad, 2013-04-03 14:59:17]

Patch.

[bsalomon, 2013-04-03 15:00:18]

lgtm

[mikerreed, 2013-04-07 22:33:40]

Does this lose the optimization we had before, where this image would be seen as
immutable during picture record and caching for the gpu?

Perhaps we can find a different fix for the surface/canvas that doesn't remove
the optimization for the image...