Enable accelerated compositing of web pages when using webkit compositor

Enable accelerated compositing of web pages when using webkit compositor

BUG=99517
TEST=http://www.webkit.org/blog/386/3d-transforms/


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=107262

[piman, 2011-10-15 02:28:52]

This needs http://codereview.chromium.org/8222028/ ,
http://codereview.chromium.org/8301019/ and
https://bugs.webkit.org/show_bug.cgi?id=70084 to land before this.
There could be a bit of churn there, so this CL might get changed accordingly.

A good portion of this change is to separate AcceleratedSurfaceContainerTouch
into 2 parts:
- the part that deals with putting the transport resource (pixmap/shm) into a GL
texture (went into ImageTransportClient implementations)
- the part that puts that GL texture into a compositor texture (depending on the
compositor implementation).

[Ben Goodger (Google), 2011-10-16 22:41:10]

http://codereview.chromium.org/8307001/diff/1/ui/gfx/compositor/layer.cc
File ui/gfx/compositor/layer.cc (right):

http://codereview.chromium.org/8307001/diff/1/ui/gfx/compositor/layer.cc#newc...
ui/gfx/compositor/layer.cc:244: if (!web_layer_.isNull()) {
What does it mean to be in here when !defined(USE_WEBKIT_COMPOSITOR)?

[piman, 2011-10-19 19:00:47]

http://codereview.chromium.org/8307001/diff/1/ui/gfx/compositor/layer.cc
File ui/gfx/compositor/layer.cc (right):

http://codereview.chromium.org/8307001/diff/1/ui/gfx/compositor/layer.cc#newc...
ui/gfx/compositor/layer.cc:244: if (!web_layer_.isNull()) {
On 2011/10/16 22:41:10, Ben Goodger (Google) wrote:
> What does it mean to be in here when !defined(USE_WEBKIT_COMPOSITOR)?

Fixed with the change in the previous patch.

[Ben Goodger (Google), 2011-10-19 19:07:00]

backer should provide primary review on this one.

[jonathan.backer, 2011-10-20 14:42:24]

On 2011/10/19 19:07:00, Ben Goodger (Google) wrote:
> backer should provide primary review on this one.

Did a first pass. In the future, could we break these into smaller CLs? 
- say one for the refactor of the accelerated surface container code
- and another for the CC implementation.
It'll make reviewing easier (and more productive).

I'm seeing a regression with GYP_DEFINES="touchui=1 use_aura=1
use_webkit_compositor=1" versus GYP_DEFINES="touchui=1 use_aura=1":
- go to google.com
- repeatedly show and hide the keyboard by clicking in the omnibox and then
clicking in the white area of the webpage
- sometimes the keyboard doesn't display

[jonathan.backer, 2011-10-20 14:42:50]

http://codereview.chromium.org/8307001/diff/4001/chrome/chrome_browser.gypi
File chrome/chrome_browser.gypi (right):

http://codereview.chromium.org/8307001/diff/4001/chrome/chrome_browser.gypi#n...
chrome/chrome_browser.gypi:4196: ],
Maybe use your trick with the else condition to exclude these? If you think it
is more readable.

http://codereview.chromium.org/8307001/diff/4001/ui/gfx/compositor/compositor.h
File ui/gfx/compositor/compositor.h (right):

http://codereview.chromium.org/8307001/diff/4001/ui/gfx/compositor/compositor...
ui/gfx/compositor/compositor.h:30: virtual bool MakeSharedContextCurrent() = 0;
Idea: Put GetInstance() in this class. Have it return NULL on Windows. Then you
don't have to plumb this through to image_transport_client.

http://codereview.chromium.org/8307001/diff/4001/ui/gfx/compositor/layer.h
File ui/gfx/compositor/layer.h (right):

http://codereview.chromium.org/8307001/diff/4001/ui/gfx/compositor/layer.h#ne...
ui/gfx/compositor/layer.h:43: public WebKit::WebContentLayerClient {
I plead ignorance here: but if we're now backed by a WebLayer instead of a
WebContentLayer, should we now be a WebLayerClient instead of a
WebContentLayerClient?

[piman, 2011-10-20 21:37:57]

http://codereview.chromium.org/8307001/diff/4001/chrome/chrome_browser.gypi
File chrome/chrome_browser.gypi (right):

http://codereview.chromium.org/8307001/diff/4001/chrome/chrome_browser.gypi#n...
chrome/chrome_browser.gypi:4196: ],
On 2011/10/20 14:42:50, jonathan.backer wrote:
> Maybe use your trick with the else condition to exclude these? If you think it
> is more readable.

Done.

http://codereview.chromium.org/8307001/diff/4001/ui/gfx/compositor/compositor.h
File ui/gfx/compositor/compositor.h (right):

http://codereview.chromium.org/8307001/diff/4001/ui/gfx/compositor/compositor...
ui/gfx/compositor/compositor.h:30: virtual bool MakeSharedContextCurrent() = 0;
On 2011/10/20 14:42:50, jonathan.backer wrote:
> Idea: Put GetInstance() in this class. Have it return NULL on Windows. Then
you
> don't have to plumb this through to image_transport_client.

Once we have the RenderWidgetHostViewAura, I'm hoping to change the CC version
not be a leaky singleton (have it created and destroyed with the Desktop), so
that we don't leak it and to reduce the proliferation of singletons. Also when
we want to run GL out-of-process, I suspect we'll need more context to create
this SharedResource, e.g. the connection to the GPU process, and I suspect the
GetInstance paradigm will be revisited.
So I'd like not to bake this into the API. I guess it's minor, we can always
change it, but I'm worried that more pieces will start relying on it - I'd like
to avoid that.

http://codereview.chromium.org/8307001/diff/4001/ui/gfx/compositor/layer.h
File ui/gfx/compositor/layer.h (right):

http://codereview.chromium.org/8307001/diff/4001/ui/gfx/compositor/layer.h#ne...
ui/gfx/compositor/layer.h:43: public WebKit::WebContentLayerClient {
On 2011/10/20 14:42:50, jonathan.backer wrote:
> I plead ignorance here: but if we're now backed by a WebLayer instead of a
> WebContentLayer, should we now be a WebLayerClient instead of a
> WebContentLayerClient?

We are backed by either a WebContentLayer if we are a software layer, or by a
WebExternalTextureLayer for the renderer when it's accelerated.
Both of them derive from WebLayer.

In the first case, WebContentLayerClient provides the interface to paint the
layer. It won't be called if we are backed by a WebExternalTextureLayer, but it
needs to be there for the first case. An alternative would be to have a separate
class to just hop back to the Layer, that would be created with the
WebContentLayer and destroyed when we switch to the WebExternalTextureLayer, but
I think that only adds complexity, with no clear benefit.

[piman, 2011-10-24 20:11:33]

Ping.

[jonathan.backer, 2011-10-24 20:36:01]

On 2011/10/24 20:11:33, piman wrote:
> Ping.

I've one of my CLs with the WK compositor in browser CL :-D, so I should have
more cycles soon.

In the meantime, have you investigated the TOUCH_UI related regression I pointed
out above?

[jonathan.backer, 2011-10-24 20:38:29]

Err... s/one/merged one

[piman, 2011-10-24 20:57:36]

On Mon, Oct 24, 2011 at 1:36 PM, <backer@chromium.org> wrote:

> On 2011/10/24 20:11:33, piman wrote:
>
>> Ping.
>>
>
> I've one of my CLs with the WK compositor in browser CL :-D, so I should
> have
> more cycles soon.
>
> In the meantime, have you investigated the TOUCH_UI related regression I
> pointed
> out above?
>

Sorry, I missed that... Taking a look.


>
>
http://codereview.chromium.**org/8307001/<http://codereview.chromium.org/8307...
>

[jonathan.backer, 2011-10-24 21:37:33]

http://codereview.chromium.org/8307001/diff/4001/ui/gfx/compositor/compositor.h
File ui/gfx/compositor/compositor.h (right):

http://codereview.chromium.org/8307001/diff/4001/ui/gfx/compositor/compositor...
ui/gfx/compositor/compositor.h:30: virtual bool MakeSharedContextCurrent() = 0;
One of the issues that I had with external textures is racing with WebKit ---
sometimes we had external textures coming in before we had a compositor set up. 
We already had SharedResources to share shaders between different compositors
(at one time we could have up to 8 created --- not necessary with CompositorCC
on aura?), so I just bound the external texture to the shared context. If you
can guarantee no races (only one compositor and it's always around when external
textures are around), you might be able to nuke it altogether.

http://codereview.chromium.org/8307001/diff/20001/content/browser/renderer_ho...
File content/browser/renderer_host/accelerated_surface_container_linux_cc.cc
(right):

http://codereview.chromium.org/8307001/diff/20001/content/browser/renderer_ho...
content/browser/renderer_host/accelerated_surface_container_linux_cc.cc:54:
image_transport_client_->Acquire();
I'm confused as to why this works (seems to on my desktop). We're calling a
glXBindTexImageEXT and a glXReleaseTexImageEXT without drawing the associated
texture.

I'm not sure that this is an error (spec is here:
http://www.opengl.org/registry/specs/EXT/texture_from_pixmap.txt), but I bet
it's not what driver vendors are expecting.

[piman, 2011-10-25 02:51:09]

PTAL, there's a fix for the touchui on-screen keyboard, in most cases.

However while debugging this, I found that the image transport, as it exists
today, is racy:
We have 2 clients talking to X (gpu process, browser process), but there is no
synchronization to ensure the X stream is ordered between the 2 clients. E.g.
gpu process sends draw commands to X (through gl and swapbuffers), and signals
the browser process to use the result of that. The browser process sends draw
commands to X to use this result, however nothing ensures that the commands the
browser process happen *after* the gpu process commands.

Because of the timing, the current compositor is less likely to be affected, but
is not immune to it and could show this when under load.

The simple way to fix it would be to add glXWaitGL/XSync on the gpu process, but
that would be disastrous in terms of performance.
The better way to fix it is to use the XSYNC extension to ensure ordering, but
this requires a bit of infrastructure.
I left it out of this CL for now.

Moving forward, if we're going towards running GL in the GPU process, then this
all becomes moot, so I'm leaning towards not spending time on fixing this for
now.

http://codereview.chromium.org/8307001/diff/4001/ui/gfx/compositor/compositor.h
File ui/gfx/compositor/compositor.h (right):

http://codereview.chromium.org/8307001/diff/4001/ui/gfx/compositor/compositor...
ui/gfx/compositor/compositor.h:30: virtual bool MakeSharedContextCurrent() = 0;
On 2011/10/24 21:37:33, jonathan.backer wrote:
> One of the issues that I had with external textures is racing with WebKit ---
> sometimes we had external textures coming in before we had a compositor set
up. 
> We already had SharedResources to share shaders between different compositors
> (at one time we could have up to 8 created --- not necessary with CompositorCC
> on aura?), so I just bound the external texture to the shared context. If you
> can guarantee no races (only one compositor and it's always around when
external
> textures are around), you might be able to nuke it altogether.

As long as the rendering is in the browser process, we'll still need a way to
make a context current that shares its resources with the compositor context.
Especially with an out-of-thread compositor, it has to be a different one. So I
will keep this interface, however, I will look at whether we can get it from the
compositor (as you mention, if we can guarantee we always have a compositor), so
that we can scope it nicely.
When we move the GL rendering out-of-process, we won't need this interface any
more.

http://codereview.chromium.org/8307001/diff/20001/content/browser/renderer_ho...
File content/browser/renderer_host/accelerated_surface_container_linux_cc.cc
(right):

http://codereview.chromium.org/8307001/diff/20001/content/browser/renderer_ho...
content/browser/renderer_host/accelerated_surface_container_linux_cc.cc:54:
image_transport_client_->Acquire();
On 2011/10/24 21:37:33, jonathan.backer wrote:
> I'm confused as to why this works (seems to on my desktop). We're calling a
> glXBindTexImageEXT and a glXReleaseTexImageEXT without drawing the associated
> texture.

We're calling Release then Bind. When we draw, the texture is bound.

> I'm not sure that this is an error (spec is here:
> http://www.opengl.org/registry/specs/EXT/texture_from_pixmap.txt), but I bet
> it's not what driver vendors are expecting.

This is what we do in the Chrome OS window manager - see
http://git.chromium.org/gitweb/?p=chromiumos/platform/window_manager.git;a=bl...

[jonathan.backer, 2011-10-25 13:21:01]

LGTM

About the races: could you be more specific? We've got very little X --- mostly
on setup and teardown. It's OK if these paths are slower because they are
infrequent. So we can XSync and glFinish.

The GLX image transport path has the most X. But we're roughly mimicing a
compositing window manager. Do compositing windows managers use the XSync
extension? I just assumed that there was a bit of synchronization logic in
XComposite and GLX_tfp at work (hence the need to Bind and Release).

With GL it seems mostly sufficient to glFlush for synchronization between
threads/processes. XFlush is insufficient? (I know that in theory these things
can go over the wire, but I suspect that in practice it's all going to a local X
server.)

http://codereview.chromium.org/8307001/diff/20001/content/browser/renderer_ho...
File content/browser/renderer_host/accelerated_surface_container_linux_cc.cc
(right):

http://codereview.chromium.org/8307001/diff/20001/content/browser/renderer_ho...
content/browser/renderer_host/accelerated_surface_container_linux_cc.cc:54:
image_transport_client_->Acquire();
Got it. Sounds good.

On 2011/10/25 02:51:10, piman wrote:
> On 2011/10/24 21:37:33, jonathan.backer wrote:
> > I'm confused as to why this works (seems to on my desktop). We're calling a
> > glXBindTexImageEXT and a glXReleaseTexImageEXT without drawing the
associated
> > texture.
> 
> We're calling Release then Bind. When we draw, the texture is bound.
> 
> > I'm not sure that this is an error (spec is here:
> > http://www.opengl.org/registry/specs/EXT/texture_from_pixmap.txt), but I bet
> > it's not what driver vendors are expecting.
> 
> This is what we do in the Chrome OS window manager - see
>
http://git.chromium.org/gitweb/?p=chromiumos/platform/window_manager.git;a=bl...

[piman, 2011-10-25 16:37:00]

On Tue, Oct 25, 2011 at 6:21 AM, <backer@chromium.org> wrote:

> LGTM
>
> About the races: could you be more specific? We've got very little X ---
> mostly
> on setup and teardown. It's OK if these paths are slower because they are
> infrequent. So we can XSync and glFinish.
>

I don't think it would be OK, because we'd need to call that every frame, on
swapbuffers.


>
> The GLX image transport path has the most X. But we're roughly mimicing a
> compositing window manager. Do compositing windows managers use the XSync
> extension?


They do for mapping windows (so that they don't draw windows that are not
finished painting), but mostly they rely on the Damage extension - i.e. they
get notified by X when the painting has finished, so that they draw after
the bits are there.


> I just assumed that there was a bit of synchronization logic in
> XComposite and GLX_tfp at work (hence the need to Bind and Release).
>

I think it ensures atomicity (i.e. you won't draw a frame that's in
progress) but not ordering. The spec is vague...


> With GL it seems mostly sufficient to glFlush for synchronization between
> threads/processes. XFlush is insufficient? (I know that in theory these
> things
> can go over the wire, but I suspect that in practice it's all going to a
> local X
> server.)


The spec doesn't say anything about XFlush. Implementation-wise, each X
client is a socket, and X does a select on all sockets, and handles data
coming from any socket with data, AFAIK without regard to ordering (the
information is lost at this point). Say, if X is busy dealing with the
previous frame, and meanwhile both the GPU and the browser add their
commands, when X is done with the previous frame, it does the select which
tells it that that both sockets have data - but it doesn't have a way to
know which one has got data first. That's why the XSYNC extension has been
introduced.


>
>
> http://codereview.chromium.**org/8307001/diff/20001/**
> content/browser/renderer_host/**accelerated_surface_container_**
>
linux_cc.cc<http://codereview.chromium.org/8307001/diff/20001/content/browser/renderer_host/accelerated_surface_container_linux_cc.cc>
> File
> content/browser/renderer_host/**accelerated_surface_container_**
> linux_cc.cc
> (right):
>
> http://codereview.chromium.**org/8307001/diff/20001/**
> content/browser/renderer_host/**accelerated_surface_container_**
>
linux_cc.cc#newcode54<http://codereview.chromium.org/8307001/diff/20001/content/browser/renderer_host/accelerated_surface_container_linux_cc.cc#newcode54>
> content/browser/renderer_host/**accelerated_surface_container_**
> linux_cc.cc:54:
> image_transport_client_->**Acquire();
> Got it. Sounds good.
>
>
> On 2011/10/25 02:51:10, piman wrote:
>
>> On 2011/10/24 21:37:33, jonathan.backer wrote:
>> > I'm confused as to why this works (seems to on my desktop). We're
>>
> calling a
>
>> > glXBindTexImageEXT and a glXReleaseTexImageEXT without drawing the
>>
> associated
>
>> > texture.
>>
>
>  We're calling Release then Bind. When we draw, the texture is bound.
>>
>
>  > I'm not sure that this is an error (spec is here:
>> >
http://www.opengl.org/**registry/specs/EXT/texture_**from_pixmap.txt<http://w...
>> ),
>>
> but I bet
>
>> > it's not what driver vendors are expecting.
>>
>
>  This is what we do in the Chrome OS window manager - see
>>
>
> http://git.chromium.org/**gitweb/?p=chromiumos/platform/**
> window_manager.git;a=blob;f=**compositor/gl/opengl_visitor.**cc;h=**
>
45a18b2a8932f5871a7ab3c7159398**cc9bb78d31;hb=HEAD#l89<http://git.chromium.org/gitweb/?p=chromiumos/platform/window_manager.git;a=blob;f=compositor/gl/opengl_visitor.cc;h=45a18b2a8932f5871a7ab3c7159398cc9bb78d31;hb=HEAD#l89>
>
>
http://codereview.chromium.**org/8307001/<http://codereview.chromium.org/8307...
>

[commit-bot: I haz the power, 2011-10-25 16:46:28]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/piman@chromium.org/8307001/26001

[commit-bot: I haz the power, 2011-10-25 16:46:34]

Presubmit check for 8307001-26001 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for:
content/browser/renderer_host/accelerated_surface_container_linux.h,content/browser/renderer_host/accelerated_surface_container_linux_cc.cc,content/browser/renderer_host/accelerated_surface_container_linux_gl.cc,content/browser/renderer_host/image_transport_client.cc,content/browser/renderer_host/image_transport_client.h,content/browser/renderer_host/render_widget_host_view_aura.cc

Presubmit checks took 2.0s to calculate.

Was the presubmit check useful? Please send feedback & hate mail to
maruel@chromium.org!

[piman, 2011-10-25 17:42:35]

On 2011/10/25 16:46:34, I haz the power (commit-bot) wrote:
> Presubmit check for 8307001-26001 failed and returned exit status 1.
> 
> Running presubmit commit checks ...
> 
> ** Presubmit ERRORS **
> Missing LGTM from an OWNER for:
>
content/browser/renderer_host/accelerated_surface_container_linux.h,content/browser/renderer_host/accelerated_surface_container_linux_cc.cc,content/browser/renderer_host/accelerated_surface_container_linux_gl.cc,content/browser/renderer_host/image_transport_client.cc,content/browser/renderer_host/image_transport_client.h,content/browser/renderer_host/render_widget_host_view_aura.cc

Ping Ben for OWNERS approval in content/browser

[Ben Goodger (Google), 2011-10-25 18:24:14]

LGTM, see note:

http://codereview.chromium.org/8307001/diff/26001/content/browser/renderer_ho...
File content/browser/renderer_host/image_transport_client.h (right):

http://codereview.chromium.org/8307001/diff/26001/content/browser/renderer_ho...
content/browser/renderer_host/image_transport_client.h:17: class
ImageTransportClient {
Note what this is for.

[commit-bot: I haz the power, 2011-10-25 18:34:32]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/piman@chromium.org/8307001/29002

[commit-bot: I haz the power, 2011-10-25 19:30:07]

Commit queue patch verification failed without an error message.
Something went wrong, probably a crash, a hickup or simply
the monkeys went out for diner.
Ping the relevant dude on a on-needed basis.

[commit-bot: I haz the power, 2011-10-25 19:52:57]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/piman@chromium.org/8307001/29002

[commit-bot: I haz the power, 2011-10-25 21:43:04]

Try job failure for 8307001-29002 (retry) on win_rel for step "ui_tests".
It's a second try, previously, step "ui_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2011-10-25 22:01:51]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/piman@chromium.org/8307001/29002

[commit-bot: I haz the power, 2011-10-25 22:08:08]

Try job failure for 8307001-29002 on win_rel for step "update".
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

Step "update" is always a major failure.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2011-10-25 22:24:40]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/piman@chromium.org/8307001/35001

[commit-bot: I haz the power, 2011-10-26 00:54:48]

Change committed as 107262