android_webview: changes to support Android GraphicBuffers

android_webview: changes to support Android GraphicBuffers


BUG=175012
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=192882

[kaanb, 2013-03-29 04:20:23]

This will be the first patch to land for the Android GraphicBuffer support in
Chromium. It is a subset of the patch that was under review for a while:
https://codereview.chromium.org/12386027/

[joth, 2013-04-02 19:12:11]

https://codereview.chromium.org/13135004/diff/9001/android_webview/browser/gr...
File android_webview/browser/graphic_buffer_factory_impl.h (right):

https://codereview.chromium.org/13135004/diff/9001/android_webview/browser/gr...
android_webview/browser/graphic_buffer_factory_impl.h:1: // Copyright (c) 2013
The Chromium Authors. All rights reserved.
drop (c) in all the new files (like you did in the cc/ file already)

https://codereview.chromium.org/13135004/diff/9001/android_webview/common/aw_...
File android_webview/common/aw_switches.cc (right):

https://codereview.chromium.org/13135004/diff/9001/android_webview/common/aw_...
android_webview/common/aw_switches.cc:12: const char kUseZeroCopyBuffers[] =
"use-zero-copy-buffers";
right now setting this flag would do nothing, right?
The point of this is, after the next patch lands, then setting the graphic
buffer factory will unconditionally push us into gralloc mode and so we need
this flag to guard that?

https://codereview.chromium.org/13135004/diff/9001/android_webview/renderer/a...
File android_webview/renderer/aw_content_renderer_client.cc (right):

https://codereview.chromium.org/13135004/diff/9001/android_webview/renderer/a...
android_webview/renderer/aw_content_renderer_client.cc:116:
switches::kUseZeroCopyBuffers)
rather than check the switch here, I'd do it once-off at the point where we set
the factory pointer. (Although it doesn't make major difference either way as
we're going to remove this switch eventually)

https://codereview.chromium.org/13135004/diff/9001/android_webview/renderer/a...
File android_webview/renderer/aw_content_renderer_client.h (right):

https://codereview.chromium.org/13135004/diff/9001/android_webview/renderer/a...
android_webview/renderer/aw_content_renderer_client.h:46: virtual
cc::GraphicBuffer::Factory GetGraphicBufferFactory() const OVERRIDE;
you need to add this to the base class too. Otherwise the OVERRIDE will cause
build failure.

https://codereview.chromium.org/13135004/diff/9001/cc/resources/graphic_buffer.h
File cc/resources/graphic_buffer.h (right):

https://codereview.chromium.org/13135004/diff/9001/cc/resources/graphic_buffe...
cc/resources/graphic_buffer.h:20: class GraphicBuffer {
can we make and statements about the thread safety of this class?

https://codereview.chromium.org/13135004/diff/9001/cc/resources/graphic_buffe...
cc/resources/graphic_buffer.h:22: // This factory is called in
ResourceProvider::CreateGraphicBuffer() to
this comment is incorrect w.r.t this patch in isolation. (no other cc/ code is
using GraphicBuffer at all).

Also, rater than where it's used from, I'm more interested in what is the public
API I tell cc/ which factory it should be using. e.g. Is there a setter
somewhere?

https://codereview.chromium.org/13135004/diff/9001/cc/resources/graphic_buffe...
cc/resources/graphic_buffer.h:33: // Unlocks the buffer. Returns 0 on success,
non-zero otherwise.
Also document:-
1/ can LockForRead be followed by LockForWrite (lock upgrade) or is Unlock the
only valid call after either Lock?
2/ if you can upgrade the lock, how many Unlock callss are needed?
3/ is it ok to call Unlock() even when it's not locked?
4/ is it Ok to destruct while still locked.

[kaanb, 2013-04-02 23:16:54]

https://codereview.chromium.org/13135004/diff/9001/android_webview/browser/gr...
File android_webview/browser/graphic_buffer_factory_impl.h (right):

https://codereview.chromium.org/13135004/diff/9001/android_webview/browser/gr...
android_webview/browser/graphic_buffer_factory_impl.h:1: // Copyright (c) 2013
The Chromium Authors. All rights reserved.
On 2013/04/02 19:12:11, joth wrote:
> drop (c) in all the new files (like you did in the cc/ file already)

Done.

https://codereview.chromium.org/13135004/diff/9001/android_webview/common/aw_...
File android_webview/common/aw_switches.cc (right):

https://codereview.chromium.org/13135004/diff/9001/android_webview/common/aw_...
android_webview/common/aw_switches.cc:12: const char kUseZeroCopyBuffers[] =
"use-zero-copy-buffers";
On 2013/04/02 19:12:11, joth wrote:
> right now setting this flag would do nothing, right?
> The point of this is, after the next patch lands, then setting the graphic
> buffer factory will unconditionally push us into gralloc mode and so we need
> this flag to guard that?

Yes. Also it's recommended practice to guard any new feature with a flag.

https://codereview.chromium.org/13135004/diff/9001/android_webview/renderer/a...
File android_webview/renderer/aw_content_renderer_client.cc (right):

https://codereview.chromium.org/13135004/diff/9001/android_webview/renderer/a...
android_webview/renderer/aw_content_renderer_client.cc:116:
switches::kUseZeroCopyBuffers)
On 2013/04/02 19:12:11, joth wrote:
> rather than check the switch here, I'd do it once-off at the point where we
set
> the factory pointer. (Although it doesn't make major difference either way as
> we're going to remove this switch eventually)

Done.

https://codereview.chromium.org/13135004/diff/9001/android_webview/renderer/a...
File android_webview/renderer/aw_content_renderer_client.h (right):

https://codereview.chromium.org/13135004/diff/9001/android_webview/renderer/a...
android_webview/renderer/aw_content_renderer_client.h:46: virtual
cc::GraphicBuffer::Factory GetGraphicBufferFactory() const OVERRIDE;
On 2013/04/02 19:12:11, joth wrote:
> you need to add this to the base class too. Otherwise the OVERRIDE will cause
> build failure.

I'd prefer not to add new files to this patch, so I removed the OVERRIDE.

https://codereview.chromium.org/13135004/diff/9001/cc/resources/graphic_buffer.h
File cc/resources/graphic_buffer.h (right):

https://codereview.chromium.org/13135004/diff/9001/cc/resources/graphic_buffe...
cc/resources/graphic_buffer.h:20: class GraphicBuffer {
On 2013/04/02 19:12:11, joth wrote:
> can we make and statements about the thread safety of this class?

Done.

https://codereview.chromium.org/13135004/diff/9001/cc/resources/graphic_buffe...
cc/resources/graphic_buffer.h:22: // This factory is called in
ResourceProvider::CreateGraphicBuffer() to
On 2013/04/02 19:12:11, joth wrote:
> this comment is incorrect w.r.t this patch in isolation. (no other cc/ code is
> using GraphicBuffer at all).
> 
> Also, rater than where it's used from, I'm more interested in what is the
public
> API I tell cc/ which factory it should be using. e.g. Is there a setter
> somewhere?

Done.

https://codereview.chromium.org/13135004/diff/9001/cc/resources/graphic_buffe...
cc/resources/graphic_buffer.h:33: // Unlocks the buffer. Returns 0 on success,
non-zero otherwise.
On 2013/04/02 19:12:11, joth wrote:
> Also document:-
> 1/ can LockForRead be followed by LockForWrite (lock upgrade) or is Unlock the
> only valid call after either Lock?
> 2/ if you can upgrade the lock, how many Unlock callss are needed?
> 3/ is it ok to call Unlock() even when it's not locked?
> 4/ is it Ok to destruct while still locked.
> 
> 

Actually the Lock() is somewhat of a misnomer. Looks like the rest of the
compositor uses Map/Unmap for the same concept. There aren't any mutex style
synchronization primitives involved in these operations. I added more comments
on these methods though.

[jamesr, 2013-04-03 03:14:37]

https://codereview.chromium.org/13135004/diff/17001/cc/resources/graphic_buff...
File cc/resources/graphic_buffer.h (right):

https://codereview.chromium.org/13135004/diff/17001/cc/resources/graphic_buff...
cc/resources/graphic_buffer.h:35: typedef
base::Callback<scoped_ptr<cc::GraphicBuffer>(const gfx::Size&)>
gfx::Size should be passed by value, not const ref. That might make the line
wrapping easier

https://codereview.chromium.org/13135004/diff/17001/cc/resources/graphic_buff...
cc/resources/graphic_buffer.h:36: Factory;
call this Create, not Factory

https://codereview.chromium.org/13135004/diff/17001/cc/resources/graphic_buff...
cc/resources/graphic_buffer.h:39: // Locks the buffer for the usage specified so
the client
if these are analogous to map/unmap in other parts of the code, why aren't they
just called map/unmap?

https://codereview.chromium.org/13135004/diff/17001/cc/resources/graphic_buff...
cc/resources/graphic_buffer.h:43: // Returns true on success, false on failure.
returning a bool on this API is useless unless the user of this API can respond
in a useful way to an error.  If the only useful thing a caller can do is
DCHECK, you should that check further up the stack.

https://codereview.chromium.org/13135004/diff/17001/cc/resources/graphic_buff...
cc/resources/graphic_buffer.h:44: virtual bool LockForRead(void** vaddr) = 0;
LockForRead() isn't used at all in the big patch and I can't imagine how it
would be.  Drop it if unless there's a need for this.

https://codereview.chromium.org/13135004/diff/17001/cc/resources/graphic_buff...
cc/resources/graphic_buffer.h:46: // Unlocks the buffer. Returns true on
success, false otherwise.
leave a blank line between functions.

how could unlock fail?  What would that even mean?

https://codereview.chromium.org/13135004/diff/17001/cc/resources/graphic_buff...
cc/resources/graphic_buffer.h:48: // Returns the size of the buffer
do you need a size getter?  whoever allocates this GraphicsBuffer must know the
size of the buffer created and probably has to store the size anyway, so this
seems redundant.

[kaanb, 2013-04-03 04:39:53]

https://codereview.chromium.org/13135004/diff/17001/cc/resources/graphic_buff...
File cc/resources/graphic_buffer.h (right):

https://codereview.chromium.org/13135004/diff/17001/cc/resources/graphic_buff...
cc/resources/graphic_buffer.h:35: typedef
base::Callback<scoped_ptr<cc::GraphicBuffer>(const gfx::Size&)>
On 2013/04/03 03:14:38, jamesr wrote:
> gfx::Size should be passed by value, not const ref. That might make the line
> wrapping easier

Done.

https://codereview.chromium.org/13135004/diff/17001/cc/resources/graphic_buff...
cc/resources/graphic_buffer.h:36: Factory;
On 2013/04/03 03:14:38, jamesr wrote:
> call this Create, not Factory

Done.

https://codereview.chromium.org/13135004/diff/17001/cc/resources/graphic_buff...
cc/resources/graphic_buffer.h:39: // Locks the buffer for the usage specified so
the client
On 2013/04/03 03:14:38, jamesr wrote:
> if these are analogous to map/unmap in other parts of the code, why aren't
they
> just called map/unmap?

Done.

https://codereview.chromium.org/13135004/diff/17001/cc/resources/graphic_buff...
cc/resources/graphic_buffer.h:43: // Returns true on success, false on failure.
On 2013/04/03 03:14:38, jamesr wrote:
> returning a bool on this API is useless unless the user of this API can
respond
> in a useful way to an error.  If the only useful thing a caller can do is
> DCHECK, you should that check further up the stack.

Done.

https://codereview.chromium.org/13135004/diff/17001/cc/resources/graphic_buff...
cc/resources/graphic_buffer.h:44: virtual bool LockForRead(void** vaddr) = 0;
On 2013/04/03 03:14:38, jamesr wrote:
> LockForRead() isn't used at all in the big patch and I can't imagine how it
> would be.  Drop it if unless there's a need for this.

Done.

https://codereview.chromium.org/13135004/diff/17001/cc/resources/graphic_buff...
cc/resources/graphic_buffer.h:44: virtual bool LockForRead(void** vaddr) = 0;
On 2013/04/03 03:14:38, jamesr wrote:
> LockForRead() isn't used at all in the big patch and I can't imagine how it
> would be.  Drop it if unless there's a need for this.

Done.

https://codereview.chromium.org/13135004/diff/17001/cc/resources/graphic_buff...
cc/resources/graphic_buffer.h:46: // Unlocks the buffer. Returns true on
success, false otherwise.
On 2013/04/03 03:14:38, jamesr wrote:
> leave a blank line between functions.
> 
> how could unlock fail?  What would that even mean?

Done.

https://codereview.chromium.org/13135004/diff/17001/cc/resources/graphic_buff...
cc/resources/graphic_buffer.h:48: // Returns the size of the buffer
On 2013/04/03 03:14:38, jamesr wrote:
> do you need a size getter?  whoever allocates this GraphicsBuffer must know
the
> size of the buffer created and probably has to store the size anyway, so this
> seems redundant.

Done.

[Jói, 2013-04-03 14:01:02]

DEPS LGTM

[kaanb, 2013-04-04 20:55:45]

I renamed cc::GraphicBuffer to gfx::PixelBuffer and now it moves under ui/gl.
Please take another look.

[jamesr, 2013-04-04 21:05:27]

https://codereview.chromium.org/13135004/diff/34001/cc/cc.gyp
File cc/cc.gyp (right):

https://codereview.chromium.org/13135004/diff/34001/cc/cc.gyp#newcode1
cc/cc.gyp:1: # Copyright 2012 The Chromium Authors. All rights reserved.
Don't do this

https://codereview.chromium.org/13135004/diff/34001/ui/gl/pixel_buffer.h
File ui/gl/pixel_buffer.h (right):

https://codereview.chromium.org/13135004/diff/34001/ui/gl/pixel_buffer.h#newc...
ui/gl/pixel_buffer.h:30: // The factory is set by the
ContentRendererClient::GetPixelBufferFactory()
I don't think it's very useful to put a comment in a lower-level component like
ui/gl/ about what a higher-level component does.  ui/gl/ should make sense
without depending on behaviors of content.

[joth, 2013-04-04 21:53:33]

lg2m w/ some comment fixes

https://codereview.chromium.org/13135004/diff/32001/cc/cc.gyp
File cc/cc.gyp (left):

https://codereview.chromium.org/13135004/diff/32001/cc/cc.gyp#oldcode1
cc/cc.gyp:1: # Copyright (c) 2012 The Chromium Authors. All rights reserved.
drop this file from the CL :)

https://codereview.chromium.org/13135004/diff/32001/ui/gl/pixel_buffer.h
File ui/gl/pixel_buffer.h (right):

https://codereview.chromium.org/13135004/diff/32001/ui/gl/pixel_buffer.h#newc...
ui/gl/pixel_buffer.h:16: // of Android framework.
comment should be generalized. 
(could note the design evolved from a generalization of the GB API on android)

https://codereview.chromium.org/13135004/diff/32001/ui/gl/pixel_buffer.h#newc...
ui/gl/pixel_buffer.h:18: // THREADING CONSIDERATIONS:
also mention about the other non-mapping methods - are they OK to use on
multiple threads?

https://codereview.chromium.org/13135004/diff/32001/ui/gl/pixel_buffer.h#newc...
ui/gl/pixel_buffer.h:21: // read access, none of the threads are blocked.
remove - there is no read access now.

https://codereview.chromium.org/13135004/diff/32001/ui/gl/pixel_buffer.h#newc...
ui/gl/pixel_buffer.h:25: // - shall not block the caller
This comment is too directed to the implementer, and we can't require other
platforms to confirm to this.

I'd say "Mapping a buffer simultaneously on multiple threads is undefined, and
not allowed"

also, Lock -> Map.

https://codereview.chromium.org/13135004/diff/32001/ui/gl/pixel_buffer.h#newc...
ui/gl/pixel_buffer.h:27: // into an indeterminate state.
? this is a comment to the implementer, not the user, and there's no error path
now?

https://codereview.chromium.org/13135004/diff/32001/ui/gl/pixel_buffer.h#newc...
ui/gl/pixel_buffer.h:48: // Returns the stride for the graphic buffer.
in pixels or bytes?

[kaanb, 2013-04-04 22:26:18]

https://codereview.chromium.org/13135004/diff/32001/cc/cc.gyp
File cc/cc.gyp (left):

https://codereview.chromium.org/13135004/diff/32001/cc/cc.gyp#oldcode1
cc/cc.gyp:1: # Copyright (c) 2012 The Chromium Authors. All rights reserved.
On 2013/04/04 21:53:34, joth wrote:
> drop this file from the CL :)

Done.

https://codereview.chromium.org/13135004/diff/32001/ui/gl/pixel_buffer.h
File ui/gl/pixel_buffer.h (right):

https://codereview.chromium.org/13135004/diff/32001/ui/gl/pixel_buffer.h#newc...
ui/gl/pixel_buffer.h:16: // of Android framework.
On 2013/04/04 21:53:34, joth wrote:
> comment should be generalized. 
> (could note the design evolved from a generalization of the GB API on android)

Done.

https://codereview.chromium.org/13135004/diff/32001/ui/gl/pixel_buffer.h#newc...
ui/gl/pixel_buffer.h:18: // THREADING CONSIDERATIONS:
On 2013/04/04 21:53:34, joth wrote:
> also mention about the other non-mapping methods - are they OK to use on
> multiple threads?
> 

Done.

https://codereview.chromium.org/13135004/diff/32001/ui/gl/pixel_buffer.h#newc...
ui/gl/pixel_buffer.h:21: // read access, none of the threads are blocked.
On 2013/04/04 21:53:34, joth wrote:
> remove - there is no read access now.

Done.

https://codereview.chromium.org/13135004/diff/32001/ui/gl/pixel_buffer.h#newc...
ui/gl/pixel_buffer.h:25: // - shall not block the caller
On 2013/04/04 21:53:34, joth wrote:
> This comment is too directed to the implementer, and we can't require other
> platforms to confirm to this.
> 
> I'd say "Mapping a buffer simultaneously on multiple threads is undefined, and
> not allowed"
> 
> also, Lock -> Map.

I've somewhat modified your suggestion, PTAL.

https://codereview.chromium.org/13135004/diff/32001/ui/gl/pixel_buffer.h#newc...
ui/gl/pixel_buffer.h:27: // into an indeterminate state.
On 2013/04/04 21:53:34, joth wrote:
> ? this is a comment to the implementer, not the user, and there's no error
path
> now?

Removed

https://codereview.chromium.org/13135004/diff/32001/ui/gl/pixel_buffer.h#newc...
ui/gl/pixel_buffer.h:48: // Returns the stride for the graphic buffer.
On 2013/04/04 21:53:34, joth wrote:
> in pixels or bytes?
pixels. Clarified the comment.

https://codereview.chromium.org/13135004/diff/34001/cc/cc.gyp
File cc/cc.gyp (right):

https://codereview.chromium.org/13135004/diff/34001/cc/cc.gyp#newcode1
cc/cc.gyp:1: # Copyright 2012 The Chromium Authors. All rights reserved.
On 2013/04/04 21:05:27, jamesr wrote:
> Don't do this

Reverted this file.

https://codereview.chromium.org/13135004/diff/34001/ui/gl/pixel_buffer.h
File ui/gl/pixel_buffer.h (right):

https://codereview.chromium.org/13135004/diff/34001/ui/gl/pixel_buffer.h#newc...
ui/gl/pixel_buffer.h:30: // The factory is set by the
ContentRendererClient::GetPixelBufferFactory()
On 2013/04/04 21:05:27, jamesr wrote:
> I don't think it's very useful to put a comment in a lower-level component
like
> ui/gl/ about what a higher-level component does.  ui/gl/ should make sense
> without depending on behaviors of content.

Removed.

[joth, 2013-04-04 22:41:46]

lgtm

https://codereview.chromium.org/13135004/diff/40001/ui/gl/pixel_buffer.h
File ui/gl/pixel_buffer.h (right):

https://codereview.chromium.org/13135004/diff/40001/ui/gl/pixel_buffer.h#newc...
ui/gl/pixel_buffer.h:28: // Frees a previously allocated buffer. Greeing a
buffer that is still
type: Freeing

[kaanb, 2013-04-04 22:58:44]

https://codereview.chromium.org/13135004/diff/40001/ui/gl/pixel_buffer.h
File ui/gl/pixel_buffer.h (right):

https://codereview.chromium.org/13135004/diff/40001/ui/gl/pixel_buffer.h#newc...
ui/gl/pixel_buffer.h:28: // Frees a previously allocated buffer. Greeing a
buffer that is still
On 2013/04/04 22:41:46, joth wrote:
> type: Freeing

Done.

[kaanb, 2013-04-05 01:29:13]

+gman as reviewer as apatrick is out-of-office

[kaanb, 2013-04-05 23:28:10]

FYI

I've renamed the PixelBuffer interface to GpuMemoryBuffer per the discussions in
https://codereview.chromium.org/13543007/

[greggman, 2013-04-08 17:24:26]

lgtm

[commit-bot: I haz the power, 2013-04-08 17:25:26]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kaanb@chromium.org/13135004/54001

[commit-bot: I haz the power, 2013-04-08 17:31:19]

Sorry for I got bad news for ya.
Compile failed with a clobber build on mac.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac&number...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-04-08 17:34:23]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kaanb@chromium.org/13135004/58004

[commit-bot: I haz the power, 2013-04-08 17:57:39]

Sorry for I got bad news for ya.
Compile failed with a clobber build on android_dbg.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-04-08 18:03:58]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kaanb@chromium.org/13135004/9006

[commit-bot: I haz the power, 2013-04-08 18:19:02]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kaanb@chromium.org/13135004/62004

[commit-bot: I haz the power, 2013-04-08 20:38:16]

Change committed as 192882