Unobfuscate RWHVMac accelerated compositing code path by separating it from plugin code

Unobfuscate RWHVMac accelerated compositing code path by separating it from plugin code.

This simplification (and multi-race-condition-bug-fix) unfortunately comes with one subtle regression which is that as soon as a OSX chrome window renders accelerated content, it becomes permanently non-opaque. This causes the square gray lines to show up in the upper window corners that currently only show up if a tab has accelerated content. When I tried to support dynamically changing setOpaque, there were rare occurrences of black/transparent flashing when rapidly switching tabs between GPU and non-GPU content. Since we know that we have to fix the window border issue for real (crbug.com/56154) a little more of it is an acceptable trade-off for a reduction in flashing.

This cleanup also seems to fix:
- all black/transparent flashing (I can't repro it anymore)
- window immobilization (104996)
- slider widget jumps around during window resize (104996)

BUG=121129, 56154, 104996, 119640, 81733
TEST=Open lots of webgl and software tabs and rapidly switch between them, make sure things update properly, resize window, etc.

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

[jbates, 2012-04-04 00:53:21]

kbr: please review
sky: content/browser/OWNERS
avi: content/OWNERS

[Avi (use Gerrit), 2012-04-04 02:54:43]

This is way over my head, but you have owner approval. LGTM. Get approval from
someone who has a clue.

CC-ing Nico. Did you ever find an answer to the shadow question?

[Nico, 2012-04-04 03:18:45]

As discussed over irc, users seem to think the window shadow regression is a lot
worse than the occasional black flash.

[jbates, 2012-04-04 16:56:41]

On 2012/04/04 03:18:45, Nico wrote:
> As discussed over irc, users seem to think the window shadow regression is a
lot
> worse than the occasional black flash.

Are we talking about the shadow or the subtle gray lines in the top corners? (I
don't think the shadow changes with this CL.) I'm a graphics guy and I didn't
notice the gray lines until it was pointed out to me by bug 56154.

Going forward, with flash triggering accelerated compositing, many more websites
than before will exhibit the square borders. The jank and bugs that this CL
fixes seem worth a temporary regression to subtle square borders that I'd argue
are only noticed by very particular users. Whereas, occasional black/transparent
flashes are going to be noticed by most users. I'm concerned about how flashing
affects the average user's perception of chrome's quality.

[Ken Russell (switch to Gerrit), 2012-04-04 17:27:17]

LGTM. Nice work. It's great that this fixes so many preexisting issues.

A few minor nits. Also, please test by manually returning NULL from
IOSurfaceSupportImpl::GetInstance in
src/ui/gfx/surface/io_surface_support_mac.cc . That will exercise the 10.5 code
path, so we can make sure that the software fallbacks work correctly.

https://chromiumcodereview.appspot.com/9958100/diff/7001/content/browser/rend...
File content/browser/renderer_host/compositing_iosurface_mac.h (right):

https://chromiumcodereview.appspot.com/9958100/diff/7001/content/browser/rend...
content/browser/renderer_host/compositing_iosurface_mac.h:32: void
DrawIOSurface(uint64 io_surface_handle, NSView* view);
I think this should be made private, so that the public API is just SetIOSurface
and DrawLastIOSurface. Anyone wanting to call DrawIOSurface could make those two
calls in succession.

https://chromiumcodereview.appspot.com/9958100/diff/7001/content/browser/rend...
content/browser/renderer_host/compositing_iosurface_mac.h:83: NSOpenGLContext*
glContext,
Indentation is off.

https://chromiumcodereview.appspot.com/9958100/diff/7001/content/browser/rend...
File content/browser/renderer_host/compositing_iosurface_mac.mm (right):

https://chromiumcodereview.appspot.com/9958100/diff/7001/content/browser/rend...
content/browser/renderer_host/compositing_iosurface_mac.mm:79:
glContext.release(), cglContext);
Indentation is off.

https://chromiumcodereview.appspot.com/9958100/diff/7001/content/browser/rend...
content/browser/renderer_host/compositing_iosurface_mac.mm:103: NSView* view) {
Indentation.

https://chromiumcodereview.appspot.com/9958100/diff/7001/content/browser/rend...
File content/browser/renderer_host/render_widget_host_view_mac.mm (right):

https://chromiumcodereview.appspot.com/9958100/diff/7001/content/browser/rend...
content/browser/renderer_host/render_widget_host_view_mac.mm:1695: // appears
during tab switching otherwise.
I think a TODO should be added here. I don't remember exactly how the
bookkeeping works right now, but if the underlay surface count could go back to
0 when all composited tabs were removed from a window (i.e., all tabs rendered
in software), would the flashes upon tab switch still be present?

https://chromiumcodereview.appspot.com/9958100/diff/7001/content/browser/rend...
content/browser/renderer_host/render_widget_host_view_mac.mm:1807:
renderWidgetHostView_->CompositorSwapBuffersComplete();
It's a little confusing that this is on the software path. Presumably it's
needed to unblock the GPU process during switches from composited to
non-composited mode or similar. A comment would be useful.

[Nico, 2012-04-04 17:56:56]

On 2012/04/04 16:56:41, jbates wrote:
> Are we talking about the shadow or the subtle gray lines in the top corners?

I think those lines are drawn as part of the shadow.

> (I don't think the shadow changes with this CL.) I'm a graphics guy and I
didn't
> notice the gray lines until it was pointed out to me by bug 56154.

As mentioned over gtalk, when we had a regression to always have the non-rounded
corners, users did complain.

> Going forward, with flash triggering accelerated compositing, many more
websites
> than before will exhibit the square borders. The jank and bugs that this CL
> fixes seem worth a temporary regression to subtle square borders that I'd
argue
> are only noticed by very particular users. Whereas, occasional
black/transparent
> flashes are going to be noticed by most users. I'm concerned about how
flashing
> affects the average user's perception of chrome's quality.

Going forward, we should fix both issues since they both affect the average
user's perception of chrome's quality. We should fix the shadow thing first.

[Ken Russell (switch to Gerrit), 2012-04-04 18:32:12]

On 2012/04/04 17:56:56, Nico wrote:
> On 2012/04/04 16:56:41, jbates wrote:
> > Are we talking about the shadow or the subtle gray lines in the top corners?
> 
> I think those lines are drawn as part of the shadow.
> 
> > (I don't think the shadow changes with this CL.) I'm a graphics guy and I
> didn't
> > notice the gray lines until it was pointed out to me by bug 56154.
> 
> As mentioned over gtalk, when we had a regression to always have the
non-rounded
> corners, users did complain.
> 
> > Going forward, with flash triggering accelerated compositing, many more
> websites
> > than before will exhibit the square borders. The jank and bugs that this CL
> > fixes seem worth a temporary regression to subtle square borders that I'd
> argue
> > are only noticed by very particular users. Whereas, occasional
> black/transparent
> > flashes are going to be noticed by most users. I'm concerned about how
> flashing
> > affects the average user's perception of chrome's quality.
> 
> Going forward, we should fix both issues since they both affect the average
> user's perception of chrome's quality. We should fix the shadow thing first.

If you mean that this patch should not be committed at all until the issue of
non-rounded shadows for windows containing composited content is fixed, I
disagree. This patch fixes multiple nasty behaviors associated with the display
of composited content that are objectively (not subjectively) worse than the
shadow not being rounded.

[Nico, 2012-04-04 18:37:35]

> are objectively (not subjectively) worse than the
> shadow not being rounded.

Uh huh.

[Ken Russell (switch to Gerrit), 2012-04-04 18:39:23]

On 2012/04/04 18:37:35, Nico wrote:
> > are objectively (not subjectively) worse than the
> > shadow not being rounded.
> 
> Uh huh.

To confirm, you support committing this work, and then doing follow-on work to
fix the rounded corner issue with windows that (ever) contained composited
content?

[Nico, 2012-04-04 18:41:22]

On 2012/04/04 18:39:23, kbr wrote:
> On 2012/04/04 18:37:35, Nico wrote:
> > > are objectively (not subjectively) worse than the
> > > shadow not being rounded.
> > 
> > Uh huh.
> 
> To confirm, you support committing this work, and then doing follow-on work to
> fix the rounded corner issue with windows that (ever) contained composited
> content?

No, I was commenting on the "objectively (not subjectively)" bit.

Code wins, so since you guys are working on this your opinion trumps mine, and
you're free to land. I disagree with that order though.

[jbates, 2012-04-04 19:21:50]

https://chromiumcodereview.appspot.com/9958100/diff/7001/content/browser/rend...
File content/browser/renderer_host/compositing_iosurface_mac.h (right):

https://chromiumcodereview.appspot.com/9958100/diff/7001/content/browser/rend...
content/browser/renderer_host/compositing_iosurface_mac.h:32: void
DrawIOSurface(uint64 io_surface_handle, NSView* view);
On 2012/04/04 17:27:17, kbr wrote:
> I think this should be made private, so that the public API is just
SetIOSurface
> and DrawLastIOSurface. Anyone wanting to call DrawIOSurface could make those
two
> calls in succession.

Done. (Just removed since there's no use-case for it anyway.)

https://chromiumcodereview.appspot.com/9958100/diff/7001/content/browser/rend...
content/browser/renderer_host/compositing_iosurface_mac.h:83: NSOpenGLContext*
glContext,
On 2012/04/04 17:27:17, kbr wrote:
> Indentation is off.

Done.

https://chromiumcodereview.appspot.com/9958100/diff/7001/content/browser/rend...
File content/browser/renderer_host/compositing_iosurface_mac.mm (right):

https://chromiumcodereview.appspot.com/9958100/diff/7001/content/browser/rend...
content/browser/renderer_host/compositing_iosurface_mac.mm:79:
glContext.release(), cglContext);
On 2012/04/04 17:27:17, kbr wrote:
> Indentation is off.

Done.

https://chromiumcodereview.appspot.com/9958100/diff/7001/content/browser/rend...
content/browser/renderer_host/compositing_iosurface_mac.mm:103: NSView* view) {
On 2012/04/04 17:27:17, kbr wrote:
> Indentation.

Done.

https://chromiumcodereview.appspot.com/9958100/diff/7001/content/browser/rend...
File content/browser/renderer_host/render_widget_host_view_mac.mm (right):

https://chromiumcodereview.appspot.com/9958100/diff/7001/content/browser/rend...
content/browser/renderer_host/render_widget_host_view_mac.mm:1695: // appears
during tab switching otherwise.
On 2012/04/04 17:27:17, kbr wrote:
> I think a TODO should be added here. I don't remember exactly how the
> bookkeeping works right now, but if the underlay surface count could go back
to
> 0 when all composited tabs were removed from a window (i.e., all tabs rendered
> in software), would the flashes upon tab switch still be present?

Done. Added a TODO about cleaning up this code and pointed to bug 56154.
Allowing the count to go back to zero when the RWHVM was removed from the window
(backgrounded) caused the occasional black flash so I expect the same to occur
if the count went to zero when the RWHVM was deleted (which may occur during
back/forward navigation in addition to closing tabs).

https://chromiumcodereview.appspot.com/9958100/diff/7001/content/browser/rend...
content/browser/renderer_host/render_widget_host_view_mac.mm:1807:
renderWidgetHostView_->CompositorSwapBuffersComplete();
On 2012/04/04 17:27:17, kbr wrote:
> It's a little confusing that this is on the software path. Presumably it's
> needed to unblock the GPU process during switches from composited to
> non-composited mode or similar. A comment would be useful.

Done.

[sky, 2012-04-04 20:58:08]

I'm rubber stamping this. The other reviews have you covered: LGTM

[commit-bot: I haz the power, 2012-04-05 01:09:37]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jbates@chromium.org/9958100/9002

[commit-bot: I haz the power, 2012-04-05 01:27:16]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jbates@chromium.org/9958100/22001

[commit-bot: I haz the power, 2012-04-05 01:52:21]

Try job failure for 9958100-22001 (retry) on linux_chromeos for step "compile"
(clobber build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[commit-bot: I haz the power, 2012-04-05 16:15:03]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jbates@chromium.org/9958100/22001

[commit-bot: I haz the power, 2012-04-05 19:39:34]

Change committed as 130989

[Nico, 2012-04-06 16:01:24]

Does this do the right thing on 10.5 (which doesn't have IOSurfaces)?

http://codereview.chromium.org/9958100/diff/22001/content/browser/renderer_ho...
File content/browser/renderer_host/render_widget_host_view_mac.mm (right):

http://codereview.chromium.org/9958100/diff/22001/content/browser/renderer_ho...
content/browser/renderer_host/render_widget_host_view_mac.mm:1780: - (void)
globalFrameDidChange:(NSNotification*)notification {
no space after )

[jbates, 2012-04-06 16:41:32]

On 2012/04/06 16:01:24, Nico wrote:
> Does this do the right thing on 10.5 (which doesn't have IOSurfaces)?

10.5 will no longer get accelerated support with this patch.

> 
>
http://codereview.chromium.org/9958100/diff/22001/content/browser/renderer_ho...
> File content/browser/renderer_host/render_widget_host_view_mac.mm (right):
> 
>
http://codereview.chromium.org/9958100/diff/22001/content/browser/renderer_ho...
> content/browser/renderer_host/render_widget_host_view_mac.mm:1780: - (void)
> globalFrameDidChange:(NSNotification*)notification {
> no space after )

[Nico, 2012-04-06 16:42:46]

On Fri, Apr 6, 2012 at 9:41 AM,  <jbates@chromium.org> wrote:
> On 2012/04/06 16:01:24, Nico wrote:
>>
>> Does this do the right thing on 10.5 (which doesn't have IOSurfaces)?
>
>
> 10.5 will no longer get accelerated support with this patch.

Did it before? What do you mean with "accelerated support"?

[jbates, 2012-04-06 16:45:58]

On 2012/04/06 16:42:46, Nico wrote:
> On Fri, Apr 6, 2012 at 9:41 AM,  <mailto:jbates@chromium.org> wrote:
> > On 2012/04/06 16:01:24, Nico wrote:
> >>
> >> Does this do the right thing on 10.5 (which doesn't have IOSurfaces)?
> >
> >
> > 10.5 will no longer get accelerated support with this patch.
> 
> Did it before? What do you mean with "accelerated support"?

I meant to say accelerated compositing and associated features. Accelerated
compositing will fail to initialize, but accelerated plugins should continue to
work with the existing plugin code path. It did work before via the
DIBImageTransportSurface.

[Nico, 2012-04-06 17:38:42]

On Fri, Apr 6, 2012 at 9:45 AM,  <jbates@chromium.org> wrote:
> On 2012/04/06 16:42:46, Nico wrote:
>
>> On Fri, Apr 6, 2012 at 9:41 AM,  <mailto:jbates@chromium.org> wrote:
>> > On 2012/04/06 16:01:24, Nico wrote:
>> >>
>> >> Does this do the right thing on 10.5 (which doesn't have IOSurfaces)?
>> >
>> >
>> > 10.5 will no longer get accelerated support with this patch.
>
>
>> Did it before? What do you mean with "accelerated support"?
>
>
> I meant to say accelerated compositing and associated features. Accelerated
> compositing will fail to initialize, but accelerated plugins should continue
> to
> work with the existing plugin code path. It did work before via the
> DIBImageTransportSurface.

So this CL breaks e.g. poster circle on 10.5?

[jbates, 2012-04-06 17:54:58]

On 2012/04/06 17:38:42, Nico wrote:
> On Fri, Apr 6, 2012 at 9:45 AM,  <mailto:jbates@chromium.org> wrote:
> > On 2012/04/06 16:42:46, Nico wrote:
> >
> >> On Fri, Apr 6, 2012 at 9:41 AM, &nbsp;<mailto:jbates@chromium.org> wrote:
> >> > On 2012/04/06 16:01:24, Nico wrote:
> >> >>
> >> >> Does this do the right thing on 10.5 (which doesn't have IOSurfaces)?
> >> >
> >> >
> >> > 10.5 will no longer get accelerated support with this patch.
> >
> >
> >> Did it before? What do you mean with "accelerated support"?
> >
> >
> > I meant to say accelerated compositing and associated features. Accelerated
> > compositing will fail to initialize, but accelerated plugins should continue
> > to
> > work with the existing plugin code path. It did work before via the
> > DIBImageTransportSurface.
> 
> So this CL breaks e.g. poster circle on 10.5?

Yes, 3D CSS and WebGL will no longer work on 10.5. Supporting the DIB code path
has been the cause of some of the race conditions because it required the
browser process to be involved in the creation of surfaces. IOSurfaces match our
Windows implementation which does not require unscheduling the GPU process to
reallocate the surface. I think this is an important improvement that will help
reduce future regressions. Feel free to come by to discuss.

[vangelis, 2012-04-06 17:55:37]

On 2012/04/06 17:54:58, jbates wrote:
> On 2012/04/06 17:38:42, Nico wrote:
> > On Fri, Apr 6, 2012 at 9:45 AM,  <mailto:jbates@chromium.org> wrote:
> > > On 2012/04/06 16:42:46, Nico wrote:
> > >
> > >> On Fri, Apr 6, 2012 at 9:41 AM, &nbsp;<mailto:jbates@chromium.org> wrote:
> > >> > On 2012/04/06 16:01:24, Nico wrote:
> > >> >>
> > >> >> Does this do the right thing on 10.5 (which doesn't have IOSurfaces)?
> > >> >
> > >> >
> > >> > 10.5 will no longer get accelerated support with this patch.
> > >
> > >
> > >> Did it before? What do you mean with "accelerated support"?
> > >
> > >
> > > I meant to say accelerated compositing and associated features.
Accelerated
> > > compositing will fail to initialize, but accelerated plugins should
continue
> > > to
> > > work with the existing plugin code path. It did work before via the
> > > DIBImageTransportSurface.
> > 
> > So this CL breaks e.g. poster circle on 10.5?
> 
> Yes, 3D CSS and WebGL will no longer work on 10.5. Supporting the DIB code
path
> has been the cause of some of the race conditions because it required the
> browser process to be involved in the creation of surfaces. IOSurfaces match
our
> Windows implementation which does not require unscheduling the GPU process to
> reallocate the surface. I think this is an important improvement that will
help
> reduce future regressions. Feel free to come by to discuss.

I don't think 3D CSS ever worked in 10.5 . WebGL was working via a readback.

[Nico, 2012-04-06 18:00:40]

On Fri, Apr 6, 2012 at 10:54 AM,  <jbates@chromium.org> wrote:
> On 2012/04/06 17:38:42, Nico wrote:
>>
>> On Fri, Apr 6, 2012 at 9:45 AM,  <mailto:jbates@chromium.org> wrote:
>> > On 2012/04/06 16:42:46, Nico wrote:
>> >
>> >> On Fri, Apr 6, 2012 at 9:41 AM, &nbsp;<mailto:jbates@chromium.org>
>> >> wrote:
>> >> > On 2012/04/06 16:01:24, Nico wrote:
>> >> >>
>> >> >> Does this do the right thing on 10.5 (which doesn't have
>> >> >> IOSurfaces)?
>> >> >
>> >> >
>> >> > 10.5 will no longer get accelerated support with this patch.
>> >
>> >
>> >> Did it before? What do you mean with "accelerated support"?
>> >
>> >
>> > I meant to say accelerated compositing and associated features.
>> > Accelerated
>> > compositing will fail to initialize, but accelerated plugins should
>> > continue
>> > to
>> > work with the existing plugin code path. It did work before via the
>> > DIBImageTransportSurface.
>
>
>> So this CL breaks e.g. poster circle on 10.5?
>
>
> Yes, 3D CSS and WebGL will no longer work on 10.5. Supporting the DIB code
> path
> has been the cause of some of the race conditions because it required the
> browser process to be involved in the creation of surfaces. IOSurfaces match
> our
> Windows implementation which does not require unscheduling the GPU process
> to
> reallocate the surface. I think this is an important improvement that will
> help
> reduce future regressions. Feel free to come by to discuss.

I'm fine with not supporting WebGL on 10.5 (I voted for not supporting
it on 10.5 when we wrote this code initially), but you might want to
check with your PM if they see any issues with this. And the CL
description should've mentioned this explicitly as it's a significant
behavior change.

[vangelis, 2012-04-06 18:38:58]

> I'm fine with not supporting WebGL on 10.5 (I voted for not supporting
> it on 10.5 when we wrote this code initially), but you might want to
> check with your PM if they see any issues with this. And the CL
> description should've mentioned this explicitly as it's a significant
> behavior change.

Do we know for a fact that anything changed for WebGL support with this change?
Even before it, WebGL wasn't going through the composited path in 10.5 .

[Nico, 2012-04-06 18:43:22]

On Fri, Apr 6, 2012 at 11:38 AM,  <vangelis@google.com> wrote:
>
>> I'm fine with not supporting WebGL on 10.5 (I voted for not supporting
>> it on 10.5 when we wrote this code initially), but you might want to
>> check with your PM if they see any issues with this. And the CL
>> description should've mentioned this explicitly as it's a significant
>> behavior change.
>
>
> Do we know for a fact that anything changed for WebGL support with this
> change?

I don't, that's why I'm asking :-)

[jbates, 2012-04-06 20:14:35]

On 2012/04/06 18:43:22, Nico wrote:
> On Fri, Apr 6, 2012 at 11:38 AM,  <mailto:vangelis@google.com> wrote:
> >
> >> I'm fine with not supporting WebGL on 10.5 (I voted for not supporting
> >> it on 10.5 when we wrote this code initially), but you might want to
> >> check with your PM if they see any issues with this. And the CL
> >> description should've mentioned this explicitly as it's a significant
> >> behavior change.
> >
> >
> > Do we know for a fact that anything changed for WebGL support with this
> > change?
> 
> I don't, that's why I'm asking :-)

This change only disables DIBImageTransportSurface for 10.5. But the 10.5 bots
did show WebGLAllowed failures after this CL landed, so it seems WebGL no longer
works. I guess WebGL was using DIBImageTransportSurface?