Refactoring ImageBuffer to decouple it from Canvas2DLayerBridge

Refactoring ImageBuffer to decouple it from Canvas2DLayerBridge

This change adds a new abstraction layer called ImageBufferSurface which is
used to provide a rendering surface to ImageBuffers.  The motivation is to
move the construction of the canvas layer bridge into HTMLCanvasElement, which
will make it possible, in a future change, to attach it to a per-WebView
Canvas2DLayerManager. Having one Manager per WebView (instead of per renderer
process) will make it possible to have canvas resource management policies
that depend on whether the owning WebView is active.

Other effects of this change:
* Changed all call sites that used Unaccelerated image buffers to stop
  using platform surfaces, and used malloc'ed buffers instead.
* Non-drawable image buffers no longer create an SkCanvas and a GraphicsContext
* It is now possible to create an accelerated ImageBuffer without a
  Canvas2DLayerBridge (useful for accelerated image filters).
* Buffer creation fallback sequence is no longer baked-in to the ImageBuffer
  constructor, making it possible for different call sites to have different
  fallback logic.
* ImageBuffer no longer provides support for device scale factors

BUG=244427
R=senorblanco,jamesr

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=163572

[Stephen Chennney, 2013-12-04 18:56:06]

On 2013/12/04 18:27:10, junov wrote:

Please don't land this until the GraphicsContext move from core/platform to
platform has happened, maybe tomorrow if all goes well.

[Justin Novosad, 2013-12-04 19:27:23]

On 2013/12/04 18:56:06, Stephen Chenney wrote:
> On 2013/12/04 18:27:10, junov wrote:
> 
> Please don't land this until the GraphicsContext move from core/platform to
> platform has happened, maybe tomorrow if all goes well.

Awesome!  By the way, do you have any intention to move
GraphicsContext3D+SharedGraphicsContext3D out of core?  In this CL, I had to put
the accelerated derivatives of ImageBufferSurface in core because of dependency
on GraphicsContext3D.

[Stephen White, 2013-12-04 21:18:39]

Overall, this is great work. It separates concerns of the different use cases
nicely, and clarifies the ownership of the various SkCanvases which is a huge
improvement. Fantastic stuff!

https://codereview.chromium.org/104023007/diff/60001/Source/core/core.gypi
File Source/core/core.gypi (right):

https://codereview.chromium.org/104023007/diff/60001/Source/core/core.gypi#ne...
Source/core/core.gypi:2917:
'platform/graphics/gpu/NonDrawableAcceleratedImageBufferSurface.cpp',
Naming nit: NonDrawable might imply you can't draw from it, either. Since we
have Canvas2DImageBufferSurface, how about WebGLImageBufferSurface?

https://codereview.chromium.org/104023007/diff/60001/Source/core/html/HTMLCan...
File Source/core/html/HTMLCanvasElement.cpp (right):

https://codereview.chromium.org/104023007/diff/60001/Source/core/html/HTMLCan...
Source/core/html/HTMLCanvasElement.cpp:455: surface = adoptPtr(new
UnacceleratedImageBufferSurface(size(), opacityMode, m_deviceScaleFactor));
I'd prefer a Create() method on the surface subclasses that returns a
PassOwnPtr. Then we don't need to call isValid() or adoptPtr() at the call
sites, making them more robust.

https://codereview.chromium.org/104023007/diff/60001/Source/core/html/canvas/...
File Source/core/html/canvas/WebGLRenderingContext.cpp (right):

https://codereview.chromium.org/104023007/diff/60001/Source/core/html/canvas/...
Source/core/html/canvas/WebGLRenderingContext.cpp:5463: if (!surface->isValid())
Do we need a null ptr check here?

https://codereview.chromium.org/104023007/diff/60001/Source/core/platform/Dra...
File Source/core/platform/DragImage.cpp (right):

https://codereview.chromium.org/104023007/diff/60001/Source/core/platform/Dra...
Source/core/platform/DragImage.cpp:168: if (!surface->isValid())
Same here.

https://codereview.chromium.org/104023007/diff/60001/Source/core/platform/gra...
File Source/core/platform/graphics/Canvas2DLayerBridge.cpp (right):

https://codereview.chromium.org/104023007/diff/60001/Source/core/platform/gra...
Source/core/platform/graphics/Canvas2DLayerBridge.cpp:65: RefPtr<SkSurface>
surface(createSurface(context.get(), size, msaaSampleCount));
Maybe createSurface() should be renamed createSkSurface(), now that we have
ImageBufferSurface to confuse it with.

https://codereview.chromium.org/104023007/diff/60001/Source/core/platform/gra...
File Source/core/platform/graphics/Canvas2DLayerManagerTest.cpp (right):

https://codereview.chromium.org/104023007/diff/60001/Source/core/platform/gra...
Source/core/platform/graphics/Canvas2DLayerManagerTest.cpp:236:
layer->destroy();
As discussed, let's mock out the ImageBufferSurface as well, so it can take care
of cleaning up the layer bridge too.

https://codereview.chromium.org/104023007/diff/60001/Source/core/platform/gra...
File Source/core/platform/graphics/GraphicsContext.cpp (right):

https://codereview.chromium.org/104023007/diff/60001/Source/core/platform/gra...
Source/core/platform/graphics/GraphicsContext.cpp:69: RefPtr<SkBaseDevice>
device = adoptRef(canvas->createCompatibleDevice(SkBitmap::kARGB_8888_Config,
size.width(), size.height(), opacityMode == Opaque));
It's a little confusing to have a canvas that's passed-in, but is not the one
that's stored in m_canvas. I think we should pass in the device instead, and
have the caller call createCompatibleDevice.

https://codereview.chromium.org/104023007/diff/60001/Source/core/platform/gra...
File Source/core/platform/graphics/GraphicsContextTest.cpp (right):

https://codereview.chromium.org/104023007/diff/60001/Source/core/platform/gra...
Source/core/platform/graphics/GraphicsContextTest.cpp:249: ImageBuffer
alphaImage(surface.release());
Ooooh, ImageBuffer on the stack. *mindblown*

https://codereview.chromium.org/104023007/diff/60001/Source/core/platform/gra...
File Source/core/platform/graphics/ImageBuffer.cpp (right):

https://codereview.chromium.org/104023007/diff/60001/Source/core/platform/gra...
Source/core/platform/graphics/ImageBuffer.cpp:60: if (m_surface->isValid() ||
m_surface->canvas()) {
Huh?

https://codereview.chromium.org/104023007/diff/60001/Source/core/platform/gra...
File Source/core/platform/graphics/gpu/AcceleratedImageBufferSurface.h (right):

https://codereview.chromium.org/104023007/diff/60001/Source/core/platform/gra...
Source/core/platform/graphics/gpu/AcceleratedImageBufferSurface.h:42:
AcceleratedImageBufferSurface(const IntSize&, OpacityMode = NonOpaque, float
resolutionScale = 1.0f, int msaaSampleCount = 0);
I'm 99.99% certain we don't support hiDPI canvas. Do we still need
resolutionScale here?

[Justin Novosad, 2013-12-06 21:41:09]

New Patch.

Addresses all review feedback received so far.
Most significant differences:
* Removed notion of device scale factor in ImageBuffer. It was no longer
necessary since we do not use High-def canvases. The few places where the
scaling was needed were easy to refactor to simply apply a scale directly to the
GraphicsContext.
* Added back create methods on ImageBuffer, with a shorthand create for creating
unaccelerated buffers
* Re-based on top of recent CL that moved a lot of files from core/platform to
platform

[Stephen White, 2013-12-09 15:33:07]

https://codereview.chromium.org/104023007/diff/150001/Source/core/frame/Image...
File Source/core/frame/ImageBitmap.cpp (right):

https://codereview.chromium.org/104023007/diff/150001/Source/core/frame/Image...
Source/core/frame/ImageBitmap.cpp:15: #include
"platform/graphics/UnacceleratedImageBufferSurface.h"
Please remove this if it's no longer necessary.

https://codereview.chromium.org/104023007/diff/150001/Source/core/html/HTMLCa...
File Source/core/html/HTMLCanvasElement.cpp (right):

https://codereview.chromium.org/104023007/diff/150001/Source/core/html/HTMLCa...
Source/core/html/HTMLCanvasElement.cpp:434: surface = adoptPtr(new
WebGLImageBufferSurface(size(), opacityMode));
Nit: I'd just do an early-return here, to save indentation below.

https://codereview.chromium.org/104023007/diff/150001/Source/core/html/HTMLCa...
Source/core/html/HTMLCanvasElement.cpp:468: OwnPtr<ImageBufferSurface> surface =
createImageBufferSurface(deviceSize, &msaaSampleCount);
Do we need to check for a NULL surface here? If it fails to allocate, we
probably want to catch that, rather than a NULL ImageBuffer, no? Or does the
check below catch both?

https://codereview.chromium.org/104023007/diff/150001/Source/core/html/HTMLCa...
Source/core/html/HTMLCanvasElement.cpp:477: if (m_imageBuffer->context()) {
Under what conditions does the image buffer have no context? Is that for WebGL?
If so, perhaps we could do an early-return here, since the scheduleLayerUpdate()
below only happens for 2D canvas anyway.

https://codereview.chromium.org/104023007/diff/150001/Source/core/rendering/s...
File Source/core/rendering/svg/RenderSVGResourceFilter.cpp (right):

https://codereview.chromium.org/104023007/diff/150001/Source/core/rendering/s...
Source/core/rendering/svg/RenderSVGResourceFilter.cpp:132: if
(!surface->isValid())
NULL check here as well?

https://codereview.chromium.org/104023007/diff/150001/Source/platform/graphic...
File Source/platform/graphics/Canvas2DImageBufferSurface.h (right):

https://codereview.chromium.org/104023007/diff/150001/Source/platform/graphic...
Source/platform/graphics/Canvas2DImageBufferSurface.h:52:
m_layerBridge->destroy();
Not new to this patch, but since this doesn't actually destroy the bridge, just
renders it "inoperable", maybe it should have a different name?

https://codereview.chromium.org/104023007/diff/150001/Source/platform/graphic...
Source/platform/graphics/Canvas2DImageBufferSurface.h:56: virtual void
aboutToUse() OVERRIDE { m_layerBridge->aboutToUse(); }
Things like this are usually "willUse()" in Blink.

https://codereview.chromium.org/104023007/diff/150001/Source/platform/graphic...
File Source/platform/graphics/UnacceleratedImageBufferSurface.h (right):

https://codereview.chromium.org/104023007/diff/150001/Source/platform/graphic...
Source/platform/graphics/UnacceleratedImageBufferSurface.h:46: virtual bool
isValid() const OVERRIDE { return m_canvas; }
Still not fond of an object that can be in an invalid state, but my
understanding is that these semantics are necessary for the mailbox
implementation in the layer bridge.

[Justin Novosad, 2013-12-09 16:37:30]

https://codereview.chromium.org/104023007/diff/150001/Source/core/html/HTMLCa...
File Source/core/html/HTMLCanvasElement.cpp (right):

https://codereview.chromium.org/104023007/diff/150001/Source/core/html/HTMLCa...
Source/core/html/HTMLCanvasElement.cpp:468: OwnPtr<ImageBufferSurface> surface =
createImageBufferSurface(deviceSize, &msaaSampleCount);
On 2013/12/09 15:33:08, Stephen White wrote:
> Do we need to check for a NULL surface here? If it fails to allocate, we
> probably want to catch that, rather than a NULL ImageBuffer, no? Or does the
> check below catch both?
It was caught bey the check below, but I'll move the check to just check
surface->isValid(), because that is the one that can fail (without crashing).

https://codereview.chromium.org/104023007/diff/150001/Source/core/rendering/s...
File Source/core/rendering/svg/RenderSVGResourceFilter.cpp (right):

https://codereview.chromium.org/104023007/diff/150001/Source/core/rendering/s...
Source/core/rendering/svg/RenderSVGResourceFilter.cpp:132: if
(!surface->isValid())
On 2013/12/09 15:33:08, Stephen White wrote:
> NULL check here as well?

Not necessary. If the above 'new' fails, an OOM exception will crash the
process.  The previous 'if' needed a NULL ptr check for the case where
'accelerated' is false.  For clarity, I'll re-write it as if (!accelerated ||
!surface->isValid())

https://codereview.chromium.org/104023007/diff/150001/Source/platform/graphic...
File Source/platform/graphics/Canvas2DImageBufferSurface.h (right):

https://codereview.chromium.org/104023007/diff/150001/Source/platform/graphic...
Source/platform/graphics/Canvas2DImageBufferSurface.h:56: virtual void
aboutToUse() OVERRIDE { m_layerBridge->aboutToUse(); }
On 2013/12/09 15:33:08, Stephen White wrote:
> Things like this are usually "willUse()" in Blink.
Ah yes, 'aboutTo' is a skia-ism. Thanks.

https://codereview.chromium.org/104023007/diff/150001/Source/platform/graphic...
File Source/platform/graphics/UnacceleratedImageBufferSurface.h (right):

https://codereview.chromium.org/104023007/diff/150001/Source/platform/graphic...
Source/platform/graphics/UnacceleratedImageBufferSurface.h:46: virtual bool
isValid() const OVERRIDE { return m_canvas; }
On 2013/12/09 15:33:08, Stephen White wrote:
> Still not fond of an object that can be in an invalid state, but my
> understanding is that these semantics are necessary for the mailbox
> implementation in the layer bridge.

Actually, it is so that we can handle asynchronous graphics context losses.

[Justin Novosad, 2013-12-09 19:52:27]

New patch. Applied corrections + fix for mac build

[Stephen White, 2013-12-09 20:47:44]

LGTM

https://codereview.chromium.org/104023007/diff/210001/Source/platform/graphic...
File Source/platform/graphics/Canvas2DImageBufferSurface.h (right):

https://codereview.chromium.org/104023007/diff/210001/Source/platform/graphic...
Source/platform/graphics/Canvas2DImageBufferSurface.h:52:
m_layerBridge->beginDestruction();
Naming nit:  better, but maybe removeFromCompositor()? (Can be another CL).

[Justin Novosad, 2013-12-09 21:18:44]

On 2013/12/09 20:47:44, Stephen White wrote:
> LGTM
> 
>
https://codereview.chromium.org/104023007/diff/210001/Source/platform/graphic...
> File Source/platform/graphics/Canvas2DImageBufferSurface.h (right):
> 
>
https://codereview.chromium.org/104023007/diff/210001/Source/platform/graphic...
> Source/platform/graphics/Canvas2DImageBufferSurface.h:52:
> m_layerBridge->beginDestruction();
> Naming nit:  better, but maybe removeFromCompositor()? (Can be another CL).

Yeah another CL 'cause we'd also want to change the names of the functions and
data members used by that method away from the destruction theme.

[Justin Novosad, 2013-12-09 21:19:58]

Still need approval from an OWNER of Source/web

[eseidel, 2013-12-09 21:42:19]

web/ changes lgtm.

[commit-bot: I haz the power, 2013-12-09 21:48:25]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/junov@chromium.org/104023007/210001

[commit-bot: I haz the power, 2013-12-09 21:48:40]

Failed to apply patch for
Source/core/platform/graphics/CrossfadeGeneratedImage.cpp:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  can't find file to patch at input line 6
  Perhaps you used the wrong -p or --strip option?
  The text leading up to this was:
  --------------------------
  |Index: Source/core/platform/graphics/CrossfadeGeneratedImage.cpp
  |diff --git a/Source/core/platform/graphics/CrossfadeGeneratedImage.cpp
b/Source/core/platform/graphics/CrossfadeGeneratedImage.cpp
  |index
bff62eaa18fae87b2a787b4e68d10fe2872528e6..191a0d5987bb99caa922cbf651661522688c4b2d
100644
  |--- a/Source/core/platform/graphics/CrossfadeGeneratedImage.cpp
  |+++ b/Source/core/platform/graphics/CrossfadeGeneratedImage.cpp
  --------------------------
  No file to patch.  Skipping patch.
  1 out of 1 hunk ignored

Patch:       Source/core/platform/graphics/CrossfadeGeneratedImage.cpp
Index: Source/core/platform/graphics/CrossfadeGeneratedImage.cpp
diff --git a/Source/core/platform/graphics/CrossfadeGeneratedImage.cpp
b/Source/core/platform/graphics/CrossfadeGeneratedImage.cpp
index
bff62eaa18fae87b2a787b4e68d10fe2872528e6..191a0d5987bb99caa922cbf651661522688c4b2d
100644
--- a/Source/core/platform/graphics/CrossfadeGeneratedImage.cpp
+++ b/Source/core/platform/graphics/CrossfadeGeneratedImage.cpp
@@ -95,7 +95,7 @@ void CrossfadeGeneratedImage::draw(GraphicsContext* context,
const FloatRect& ds
 
 void CrossfadeGeneratedImage::drawPattern(GraphicsContext* context, const
FloatRect& srcRect, const FloatSize& scale, const FloatPoint& phase,
CompositeOperator compositeOp, const FloatRect& dstRect, blink::WebBlendMode
blendMode, const IntSize& repeatSpacing)
 {
-    OwnPtr<ImageBuffer> imageBuffer = ImageBuffer::create(m_size, 1,
context->isAccelerated() ? Accelerated : Unaccelerated);
+    OwnPtr<ImageBuffer> imageBuffer = context->createCompatibleBuffer(m_size);
     if (!imageBuffer)
         return;

[commit-bot: I haz the power, 2013-12-09 22:22:33]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/junov@chromium.org/104023007/230001

[commit-bot: I haz the power, 2013-12-10 01:28:08]

Retried try job too often on linux_blink for step(s) webkit_tests,
webkit_unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_blin...

[commit-bot: I haz the power, 2013-12-10 16:40:13]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/junov@chromium.org/104023007/310001

[commit-bot: I haz the power, 2013-12-10 17:03:39]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/junov@chromium.org/104023007/330001

[commit-bot: I haz the power, 2013-12-10 18:01:51]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/junov@chromium.org/104023007/10044

[commit-bot: I haz the power, 2013-12-10 18:02:26]

Failed to apply patch for Source/platform/graphics/ImageBufferSurface.cpp:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  A         Source/platform/graphics/ImageBufferSurface.cpp
  Copied Source/core/rendering/LayoutRectRecorder.h ->
Source/platform/graphics/ImageBufferSurface.cpp
  patching file Source/platform/graphics/ImageBufferSurface.cpp
  Hunk #1 FAILED at 28.
  1 out of 1 hunk FAILED -- saving rejects to file
Source/platform/graphics/ImageBufferSurface.cpp.rej

Patch:   NR 
Source/core/rendering/LayoutRectRecorder.h->Source/platform/graphics/ImageBufferSurface.cpp
Index: Source/platform/graphics/ImageBufferSurface.cpp
diff --git a/Source/core/rendering/LayoutRectRecorder.h
b/Source/platform/graphics/ImageBufferSurface.cpp
similarity index 67%
copy from Source/core/rendering/LayoutRectRecorder.h
copy to Source/platform/graphics/ImageBufferSurface.cpp
index
9fd331e2aca95cb262bbbd4e738d3b78286caa2d..c5e22dbdd036ea8fb2057d573de8ed9d25f96f95
100644
--- a/Source/core/rendering/LayoutRectRecorder.h
+++ b/Source/platform/graphics/ImageBufferSurface.cpp
@@ -28,32 +28,33 @@
  * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
  */
 
-#ifndef LayoutRectRecorder_h
-#define LayoutRectRecorder_h
+#include "config.h"
 
-#include "platform/geometry/LayoutRect.h"
-#include "wtf/Noncopyable.h"
+#include "platform/graphics/ImageBufferSurface.h"
 
-namespace WebCore {
-
-class RenderLayerModelObject;
-class RenderObject;
+#include "third_party/skia/include/core/SkCanvas.h"
+#include "third_party/skia/include/core/SkDevice.h"
 
-class LayoutRectRecorder {
-    WTF_MAKE_NONCOPYABLE(LayoutRectRecorder);
-public:
-    LayoutRectRecorder(RenderObject&, bool skipRecording = false);
-    ~LayoutRectRecorder();
+namespace WebCore {
 
-    static bool shouldRecordLayoutRects();
+void ImageBufferSurface::clear()
+{
+    // Clear the background transparent or opaque, as required. It would be
nice if this wasn't
+    // required, but the canvas is currently filled with the magic transparency
+    // color. Can we have another way to manage this?
+    if (isValid()) {
+        if (m_opacityMode == Opaque)
+            canvas()->drawARGB(255, 0, 0, 0, SkXfermode::kSrc_Mode);
+        else
+            canvas()->drawARGB(0, 0, 0, 0, SkXfermode::kClear_Mode);
+    }
+}
+
+const SkBitmap& ImageBufferSurface::bitmap() const
+{
+    ASSERT(canvas());
+    return canvas()->getTopDevice()->accessBitmap(false);
+}
 
-private:
-    RenderObject& m_object;
-    RenderLayerModelObject* m_repaintContainer;
-    unsigned m_skipRecording : 1;
-};
 
 } // namespace WebCore
-
-#endif // LayoutRectRecorder_h
-

[commit-bot: I haz the power, 2013-12-10 18:08:06]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/junov@chromium.org/104023007/380001

[Justin Novosad, 2013-12-10 18:25:45]

Gah! This patch is too big. I am stuck in an infinite loop of rebase+merge+fix
errors

[commit-bot: I haz the power, 2013-12-10 18:38:16]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/junov@chromium.org/104023007/400001

[commit-bot: I haz the power, 2013-12-10 19:33:12]

Retried try job too often on mac_blink_rel for step(s) blink_platform_unittests,
webkit_lint, webkit_python_tests, webkit_tests, webkit_unit_tests, wtf_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_blink_...

[commit-bot: I haz the power, 2013-12-10 20:26:10]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/junov@chromium.org/104023007/420001

[commit-bot: I haz the power, 2013-12-10 22:08:54]

Change committed as 163572