cc: Introduce embedded_surfaces in metadata for surface Ids in draw quads

cc: Introduce embedded_surfaces in metadata for surface Ids in draw quads

cc::Surface and SurfaceDependencyTracker used CompositorFrameMetadata's
referenced_surfaces to determine dependencies of a CompositorFrame. This
causes issues when referenced_surfaces contains surface IDs of hidden
windows. Hidden windows do not generate CompositorFrames and so the
parent CompositorFrame would block for four BeginFrames waiting for a
child CompositorFrame that would never arrive.

This effectively reduced the frame rate from 60fps to 15fps. This
CL addresses this issue by introducing an embedded_surfaces set in
CompositorFrameMetadata that only refers to surface IDs for which
SurfaceDrawQuads were generated and thus will be visually present
in the frame.

In a subsequent CL, referenced_surfaces will be renamed to
retained_surfaces and will only contain the set of surface IDs to
which the parent wishes to retain the corresponding surfaces but does
not embed them in SurfaceDrawQuads.

Once generalized frame eviction is complete, we can drop the
retained_surfaces field all together.

BUG=672962
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

Review-Url: https://codereview.chromium.org/2803913004
Cr-Commit-Position: refs/heads/master@{#462935}
Committed: https://chromium.googlesource.com/chromium/src/+/a4f81d1fe5d4a83f62f5c99b72c73dc5d62be81d

[Fady Samuel, 2017-04-06 22:27:16]

Description was changed from

==========
cc: Introduce embedded_surfaces in metadata for surface Ids in draw quads

BUG=
==========

to

==========
cc: Introduce embedded_surfaces in metadata for surface Ids in draw quads

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[Fady Samuel, 2017-04-07 00:07:44]

Description was changed from

==========
cc: Introduce embedded_surfaces in metadata for surface Ids in draw quads

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
cc: Introduce embedded_surfaces in metadata for surface Ids in draw quads

cc::Surface and SurfaceDependencyTracker used CompositorFrameMetadata's
referenced_surfaces to determine dependencies of a CompositorFrame. This
causes issues when referenced_surfaces contains surface IDs of hidden 
windows. Hidden windows do not generate CompositorFrames and so the 
parent CompositorFrame would block for four BeginFrames waiting for a
child CompositorFrame that would never arrive.

This effectively reduced the frame rate from 60fps to 15fps. This
CL addresses this issue by introducing an embedded_surfaces set in
CompositorFrameMetadata that only refers to surface IDs for which 
SurfaceDrawQuads were generated and thus will be visually present
in the frame.

In a subsequent CL, referenced_surfaces will be renamed to 
retained_surfaces and will only contain the set of surface IDs to
which the parent wishes to retain the corresponding surfaces but does
not embed them in SurfaceDrawQuads.

Once generalized frame eviction is complete, we can drop the 
retained_surfaces field all together.

BUG=672962
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[Fady Samuel, 2017-04-07 00:07:49]

The CQ bit was checked by fsamuel@chromium.org to run a CQ dry run

[Fady Samuel, 2017-04-07 00:08:17]

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

[Fady Samuel, 2017-04-07 00:08:18]

PTAL Dana!

[commit-bot: I haz the power, 2017-04-07 00:08:28]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-07 00:12:51]

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

[commit-bot: I haz the power, 2017-04-07 00:12:52]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Fady Samuel, 2017-04-07 01:19:37]

The CQ bit was checked by fsamuel@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-07 01:20:05]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-07 02:13:36]

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

[commit-bot: I haz the power, 2017-04-07 02:13:37]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[Fady Samuel, 2017-04-07 02:58:43]

The CQ bit was checked by fsamuel@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-07 02:59:10]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Fady Samuel, 2017-04-07 03:04:16]

The CQ bit was checked by fsamuel@chromium.org to run a CQ dry run

[Fady Samuel, 2017-04-07 03:04:28]

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

[Fady Samuel, 2017-04-07 03:04:29]

+tsepez@ for IPC.

[commit-bot: I haz the power, 2017-04-07 03:04:40]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-07 03:46:40]

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

[commit-bot: I haz the power, 2017-04-07 03:46:41]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[Fady Samuel, 2017-04-07 03:52:07]

The CQ bit was checked by fsamuel@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-07 03:52:45]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-07 05:21:40]

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

[commit-bot: I haz the power, 2017-04-07 05:21:41]

Dry run: This issue passed the CQ dry run.

[danakj, 2017-04-07 16:41:17]

LGTM

https://codereview.chromium.org/2803913004/diff/120001/cc/layers/append_quads...
File cc/layers/append_quads_data.h (right):

https://codereview.chromium.org/2803913004/diff/120001/cc/layers/append_quads...
cc/layers/append_quads_data.h:34: std::vector<SurfaceId> embedded_surfaces;
Comment explaining what this is so layer authors can know what to do with it

https://codereview.chromium.org/2803913004/diff/120001/cc/output/compositor_f...
File cc/output/compositor_frame_metadata.h (right):

https://codereview.chromium.org/2803913004/diff/120001/cc/output/compositor_f...
cc/output/compositor_frame_metadata.h:82: // TODO(fsamuel): This should go away
once generalized frame eviction is
It'd be nice to explain /why/ this should go away

https://codereview.chromium.org/2803913004/diff/120001/cc/surfaces/surface_de...
File cc/surfaces/surface_dependency_tracker.cc (right):

https://codereview.chromium.org/2803913004/diff/120001/cc/surfaces/surface_de...
cc/surfaces/surface_dependency_tracker.cc:121: for (const SurfaceId& surface_id
: pending_frame.metadata.embedded_surfaces) {
tfw u hit 80 columns exactly http://i.imgur.com/HXr2NvL.jpg

[Fady Samuel, 2017-04-07 17:16:27]

Thanks Dana! PTAL Tom!

https://codereview.chromium.org/2803913004/diff/120001/cc/layers/append_quads...
File cc/layers/append_quads_data.h (right):

https://codereview.chromium.org/2803913004/diff/120001/cc/layers/append_quads...
cc/layers/append_quads_data.h:34: std::vector<SurfaceId> embedded_surfaces;
On 2017/04/07 16:41:17, danakj wrote:
> Comment explaining what this is so layer authors can know what to do with it

Done.

https://codereview.chromium.org/2803913004/diff/120001/cc/output/compositor_f...
File cc/output/compositor_frame_metadata.h (right):

https://codereview.chromium.org/2803913004/diff/120001/cc/output/compositor_f...
cc/output/compositor_frame_metadata.h:82: // TODO(fsamuel): This should go away
once generalized frame eviction is
On 2017/04/07 16:41:17, danakj wrote:
> It'd be nice to explain /why/ this should go away

Done.

https://codereview.chromium.org/2803913004/diff/120001/cc/surfaces/surface_de...
File cc/surfaces/surface_dependency_tracker.cc (right):

https://codereview.chromium.org/2803913004/diff/120001/cc/surfaces/surface_de...
cc/surfaces/surface_dependency_tracker.cc:121: for (const SurfaceId& surface_id
: pending_frame.metadata.embedded_surfaces) {
On 2017/04/07 16:41:17, danakj wrote:
> tfw u hit 80 columns exactly http://i.imgur.com/HXr2NvL.jpg

Acknowledged.

[Fady Samuel, 2017-04-07 17:16:46]

The CQ bit was checked by fsamuel@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-07 17:17:24]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Tom Sepez, 2017-04-07 17:27:13]

LGTM as we are already passing surface IDs around in this manner.

[commit-bot: I haz the power, 2017-04-07 18:18:59]

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

[commit-bot: I haz the power, 2017-04-07 18:19:00]

Dry run: This issue passed the CQ dry run.

[Fady Samuel, 2017-04-07 18:20:11]

The CQ bit was checked by fsamuel@chromium.org

[Fady Samuel, 2017-04-07 18:20:12]

The patchset sent to the CQ was uploaded after l-g-t-m from danakj@chromium.org
Link to the patchset: https://codereview.chromium.org/2803913004/#ps140001
(title: "Addressed Dana's comments")

[commit-bot: I haz the power, 2017-04-07 18:21:06]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-07 18:32:43]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1491589211526570,
"parent_rev": "d2bb15449ca92f88f8f4e9f6a93a96d567b00e2c", "commit_rev":
"a4f81d1fe5d4a83f62f5c99b72c73dc5d62be81d"}

[commit-bot: I haz the power, 2017-04-07 18:38:16]

Description was changed from

==========
cc: Introduce embedded_surfaces in metadata for surface Ids in draw quads

cc::Surface and SurfaceDependencyTracker used CompositorFrameMetadata's
referenced_surfaces to determine dependencies of a CompositorFrame. This
causes issues when referenced_surfaces contains surface IDs of hidden 
windows. Hidden windows do not generate CompositorFrames and so the 
parent CompositorFrame would block for four BeginFrames waiting for a
child CompositorFrame that would never arrive.

This effectively reduced the frame rate from 60fps to 15fps. This
CL addresses this issue by introducing an embedded_surfaces set in
CompositorFrameMetadata that only refers to surface IDs for which 
SurfaceDrawQuads were generated and thus will be visually present
in the frame.

In a subsequent CL, referenced_surfaces will be renamed to 
retained_surfaces and will only contain the set of surface IDs to
which the parent wishes to retain the corresponding surfaces but does
not embed them in SurfaceDrawQuads.

Once generalized frame eviction is complete, we can drop the 
retained_surfaces field all together.

BUG=672962
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
cc: Introduce embedded_surfaces in metadata for surface Ids in draw quads

cc::Surface and SurfaceDependencyTracker used CompositorFrameMetadata's
referenced_surfaces to determine dependencies of a CompositorFrame. This
causes issues when referenced_surfaces contains surface IDs of hidden
windows. Hidden windows do not generate CompositorFrames and so the
parent CompositorFrame would block for four BeginFrames waiting for a
child CompositorFrame that would never arrive.

This effectively reduced the frame rate from 60fps to 15fps. This
CL addresses this issue by introducing an embedded_surfaces set in
CompositorFrameMetadata that only refers to surface IDs for which
SurfaceDrawQuads were generated and thus will be visually present
in the frame.

In a subsequent CL, referenced_surfaces will be renamed to
retained_surfaces and will only contain the set of surface IDs to
which the parent wishes to retain the corresponding surfaces but does
not embed them in SurfaceDrawQuads.

Once generalized frame eviction is complete, we can drop the
retained_surfaces field all together.

BUG=672962
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

Review-Url: https://codereview.chromium.org/2803913004
Cr-Commit-Position: refs/heads/master@{#462935}
Committed:
https://chromium.googlesource.com/chromium/src/+/a4f81d1fe5d4a83f62f5c99b72c7...
==========

[commit-bot: I haz the power, 2017-04-07 18:38:18]

Committed patchset #8 (id:140001) as
https://chromium.googlesource.com/chromium/src/+/a4f81d1fe5d4a83f62f5c99b72c7...