Render Core Animation plugins through WebKit's compositor rather than

Render Core Animation plugins through WebKit's compositor rather than
directly to the screen in the browser process.

The new composited code path is now the default, though the old code path
has been left in place under a command line flag while we gain confidence.
Issue 105344 has been filed about removing the old code path.

The new code path does not currently support 10.5. The consequence is that
plugins using the InvalidatingCoreAnimation rendering model will not work
on this version of Mac OS. Pepper 3D is not affected; it now uses a
different rendering path.

Changed the type of IOSurfaces' IDs from uint64 to uint32 in a few places
throughout the code to match the IOSurfaceID typedef in the system header.
This was necessary in order to simplify integration with Chrome's OpenGL
code.

There is a known problem in the new code path with garbage occasionally
being drawn to the plugin's area during live resizing of Core Animation
plugins. Issue 105346 has been filed to track this. It is unclear whether
the additional complexity of the fix that is likely needed is worth it.

Tested manually with the following content, with and without the
--disable-composited-core-animation-plugins flag:
 - YouTube (does not trigger this code path)
 - Google+ Hangouts
 - http://unity3d.com/gallery/demos/live-demos (Unity 3D)
 - http://www.erain.com/labs/molehill/ (Stage 3D in Flash 11)
 - http://www.nissan-stagejuk3d.com/
    (Stage 3D in Flash 11, live resizing; web site is flaky, sometimes
    fails to start)

BUG=38967
TEST=manual testing with above test cases

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

[Ken Russell (switch to Gerrit), 2011-11-24 05:02:20]

Please review:

stuartmorgan, pinkerton: everything

OWNERS:
jam: content/browser/, content/common/, content/public/, content/renderer/
ben: ui/gfx/surface/
ananta: webkit/plugins/npapi/

Thanks.

[jam, 2011-11-24 21:23:38]

content lgtm

[stuartmorgan, 2011-11-28 09:12:26]

On 2011/11/24 05:02:20, kbr wrote:
> The new code path does not currently support 10.5. The consequence is that
> plugins using the InvalidatingCoreAnimation rendering model will not work
> on this version of Mac OS.

Is there a plan for resolving that, or do we just want to give up on CA support
on 10.5? On one hand, ICA is currently broken (I've had a couple reports that
repaint calls aren't actually triggering a repaint), so we won't really regress
anything new, but on the other hand these requests are from plugin vendors who
want to use ICA on 10.5, so I was hoping to fix it.

[stuartmorgan, 2011-11-28 10:09:04]

Have you done any tests that actually test the compositing? i.e., that
transparency works, and that overlaying HTML content works?

We could either construct some tests, or we could see if Flash currently honors
the flag in crbug.com/40997 and then test some wmode=transparent cases.

http://codereview.chromium.org/8678037/diff/6003/content/common/plugin_messag...
File content/common/plugin_messages.h (right):

http://codereview.chromium.org/8678037/diff/6003/content/common/plugin_messag...
content/common/plugin_messages.h:449: bool /* opaque */)
Do we expect to need opaque in the future for some non-NPAPI use? CA plugins
should always be considered transparent, so if not we can remove this argument.
IIRC the argument on the other function was when we were trying to hack up an
intermediate state where some CA plugins were opaque and others weren't, as a
stopgap to having compositing.

http://codereview.chromium.org/8678037/diff/6003/content/plugin/webplugin_acc...
File content/plugin/webplugin_accelerated_surface_proxy_mac.cc (right):

http://codereview.chromium.org/8678037/diff/6003/content/plugin/webplugin_acc...
content/plugin/webplugin_accelerated_surface_proxy_mac.cc:33: if (composited)
So if !composited, failure to create a surface returns a surfaceless object? Is
that actually what we want?

http://codereview.chromium.org/8678037/diff/6003/ui/gfx/surface/accelerated_s...
File ui/gfx/surface/accelerated_surface_mac.cc (right):

http://codereview.chromium.org/8678037/diff/6003/ui/gfx/surface/accelerated_s...
ui/gfx/surface/accelerated_surface_mac.cc:149: static GLuint
CreateTexture(GLenum target) {
Needs a method-level comment (which should probably explain ownership of the
returned texture)

http://codereview.chromium.org/8678037/diff/6003/webkit/plugins/npapi/plugin_...
File webkit/plugins/npapi/plugin_host.cc (right):

http://codereview.chromium.org/8678037/diff/6003/webkit/plugins/npapi/plugin_...
webkit/plugins/npapi/plugin_host.cc:842: composited ?
Bring this up to the previous line?

http://codereview.chromium.org/8678037/diff/6003/webkit/plugins/npapi/webplug...
File webkit/plugins/npapi/webplugin_delegate_impl.h (right):

http://codereview.chromium.org/8678037/diff/6003/webkit/plugins/npapi/webplug...
webkit/plugins/npapi/webplugin_delegate_impl.h:478: bool composited_; // If CA
plugin, whether it's rendering via compositor.
Needs one more space before //

[Ken Russell (switch to Gerrit), 2011-11-28 23:00:03]

On 2011/11/28 09:12:26, stuartmorgan wrote:
> On 2011/11/24 05:02:20, kbr wrote:
> > The new code path does not currently support 10.5. The consequence is that
> > plugins using the InvalidatingCoreAnimation rendering model will not work
> > on this version of Mac OS.
> 
> Is there a plan for resolving that, or do we just want to give up on CA
support
> on 10.5? On one hand, ICA is currently broken (I've had a couple reports that
> repaint calls aren't actually triggering a repaint), so we won't really
regress
> anything new, but on the other hand these requests are from plugin vendors who
> want to use ICA on 10.5, so I was hoping to fix it.

It's technically feasible. I personally have no plan to address it but would be
happy to help make it work. Should I file a follow-on bug?

[Ken Russell (switch to Gerrit), 2011-11-28 23:40:15]

On 2011/11/28 10:09:04, stuartmorgan wrote:
> Have you done any tests that actually test the compositing? i.e., that
> transparency works, and that overlaying HTML content works?

I haven't, but have verified using --show-composited-layer-borders that a
compositing layer is correctly created to host the plugin. Since the compositor
is very well tested at this point, I assume that these scenarios should work
fine.

> We could either construct some tests, or we could see if Flash currently
honors
> the flag in crbug.com/40997 and then test some wmode=transparent cases.

Happy to run more tests, though if new ones are needed, I'd appreciate help
constructing them. The behavior of CA plugins already seems to be improved with
this patch. For example, some of the pages I tested contain Flash content which
takes a long time to load; these pages behave much better during tab switching
with this patch.

Since the M17 deadline is approaching, I suggest the best avenue forward is to
land this ASAP and then do follow-on testing and work; for example, fixing
crbug.com/40997 should be easy.

http://codereview.chromium.org/8678037/diff/6003/content/common/plugin_messag...
File content/common/plugin_messages.h (right):

http://codereview.chromium.org/8678037/diff/6003/content/common/plugin_messag...
content/common/plugin_messages.h:449: bool /* opaque */)
On 2011/11/28 10:09:04, stuartmorgan wrote:
> Do we expect to need opaque in the future for some non-NPAPI use? CA plugins
> should always be considered transparent, so if not we can remove this
argument.
> IIRC the argument on the other function was when we were trying to hack up an
> intermediate state where some CA plugins were opaque and others weren't, as a
> stopgap to having compositing.

I wasn't sure whether the opaque flag was needed. I vaguely recall that it was
needed to distinguish between Core Animation and Pepper 3D. Removed.

http://codereview.chromium.org/8678037/diff/6003/content/plugin/webplugin_acc...
File content/plugin/webplugin_accelerated_surface_proxy_mac.cc (right):

http://codereview.chromium.org/8678037/diff/6003/content/plugin/webplugin_acc...
content/plugin/webplugin_accelerated_surface_proxy_mac.cc:33: if (composited)
On 2011/11/28 10:09:04, stuartmorgan wrote:
> So if !composited, failure to create a surface returns a surfaceless object?
Is
> that actually what we want?

That's the way the current code path works and I think it's safest to leave it
that way. Once we can throw out the old code path we can add DCHECKS throughout
this class that surface_ != NULL.

http://codereview.chromium.org/8678037/diff/6003/ui/gfx/surface/accelerated_s...
File ui/gfx/surface/accelerated_surface_mac.cc (right):

http://codereview.chromium.org/8678037/diff/6003/ui/gfx/surface/accelerated_s...
ui/gfx/surface/accelerated_surface_mac.cc:149: static GLuint
CreateTexture(GLenum target) {
On 2011/11/28 10:09:04, stuartmorgan wrote:
> Needs a method-level comment (which should probably explain ownership of the
> returned texture)

Done.

http://codereview.chromium.org/8678037/diff/6003/webkit/plugins/npapi/plugin_...
File webkit/plugins/npapi/plugin_host.cc (right):

http://codereview.chromium.org/8678037/diff/6003/webkit/plugins/npapi/plugin_...
webkit/plugins/npapi/plugin_host.cc:842: composited ?
On 2011/11/28 10:09:04, stuartmorgan wrote:
> Bring this up to the previous line?

Done.

http://codereview.chromium.org/8678037/diff/6003/webkit/plugins/npapi/webplug...
File webkit/plugins/npapi/webplugin_delegate_impl.h (right):

http://codereview.chromium.org/8678037/diff/6003/webkit/plugins/npapi/webplug...
webkit/plugins/npapi/webplugin_delegate_impl.h:478: bool composited_; // If CA
plugin, whether it's rendering via compositor.
On 2011/11/28 10:09:04, stuartmorgan wrote:
> Needs one more space before //

Done.

[Ken Russell (switch to Gerrit), 2011-11-29 03:55:09]

bauerb: looking for backup OWNERS review of webkit/plugins/npapi/ in case ananta
is out tomorrow.

[stuartmorgan, 2011-11-29 11:46:40]

On 2011/11/28 23:00:03, kbr wrote:
> It's technically feasible. I personally have no plan to address it but would
be
> happy to help make it work. Should I file a follow-on bug?

Sure, sounds good. We should talk about whether we actually want to do it, and
loop Mark in to discuss the future of 10.5 in general, but it would be nice to
have a bug tracking that discussion at least.

Basically I did it before because it was easy given the 10.5 surface-wrapper
support, even though there was not actually any demand at the time; now there's
some demand, but we need to weigh that against the question of how hard it would
be to make work in the new model.

[stuartmorgan, 2011-11-29 11:49:39]

LGTM

My questions about testing the actual compositing cases were mostly out of
curiosity; I'm all for being able to move to the new architecture as a first
step even if for some crazy reason the new functionality doesn't actually work
yet.

I think I can spare some cycles to do the Flash testing with a local patch for
40997 once this lands.

[jam, 2011-11-29 18:56:21]

http://codereview.chromium.org/8678037/diff/14001/webkit/plugins/npapi/plugin...
File webkit/plugins/npapi/plugin_host.cc (right):

http://codereview.chromium.org/8678037/diff/14001/webkit/plugins/npapi/plugin...
webkit/plugins/npapi/plugin_host.cc:77: // We can't reference the
kDisableCompositedCoreAnimationPlugins
duplicating the switch seems unfortunate. perhaps this can go in
ui/gfx/compositor/compositor_switches.h?

[Ben Goodger (Google), 2011-11-29 19:03:22]

lgtm

[Bernhard Bauer, 2011-11-29 21:03:38]

webkit/plugins/ LGTM.

[Ken Russell (switch to Gerrit), 2011-11-30 00:23:32]

http://codereview.chromium.org/8678037/diff/14001/webkit/plugins/npapi/plugin...
File webkit/plugins/npapi/plugin_host.cc (right):

http://codereview.chromium.org/8678037/diff/14001/webkit/plugins/npapi/plugin...
webkit/plugins/npapi/plugin_host.cc:77: // We can't reference the
kDisableCompositedCoreAnimationPlugins
On 2011/11/29 18:56:21, John Abd-El-Malek wrote:
> duplicating the switch seems unfortunate. perhaps this can go in
> ui/gfx/compositor/compositor_switches.h?

Thanks for the suggestion. Ended up putting it in ui_base_switches.h/cc after
talking with ben.

[jam, 2011-11-30 00:25:09]

webkit lgtm

[commit-bot: I haz the power, 2011-11-30 02:00:12]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kbr@chromium.org/8678037/27001

[commit-bot: I haz the power, 2011-11-30 04:42:24]

Change committed as 112126