Move context-related callbacks into OutputSurface

Move swapcomplete callback into OutputSurface

Things like swap are concepts of an OutputSurface, not a context. Moving the
callbacks to OutputSurface will also reduce cc's knowledge of the context
guts and let us swap out the GL interface.

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

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

[jamesr, 2013-03-08 03:07:35]

I've flubbed the lost context hookup somehow so a few cc_unittests in
LayerTreeHostContextTest fail, but this is looking fairly nice otherwise.  The
goal here is to first isolate these callbacks into cc::OutputSurface (exposed
through OutputSurfaceClient) and then move the code that registers+invokes the
callbacks into subclasses of cc::OutputSurface so that, for instance, the
content/renderer/ implementation can hook directly into
WebGraphicsContext3DCommandBufferImpl or even something underneath that so we
don't have to patch WebGraphicsContext3D every time we need a new callback.

[piman, 2013-03-08 05:11:56]

https://codereview.chromium.org/12545018/diff/4004/cc/output_surface.h
File cc/output_surface.h (right):

https://codereview.chromium.org/12545018/diff/4004/cc/output_surface.h#newcod...
cc/output_surface.h:101: scoped_ptr<OutputSurfaceCallbacks> callbacks_;
nit: we probably want to have the callbacks outlive the context (i.e. a few
lines higher), just in case the context calls it at destruction.

https://codereview.chromium.org/12545018/diff/4004/cc/output_surface_client.h
File cc/output_surface_client.h (right):

https://codereview.chromium.org/12545018/diff/4004/cc/output_surface_client.h...
cc/output_surface_client.h:20: virtual void OnSwapBuffersComplete() = 0;
Does it make any sense to merge this with OnSendFrameToParentCompositorAck
somehow? They sort of have the same purpose... I don't know if it makes sense.

[piman, 2013-03-08 05:12:28]

By the way, this is cool.

[jamesr, 2013-03-14 23:39:29]

This passes tests and seems to work in practice as far as I can tell.  Turns out
there was a very subtle difference in the timing of callback registration - if
the very first makeContextCurrent() call (which is made from
OutputSurface::BindToClient) succeeded, but the second (which is made from
GLRenderer::Initialize) failed, then the current code would *not* register for
onContextLost but the earlier versions of this patch would.  I've made callback
registration and clearing explicit on OutputSurface to get the exact same timing
as the previous code.

I believe that SendFrameToParentCompositorAck() and the memory management
callbacks can both be migrated to this model, but neither are entirely trivial
and I'd rather take them one a time.

[piman, 2013-03-15 03:20:38]

https://codereview.chromium.org/12545018/diff/18001/cc/layer_tree_host_impl.cc
File cc/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/12545018/diff/18001/cc/layer_tree_host_impl.c...
cc/layer_tree_host_impl.cc:1248: output_surface_->RegisterCallbacks();
It's rather unfortunate that we have this very complicated dance to initialize
the OutputSurface, which is at the API boundary.

What would it take for the callbacks to not behave badly if we were setting them
at Bind time?

https://codereview.chromium.org/12545018/diff/18001/cc/output_surface.h
File cc/output_surface.h (right):

https://codereview.chromium.org/12545018/diff/18001/cc/output_surface.h#newco...
cc/output_surface.h:81: void ClearCallbacks();
This isn't called anywhere (that I can see?)

[jamesr, 2013-03-24 23:42:23]

This passes tests. The difference in behavior was whether we fired lost context
notifications before the GLRenderer successfully initialized, so this just
suppresses DidLoseOutputSurface calls if the context isn't initialized.

This moves all callbacks except for the memory allocation callback, which is a
bit more complicated since it has a WebKit type.  I'll work on that seperately
and see (depending on how big it ends up being) if it makes more sense to fold
into this or treat it separately.

[piman, 2013-03-25 18:18:06]

lgtm

[enne (OOO), 2013-03-25 18:29:19]

Wow, this really cleans things up!

https://codereview.chromium.org/12545018/diff/39001/cc/output/output_surface.cc
File cc/output/output_surface.cc (right):

https://codereview.chromium.org/12545018/diff/39001/cc/output/output_surface....
cc/output/output_surface.cc:28: public
WebKit::WebGraphicsContext3D::WebGraphicsSwapBuffersCompleteCallbackCHROMIUM,
style nit: 80 col.  ;)

https://codereview.chromium.org/12545018/diff/39001/cc/output/output_surface....
cc/output/output_surface.cc:32: : client_(client) {}
style nit: four space indent.

https://codereview.chromium.org/12545018/diff/39001/cc/trees/layer_tree_host_...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/12545018/diff/39001/cc/trees/layer_tree_host_...
cc/trees/layer_tree_host_impl.cc:1055: if (renderer_)
Why do you need to not send this notification if there's no renderer?

[danakj, 2013-03-25 18:40:43]

https://codereview.chromium.org/12545018/diff/39001/cc/trees/layer_tree_host_...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/12545018/diff/39001/cc/trees/layer_tree_host_...
cc/trees/layer_tree_host_impl.cc:1055: if (renderer_)
On 2013/03/25 18:29:19, enne wrote:
> Why do you need to not send this notification if there's no renderer?

The unit tests were built in a world where losing the context when there's no
renderer_ would not call this, since the callback came through the renderer
before.

Should we TODO() to adjust the tests to expect this instead?

[jamesr, 2013-03-25 20:34:21]

On 2013/03/25 18:40:43, danakj wrote:
>
https://codereview.chromium.org/12545018/diff/39001/cc/trees/layer_tree_host_...
> File cc/trees/layer_tree_host_impl.cc (right):
> 
>
https://codereview.chromium.org/12545018/diff/39001/cc/trees/layer_tree_host_...
> cc/trees/layer_tree_host_impl.cc:1055: if (renderer_)
> On 2013/03/25 18:29:19, enne wrote:
> > Why do you need to not send this notification if there's no renderer?
> 
> The unit tests were built in a world where losing the context when there's no
> renderer_ would not call this, since the callback came through the renderer
> before.

That's right.  The unit tests behave differently if the lost context callback
arrives in between OutputSurface::BindToClient() and GLRenderer initialization,
the latter of which fails in some unit tests because the second
makeContextCurrent() call fails.

> 
> Should we TODO() to adjust the tests to expect this instead?

I'll leave a TODO() on myself to try to resolve this.  To be honest I can't
really understand exactly what the unit tests are trying to get at so I've just
preserved the existing behavior exactly.

[commit-bot: I haz the power, 2013-03-25 21:10:05]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jamesr@chromium.org/12545018/48002

[commit-bot: I haz the power, 2013-03-26 03:29:55]

The commit queue went berserk retrying too often for a
seemingly flaky test. Builder is android_dbg, revision is HEAD

[commit-bot: I haz the power, 2013-03-26 03:40:46]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jamesr@chromium.org/12545018/48002

[commit-bot: I haz the power, 2013-03-26 06:53:02]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jamesr@chromium.org/12545018/48002

[commit-bot: I haz the power, 2013-03-26 12:47:52]

The commit queue went berserk retrying too often for a
seemingly flaky test. Builder is android_dbg, revision is HEAD

[commit-bot: I haz the power, 2013-03-26 15:04:19]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jamesr@chromium.org/12545018/48002

[jamesr, 2013-03-26 19:44:33]

Patch appears to do something weird on android.  Will attempt to figure what
that is before landing.

[jamesr, 2013-04-12 22:21:56]

This still fails on android trybots in contentshell_instrumentation_tests with
failures like this:

C  246s Main  [FAIL]
org.chromium.content.browser.AddressDetectionTest#testAddressLimits:
C  246s Main  junit.framework.AssertionFailedError
C  246s Main  	at
org.chromium.content.browser.ContentDetectionTestBase.assertWaitForPageScaleFactorMatch(ContentDetectionTestBase.java:108)
C  246s Main  	at
org.chromium.content.browser.AddressDetectionTest.testAddressLimits(AddressDetectionTest.java:60)
C  246s Main  	at java.lang.reflect.Method.invokeNative(Native Method)
C  246s Main  	at
android.test.InstrumentationTestCase.runMethod(InstrumentationTestCase.java:214)
C  246s Main  	at
android.test.InstrumentationTestCase.runTest(InstrumentationTestCase.java:199)
C  246s Main  	at
android.test.ActivityInstrumentationTestCase2.runTest(ActivityInstrumentationTestCase2.java:192)
C  246s Main  	at
android.test.AndroidTestRunner.runTest(AndroidTestRunner.java:169)
C  246s Main  	at
android.test.AndroidTestRunner.runTest(AndroidTestRunner.java:154)
C  246s Main  	at
android.test.InstrumentationTestRunner.onStart(InstrumentationTestRunner.java:545)
C  246s Main  	at
android.app.Instrumentation$InstrumentationThread.run(Instrumentation.java:1551)

but it doesn't fail for me locally when running these tests on a prime.  Alex -
would you mind eyeballing the change and trying to see what I might have messed
up here?  I'm guessing there is some subtle change in the order of something,
but I really am at a loss for what it could be and am not familiar enough with
how contentshell_instrumentation_tests work on android to attempt debugging via
trybot.

[aelias_OOO_until_Jul13, 2013-04-15 04:36:55]

[+aruslan who wrote this part of the test infra] That code waits until the page
scale factor sent in the frame metadata converges to the expected value (set in
the meta viewport tag) before the test starts to send input events.

I can't see what in your code that might've broken that, my first guess would be
general trybot flakiness (as these instrumentation tests have been flaky in the
past).

[aruslan, 2013-04-15 13:47:50]

On 2013/04/15 04:36:55, aelias wrote:
> [+aruslan who wrote this part of the test infra] That code waits until the
page
> scale factor sent in the frame metadata converges to the expected value (set
in
> the meta viewport tag) before the test starts to send input events.
> 
> I can't see what in your code that might've broken that, my first guess would
be
> general trybot flakiness (as these instrumentation tests have been flaky in
the
> past).

Given that _all_ tests waiting for the correct scale factor have failed, I'd
suspect some systemic issue.  Furthermore, I don't think any of these tests were
flaky once the workaround was deployed to wait till the scale factor in the
frame metadata converges to the expected value (this is the workaround to
http://crbug.com/179511).

The cause of the failure might be something as stupid as the floating point
equality comparison in "expected==actual".

Would it be possible to re-run Android tests with added logging of the expected
scale factor and the actual frame metadata scale factors?
Something like 
Log.e("blahblah", "Expected scale factor = " + expectedScale + "; the frame
metadata scale factor = " + getContentViewCore().getScale() + "; equals = " +
(getContentViewCore().getScale() == expectedScale));
in the assertWaitForPageScaleFactorMatch()
in
content/public/android/javatests/src/org/chromium/content/browser/ContentDetectionTestBase.java
?

[aruslan, 2013-04-16 15:04:37]

On 2013/04/12 22:21:56, jamesr wrote:
> This still fails on android trybots in contentshell_instrumentation_tests with
> failures like this:
> 
> C  246s Main  [FAIL]
> org.chromium.content.browser.AddressDetectionTest#testAddressLimits:
> C  246s Main  junit.framework.AssertionFailedError
> C  246s Main  	at
>
org.chromium.content.browser.ContentDetectionTestBase.assertWaitForPageScaleFactorMatch(ContentDetectionTestBase.java:108)
> C  246s Main  	at
>
org.chromium.content.browser.AddressDetectionTest.testAddressLimits(AddressDetectionTest.java:60)
> C  246s Main  	at java.lang.reflect.Method.invokeNative(Native Method)
> C  246s Main  	at
>
android.test.InstrumentationTestCase.runMethod(InstrumentationTestCase.java:214)
> C  246s Main  	at
> android.test.InstrumentationTestCase.runTest(InstrumentationTestCase.java:199)
> C  246s Main  	at
>
android.test.ActivityInstrumentationTestCase2.runTest(ActivityInstrumentationTestCase2.java:192)
> C  246s Main  	at
> android.test.AndroidTestRunner.runTest(AndroidTestRunner.java:169)
> C  246s Main  	at
> android.test.AndroidTestRunner.runTest(AndroidTestRunner.java:154)
> C  246s Main  	at
>
android.test.InstrumentationTestRunner.onStart(InstrumentationTestRunner.java:545)
> C  246s Main  	at
>
android.app.Instrumentation$InstrumentationThread.run(Instrumentation.java:1551)
> 
> but it doesn't fail for me locally when running these tests on a prime.  Alex
-
> would you mind eyeballing the change and trying to see what I might have
messed
> up here?  I'm guessing there is some subtle change in the order of something,
> but I really am at a loss for what it could be and am not familiar enough with
> how contentshell_instrumentation_tests work on android to attempt debugging
via
> trybot.

jamesr@ -- the trybot failures are NOT related to your CL, and I think the tests
are just plain broken (either the tests themselves, or the underlying
functionality).

Flakiness dashboard shows interesting numbers: tests are failing on ICS, but
succeeding on JB: http://crbug.com/231484

[jamesr, 2013-04-16 18:03:11]

On 2013/04/16 15:04:37, aruslan wrote:
> jamesr@ -- the trybot failures are NOT related to your CL, and I think the
tests
> are just plain broken (either the tests themselves, or the underlying
> functionality).

OK, thank you for investigating.  I'll go ahead and land.

> 
> Flakiness dashboard shows interesting numbers: tests are failing on ICS, but
> succeeding on JB: http://crbug.com/231484

[jamesr, 2013-04-16 18:07:23]

Committed patchset #11 manually as r194394 (presubmit successful).

[aelias_OOO_until_Jul13, 2013-04-16 22:10:24]

This patch got reverted because it does break the bots after all.  Sorry about
the confusion.

Our latest theory is that this only breaks on ICS.  I'll try flashing ICS to one
of my devices and see if I can repro the problem.

[jamesr, 2013-04-16 22:30:05]

On 2013/04/16 22:10:24, aelias wrote:
> This patch got reverted because it does break the bots after all.  Sorry about
> the confusion.
> 
> Our latest theory is that this only breaks on ICS.  I'll try flashing ICS to
one
> of my devices and see if I can repro the problem.

Thanks!  That would be super useful.  I was thinking it might be sufficient to
hack up VSyncMonitor.java to always go down the ICS path, even on JB, but I
probably won't get to it today.

[jamesr, 2013-04-17 00:15:34]

https://codereview.chromium.org/12545018/diff/92001/cc/output/gl_renderer.cc
File cc/output/gl_renderer.cc (left):

https://codereview.chromium.org/12545018/diff/92001/cc/output/gl_renderer.cc#...
cc/output/gl_renderer.cc:1893: onSwapBuffersComplete();
aha!  This is the change that broke Android.  As it turns out, Android relies on
ReceiveCompositorFrameAck for rate limiting instead of SwapBuffersComplete
callbacks.  I don't fully understand how this works but it's easy enough to
preserve the behavior.

I think the more interesting questions are:
1.) Given that this completely breaks the world on android (the web content
doesn't display at all), should this have been a louder failure on the
trybots/bots?  The failure we did see was so obscure/racy that none of us
thought it was real.

2.) How is Android set up to swap this way and can we have unit tests using this
path so we make sure it works?

I'll look in to (2).  Not sure what to do about (1)

[jamesr, 2013-04-17 00:20:58]

On 2013/04/17 00:15:34, jamesr wrote:
> 2.) How is Android set up to swap this way and can we have unit tests using
this
> path so we make sure it works?

Answering my one question a bit, the trick here is android sets
cc::switches::kEnableCompositorFrameMessage (aka
"enable-compositor-frame-message") by default which triggers the compositor
frame sending/acking.  I'm not sure how, but it appears that something's also
causing this to suppress the normal swap complete callbacks.  I'll write a unit
test that sets this bit to true and verifies that we treat the ACKs as swap
complete calls, if that's the desired semantics.

[aelias_OOO_until_Jul13, 2013-04-17 00:22:04]

[+sievers and skyostil who are currently working in this area]

[jamesr, 2013-04-17 00:28:19]

Rest of the mysteries appear to be answered in
content/renderer/gpu/compositor_output_surface.cc, assuming android's using
composite-to-mailbox currently.

[aelias_OOO_until_Jul13, 2013-04-17 00:54:23]

Yes, Android is using --composite-to-mailbox and will continue to do so until we
switch to ubercomp.

[no sievers, 2013-04-17 01:11:54]

On 2013/04/17 00:20:58, jamesr wrote:
> On 2013/04/17 00:15:34, jamesr wrote:
> > 2.) How is Android set up to swap this way and can we have unit tests using
> this
> > path so we make sure it works?
> 
> Answering my one question a bit, the trick here is android sets
> cc::switches::kEnableCompositorFrameMessage (aka
> "enable-compositor-frame-message") by default which triggers the compositor
> frame sending/acking.  I'm not sure how, but it appears that something's also
> causing this to suppress the normal swap complete callbacks.

It's because we use MailboxOutputSurface which does not call SwapBuffers(), so
we never get normal swap complete callbacks from WGC3D. We also skip other
backbuffer related calls (glResizeCHROMIUM, glEnsure/glDiscardBackbuffer).

[jamesr, 2013-04-17 01:15:19]

On 2013/04/17 01:11:54, Daniel Sievers wrote:
> On 2013/04/17 00:20:58, jamesr wrote:
> > On 2013/04/17 00:15:34, jamesr wrote:
> > > 2.) How is Android set up to swap this way and can we have unit tests
using
> > this
> > > path so we make sure it works?
> > 
> > Answering my one question a bit, the trick here is android sets
> > cc::switches::kEnableCompositorFrameMessage (aka
> > "enable-compositor-frame-message") by default which triggers the compositor
> > frame sending/acking.  I'm not sure how, but it appears that something's
also
> > causing this to suppress the normal swap complete callbacks.
> 
> It's because we use MailboxOutputSurface which does not call SwapBuffers(), so
> we never get normal swap complete callbacks from WGC3D. We also skip other
> backbuffer related calls (glResizeCHROMIUM, glEnsure/glDiscardBackbuffer).

OK.  That makes sense, although this is why I'm trying to push things like this
out of cc/ and in to the OutputSurface implementation.  I'll just make the
compositor frame ACK equivalent to a SwapBuffers ACK for this patch (since
that's what it is today).

[no sievers, 2013-04-17 01:27:58]

On 2013/04/17 00:15:34, jamesr wrote:> 
> 2.) How is Android set up to swap this way and can we have unit tests using
this
> path so we make sure it works?
> 
> I'll look in to (2).  Not sure what to do about (1)

The only unit tests I added when adding MailboxOutputSurface were ones that work
on the base class alone, because MailboxOutputSurface derives from
CompositorOutputSurface, which has a lot of IPC stuff (might be easier to unit
test if that stuff was pulled out into a helper class).

So basically the unit tests I added make sure that
a) backbuffer-related APIs are *not* called from gl_renderer.cc directly (only
from OutputSurface)
b) SendFrameToParentCompositor is called when settings.compositor_frame_message
is enabled.

I've also been wanting to add a content_browser_test that checks if we can
receive a couple of frames, which would probably have caught this.

[jamesr, 2013-04-17 01:38:22]

This adds a (somewhat dumb) unit test to make sure acking the compositor frame
data produces a SwapComplete callback.

I think the path forward from here is:

1.) Unify SwapBuffers and SendFrameToParent into one call on OutputSurface that
takes the data
2.) Unify the SwapBuffersComplete and FrameToParentCompositorAck into one call
on OutputSurfaceClient

then there won't be any confusion, at least in cc/ land, about what frames are
going up and what are coming back.

piman@ - mind taking a glance at the addition to
layer_tree_host_impl_unittest.cc to see if it's testing the implementation too
much?  I don't love this style of testing but otherwise this patch works fine on
cc_unittests but completely explodes android, which is kinda bad.

[jamesr, 2013-04-17 22:06:07]

Committed patchset #13 manually as r194700.