Allow DrawingBuffer to present to a bitmap.

Allow DrawingBuffer to present to a bitmap.

The WebExternalTextureLayer can now ask the DrawingBuffer to present to a bitmap in a mailbox, which will be used with the software renderer and webgl.

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

[jbauman, 2013-06-26 11:33:53]

Ken, do you think this approach makes sense? This goes with
https://codereview.chromium.org/17859002/

[Ken Russell (switch to Gerrit), 2013-06-27 13:41:01]

The overall approach LGTM. I would have used a different argument than a bool
but it's fine. A couple of points.

https://codereview.chromium.org/17859003/diff/1/Source/core/platform/graphics...
File Source/core/platform/graphics/chromium/Canvas2DLayerBridge.cpp (right):

https://codereview.chromium.org/17859003/diff/1/Source/core/platform/graphics...
Source/core/platform/graphics/chromium/Canvas2DLayerBridge.cpp:175:
ASSERT(!bitmap);
How will this work? Is there a guarantee somewhere that we won't request
hardware accelerated 2D canvas if the software renderer is used?

https://codereview.chromium.org/17859003/diff/1/Source/core/platform/graphics...
File Source/core/platform/graphics/gpu/DrawingBuffer.cpp (right):

https://codereview.chromium.org/17859003/diff/1/Source/core/platform/graphics...
Source/core/platform/graphics/gpu/DrawingBuffer.cpp:203:
m_context->readPixels(0, 0, size().width(), size().height(),
GraphicsContext3D::RGBA, GraphicsContext3D::UNSIGNED_BYTE, pixels);
We'd better be careful that the user hasn't set the unpack alignment to 8.

https://codereview.chromium.org/17859003/diff/1/Source/core/platform/graphics...
Source/core/platform/graphics/gpu/DrawingBuffer.cpp:205: swap(pixels[i + 0],
pixels[i + 2]);
I'm pretty sure this needs to be conditional depending on the platform. See
GraphicsContext3D::paintRenderingResultsToImageData . If there's a possibility
for code reuse that would be better.

[jbauman, 2013-07-01 22:27:39]

PTAL. I've added WebExternalBitmap, which is used to encapsulate a
base::SharedMemory for this. Also, I've moved the implementation of
readBackFramebuffer from WebGraphicsContext into GraphicsContext3D, and used it
in this case as well.

On 2013/06/27 13:41:01, Ken Russell wrote:
> The overall approach LGTM. I would have used a different argument than a bool
> but it's fine. A couple of points.
> 
>
https://codereview.chromium.org/17859003/diff/1/Source/core/platform/graphics...
> File Source/core/platform/graphics/chromium/Canvas2DLayerBridge.cpp (right):
> 
>
https://codereview.chromium.org/17859003/diff/1/Source/core/platform/graphics...
> Source/core/platform/graphics/chromium/Canvas2DLayerBridge.cpp:175:
> ASSERT(!bitmap);
> How will this work? Is there a guarantee somewhere that we won't request
> hardware accelerated 2D canvas if the software renderer is used?
> 
>
https://codereview.chromium.org/17859003/diff/1/Source/core/platform/graphics...
> File Source/core/platform/graphics/gpu/DrawingBuffer.cpp (right):
> 
>
https://codereview.chromium.org/17859003/diff/1/Source/core/platform/graphics...
> Source/core/platform/graphics/gpu/DrawingBuffer.cpp:203:
> m_context->readPixels(0, 0, size().width(), size().height(),
> GraphicsContext3D::RGBA, GraphicsContext3D::UNSIGNED_BYTE, pixels);
> We'd better be careful that the user hasn't set the unpack alignment to 8.
> 
>
https://codereview.chromium.org/17859003/diff/1/Source/core/platform/graphics...
> Source/core/platform/graphics/gpu/DrawingBuffer.cpp:205: swap(pixels[i + 0],
> pixels[i + 2]);
> I'm pretty sure this needs to be conditional depending on the platform. See
> GraphicsContext3D::paintRenderingResultsToImageData . If there's a possibility
> for code reuse that would be better.

[Ken Russell (switch to Gerrit), 2013-07-02 11:22:12]

LGTM. Couple of further questions.

Just checking, will the Chromium-side implementations of
WebGraphicsContext3D::readBackFramebuffer be deleted after this rolls in?

https://codereview.chromium.org/17859003/diff/5001/Source/core/platform/graph...
File Source/core/platform/graphics/GraphicsContext3D.cpp (right):

https://codereview.chromium.org/17859003/diff/5001/Source/core/platform/graph...
Source/core/platform/graphics/GraphicsContext3D.cpp:953:
m_impl->bindFramebuffer(FRAMEBUFFER, framebuffer);
Does the previous framebuffer binding need to be restored after the readback?

https://codereview.chromium.org/17859003/diff/5001/public/platform/WebExterna...
File public/platform/WebExternalBitmap.h (right):

https://codereview.chromium.org/17859003/diff/5001/public/platform/WebExterna...
public/platform/WebExternalBitmap.h:41: virtual void setSize(WebSize) = 0;
Probably should document that this invalidates any cached copies of the pointer.

https://codereview.chromium.org/17859003/diff/5001/public/platform/WebExterna...
public/platform/WebExternalBitmap.h:42: virtual void* getPointer() = 0;
Would it be better to have lock/unlock semantics for access to the pointer, like
SkBitmap's lockPixels/unlockPixels?

https://codereview.chromium.org/17859003/diff/5001/public/platform/WebExterna...
File public/platform/WebExternalTextureLayerClient.h (right):

https://codereview.chromium.org/17859003/diff/5001/public/platform/WebExterna...
public/platform/WebExternalTextureLayerClient.h:55: // OBSOLETE.
Could you please add a FIXME about removing it, pointing to this bug?

[jbauman, 2013-07-02 19:43:31]

Yeah, I'll delete the chromium implementation of
WebGraphicsContext3D::readBackFramebuffer when this is in.

https://codereview.chromium.org/17859003/diff/5001/Source/core/platform/graph...
File Source/core/platform/graphics/GraphicsContext3D.cpp (right):

https://codereview.chromium.org/17859003/diff/5001/Source/core/platform/graph...
Source/core/platform/graphics/GraphicsContext3D.cpp:953:
m_impl->bindFramebuffer(FRAMEBUFFER, framebuffer);
On 2013/07/02 11:22:12, Ken Russell wrote:
> Does the previous framebuffer binding need to be restored after the readback?

Everything that calls this already restores the framebuffer bindings.

https://codereview.chromium.org/17859003/diff/5001/public/platform/WebExterna...
File public/platform/WebExternalBitmap.h (right):

https://codereview.chromium.org/17859003/diff/5001/public/platform/WebExterna...
public/platform/WebExternalBitmap.h:41: virtual void setSize(WebSize) = 0;
On 2013/07/02 11:22:12, Ken Russell wrote:
> Probably should document that this invalidates any cached copies of the
pointer.

Ok.

https://codereview.chromium.org/17859003/diff/5001/public/platform/WebExterna...
public/platform/WebExternalBitmap.h:42: virtual void* getPointer() = 0;
On 2013/07/02 11:22:12, Ken Russell wrote:
> Would it be better to have lock/unlock semantics for access to the pointer,
like
> SkBitmap's lockPixels/unlockPixels?

No, unlock wouldn't do anything, so that would just be confusing.

https://codereview.chromium.org/17859003/diff/5001/public/platform/WebExterna...
File public/platform/WebExternalTextureLayerClient.h (right):

https://codereview.chromium.org/17859003/diff/5001/public/platform/WebExterna...
public/platform/WebExternalTextureLayerClient.h:55: // OBSOLETE.
On 2013/07/02 11:22:12, Ken Russell wrote:
> Could you please add a FIXME about removing it, pointing to this bug?

Ok, done.

[jbauman, 2013-07-02 21:07:08]

Jochen, could you review the public/ portions of this?

[jochen (gone - plz use gerrit), 2013-07-02 22:37:01]

I'd recommend asking James to have a look at this, as he's more familiar with
drawing

https://codereview.chromium.org/17859003/diff/13001/public/platform/WebExtern...
File public/platform/WebExternalBitmap.h (right):

https://codereview.chromium.org/17859003/diff/13001/public/platform/WebExtern...
public/platform/WebExternalBitmap.h:34: #include "WebSize.h"
I think it should be enough to forward declare WebSize?

https://codereview.chromium.org/17859003/diff/13001/public/platform/WebExtern...
public/platform/WebExternalBitmap.h:43: // invalidate the pointer returned from
getPointer if the size changes.
how will you guarantee this (given that this is a pure virtual interface?)

This API looks very opaque to me, I'm not sure whether that's the best solution

https://codereview.chromium.org/17859003/diff/13001/public/platform/WebExtern...
File public/platform/WebExternalTextureLayerClient.h (right):

https://codereview.chromium.org/17859003/diff/13001/public/platform/WebExtern...
public/platform/WebExternalTextureLayerClient.h:53: virtual bool
prepareMailbox(WebExternalTextureMailbox*, WebExternalBitmap*) = 0;
nit. WebExternalBitmap* = 0, then you don't need the overloaded method

[jbauman, 2013-07-02 23:14:29]

Jamesr@, could you review the public/ parts of this?

[jamesr, 2013-07-03 21:53:01]

https://codereview.chromium.org/17859003/diff/20001/Source/core/platform/grap...
File Source/core/platform/graphics/GraphicsContext3D.cpp (right):

https://codereview.chromium.org/17859003/diff/20001/Source/core/platform/grap...
Source/core/platform/graphics/GraphicsContext3D.cpp:632: unsigned bufferSize = 4
* width * height;
this should probably be size_t

https://codereview.chromium.org/17859003/diff/20001/public/platform/WebExtern...
File public/platform/WebExternalBitmap.h (right):

https://codereview.chromium.org/17859003/diff/20001/public/platform/WebExtern...
public/platform/WebExternalBitmap.h:36: struct WebSize;
since you're returning WebSize by value, i think you need to #include the header
that defines it since anyone who uses this header will need to see the
definition of WebSize anyway

https://codereview.chromium.org/17859003/diff/20001/public/platform/WebExtern...
public/platform/WebExternalBitmap.h:40: virtual WebSize getSize() = 0;
no 'get' prefix needed for this. just size().

https://codereview.chromium.org/17859003/diff/20001/public/platform/WebExtern...
public/platform/WebExternalBitmap.h:46: virtual void* getPointer() = 0;
what is the type of this? is it a pointer into the pixel buffer? that should
probably be uint8* or something similar.

we very very rarely use the 'get' prefix in blink code. i think this should be
named after what it points to

https://codereview.chromium.org/17859003/diff/20001/public/platform/WebExtern...
File public/platform/WebExternalTextureLayerClient.h (right):

https://codereview.chromium.org/17859003/diff/20001/public/platform/WebExtern...
public/platform/WebExternalTextureLayerClient.h:53: virtual bool
prepareMailbox(WebExternalTextureMailbox*, WebExternalBitmap *output_bitmap = 0)
= 0;
* goes with the type, not the parameter name. this parameter name isn't too
useful so blink style says you should omit it.

please update the comments for prepareMailbox() to say what happens if the
second parameter isn't null

[jbauman, 2013-07-03 23:16:44]

Ok, everything should be fixed. PTAL.

On 2013/07/03 21:53:01, jamesr wrote:
>
https://codereview.chromium.org/17859003/diff/20001/Source/core/platform/grap...
> File Source/core/platform/graphics/GraphicsContext3D.cpp (right):
> 
>
https://codereview.chromium.org/17859003/diff/20001/Source/core/platform/grap...
> Source/core/platform/graphics/GraphicsContext3D.cpp:632: unsigned bufferSize =
4
> * width * height;
> this should probably be size_t
> 
>
https://codereview.chromium.org/17859003/diff/20001/public/platform/WebExtern...
> File public/platform/WebExternalBitmap.h (right):
> 
>
https://codereview.chromium.org/17859003/diff/20001/public/platform/WebExtern...
> public/platform/WebExternalBitmap.h:36: struct WebSize;
> since you're returning WebSize by value, i think you need to #include the
header
> that defines it since anyone who uses this header will need to see the
> definition of WebSize anyway
> 
>
https://codereview.chromium.org/17859003/diff/20001/public/platform/WebExtern...
> public/platform/WebExternalBitmap.h:40: virtual WebSize getSize() = 0;
> no 'get' prefix needed for this. just size().
> 
>
https://codereview.chromium.org/17859003/diff/20001/public/platform/WebExtern...
> public/platform/WebExternalBitmap.h:46: virtual void* getPointer() = 0;
> what is the type of this? is it a pointer into the pixel buffer? that should
> probably be uint8* or something similar.
> 
> we very very rarely use the 'get' prefix in blink code. i think this should be
> named after what it points to
> 
>
https://codereview.chromium.org/17859003/diff/20001/public/platform/WebExtern...
> File public/platform/WebExternalTextureLayerClient.h (right):
> 
>
https://codereview.chromium.org/17859003/diff/20001/public/platform/WebExtern...
> public/platform/WebExternalTextureLayerClient.h:53: virtual bool
> prepareMailbox(WebExternalTextureMailbox*, WebExternalBitmap *output_bitmap =
0)
> = 0;
> * goes with the type, not the parameter name. this parameter name isn't too
> useful so blink style says you should omit it.
> 
> please update the comments for prepareMailbox() to say what happens if the
> second parameter isn't null

[jamesr, 2013-07-03 23:18:56]

/public/ lgtm with a couple comment nits

https://codereview.chromium.org/17859003/diff/29001/public/platform/WebExtern...
File public/platform/WebExternalBitmap.h (right):

https://codereview.chromium.org/17859003/diff/29001/public/platform/WebExtern...
public/platform/WebExternalBitmap.h:46: virtual uint8 *pixels() = 0;
* goes with the type, so "virtual uint8* pixels()". probably worth documenting
the size of this buffer (i'm guessing size().width * size.height() * 4, but it's
worth being explicit) and the format

https://codereview.chromium.org/17859003/diff/29001/public/platform/WebExtern...
File public/platform/WebExternalTextureLayerClient.h (right):

https://codereview.chromium.org/17859003/diff/29001/public/platform/WebExtern...
public/platform/WebExternalTextureLayerClient.h:53: // If the WebExternalBitmap
isn't 0, then it should be filled in with the contents of this frame.
is that in addition to the mailbox? what's the return value?

[commit-bot: I haz the power, 2013-07-08 23:44:40]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jbauman@chromium.org/17859003/22002

[commit-bot: I haz the power, 2013-07-09 01:33:41]

Change committed as 153737