Use SkImage as a backing store for copying 2d Contexts to ImageBitmaps.

Use SkImage as a backing store for copying 2d Contexts to ImageBitmaps.

This patch is dependent on skia change https://codereview.chromium.org/19729007/

BUG=260933, 166658

[arbesfeld, 2013-07-18 19:33:56]

PTAL.

[Justin Novosad, 2013-07-18 22:14:02]

On 2013/07/18 19:33:56, arbesfeld wrote:
> PTAL.

Don't use SkImage outside of core/platform/graphics.  You should probably create
a subclass of Image that wraps and SkImage.

[arbesfeld, 2013-07-19 19:16:32]

On 2013/07/18 22:14:02, junov wrote:
> On 2013/07/18 19:33:56, arbesfeld wrote:
> > PTAL.
> 
> Don't use SkImage outside of core/platform/graphics.  You should probably
create
> a subclass of Image that wraps and SkImage.

Ok, done.

[Justin Novosad, 2013-07-22 15:35:41]

https://codereview.chromium.org/19705006/diff/1/Source/core/html/canvas/Canva...
File Source/core/html/canvas/CanvasRenderingContext2D.cpp (right):

https://codereview.chromium.org/19705006/diff/1/Source/core/html/canvas/Canva...
Source/core/html/canvas/CanvasRenderingContext2D.cpp:1618: static void
drawImageToContext(SkImage* image, GraphicsContext* context, FloatRect& dest,
const FloatRect& src, CompositeOperator op)
Don't use skia classes outside of core/platform/ unless there is no other way. 
Here we could take an ImageBitmap instead of SkImage.  Extracting the SkImage
should be taken care of in GraphicsContext.

https://codereview.chromium.org/19705006/diff/1/Source/core/page/ImageBitmap.h
File Source/core/page/ImageBitmap.h (right):

https://codereview.chromium.org/19705006/diff/1/Source/core/page/ImageBitmap....
Source/core/page/ImageBitmap.h:59: SkImage* m_canvasImage;
We should no use SkImage directly.  Could we have a platform-specific subclass
of BitmapImage that encapsulates an SkImage?

https://codereview.chromium.org/19705006/diff/10001/Source/core/html/canvas/C...
File Source/core/html/canvas/CanvasRenderingContext2D.cpp (left):

https://codereview.chromium.org/19705006/diff/10001/Source/core/html/canvas/C...
Source/core/html/canvas/CanvasRenderingContext2D.cpp:1267:
actualDstRect.scale(normalizedDstRect.width() / bitmap->width(),
normalizedDstRect.height() / bitmap->height());
This is not correct. What you should be doing here is offset and clamp the
source rect.  There should not be any scaling.  Actually6, I have a CL in flight
that fixes a bug with source rect clamping here.  We should land that first so
you can re-use that code to do the right thing here.

https://codereview.chromium.org/19705006/diff/10001/Source/core/page/ImageBit...
File Source/core/page/ImageBitmap.cpp (right):

https://codereview.chromium.org/19705006/diff/10001/Source/core/page/ImageBit...
Source/core/page/ImageBitmap.cpp:38: , m_derivedFromCanvas(0)
this is a bool, so initialize it with false instead of 0.

https://codereview.chromium.org/19705006/diff/10001/Source/core/page/ImageBit...
File Source/core/page/ImageBitmap.h (right):

https://codereview.chromium.org/19705006/diff/10001/Source/core/page/ImageBit...
Source/core/page/ImageBitmap.h:55: RefPtr<Image> m_bitmap;
With what we've discussed, should this be CachedImage?

https://codereview.chromium.org/19705006/diff/10001/Source/core/platform/grap...
File Source/core/platform/graphics/Image.h (right):

https://codereview.chromium.org/19705006/diff/10001/Source/core/platform/grap...
Source/core/platform/graphics/Image.h:69: virtual bool
currentFrameKnownToBeOpaque() { return false; }
No, let's keep this pure virtual.  Implement it in derived class instead.

https://codereview.chromium.org/19705006/diff/10001/Source/core/platform/grap...
Source/core/platform/graphics/Image.h:95: virtual void destroyDecodedData() { }
same here

https://codereview.chromium.org/19705006/diff/10001/Source/core/platform/grap...
Source/core/platform/graphics/Image.h:96: virtual unsigned decodedSize() const {
return 0; }
and here.

https://codereview.chromium.org/19705006/diff/10001/Source/core/platform/grap...
File Source/core/platform/graphics/ImageBuffer.cpp (right):

https://codereview.chromium.org/19705006/diff/10001/Source/core/platform/grap...
Source/core/platform/graphics/ImageBuffer.cpp:82: m_surface =
SkSurface::NewRenderTarget(context3D->grContext(), info);
You will need adoptPtr here, once m_surface is an OwnPtr.

https://codereview.chromium.org/19705006/diff/10001/Source/core/platform/grap...
Source/core/platform/graphics/ImageBuffer.cpp:89: return canvas;
Now that createAcceleratedCanvas is a member of ImageBuffer, no need to return
the canvas.  You could just set m_canvas directly.

https://codereview.chromium.org/19705006/diff/10001/Source/core/platform/grap...
Source/core/platform/graphics/ImageBuffer.cpp:208: if (!m_surface.get())
Inside this 'if' you add ASSERT(!m_canvas) With this comment: A canvas without a
surface indicates that a platform buffer is being used. imageSnapshot() on a
platform buffer is not supported.

https://codereview.chromium.org/19705006/diff/10001/Source/core/platform/grap...
File Source/core/platform/graphics/ImageBuffer.h (right):

https://codereview.chromium.org/19705006/diff/10001/Source/core/platform/grap...
Source/core/platform/graphics/ImageBuffer.h:153: RefPtr<SkSurface> m_surface;
Make this an OwnPtr instead.

https://codereview.chromium.org/19705006/diff/10001/Source/core/platform/grap...
File Source/core/platform/graphics/SkiaImage.h (right):

https://codereview.chromium.org/19705006/diff/10001/Source/core/platform/grap...
Source/core/platform/graphics/SkiaImage.h:44: class SkiaImage : public Image {
Let's give this a blinky name. How about ReadOnlyBitmapImage?

https://codereview.chromium.org/19705006/diff/10001/Source/core/platform/grap...
Source/core/platform/graphics/SkiaImage.h:54: private:
You should overload all the virtual methods of Image that are not compatible
with the read-only semantic of SkImage and make them call ASSERT_NOT_REACHED()
with a comment that says that the call is not supported.

[arbesfeld, 2013-08-06 01:21:23]

New patch for review.

A lot of the case handling for canvas-derived ImageBitmaps could be removed if
we made a flavor of extractSubset for SkImages.

[arbesfeld, 2013-08-09 19:28:48]

Rebased with https://codereview.chromium.org/22613002/

[Justin Novosad, 2013-08-19 20:12:42]

https://codereview.chromium.org/19705006/diff/23001/Source/core/platform/grap...
File Source/core/platform/graphics/ImageBuffer.cpp (right):

https://codereview.chromium.org/19705006/diff/23001/Source/core/platform/grap...
Source/core/platform/graphics/ImageBuffer.cpp:65: void
ImageBuffer::createAcceleratedCanvas(const IntSize& size,
OwnPtr<Canvas2DLayerBridge>* outLayerBridge, OpacityMode opacityMode)
This clashes with a refactoring I have in flight.  Let's not commit this right
away

[Stephen White, 2013-08-19 20:21:21]

https://codereview.chromium.org/19705006/diff/23001/Source/core/platform/grap...
File Source/core/platform/graphics/ImageBuffer.cpp (right):

https://codereview.chromium.org/19705006/diff/23001/Source/core/platform/grap...
Source/core/platform/graphics/ImageBuffer.cpp:65: void
ImageBuffer::createAcceleratedCanvas(const IntSize& size,
OwnPtr<Canvas2DLayerBridge>* outLayerBridge, OpacityMode opacityMode)
If you don't need the SkCanvas* return value anymore, you should probably just
return a PassOwnPtr<Canvas2DLayerBridge> from this function.

[danakj, 2013-08-19 20:22:32]

https://codereview.chromium.org/19705006/diff/23001/Source/core/platform/grap...
File Source/core/platform/graphics/ImageBuffer.cpp (right):

https://codereview.chromium.org/19705006/diff/23001/Source/core/platform/grap...
Source/core/platform/graphics/ImageBuffer.cpp:65: void
ImageBuffer::createAcceleratedCanvas(const IntSize& size,
OwnPtr<Canvas2DLayerBridge>* outLayerBridge, OpacityMode opacityMode)
On 2013/08/19 20:21:21, Stephen White wrote:
> If you don't need the SkCanvas* return value anymore, you should probably just
> return a PassOwnPtr<Canvas2DLayerBridge> from this function.

SkCanvas is refcounted. Should it not go in a RefPtr?

[Stephen White, 2013-08-19 20:28:48]

On 2013/08/19 20:22:32, danakj wrote:
>
https://codereview.chromium.org/19705006/diff/23001/Source/core/platform/grap...
> File Source/core/platform/graphics/ImageBuffer.cpp (right):
> 
>
https://codereview.chromium.org/19705006/diff/23001/Source/core/platform/grap...
> Source/core/platform/graphics/ImageBuffer.cpp:65: void
> ImageBuffer::createAcceleratedCanvas(const IntSize& size,
> OwnPtr<Canvas2DLayerBridge>* outLayerBridge, OpacityMode opacityMode)
> On 2013/08/19 20:21:21, Stephen White wrote:
> > If you don't need the SkCanvas* return value anymore, you should probably
just
> > return a PassOwnPtr<Canvas2DLayerBridge> from this function.
> 
> SkCanvas is refcounted. Should it not go in a RefPtr?

Ownership of this SkCanvas is really unclear ATM -- it's created by the
Canvas2DLayerBridge, but never ref'ed, then passed to the ImageBuffer, which
stores it in an OwnPtr. Justin has promised to clean it up, though. :)

[Justin Novosad, 2013-08-19 20:35:27]

On 2013/08/19 20:28:48, Stephen White wrote:

> Ownership of this SkCanvas is really unclear ATM -- it's created by the
> Canvas2DLayerBridge, but never ref'ed, then passed to the ImageBuffer, which
> stores it in an OwnPtr. Justin has promised to clean it up, though. :)

Yep.  The plan is to refactor this to remove all the skia resource management
stuff out of Canvas2DLayerBridge and to make that class be just an interface to
the compositor.  The current design has the unfortunate side-effect of requiring
a Canvas2DLayerBridge whenever you create an accelerated ImageBuffer.