cc: Set the max frames pending from the thread proxy.

cc: Set the max frames pending from the thread proxy.

When a parent compositor is present (ie, ubercompositor), throttle to a single
pending frame.

Tests:
LayerTreeHostTestMaxPendingFrames.DelegatingRenderer
LayerTreeHostTestMaxPendingFrames.GLRenderer

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

[enne (OOO), 2013-01-04 21:42:05]

https://codereview.chromium.org/11747002/diff/2001/cc/frame_rate_controller.cc
File cc/frame_rate_controller.cc (right):

https://codereview.chromium.org/11747002/diff/2001/cc/frame_rate_controller.c...
cc/frame_rate_controller.cc:111: if (m_swapBuffersCompleteSupported &&
!m_isTimeSourceThrottling && (!m_maxFramesPending || m_numFramesPending <
m_maxFramesPending))
How about reusing throttled, i.e. "&& !throttled"?

https://codereview.chromium.org/11747002/diff/2001/cc/layer_tree_host_unittes...
File cc/layer_tree_host_unittest.cc (right):

https://codereview.chromium.org/11747002/diff/2001/cc/layer_tree_host_unittes...
cc/layer_tree_host_unittest.cc:2282: EXPECT_LT(1,
proxy->maxFramesPendingForTesting());
Why does the initialization get skipped and this is 0 instead of the settings
default of 2?

[danakj, 2013-01-04 21:44:44]

https://codereview.chromium.org/11747002/diff/2001/cc/frame_rate_controller.cc
File cc/frame_rate_controller.cc (right):

https://codereview.chromium.org/11747002/diff/2001/cc/frame_rate_controller.c...
cc/frame_rate_controller.cc:111: if (m_swapBuffersCompleteSupported &&
!m_isTimeSourceThrottling && (!m_maxFramesPending || m_numFramesPending <
m_maxFramesPending))
On 2013/01/04 21:42:05, enne wrote:
> How about reusing throttled, i.e. "&& !throttled"?

Ya good plan.

https://codereview.chromium.org/11747002/diff/2001/cc/layer_tree_host_unittes...
File cc/layer_tree_host_unittest.cc (right):

https://codereview.chromium.org/11747002/diff/2001/cc/layer_tree_host_unittes...
cc/layer_tree_host_unittest.cc:2282: EXPECT_LT(1,
proxy->maxFramesPendingForTesting());
On 2013/01/04 21:42:05, enne wrote:
> Why does the initialization get skipped and this is 0 instead of the settings
> default of 2?

No it's 2. 1 < maxFramesPending.

[enne (OOO), 2013-01-04 21:56:43]

https://codereview.chromium.org/11747002/diff/2001/cc/layer_tree_host_unittes...
File cc/layer_tree_host_unittest.cc (right):

https://codereview.chromium.org/11747002/diff/2001/cc/layer_tree_host_unittes...
cc/layer_tree_host_unittest.cc:2282: EXPECT_LT(1,
proxy->maxFramesPendingForTesting());
On 2013/01/04 21:44:45, danakj wrote:
> On 2013/01/04 21:42:05, enne wrote:
> > Why does the initialization get skipped and this is 0 instead of the
settings
> > default of 2?
> 
> No it's 2. 1 < maxFramesPending.

So really:

EXPECT_EQ(FrameRateController::defaultMaxFramesPending,
proxy->maxFramesPendingForTesting());

[danakj, 2013-01-04 22:00:23]

On 2013/01/04 21:56:43, enne wrote:
>
https://codereview.chromium.org/11747002/diff/2001/cc/layer_tree_host_unittes...
> File cc/layer_tree_host_unittest.cc (right):
> 
>
https://codereview.chromium.org/11747002/diff/2001/cc/layer_tree_host_unittes...
> cc/layer_tree_host_unittest.cc:2282: EXPECT_LT(1,
> proxy->maxFramesPendingForTesting());
> On 2013/01/04 21:44:45, danakj wrote:
> > On 2013/01/04 21:42:05, enne wrote:
> > > Why does the initialization get skipped and this is 0 instead of the
> settings
> > > default of 2?
> > 
> > No it's 2. 1 < maxFramesPending.
> 
> So really:
> 
> EXPECT_EQ(FrameRateController::defaultMaxFramesPending,
> proxy->maxFramesPendingForTesting());

Ya, I was trying to avoid hardcoding a 2. That's a good idea.

[enne (OOO), 2013-01-04 22:06:48]

Also, lgtm

[danakj, 2013-01-04 22:11:40]

thanks :)

On Fri, Jan 4, 2013 at 5:06 PM, <enne@chromium.org> wrote:

> Also, lgtm
>
>
https://codereview.chromium.**org/11747002/<https://codereview.chromium.org/1...
>

[danakj, 2013-01-04 22:24:07]

https://codereview.chromium.org/11747002/diff/2001/cc/frame_rate_controller.cc
File cc/frame_rate_controller.cc (right):

https://codereview.chromium.org/11747002/diff/2001/cc/frame_rate_controller.c...
cc/frame_rate_controller.cc:111: if (m_swapBuffersCompleteSupported &&
!m_isTimeSourceThrottling && (!m_maxFramesPending || m_numFramesPending <
m_maxFramesPending))
On 2013/01/04 21:44:45, danakj wrote:
> On 2013/01/04 21:42:05, enne wrote:
> > How about reusing throttled, i.e. "&& !throttled"?
> 
> Ya good plan.

Done.

https://codereview.chromium.org/11747002/diff/2001/cc/layer_tree_host_unittes...
File cc/layer_tree_host_unittest.cc (right):

https://codereview.chromium.org/11747002/diff/2001/cc/layer_tree_host_unittes...
cc/layer_tree_host_unittest.cc:2282: EXPECT_LT(1,
proxy->maxFramesPendingForTesting());
On 2013/01/04 21:56:43, enne wrote:
> On 2013/01/04 21:44:45, danakj wrote:
> > On 2013/01/04 21:42:05, enne wrote:
> > > Why does the initialization get skipped and this is 0 instead of the
> settings
> > > default of 2?
> > 
> > No it's 2. 1 < maxFramesPending.
> 
> So really:
> 
> EXPECT_EQ(FrameRateController::defaultMaxFramesPending,
> proxy->maxFramesPendingForTesting());

Done.

[commit-bot: I haz the power, 2013-01-04 22:24:40]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/danakj@chromium.org/11747002/17001

[commit-bot: I haz the power, 2013-01-05 02:38:59]

Commit queue rejected this change because the description was changed
between the time the change entered the commit queue and the time it
was ready to commit. You can safely check the commit box again.

[commit-bot: I haz the power, 2013-01-05 03:28:31]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/danakj@chromium.org/11747002/17001

[commit-bot: I haz the power, 2013-01-05 03:28:55]

Change committed as 175275

[nduca, 2013-01-05 04:07:44]

i would have liked to see maxFramesPending be a construction time parameter

[danakj, 2013-01-06 03:17:04]

On Fri, Jan 4, 2013 at 11:07 PM, <nduca@chromium.org> wrote:

> i would have liked to see maxFramesPending be a construction time parameter
>

The problem I see with that is the decision depends on the output surface
being used, and that is chosen later, and could potentially change over the
course of the LTHImpl. We could reconstruct framerate controller along with
outputsurface every time. I'm not sure if that sounds better or worse to
you, I don't really see the difference in the end from here.

[Nat, 2013-01-07 17:49:42]

One longstanding todo is that the vsync callback comes in via the wgc3d and
then into the proxy. Isn't that itself kinda wrong? It seems to me that
vsync is specific to the output surface --- meaning, making the FRC when
you make the outputsurface is indeed the right thing to do.

Iirw, context recreation is visible to the compositor.... so that should
still be possible.

Also, randomly, does software-mode/slaved-mode even get vsync timebase/etc?


On Sat, Jan 5, 2013 at 7:16 PM, Dana Jansens <danakj@chromium.org> wrote:

> On Fri, Jan 4, 2013 at 11:07 PM, <nduca@chromium.org> wrote:
>
>> i would have liked to see maxFramesPending be a construction time
>> parameter
>>
>
> The problem I see with that is the decision depends on the output surface
> being used, and that is chosen later, and could potentially change over the
> course of the LTHImpl. We could reconstruct framerate controller along with
> outputsurface every time. I'm not sure if that sounds better or worse to
> you, I don't really see the difference in the end from here.
>

[danakj, 2013-01-07 17:51:54]

On Mon, Jan 7, 2013 at 12:49 PM, Nat Duca <nduca@google.com> wrote:

> One longstanding todo is that the vsync callback comes in via the wgc3d
> and then into the proxy. Isn't that itself kinda wrong? It seems to me that
> vsync is specific to the output surface --- meaning, making the FRC when
> you make the outputsurface is indeed the right thing to do.
>
> Iirw, context recreation is visible to the compositor.... so that should
> still be possible.
>

Ya it is. Is there a crbug for the above? We can make a note to make it a
constructor property on there.

Also, randomly, does software-mode/slaved-mode even get vsync timebase/etc?
>

I think it should if it doesn't so we can generate animations at the same
rate that they will be displayed.


>
>
> On Sat, Jan 5, 2013 at 7:16 PM, Dana Jansens <danakj@chromium.org> wrote:
>
>> On Fri, Jan 4, 2013 at 11:07 PM, <nduca@chromium.org> wrote:
>>
>>> i would have liked to see maxFramesPending be a construction time
>>> parameter
>>>
>>
>> The problem I see with that is the decision depends on the output surface
>> being used, and that is chosen later, and could potentially change over the
>> course of the LTHImpl. We could reconstruct framerate controller along with
>> outputsurface every time. I'm not sure if that sounds better or worse to
>> you, I don't really see the difference in the end from here.
>>
>
>

[jamesr, 2013-01-08 00:18:39]

https://chromiumcodereview.appspot.com/11747002/diff/17001/cc/thread_proxy.cc
File cc/thread_proxy.cc (right):

https://chromiumcodereview.appspot.com/11747002/diff/17001/cc/thread_proxy.cc...
cc/thread_proxy.cc:949: if
(m_layerTreeHostImpl->outputSurface()->Capabilities().has_parent_compositor)
This line caused crashes on the win_aura bots (and potentially in production) -
if we get a non-null OutputSurface but BindToClient fails, *initializeSucceeded
will be false and m_layerTreeHostImpl->outputSurface() will be NULL, so this
explodes.  See https://code.google.com/p/chromium/issues/detail?id=168691

[danakj, 2013-01-08 00:58:28]

Here's a patch that shouldn't crash when initializing the renderer fails.

[commit-bot: I haz the power, 2013-01-08 01:05:29]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/danakj@chromium.org/11747002/10006

[jamesr, 2013-01-08 01:06:27]

On 2013/01/08 01:05:29, I haz the power (commit-bot) wrote:
> CQ is trying da patch. Follow status at
> https://chromium-status.appspot.com/cq/danakj%40chromium.org/11747002/10006

I unclicked the commit checkbox.  The description has NOTRY=true (why?) and I
think this should at least run through the trybots given what happened last
time, if not get another review.

[jamesr, 2013-01-08 01:07:15]

Patch set 6 LGTM, but that's no reason to skip process (including running
trybots - I would definitely run through win_aura to be sure)

[danakj, 2013-01-08 01:09:21]

On 2013/01/08 01:07:15, jamesr wrote:
> Patch set 6 LGTM, but that's no reason to skip process (including running
> trybots - I would definitely run through win_aura to be sure)

Oh thanks for spotting that. It was from before when the CQ passed but I had
edited the description so it didn't land it.

[commit-bot: I haz the power, 2013-01-08 01:11:53]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/danakj@chromium.org/11747002/10006

[commit-bot: I haz the power, 2013-01-10 19:26:13]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/danakj@chromium.org/11747002/10006

[commit-bot: I haz the power, 2013-01-10 19:26:15]

Failed to apply patch for cc/layer_tree_host_unittest.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file cc/layer_tree_host_unittest.cc
  Hunk #1 FAILED at 7.
  Hunk #2 succeeded at 22 with fuzz 2 (offset 2 lines).
  Hunk #3 succeeded at 2088 with fuzz 2 (offset 70 lines).
  1 out of 3 hunks FAILED -- saving rejects to file
cc/layer_tree_host_unittest.cc.rej

Patch:       cc/layer_tree_host_unittest.cc
Index: cc/layer_tree_host_unittest.cc
diff --git a/cc/layer_tree_host_unittest.cc b/cc/layer_tree_host_unittest.cc
index
3f879a95822d4787f13be8c5c82fb029e8305172..e8d86ccb35d3274c97d3b8ef15313fd16c53282f
100644
--- a/cc/layer_tree_host_unittest.cc
+++ b/cc/layer_tree_host_unittest.cc
@@ -7,10 +7,12 @@
 #include "base/synchronization/lock.h"
 #include "cc/content_layer.h"
 #include "cc/content_layer_client.h"
+#include "cc/frame_rate_controller.h"
 #include "cc/layer_impl.h"
 #include "cc/layer_tree_host_impl.h"
 #include "cc/layer_tree_impl.h"
 #include "cc/output_surface.h"
+#include "cc/resource_update_queue.h"
 #include "cc/single_thread_proxy.h"
 #include "cc/test/fake_content_layer.h"
 #include "cc/test/fake_content_layer_client.h"
@@ -20,8 +22,8 @@
 #include "cc/test/fake_scrollbar_layer.h"
 #include "cc/test/geometry_test_utils.h"
 #include "cc/test/layer_tree_test_common.h"
-#include "cc/resource_update_queue.h"
 #include "cc/test/occlusion_tracker_test_common.h"
+#include "cc/thread_proxy.h"
 #include "cc/timing_function.h"
 #include "testing/gmock/include/gmock/gmock.h"
 #include "third_party/WebKit/Source/Platform/chromium/public/WebSize.h"
@@ -2018,5 +2020,56 @@ TEST(LayerTreeHostTest,
PartialUpdatesWithDelegatingRendererAndSoftwareContent)
     EXPECT_EQ(0u, host->settings().maxPartialTextureUpdates);
 }
 
+class LayerTreeHostTestMaxPendingFrames : public LayerTreeHostTest {
+public:
+    LayerTreeHostTestMaxPendingFrames()
+        : LayerTreeHostTest()
+    {
+    }
+
+    virtual scoped_ptr<OutputSurface> createOutputSurface() OVERRIDE
+    {
+        if (m_delegatingRenderer)
+            return
FakeOutputSurface::CreateDelegating3d().PassAs<OutputSurface>();
+        return FakeOutputSurface::Create3d().PassAs<OutputSurface>();
+    }
+
+    virtual void beginTest() OVERRIDE
+    {
+        postSetNeedsCommitToMainThread();
+    }
+
+    virtual void drawLayersOnThread(LayerTreeHostImpl* hostImpl) OVERRIDE
+    {
+        DCHECK(hostImpl->proxy()->hasImplThread());
+
+        const ThreadProxy* proxy =
static_cast<ThreadProxy*>(hostImpl->proxy());
+        if (m_delegatingRenderer)
+            EXPECT_EQ(1, proxy->maxFramesPendingForTesting());
+        else
+            EXPECT_EQ(FrameRateController::kDefaultMaxFramesPending,
proxy->maxFramesPendingForTesting());
+        endTest();
+    }
+
+    virtual void afterTest() OVERRIDE
+    {
+    }
+
+protected:
+    bool m_delegatingRenderer;
+};
+
+TEST_F(LayerTreeHostTestMaxPendingFrames, DelegatingRenderer)
+{
+    m_delegatingRenderer = true;
+    runTest(true);
+}
+
+TEST_F(LayerTreeHostTestMaxPendingFrames, GLRenderer)
+{
+    m_delegatingRenderer = false;
+    runTest(true);
+}
+
 }  // namespace
 }  // namespace cc

[commit-bot: I haz the power, 2013-01-16 03:52:20]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/danakj@chromium.org/11747002/43001

[commit-bot: I haz the power, 2013-01-16 08:51:35]

Change committed as 177104