WIP RasterCanvasLayerAllocator

WIP Add SkRasterCanvasLayerAllocator
    
Encapsulates allocating and freeing the raster backing for a canvas.
Allows a client's subclass to provide a situationally-appropriate
backing: a BITMAP on Windows, a CGBitmapContext on Mac, on Linux a
cairo_surface_t.
    
BUG=543755
GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&issue=1763143002

[tomhudson, 2016-03-04 19:38:13]

Description was changed from

==========
WIP RasterCanvasLayerAllocator

BUG=skia:
==========

to

==========
WIP RasterCanvasLayerAllocator

BUG=skia:
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
==========

[tomhudson, 2016-03-22 15:49:47]

Description was changed from

==========
WIP RasterCanvasLayerAllocator

BUG=skia:
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
==========

to

==========
WIP Add SkRasterCanvasLayerAllocator
    
Encapsulates allocating and freeing the raster backing for a canvas.
Allows a client's subclass to provide a situationally-appropriate
backing: a BITMAP on Windows, a CGBitmapContext on Mac, on Linux a
cairo_surface_t.
    
BUG=543755
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
==========

[tomhudson, 2016-05-06 18:07:13]

tomhudson@google.com changed reviewers:
+ reed@google.com

[tomhudson, 2016-05-11 15:14:12]

Mike suggests we might want to replace getTopLayerNative() with an additional
out parameter on accessTopLayerPixels().

[reed1, 2016-05-11 18:58:25]

https://codereview.chromium.org/1763143002/diff/120001/include/core/SkCanvas.h
File include/core/SkCanvas.h (right):

https://codereview.chromium.org/1763143002/diff/120001/include/core/SkCanvas....
include/core/SkCanvas.h:1519: SkRasterCanvasLayerAllocator* fAllocator;
I presume we'll add a constructor that takes an allocator?

https://codereview.chromium.org/1763143002/diff/120001/include/core/SkRasterC...
File include/core/SkRasterCanvasLayerAllocator.h (right):

https://codereview.chromium.org/1763143002/diff/120001/include/core/SkRasterC...
include/core/SkRasterCanvasLayerAllocator.h:44: 
Lets try adding the flush() virtual, and see where we would call it in
canvas.cpp

https://codereview.chromium.org/1763143002/diff/120001/include/core/SkRasterC...
include/core/SkRasterCanvasLayerAllocator.h:66: virtual void*
allocateLayer(const SkImageInfo& info, size_t* rowBytes) = 0;
We may have to iterate more on this signature -- what if we request a certain
info and the "native" provider can't do it, but can do something else that might
be acceptable... (thinking of half-float, sRGB at the moment).

https://codereview.chromium.org/1763143002/diff/120001/src/core/SkCanvas.cpp
File src/core/SkCanvas.cpp (right):

https://codereview.chromium.org/1763143002/diff/120001/src/core/SkCanvas.cpp#...
src/core/SkCanvas.cpp:223:
fAllocator->free(fDevice->accessBitmap(false).getPixels(),
Do we need to pass the actual pixel addr to the free method, and not just the
nativecontext?

[tomhudson, 2016-05-26 16:36:17]

https://codereview.chromium.org/1763143002/diff/120001/include/core/SkCanvas.h
File include/core/SkCanvas.h (right):

https://codereview.chromium.org/1763143002/diff/120001/include/core/SkCanvas....
include/core/SkCanvas.h:1519: SkRasterCanvasLayerAllocator* fAllocator;
On 2016/05/11 18:58:24, reed1 wrote:
> I presume we'll add a constructor that takes an allocator?

With the next patchset, I'm proposing a factory on an allocator that wires the
allocator into the canvas it constructs.

https://codereview.chromium.org/1763143002/diff/120001/include/core/SkRasterC...
File include/core/SkRasterCanvasLayerAllocator.h (right):

https://codereview.chromium.org/1763143002/diff/120001/include/core/SkRasterC...
include/core/SkRasterCanvasLayerAllocator.h:44: 
On 2016/05/11 18:58:24, reed1 wrote:
> Lets try adding the flush() virtual, and see where we would call it in
> canvas.cpp

Done.

https://codereview.chromium.org/1763143002/diff/120001/include/core/SkRasterC...
include/core/SkRasterCanvasLayerAllocator.h:66: virtual void*
allocateLayer(const SkImageInfo& info, size_t* rowBytes) = 0;
On 2016/05/11 18:58:24, reed1 wrote:
> We may have to iterate more on this signature -- what if we request a certain
> info and the "native" provider can't do it, but can do something else that
might
> be acceptable... (thinking of half-float, sRGB at the moment).

Still TODO.

https://codereview.chromium.org/1763143002/diff/120001/src/core/SkCanvas.cpp
File src/core/SkCanvas.cpp (right):

https://codereview.chromium.org/1763143002/diff/120001/src/core/SkCanvas.cpp#...
src/core/SkCanvas.cpp:223:
fAllocator->free(fDevice->accessBitmap(false).getPixels(),
On 2016/05/11 18:58:24, reed1 wrote:
> Do we need to pass the actual pixel addr to the free method, and not just the
> nativecontext?

I'd assumed that allocators would have to map from buffer -> native context
(which means really I only need the actual pixel addr here); that would also
require storing the reverse mapping from native context -> buffer.

My unit test doesn't touch that; I think we won't have a well-grounded sense of
how important it is until we port Chrome to this, and we need to solve
lifecycle/construction issues first.

https://codereview.chromium.org/1763143002/diff/140001/include/core/SkCanvas.h
File include/core/SkCanvas.h (right):

https://codereview.chromium.org/1763143002/diff/140001/include/core/SkCanvas....
include/core/SkCanvas.h:1468: SkCanvas(const SkBitmap& bitmap,
sk_sp<SkRasterCanvasLayerAllocator>);
Figuring out lifetime here is a little awkward - the RasterCanvasLayerAllocator
constructs a smart pointer around (this) to pass here so that the Canvas can own
it?

Perhaps we should instead copy in the Allocator, since we really don't want to
have one instance shared between multiple canvases. That is itself awkward to
code, although we can hide the awkwardness from the API user I *think*.

Or perhaps that's not so important, and we can share these; if we don't share
these, we could simplify the API a bit.

https://codereview.chromium.org/1763143002/diff/140001/include/core/SkRasterC...
File include/core/SkRasterCanvasLayerAllocator.h (right):

https://codereview.chromium.org/1763143002/diff/140001/include/core/SkRasterC...
include/core/SkRasterCanvasLayerAllocator.h:46: SkCanvas* CreateCanvas(const
SkImageInfo& info);
If |this| becomes unshareable, probably move CreateCanvas() to a free function?

https://codereview.chromium.org/1763143002/diff/140001/include/core/SkRasterC...
include/core/SkRasterCanvasLayerAllocator.h:54: virtual void*
getNativeContext(void* buffer) = 0;
If |this| becomes unshareable, can replace with getTopNativeContext() and not
require a parameter; internal data-keeping is probably just a stack of contexts?

https://codereview.chromium.org/1763143002/diff/140001/include/core/SkRasterC...
include/core/SkRasterCanvasLayerAllocator.h:69: void free(void* buffer) {
free(buffer, this->getNativeContext(buffer)); }
If |this| becomes unshareable, can replace with freeTop() requiring no
parameters?

[tomhudson, 2016-05-26 17:29:16]

Also forgot to mention:

I expected saveLayer(nullptr, nullptr) to actually save a layer, because:

        @param bounds (may be null) This rect, if non-null, is used as a hint to
                      limit the size of the offscreen, and thus drawing may be
                      clipped to it, though that clipping is not guaranteed to
                      happen. If exact clipping is desired, use clipRect().

However, in internalSaveLayer():

    if (!this->clipRectBounds(bounds, saveLayerFlags, &ir, imageFilter)) {
        return;
    }

... hmm, tracing through there may be some other problem with my setup, since
clipRectBounds() ought to be returning true.

[reed1, 2016-07-12 19:45:41]

saveLayer(nullptr, nullptr) should be working fine.

https://fiddle.skia.org/c/2f5440e2492cd25f52f43945d059ea26

[reed1, 2016-07-12 19:52:40]

https://codereview.chromium.org/1763143002/diff/140001/include/core/SkCanvas.h
File include/core/SkCanvas.h (right):

https://codereview.chromium.org/1763143002/diff/140001/include/core/SkCanvas....
include/core/SkCanvas.h:1468: SkCanvas(const SkBitmap& bitmap,
sk_sp<SkRasterCanvasLayerAllocator>);
On 2016/05/26 16:36:17, tomhudson wrote:
> Figuring out lifetime here is a little awkward - the
RasterCanvasLayerAllocator
> constructs a smart pointer around (this) to pass here so that the Canvas can
own
> it?
> 
> Perhaps we should instead copy in the Allocator, since we really don't want to
> have one instance shared between multiple canvases. That is itself awkward to
> code, although we can hide the awkwardness from the API user I *think*.
> 
> Or perhaps that's not so important, and we can share these; if we don't share
> these, we could simplify the API a bit.

I think unique_ptr as a parameter type is fine. No need to make it sharable (at
least not right now).

[tomhudson, 2016-09-28 20:56:32]

Replaced by https://codereview.chromium.org/2309483002/