Integrating SkSurface with SkDeferredCanvas

Integrating SkSurface with SkDeferredCanvas

* Adding SkSurface_Gpu into the build.
* Adding overwite optimizations to surface copyOnWrite
* Changed general behavior of SkSurface_Gpu::onCopyOnWrite to assign the
  new texture to the Surface, and let the Image hold on to the old one.
  This is consistent with the behavior of SkSurface_Raster, and is important
  in case where the resources locked by the SkImage are referenced externally
  e.g. by a compositor.
* Removed setTexture API from SkImage_Gpu
* Added new SkDeferredCanvas constructor that takes an SkSurface
* Unit tests for SkDeferredCanvas wrapping an SkSurface (raster & gpu)

[Justin Novosad, 2013-03-26 20:26:21]

PTAL

[bsalomon, 2013-03-26 20:40:11]

On 2013/03/26 20:26:21, junov wrote:
> PTAL

The parts I get look OK :) "overwrite" doesn't seem very obvious to me, maybe
negate and call "discard" or "discardContents"?

[Stephen White, 2013-03-26 20:51:43]

https://codereview.chromium.org/12567025/diff/15001/include/core/SkSurface.h
File include/core/SkSurface.h (right):

https://codereview.chromium.org/12567025/diff/15001/include/core/SkSurface.h#...
include/core/SkSurface.h:87: void notifyContentChanged(bool overwrite = false);
Bikeshed nit:  maybe "willOverwrite", if it hasn't happened yet?  or "discard",
"discardContent" or "swap"?

Not sure if any of these are better, to be honest.

https://codereview.chromium.org/12567025/diff/15001/src/image/SkSurface_Gpu.cpp
File src/image/SkSurface_Gpu.cpp (right):

https://codereview.chromium.org/12567025/diff/15001/src/image/SkSurface_Gpu.c...
src/image/SkSurface_Gpu.cpp:94: SkGpuDevice* newDevice =
static_cast<SkGpuDevice*>(
Kind of a style thing, but you could use SkAutoTUnref<> here...

https://codereview.chromium.org/12567025/diff/15001/src/image/SkSurface_Gpu.c...
src/image/SkSurface_Gpu.cpp:105: fDevice = newDevice;
... and SkRefCnt_SafeAssign() here.

https://codereview.chromium.org/12567025/diff/15001/src/image/SkSurface_Raste...
File src/image/SkSurface_Raster.cpp (right):

https://codereview.chromium.org/12567025/diff/15001/src/image/SkSurface_Raste...
src/image/SkSurface_Raster.cpp:141: // Now fBitmap is a deep copy of itself (and
therefore different from
Nit:  this comment is not strictly true anymore, since it may not be a deep
copy.

[Justin Novosad, 2013-03-26 21:13:14]

New patch addresses all review comments so far.

I went for "canDiscardContents".

[reed1, 2013-03-27 15:31:43]

Perhaps we can simplify this CL into smaller parts?

1. do we need SkSurface::notifyContentChanged() at all? If not, lets remove it
first.

2. can we separate the canDiscardContents and the DeferredCanvas changes? Seems
like canDiscardContents is a good idea even w/o deferredcanvas?

https://codereview.chromium.org/12567025/diff/21/src/image/SkSurface.cpp
File src/image/SkSurface.cpp (right):

https://codereview.chromium.org/12567025/diff/21/src/image/SkSurface.cpp#newc...
src/image/SkSurface.cpp:66: SkASSERT(canvas->getSurfaceBase() == this || NULL ==
fCachedImage);
Why this || clause?

I presume it is because the passed canvas is the deferred canvas, not the one
built-into the surface?

[Justin Novosad, 2013-03-27 15:42:06]

On 2013/03/27 15:31:43, reed1 wrote:
> Perhaps we can simplify this CL into smaller parts?
> 
> 1. do we need SkSurface::notifyContentChanged() at all? If not, lets remove it
> first.

Not sure how important this is, but the alternative would be to make aboutToDraw
virtual, which adds a v-table lookup.  Perhaps you can enlighten me as to why
SkSurface and SkSurface_Base are separate.  What would we lost from merging
them?

> 
> 2. can we separate the canDiscardContents and the DeferredCanvas changes?
Seems
> like canDiscardContents is a good idea even w/o deferredcanvas?
> 
> https://codereview.chromium.org/12567025/diff/21/src/image/SkSurface.cpp
> File src/image/SkSurface.cpp (right):
> 
>
https://codereview.chromium.org/12567025/diff/21/src/image/SkSurface.cpp#newc...
> src/image/SkSurface.cpp:66: SkASSERT(canvas->getSurfaceBase() == this || NULL
==
> fCachedImage);
> Why this || clause?
> 
> I presume it is because the passed canvas is the deferred canvas, not the one
> built-into the surface?

[Justin Novosad, 2013-03-27 16:58:15]

>
https://codereview.chromium.org/12567025/diff/21/src/image/SkSurface.cpp#newc...
> src/image/SkSurface.cpp:66: SkASSERT(canvas->getSurfaceBase() == this || NULL
==
> fCachedImage);
> Why this || clause?
> 
> I presume it is because the passed canvas is the deferred canvas, not the one
> built-into the surface?

No, the canvas passed in is the one built-into the surface.  The this is that
SkDeferredCanvas, make call this method without knowing whether there is an
SkImage.  This issue will go away we the change we discussed earlier (going
through SkCanvas::predrawNotify)

[Justin Novosad, 2013-04-15 20:21:02]

Review closed.  This stuff is landing in smaller chunks.