Allow WebExternalTextureLayers to receive a bitmap along with a mailbox

Allow WebExternalTextureLayers to receive a bitmap along with a mailbox

When using the software renderer, texture layers can't be drawn from directly. Instead allow the WebExternalTextureLayer to request a bitmap that will be used to draw.

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

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

Nat, could you see if this approach looks right? This goes with
https://codereview.chromium.org/17859003/ .

[tfarina, 2013-06-26 16:27:37]

https://codereview.chromium.org/17859002/diff/1/cc/resources/resource_provide...
File cc/resources/resource_provider.cc (right):

https://codereview.chromium.org/17859002/diff/1/cc/resources/resource_provide...
cc/resources/resource_provider.cc:280: SkAutoLockPixels bitmapLock(bitmap);
minornit: bitmap_lock

[danakj, 2013-06-26 19:09:57]

https://codereview.chromium.org/17859002/diff/1/cc/trees/layer_tree_host.cc
File cc/trees/layer_tree_host.cc (right):

https://codereview.chromium.org/17859002/diff/1/cc/trees/layer_tree_host.cc#n...
cc/trees/layer_tree_host.cc:127: using_software_renderer_ =
!output_surface->context3d();
This would be wrong on WebView when they defer initialization of GL until later
on. They'd end up with a GLRenderer, but getting software WebGL stuff. Maybe
that's okay though?

[nduca, 2013-06-26 19:57:48]

probably best to have the ubercomp types look at this. I'm pretty far behind on
this part of our universe. :$

[piman, 2013-06-26 20:20:27]

Could we go directly to shared memory instead? The rationale is that:
- we already have the shm path in TextureMailbox
- in the Übercomp case, we won't be able to directly transport the Bitmap,
but if it's already in a shm we can

There is a slight overhead in allocating shm instead of heap memory, but
assuming we recycle them it shouldn't be an ongoing cost.
WDYT?


On Wed, Jun 26, 2013 at 12:57 PM, <nduca@chromium.org> wrote:

> probably best to have the ubercomp types look at this. I'm pretty far
> behind on
> this part of our universe. :$
>
>
https://codereview.chromium.**org/17859002/<https://codereview.chromium.org/1...
>

[jbauman, 2013-06-26 20:21:41]

On 2013/06/26 20:20:27, piman wrote:
> Could we go directly to shared memory instead? The rationale is that:
> - we already have the shm path in TextureMailbox
> - in the Übercomp case, we won't be able to directly transport the Bitmap,
> but if it's already in a shm we can
> 
> There is a slight overhead in allocating shm instead of heap memory, but
> assuming we recycle them it shouldn't be an ongoing cost.
> WDYT?
> 

It's a bit of a pain because we count allocate a base::SharedMemory outside of
content/ on non-Windows, but I can look into it more if you think it's a good
idea.
> 
> On Wed, Jun 26, 2013 at 12:57 PM, <mailto:nduca@chromium.org> wrote:
> 
> > probably best to have the ubercomp types look at this. I'm pretty far
> > behind on
> > this part of our universe. :$
> >
> >
>
https://codereview.chromium.**org/17859002/%3Chttps://codereview.chromium.org...>
> >

[boliu, 2013-06-28 01:27:29]

Sorry for the delay

https://codereview.chromium.org/17859002/diff/1/cc/trees/layer_tree_host.cc
File cc/trees/layer_tree_host.cc (right):

https://codereview.chromium.org/17859002/diff/1/cc/trees/layer_tree_host.cc#n...
cc/trees/layer_tree_host.cc:127: using_software_renderer_ =
!output_surface->context3d();
On 2013/06/26 19:09:57, danakj wrote:
> This would be wrong on WebView when they defer initialization of GL until
later
> on. They'd end up with a GLRenderer, but getting software WebGL stuff. Maybe
> that's okay though?

For android webview, we should strive to make GL mode always work correctly at
the expense of software mode. So this condition should be

!output_surface->context3d() &&
!output_surface->capabilities().deferred_gl_initialization

There is the caveat that context3d() may always be null even if
using_software_renderer_ is false, but I don't think any null checks are needed
in this patch. I think the actual usage of the mailbox resource is in
TextureLayerImpl only, which is skipped in webview software mode already.

[jbauman, 2013-07-01 22:15:55]

Ok, I've modified the patch to use SharedMemory. PTAL.

On 2013/06/26 20:20:27, piman wrote:
> Could we go directly to shared memory instead? The rationale is that:
> - we already have the shm path in TextureMailbox
> - in the Übercomp case, we won't be able to directly transport the Bitmap,
> but if it's already in a shm we can
> 
> There is a slight overhead in allocating shm instead of heap memory, but
> assuming we recycle them it shouldn't be an ongoing cost.
> WDYT?
> 
> 
> On Wed, Jun 26, 2013 at 12:57 PM, <mailto:nduca@chromium.org> wrote:
> 
> > probably best to have the ubercomp types look at this. I'm pretty far
> > behind on
> > this part of our universe. :$
> >
> >
>
https://codereview.chromium.**org/17859002/%3Chttps://codereview.chromium.org...>
> >

[piman, 2013-07-01 22:45:01]

https://codereview.chromium.org/17859002/diff/11001/cc/layers/texture_layer.cc
File cc/layers/texture_layer.cc (right):

https://codereview.chromium.org/17859002/diff/11001/cc/layers/texture_layer.c...
cc/layers/texture_layer.cc:163: return !layer_tree_host()->UsingGLRenderer();
note: layer_tree_host() is NULL when the layer is outside of the tree.
More generally we may construct layer trees before we know anything about the
system capabilities. I'm not sure UsingGLRenderer() is generally safe.

Maybe a better way would be to pass that info through PrepareTextureMailbox - I
believe that's the only place you need it currently. PrepareTextureMailbox is
called from TextureLayer::Update which is called from
LayerTreeHost::UpdateLayers, which has 2 advantages:
1- you know for sure you have a LTH
2- LTH::UpdateLayers is called from the BeginFrame flow, i.e. started from the
proxy/LTHI after proper initialization, when we have all the information (i.e.
similar to, say, the max texture size).

https://codereview.chromium.org/17859002/diff/11001/content/renderer/render_t...
File content/renderer/render_thread_impl.cc (right):

https://codereview.chromium.org/17859002/diff/11001/content/renderer/render_t...
content/renderer/render_thread_impl.cc:755: #if defined(OS_POSIX)
Why the ifdef? This method was sucessfully used on all platforms previously...

https://codereview.chromium.org/17859002/diff/11001/webkit/renderer/composito...
File webkit/renderer/compositor_bindings/web_external_texture_layer_impl.cc
(right):

https://codereview.chromium.org/17859002/diff/11001/webkit/renderer/composito...
webkit/renderer/compositor_bindings/web_external_texture_layer_impl.cc:128: if
(bitmap)
nit: needs braces

[jbauman, 2013-07-02 00:00:40]

https://codereview.chromium.org/17859002/diff/11001/cc/layers/texture_layer.cc
File cc/layers/texture_layer.cc (right):

https://codereview.chromium.org/17859002/diff/11001/cc/layers/texture_layer.c...
cc/layers/texture_layer.cc:163: return !layer_tree_host()->UsingGLRenderer();
On 2013/07/01 22:45:01, piman wrote:
> note: layer_tree_host() is NULL when the layer is outside of the tree.
> More generally we may construct layer trees before we know anything about the
> system capabilities. I'm not sure UsingGLRenderer() is generally safe.
> 
> Maybe a better way would be to pass that info through PrepareTextureMailbox -
I
> believe that's the only place you need it currently. PrepareTextureMailbox is
> called from TextureLayer::Update which is called from
> LayerTreeHost::UpdateLayers, which has 2 advantages:
> 1- you know for sure you have a LTH
> 2- LTH::UpdateLayers is called from the BeginFrame flow, i.e. started from the
> proxy/LTHI after proper initialization, when we have all the information (i.e.
> similar to, say, the max texture size).

Ok, modified PrepareTextureMailbox to do this instead.

https://codereview.chromium.org/17859002/diff/11001/content/renderer/render_t...
File content/renderer/render_thread_impl.cc (right):

https://codereview.chromium.org/17859002/diff/11001/content/renderer/render_t...
content/renderer/render_thread_impl.cc:755: #if defined(OS_POSIX)
On 2013/07/01 22:45:01, piman wrote:
> Why the ifdef? This method was sucessfully used on all platforms previously...

Sure, but I'd prefer not to have a round-trip to the browser
process.ChildThread::AllocateSharedMemory has this same #ifdef logic, but I
guess it wasn't copied over to here.

[piman, 2013-07-02 04:00:09]

https://codereview.chromium.org/17859002/diff/11001/content/renderer/render_t...
File content/renderer/render_thread_impl.cc (right):

https://codereview.chromium.org/17859002/diff/11001/content/renderer/render_t...
content/renderer/render_thread_impl.cc:755: #if defined(OS_POSIX)
On 2013/07/02 00:00:40, jbauman wrote:
> On 2013/07/01 22:45:01, piman wrote:
> > Why the ifdef? This method was sucessfully used on all platforms
previously...
> 
> Sure, but I'd prefer not to have a round-trip to the browser
> process.ChildThread::AllocateSharedMemory has this same #ifdef logic, but I
> guess it wasn't copied over to here.

I see, that's an optimization. Maybe worth separating out into a CL by itself.
We should also probably delegate to ChildThread::AllocateSharedMemory, passing
|this| or sync_message_filter() as the sender?

https://codereview.chromium.org/17859002/diff/18001/cc/trees/layer_tree_host.cc
File cc/trees/layer_tree_host.cc (right):

https://codereview.chromium.org/17859002/diff/18001/cc/trees/layer_tree_host....
cc/trees/layer_tree_host.cc:128:
output_surface->capabilities().deferred_gl_initialization;
Note: during the lifetime of the LTH, we may recreate the OutputSurface after a
lost context - see e.g. ThreadProxy::DoCreateAndInitializeOutputSurface()

I think it will happen that a recreated output surface will have a different
type than the one created at the beginning, e.g. if we completely lost the GPU
context and we're blacklisted for the session, and we want to fallback to
software. Even if not now, that's something we want to be able to do eventually.

Based on that, maybe what's best to do is to keep this flag on the
RendererCapabilities, and it should flow naturally to the LTH before the first
commit on that new surface - maybe renamed as "using_shared_memory_resources" or
something.

https://codereview.chromium.org/17859002/diff/18001/cc/trees/layer_tree_host.h
File cc/trees/layer_tree_host.h (right):

https://codereview.chromium.org/17859002/diff/18001/cc/trees/layer_tree_host....
cc/trees/layer_tree_host.h:367: bool using_gl_renderer_;
new field needs initialization....

[jbauman, 2013-07-02 20:16:19]

https://codereview.chromium.org/17859002/diff/11001/content/renderer/render_t...
File content/renderer/render_thread_impl.cc (right):

https://codereview.chromium.org/17859002/diff/11001/content/renderer/render_t...
content/renderer/render_thread_impl.cc:755: #if defined(OS_POSIX)
On 2013/07/02 04:00:09, piman wrote:
> On 2013/07/02 00:00:40, jbauman wrote:
> > On 2013/07/01 22:45:01, piman wrote:
> > > Why the ifdef? This method was sucessfully used on all platforms
> previously...
> > 
> > Sure, but I'd prefer not to have a round-trip to the browser
> > process.ChildThread::AllocateSharedMemory has this same #ifdef logic, but I
> > guess it wasn't copied over to here.
> 
> I see, that's an optimization. Maybe worth separating out into a CL by itself.
> We should also probably delegate to ChildThread::AllocateSharedMemory, passing
> |this| or sync_message_filter() as the sender?

Ok, I've removed this change and I'll do that in a separate patch.

https://codereview.chromium.org/17859002/diff/18001/cc/trees/layer_tree_host.cc
File cc/trees/layer_tree_host.cc (right):

https://codereview.chromium.org/17859002/diff/18001/cc/trees/layer_tree_host....
cc/trees/layer_tree_host.cc:128:
output_surface->capabilities().deferred_gl_initialization;
On 2013/07/02 04:00:09, piman wrote:
> Note: during the lifetime of the LTH, we may recreate the OutputSurface after
a
> lost context - see e.g. ThreadProxy::DoCreateAndInitializeOutputSurface()
> 
> I think it will happen that a recreated output surface will have a different
> type than the one created at the beginning, e.g. if we completely lost the GPU
> context and we're blacklisted for the session, and we want to fallback to
> software. Even if not now, that's something we want to be able to do
eventually.
> 
> Based on that, maybe what's best to do is to keep this flag on the
> RendererCapabilities, and it should flow naturally to the LTH before the first
> commit on that new surface - maybe renamed as "using_shared_memory_resources"
or
> something.

Ok, I'm now passing it through RendererCapabilities instead.

[piman, 2013-07-02 20:21:51]

OK, LGTM!

[piman, 2013-07-02 20:21:51]

OK, LGTM!

[jbauman, 2013-07-04 00:13:22]

Nat, could I get an OWNERS review for webkit/renderer/compositor_bindings?

[jamesr, 2013-07-04 00:32:08]

lgtm

https://codereview.chromium.org/17859002/diff/43001/webkit/renderer/composito...
File webkit/renderer/compositor_bindings/web_external_bitmap_impl.h (right):

https://codereview.chromium.org/17859002/diff/43001/webkit/renderer/composito...
webkit/renderer/compositor_bindings/web_external_bitmap_impl.h:36:
base::SharedMemory* getSharedMemory() { return shared_memory_.get(); }
this is chromium code, so it should follow chromium naming conventions. since
it's an inline trivial getter i think the best name is 'shared_memory()' and it
should be const. if it had to be out-of-line or whatever then
'GetSharedMemory()' would be a good name

[commit-bot: I haz the power, 2013-07-10 00:38:33]

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

[commit-bot: I haz the power, 2013-07-10 00:47:53]

Sorry for I got bad news for ya.
Compile failed with a clobber build on mac_rel.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...
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-07-10 00:54:11]

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

[commit-bot: I haz the power, 2013-07-10 07:33:32]

Change committed as 210773