cc: Clean up max frames/swaps pending usage.

cc: Clean up max frames/swaps pending usage.

All output surfaces used by the Renderer and UI should
only need a max swaps pending of 1, especially considering
Surfaces and CompositorTimingHistory don't support
multiple queued frames.

The output suface used by the cc::Display, however, may
have more than 1 swap pending - especially on platforms like
CrOS where the SwapAck is deferred until the buffer is actually
displayed.

This patch:
1) Changes the default max pending frames/swaps from 2 to 1.
2) Changes Blimp to have only 1 pending frame.
3) DCHECKS that all Renderers and UIs have a max swaps pending of 1.
4) Sets the default value in the constructor of
   OutpuSurface::Capabilities, rather than through
   an extra check for zero + init.

BUG=525756
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/0055b7eaee4ca249cb83ac8ddc5200cbf7a12069
Cr-Commit-Position: refs/heads/master@{#360283}

[brianderson, 2015-11-13 20:33:09]

Description was changed from

==========
cc: Clean up max frames/swaps pending usage.

All output surfaces used by the Renderer and Ui should
only need a max swaps pending of 1, especially considering
Surfaces don't yet support multiple queued frames.
This changes Blimp to have only 1 pending frame and
DCHECKS that all configs now have a max swaps pending of 1.

The output suface used by the cc::Display, however, may
have more than 1 swap pending - especially on platforms like
CrOS where the SwapAck is deferred until the buffer is actually
displayed.

BUG=
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
cc: Clean up max frames/swaps pending usage.

All output surfaces used by the Renderer and UI should
only need a max swaps pending of 1, especially considering
Surfaces and CompositorTimingHistory don't support
multiple queued frames.

The output suface used by the cc::Display, however, may
have more than 1 swap pending - especially on platforms like
CrOS where the SwapAck is deferred until the buffer is actually
displayed.

This patch:
1) Changes the default max pending frames/swaps from 2 to 1.
2) Changes Blimp to have only 1 pending frame.
3) DCHECKS that all configs have a max swaps pending of 1.
4) Sets the default value in the constructor of
   OutpuSurface::Capabilities, rather than through
   an extra check for zero + init.

BUG=525756
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[brianderson, 2015-11-13 20:36:15]

Description was changed from

==========
cc: Clean up max frames/swaps pending usage.

All output surfaces used by the Renderer and UI should
only need a max swaps pending of 1, especially considering
Surfaces and CompositorTimingHistory don't support
multiple queued frames.

The output suface used by the cc::Display, however, may
have more than 1 swap pending - especially on platforms like
CrOS where the SwapAck is deferred until the buffer is actually
displayed.

This patch:
1) Changes the default max pending frames/swaps from 2 to 1.
2) Changes Blimp to have only 1 pending frame.
3) DCHECKS that all configs have a max swaps pending of 1.
4) Sets the default value in the constructor of
   OutpuSurface::Capabilities, rather than through
   an extra check for zero + init.

BUG=525756
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
cc: Clean up max frames/swaps pending usage.

All output surfaces used by the Renderer and UI should
only need a max swaps pending of 1, especially considering
Surfaces and CompositorTimingHistory don't support
multiple queued frames.

The output suface used by the cc::Display, however, may
have more than 1 swap pending - especially on platforms like
CrOS where the SwapAck is deferred until the buffer is actually
displayed.

This patch:
1) Changes the default max pending frames/swaps from 2 to 1.
2) Changes Blimp to have only 1 pending frame.
3) DCHECKS that all Renderers and UIs have a max swaps pending of 1.
4) Sets the default value in the constructor of
   OutpuSurface::Capabilities, rather than through
   an extra check for zero + init.

BUG=525756
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[brianderson, 2015-11-13 20:40:40]

brianderson@chromium.org changed reviewers:
+ dtrainor@chromium.org, jbauman@chromium.org, sunnyps@chromium.org

[brianderson, 2015-11-13 20:40:41]

I was trying to verify that cc::Scheduler can assume max_pending_swaps is 1
everywhere.

+dtraintor: It looks like the only case where this isn't true is Blimp. Is it
okay to change it to 1?

In the process I also cleaned up how max_frames_pending gets set and added
DCHECKs.

+jbauman, sunnyps: Can you take a look at this patch overall?

https://codereview.chromium.org/1435133004/diff/20001/blimp/client/compositor...
File blimp/client/compositor/blimp_output_surface.cc (left):

https://codereview.chromium.org/1435133004/diff/20001/blimp/client/compositor...
blimp/client/compositor/blimp_output_surface.cc:17:
capabilities_.max_frames_pending = 2;
+dtrainor: Is changing this to 1 okay?

https://codereview.chromium.org/1435133004/diff/20001/cc/surfaces/onscreen_di...
File cc/surfaces/onscreen_display_client.cc (right):

https://codereview.chromium.org/1435133004/diff/20001/cc/surfaces/onscreen_di...
cc/surfaces/onscreen_display_client.cc:38: DCHECK(output_surface_);
+jbauman: Is this DCHECK valid?

[David Trainor- moved to gerrit, 2015-11-16 17:10:45]

blimp/ lgtm.

https://codereview.chromium.org/1435133004/diff/20001/blimp/client/compositor...
File blimp/client/compositor/blimp_output_surface.cc (left):

https://codereview.chromium.org/1435133004/diff/20001/blimp/client/compositor...
blimp/client/compositor/blimp_output_surface.cc:17:
capabilities_.max_frames_pending = 2;
On 2015/11/13 20:40:41, brianderson wrote:
> +dtrainor: Is changing this to 1 okay?

Yeah that should be fine.  I think (IIRC) I originally got this value from
compositor_impl_android, which is now 1 as well.

[brianderson, 2015-11-16 18:23:09]

brianderson@chromium.org changed reviewers:
+ boliu@chromium.org, sky@chromium.org

[brianderson, 2015-11-16 18:23:10]

+boliu for owners review of content/renderer/android
+sky for owners review of components/mus

[boliu, 2015-11-16 18:25:07]

rs lgtm

[sunnyps, 2015-11-16 20:17:46]

https://codereview.chromium.org/1435133004/diff/20001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/1435133004/diff/20001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_impl.cc:2303: DCHECK_EQ(1, max_frames_pending);
Why not remove the max_frames_pending capability completely? Keeping it has
potential to cause confusion about whether we're supposed to support values
other than 1.

[brianderson, 2015-11-16 20:56:58]

https://codereview.chromium.org/1435133004/diff/20001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/1435133004/diff/20001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_impl.cc:2303: DCHECK_EQ(1, max_frames_pending);
On 2015/11/16 20:17:46, sunnyps wrote:
> Why not remove the max_frames_pending capability completely? Keeping it has
> potential to cause confusion about whether we're supposed to support values
> other than 1.

I guess we could remove it from LTHI and cc::Scheduler. I'll do that as part of
this patch.

The OutputSurface for the DisplayScheduler can have more than 1 frame pending
though (for example on Chrome OS) - so we'll still need to keep it in the
OutputSurface::Capability.

[brianderson, 2015-11-16 22:32:28]

Even more code deleted after Sunny's suggestion. Ptal.

[sky, 2015-11-17 14:26:05]

sky@chromium.org changed reviewers:
+ fsamuel@google.com
- sky@chromium.org

[sky, 2015-11-17 14:26:05]

sky->fsamuel

[Fady Samuel, 2015-11-17 16:23:48]

fsamuel@chromium.org changed reviewers:
+ fsamuel@chromium.org

[Fady Samuel, 2015-11-17 16:23:49]

mus lgtm.

[jbauman, 2015-11-17 21:43:53]

lgtm

[sky, 2015-11-17 22:05:11]

sky@chromium.org changed reviewers:
+ sky@chromium.org

[sky, 2015-11-17 22:05:12]

LGTM

[brianderson, 2015-11-17 22:09:21]

The CQ bit was checked by brianderson@chromium.org

[brianderson, 2015-11-17 22:09:22]

The patchset sent to the CQ was uploaded after l-g-t-m from
dtrainor@chromium.org, boliu@chromium.org
Link to the patchset: https://codereview.chromium.org/1435133004/#ps40001
(title: "Remove SetMaxSwapsPending")

[commit-bot: I haz the power, 2015-11-17 22:10:07]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1435133004/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1435133004/40001

[commit-bot: I haz the power, 2015-11-17 22:25:00]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-11-17 22:25:01]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, no build URL)

[brianderson, 2015-11-17 22:57:38]

The CQ bit was checked by brianderson@chromium.org

[commit-bot: I haz the power, 2015-11-17 22:59:55]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1435133004/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1435133004/40001

[commit-bot: I haz the power, 2015-11-17 23:06:32]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-11-17 23:06:33]

Try jobs failed on following builders:
  chromeos_daisy_chromium_compile_only_ng on tryserver.chromium.linux
(JOB_FAILED, no build URL)
  linux_chromium_clobber_rel_ng on tryserver.chromium.linux (JOB_FAILED, no
build URL)
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)

[brianderson, 2015-11-18 02:38:17]

The CQ bit was checked by brianderson@chromium.org

[brianderson, 2015-11-18 02:38:18]

The patchset sent to the CQ was uploaded after l-g-t-m from
dtrainor@chromium.org, sky@chromium.org, boliu@chromium.org,
jbauman@chromium.org, fsamuel@chromium.org
Link to the patchset: https://codereview.chromium.org/1435133004/#ps60001
(title: "rebase")

[commit-bot: I haz the power, 2015-11-18 02:38:31]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1435133004/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1435133004/60001

[commit-bot: I haz the power, 2015-11-18 03:41:23]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-11-18 03:42:06]

Patchset 4 (id:??) landed as
https://crrev.com/0055b7eaee4ca249cb83ac8ddc5200cbf7a12069
Cr-Commit-Position: refs/heads/master@{#360283}