Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(64)

Issue 2803913004: cc: Introduce embedded_surfaces in metadata for surface Ids in draw quads (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
4 months, 1 week ago by Fady Samuel
Modified:
4 months, 1 week ago
Reviewers:
danakj, Tom Sepez
CC:
chromium-reviews, qsr+mojo_chromium.org, viettrungluu+watch_chromium.org, jam, yzshen+watch_chromium.org, abarth-chromium, Aaron Boodman, darin-cc_chromium.org, cc-bugs_chromium.org, darin (slow to review)
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

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

Patch Set 1 #

Patch Set 2 : Added unit test #

Patch Set 3 : Add missing file #

Patch Set 4 : Rebased #

Patch Set 5 : Fix CompositorFrameSinkSupportTest + add new unit test #

Patch Set 6 : Added comment + removed unnecessary code #

Patch Set 7 : Fix unit test #

Total comments: 6

Patch Set 8 : Addressed Dana's comments #

Unified diffs Side-by-side diffs Delta from patch set Stats (+186 lines, -29 lines) Patch
M cc/BUILD.gn View 1 chunk +1 line, -0 lines 0 comments Download
M cc/ipc/cc_param_traits_macros.h View 1 chunk +1 line, -0 lines 0 comments Download
M cc/ipc/compositor_frame_metadata.mojom View 1 chunk +1 line, -0 lines 0 comments Download
M cc/ipc/compositor_frame_metadata_struct_traits.h View 1 chunk +5 lines, -0 lines 0 comments Download
M cc/ipc/compositor_frame_metadata_struct_traits.cc View 1 2 3 1 chunk +1 line, -0 lines 0 comments Download
M cc/ipc/struct_traits_unittest.cc View 1 2 3 3 chunks +8 lines, -0 lines 0 comments Download
M cc/layers/append_quads_data.h View 1 2 3 4 5 6 7 2 chunks +11 lines, -1 line 0 comments Download
A cc/layers/append_quads_data.cc View 1 2 1 chunk +13 lines, -0 lines 0 comments Download
M cc/layers/layer_impl.h View 2 chunks +1 line, -1 line 0 comments Download
M cc/layers/picture_layer_impl.h View 1 chunk +1 line, -1 line 0 comments Download
M cc/layers/render_surface_impl.h View 1 chunk +1 line, -2 lines 0 comments Download
M cc/layers/surface_layer_impl.h View 1 chunk +2 lines, -1 line 0 comments Download
M cc/layers/surface_layer_impl.cc View 3 chunks +9 lines, -3 lines 0 comments Download
M cc/layers/surface_layer_impl_unittest.cc View 1 4 chunks +7 lines, -0 lines 0 comments Download
M cc/output/compositor_frame_metadata.h View 1 2 3 4 5 6 7 1 chunk +9 lines, -0 lines 0 comments Download
M cc/surfaces/compositor_frame_sink_support_unittest.cc View 1 2 3 4 2 chunks +61 lines, -13 lines 0 comments Download
M cc/surfaces/surface.cc View 1 2 3 4 1 chunk +1 line, -2 lines 0 comments Download
M cc/surfaces/surface_dependency_tracker.cc View 1 2 3 4 2 chunks +3 lines, -5 lines 0 comments Download
M cc/trees/layer_tree_host_impl.h View 1 2 3 2 chunks +2 lines, -0 lines 0 comments Download
M cc/trees/layer_tree_host_impl.cc View 1 2 3 2 chunks +4 lines, -0 lines 0 comments Download
M cc/trees/layer_tree_host_impl_unittest.cc View 1 2 3 4 5 6 3 chunks +44 lines, -0 lines 0 comments Download
Commit queue not available (can’t edit this change).

Messages

Total messages: 37 (30 generated)
Fady Samuel
PTAL Dana!
4 months, 1 week ago (2017-04-07 00:08:18 UTC) #5
Fady Samuel
+tsepez@ for IPC.
4 months, 1 week ago (2017-04-07 03:04:29 UTC) #17
danakj
LGTM https://codereview.chromium.org/2803913004/diff/120001/cc/layers/append_quads_data.h File cc/layers/append_quads_data.h (right): https://codereview.chromium.org/2803913004/diff/120001/cc/layers/append_quads_data.h#newcode34 cc/layers/append_quads_data.h:34: std::vector<SurfaceId> embedded_surfaces; Comment explaining what this is so ...
4 months, 1 week ago (2017-04-07 16:41:17 UTC) #25
Fady Samuel
Thanks Dana! PTAL Tom! https://codereview.chromium.org/2803913004/diff/120001/cc/layers/append_quads_data.h File cc/layers/append_quads_data.h (right): https://codereview.chromium.org/2803913004/diff/120001/cc/layers/append_quads_data.h#newcode34 cc/layers/append_quads_data.h:34: std::vector<SurfaceId> embedded_surfaces; On 2017/04/07 16:41:17, ...
4 months, 1 week ago (2017-04-07 17:16:27 UTC) #26
Tom Sepez
LGTM as we are already passing surface IDs around in this manner.
4 months, 1 week ago (2017-04-07 17:27:13 UTC) #29
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2803913004/140001
4 months, 1 week ago (2017-04-07 18:21:06 UTC) #34
commit-bot: I haz the power
4 months, 1 week ago (2017-04-07 18:38:18 UTC) #37
Message was sent while issue was closed.
Committed patchset #8 (id:140001) as
https://chromium.googlesource.com/chromium/src/+/a4f81d1fe5d4a83f62f5c99b72c7...
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld b40b6558b