Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2861593002:
cc: Only add surface ID to embedded_surfaces if fallback does not match (Closed)
|
Created:
3 years, 7 months ago by Fady Samuel Modified:
3 years, 7 months ago Reviewers:
danakj CC:
chromium-reviews, cc-bugs_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
Descriptioncc: Only add surface ID to embedded_surfaces if fallback does not match
A child surface ID is either a dependency (something the parents wants
to block its CompositorFrame on and may not exist in the display
compositor yet) or a reference (it is known to exist in the display
compositor).
Dependencies are held in CompositorFrameMetadata::embedded_surfaces, and
references are held in CompositorFrameMetadata::referenced_surfaces.
It was possible for a surface ID to end up in both places if the
primary and fallback SurfaceInfos in a SurfaceLayer matched. This
causes issues in an upcoming CL https://codereview.chromium.org/2831213004/
that "closes" an old fallback surface to receiving new CompositorFrames
if a new primary surface is being requested by a parent to reduce latency.
BUG=672962, 711948
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
Review-Url: https://codereview.chromium.org/2861593002
Cr-Commit-Position: refs/heads/master@{#469482}
Committed: https://chromium.googlesource.com/chromium/src/+/3ca7863c41560e94efdfdb21f0b1b4231e4a589e
Patch Set 1 #Patch Set 2 : Updated #
Total comments: 4
Patch Set 3 : Addressed Dana's comments #Patch Set 4 : Added a comment #
Messages
Total messages: 27 (16 generated)
Description was changed from ========== cc: Only add surface ID to embedded_surfaces if fallback does not match BUG= ========== to ========== cc: Only add surface ID to embedded_surfaces if fallback does not match BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
Description was changed from ========== cc: Only add surface ID to embedded_surfaces if fallback does not match BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== cc: Only add surface ID to embedded_surfaces if fallback does not match A child surface ID is either a dependency (something the parents wants to block its CompositorFrame on and may not exist in the display compositor yet) or a reference (it is known to exist in the display compositor). Dependencies are held in CompositorFrameMetadata::embedded_surfaces, and references are held in CompositorFrameMetadata::referenced_surfaces. It was possible for a surface ID to end up in both places if the primary and fallback SurfaceInfos in a SurfaceLayer matched. This causes issues in an upcoming CL https://codereview.chromium.org/2831213004/ that "closes" an old fallback surface to receiving new CompositorFrames if a new primary surface is being requested by a parent to reduce latency. BUG=672962 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
Description was changed from ========== cc: Only add surface ID to embedded_surfaces if fallback does not match A child surface ID is either a dependency (something the parents wants to block its CompositorFrame on and may not exist in the display compositor yet) or a reference (it is known to exist in the display compositor). Dependencies are held in CompositorFrameMetadata::embedded_surfaces, and references are held in CompositorFrameMetadata::referenced_surfaces. It was possible for a surface ID to end up in both places if the primary and fallback SurfaceInfos in a SurfaceLayer matched. This causes issues in an upcoming CL https://codereview.chromium.org/2831213004/ that "closes" an old fallback surface to receiving new CompositorFrames if a new primary surface is being requested by a parent to reduce latency. BUG=672962 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== cc: Only add surface ID to embedded_surfaces if fallback does not match A child surface ID is either a dependency (something the parents wants to block its CompositorFrame on and may not exist in the display compositor yet) or a reference (it is known to exist in the display compositor). Dependencies are held in CompositorFrameMetadata::embedded_surfaces, and references are held in CompositorFrameMetadata::referenced_surfaces. It was possible for a surface ID to end up in both places if the primary and fallback SurfaceInfos in a SurfaceLayer matched. This causes issues in an upcoming CL https://codereview.chromium.org/2831213004/ that "closes" an old fallback surface to receiving new CompositorFrames if a new primary surface is being requested by a parent to reduce latency. BUG=672962 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
The CQ bit was checked by fsamuel@chromium.org to run a CQ dry run
fsamuel@chromium.org changed reviewers: + danakj@chromium.org
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== cc: Only add surface ID to embedded_surfaces if fallback does not match A child surface ID is either a dependency (something the parents wants to block its CompositorFrame on and may not exist in the display compositor yet) or a reference (it is known to exist in the display compositor). Dependencies are held in CompositorFrameMetadata::embedded_surfaces, and references are held in CompositorFrameMetadata::referenced_surfaces. It was possible for a surface ID to end up in both places if the primary and fallback SurfaceInfos in a SurfaceLayer matched. This causes issues in an upcoming CL https://codereview.chromium.org/2831213004/ that "closes" an old fallback surface to receiving new CompositorFrames if a new primary surface is being requested by a parent to reduce latency. BUG=672962 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== cc: Only add surface ID to embedded_surfaces if fallback does not match A child surface ID is either a dependency (something the parents wants to block its CompositorFrame on and may not exist in the display compositor yet) or a reference (it is known to exist in the display compositor). Dependencies are held in CompositorFrameMetadata::embedded_surfaces, and references are held in CompositorFrameMetadata::referenced_surfaces. It was possible for a surface ID to end up in both places if the primary and fallback SurfaceInfos in a SurfaceLayer matched. This causes issues in an upcoming CL https://codereview.chromium.org/2831213004/ that "closes" an old fallback surface to receiving new CompositorFrames if a new primary surface is being requested by a parent to reduce latency. BUG=672962, 711948 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
Description was changed from ========== cc: Only add surface ID to embedded_surfaces if fallback does not match A child surface ID is either a dependency (something the parents wants to block its CompositorFrame on and may not exist in the display compositor yet) or a reference (it is known to exist in the display compositor). Dependencies are held in CompositorFrameMetadata::embedded_surfaces, and references are held in CompositorFrameMetadata::referenced_surfaces. It was possible for a surface ID to end up in both places if the primary and fallback SurfaceInfos in a SurfaceLayer matched. This causes issues in an upcoming CL https://codereview.chromium.org/2831213004/ that "closes" an old fallback surface to receiving new CompositorFrames if a new primary surface is being requested by a parent to reduce latency. BUG=672962, 711948 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== cc: Only add surface ID to embedded_surfaces if fallback does not match A child surface ID is either a dependency (something the parents wants to block its CompositorFrame on and may not exist in the display compositor yet) or a reference (it is known to exist in the display compositor). Dependencies are held in CompositorFrameMetadata::embedded_surfaces, and references are held in CompositorFrameMetadata::referenced_surfaces. It was possible for a surface ID to end up in both places if the primary and fallback SurfaceInfos in a SurfaceLayer matched. This causes issues in an upcoming CL https://codereview.chromium.org/2831213004/ that "closes" an old fallback surface to receiving new CompositorFrames if a new primary surface is being requested by a parent to reduce latency. BUG=672962, 711948 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
https://codereview.chromium.org/2861593002/diff/20001/cc/layers/surface_layer... File cc/layers/surface_layer_impl.cc (right): https://codereview.chromium.org/2861593002/diff/20001/cc/layers/surface_layer... cc/layers/surface_layer_impl.cc:68: bool needs_synchronization = nit: needs_fallback? https://codereview.chromium.org/2861593002/diff/20001/cc/layers/surface_layer... cc/layers/surface_layer_impl.cc:72: needs_synchronization ? &append_quads_data->embedded_surfaces : nullptr); The side effects here are very unclear, with the relationship that this is preventing embedded surfaces from adding the the surface id. Proposal: Remove the |embedded_surfaces| from CreateSurfaceDrawQuad, and do the push_back here directly instead.
PTAL Dana! https://codereview.chromium.org/2861593002/diff/20001/cc/layers/surface_layer... File cc/layers/surface_layer_impl.cc (right): https://codereview.chromium.org/2861593002/diff/20001/cc/layers/surface_layer... cc/layers/surface_layer_impl.cc:68: bool needs_synchronization = On 2017/05/04 19:14:08, danakj wrote: > nit: needs_fallback? Done. https://codereview.chromium.org/2861593002/diff/20001/cc/layers/surface_layer... cc/layers/surface_layer_impl.cc:72: needs_synchronization ? &append_quads_data->embedded_surfaces : nullptr); On 2017/05/04 19:14:08, danakj wrote: > The side effects here are very unclear, with the relationship that this is > preventing embedded surfaces from adding the the surface id. Proposal: Remove > the |embedded_surfaces| from CreateSurfaceDrawQuad, and do the push_back here > directly instead. Done.
The CQ bit was checked by fsamuel@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
It still ends up in both here right? https://cs.chromium.org/chromium/src/cc/trees/layer_tree_host_impl.cc?rcl=ca5...
On 2017/05/04 20:15:37, danakj wrote: > It still ends up in both here right? > https://cs.chromium.org/chromium/src/cc/trees/layer_tree_host_impl.cc?rcl=ca5... Why do we add to one list via AppendQuadsData and to the other list via this separate path?
On 2017/05/04 20:16:03, danakj wrote: > On 2017/05/04 20:15:37, danakj wrote: > > It still ends up in both here right? > > > https://cs.chromium.org/chromium/src/cc/trees/layer_tree_host_impl.cc?rcl=ca5... > > Why do we add to one list via AppendQuadsData and to the other list via this > separate path? One path goes over all SurfaceLayers including clipped/occluded/etc (referenced_surfaces) and one goes over only things that need DrawQuads (embedded_surfaces). This is because we keep around all apps on Chrome OS (for now) just in case we need them (move windows/minimize windows/task switcher/overview mode/etc) even if they're not on screen in referenced_surfaces. The end state I want to reach is: 1. embedded_surfaces becomes dependencies that are not necessarily available yet. 2. referenced_surfaces is references that viz already knows about that have corresponding DrawQuads. 3. Things that we don't need right now go in a separate viz-managed eviction candidate vector.
On 2017/05/04 20:20:40, Fady Samuel wrote: > On 2017/05/04 20:16:03, danakj wrote: > > On 2017/05/04 20:15:37, danakj wrote: > > > It still ends up in both here right? > > > > > > https://cs.chromium.org/chromium/src/cc/trees/layer_tree_host_impl.cc?rcl=ca5... > > > > Why do we add to one list via AppendQuadsData and to the other list via this > > separate path? > > One path goes over all SurfaceLayers including clipped/occluded/etc > (referenced_surfaces) and one goes over only things that need DrawQuads > (embedded_surfaces). > > This is because we keep around all apps on Chrome OS (for now) just in case we > need them (move windows/minimize windows/task switcher/overview mode/etc) even > if they're not on screen in referenced_surfaces. > > The end state I want to reach is: > > 1. embedded_surfaces becomes dependencies that are not necessarily available > yet. > 2. referenced_surfaces is references that viz already knows about that have > corresponding DrawQuads. > 3. Things that we don't need right now go in a separate viz-managed eviction > candidate vector. Note that if surface_synchronization is not on, we only use primary surfaces and if a thing is a primary surface then the display compositor already knows about it.
So you're saying the fallback_surface_info_ is null if surface sync is off? This is not at all clear from reading the code in SurfaceLayerImpl. LGTM for this then tho.
On 2017/05/04 20:26:46, danakj wrote: > So you're saying the fallback_surface_info_ is null if surface sync is off? This > is not at all clear from reading the code in SurfaceLayerImpl. LGTM for this > then tho. Yes, added a comment on SetFallbackSurfaceInfo! Thanks! CQ'ing.
The CQ bit was checked by fsamuel@chromium.org
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/2861593002/#ps60001 (title: "Added a comment")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 60001, "attempt_start_ts": 1493929999587300,
"parent_rev": "9996adeb0f5905aff14fe3c4f14300aec1fb0bbf", "commit_rev":
"3ca7863c41560e94efdfdb21f0b1b4231e4a589e"}
Message was sent while issue was closed.
Description was changed from ========== cc: Only add surface ID to embedded_surfaces if fallback does not match A child surface ID is either a dependency (something the parents wants to block its CompositorFrame on and may not exist in the display compositor yet) or a reference (it is known to exist in the display compositor). Dependencies are held in CompositorFrameMetadata::embedded_surfaces, and references are held in CompositorFrameMetadata::referenced_surfaces. It was possible for a surface ID to end up in both places if the primary and fallback SurfaceInfos in a SurfaceLayer matched. This causes issues in an upcoming CL https://codereview.chromium.org/2831213004/ that "closes" an old fallback surface to receiving new CompositorFrames if a new primary surface is being requested by a parent to reduce latency. BUG=672962, 711948 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== cc: Only add surface ID to embedded_surfaces if fallback does not match A child surface ID is either a dependency (something the parents wants to block its CompositorFrame on and may not exist in the display compositor yet) or a reference (it is known to exist in the display compositor). Dependencies are held in CompositorFrameMetadata::embedded_surfaces, and references are held in CompositorFrameMetadata::referenced_surfaces. It was possible for a surface ID to end up in both places if the primary and fallback SurfaceInfos in a SurfaceLayer matched. This causes issues in an upcoming CL https://codereview.chromium.org/2831213004/ that "closes" an old fallback surface to receiving new CompositorFrames if a new primary surface is being requested by a parent to reduce latency. BUG=672962, 711948 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel Review-Url: https://codereview.chromium.org/2861593002 Cr-Commit-Position: refs/heads/master@{#469482} Committed: https://chromium.googlesource.com/chromium/src/+/3ca7863c41560e94efdfdb21f0b1... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/3ca7863c41560e94efdfdb21f0b1... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
