New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 676384 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: Feb 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Feature


Sign in to add a comment

Replace SurfaceSequences with SurfaceReferences

Project Member Reported by fsam...@chromium.org, Dec 21 2016

Issue description

This is a tracking bug for replacing all usage of SurfaceSequence with SurfaceReference and ultimately deleting SurfaceSequences. As mentioned here https://go/surface-lifetime-management SurfaceSequences aren't really a good fit for an out-of-process display compositor.

Here's roughly the work I think needs to get done.

1. SurfaceReference derives from SurfaceReferenceBase.
2. Write a surface_reference_factory.mojom or something that creates references or destroys references. Implement a MojoSurfaceReferenceFactory (I hate that name, blah). MojoCompositorFrameSink dispenses a MojoSurfaceReferenceFactory associated interface. 
3. Replace all SurfaceReferenceFactory implementations with MojoSurfaceReferenceFactory.
4. Delete SurfaceSequence, all associated SurfaceReferences and SurfaceReferenceFactories, and cleanup SurfaceManager.
 
Blockedon: 655231
Owner: samans@chromium.org
Status: Assigned (was: Untriaged)
It just occurred to me that SurfaceReference = (parent surface ID, child surface ID), and currently in most places in Chrome, we don't know the parent surface ID from the parent client because surface IDs are allocated in the browser process. Thus, while we can quickly transition Mus+Ash to SurfaceReferences everywhere, it'll be harder to get rid of SurfaceSequences throughout Chrome today because we do not the surface ID in the client.

As a pre-requisite, surface IDs must be passed along to the child. In other words, direct client-to-client resize is a blocker for 4.


Comment 2 by danakj@chromium.org, Jan 10 2017

Why do you need the parent id in the child, just add it when the child tells the parent about the reference?
The other way around: the parent needs to know its own ID (it does not now in many cases). A SurfaceReference for a child is allocated by the parent.

Comment 4 by danakj@chromium.org, Jan 10 2017

Hm, doesn't the thing that actually use the references know all the ids and can determine the parent id from the channel its talking from or no?
Not yet. Currently, a lot of clients allocate their surface IDs in the browser (we are changing this now). Today, a top level renderer does not know its own surface ID. Instead, what it does is it generates a "SurfaceSequence" that is a FrameSinkId + a sequence number and tells the display compositor to attach that SurfaceSequence as a dependency to a provided child surface ID. This means the display compositor doesn't have a nice clean reachability DAG. We are fixing this now. However, we need to tell the parent about its own surface ID before we can do this. We do this already in Mus+ash but not in Chrome. We need this as part of the unified display compositor work.
Roughly what I think needs to happen in Mus+Ash:


ClientSurfaceEmbedder has a SurfaceReferenceFactory 

That SurfaceReferenceFactory should know about WindowCompositorFrameSink. WindowCompositorFrameSink is owned by LayerTreeHost, so maybe WindowCompositorFrameSink has a SurfaceReferenceFactory and dispenses a ref ptr to it that WindowPortMus holds onto and passes to ClientSurfaceEmbedder.
Project Member

Comment 7 by bugdroid1@chromium.org, Jan 13 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/d5db25071f9cdb10c402e2131f60a46b0c557b74

commit d5db25071f9cdb10c402e2131f60a46b0c557b74
Author: samans <samans@chromium.org>
Date: Fri Jan 13 23:18:27 2017

Passing FrameSinkId to WindowCompositorFrameSink

WindowCompositorFrameSink needs to know its surface id to add and remove
references, which means it should know its frame sink id.

BUG= 676384 

Review-Url: https://codereview.chromium.org/2626023007
Cr-Commit-Position: refs/heads/master@{#443716}

[modify] https://crrev.com/d5db25071f9cdb10c402e2131f60a46b0c557b74/content/renderer/gpu/render_widget_compositor.cc
[modify] https://crrev.com/d5db25071f9cdb10c402e2131f60a46b0c557b74/content/renderer/gpu/render_widget_compositor.h
[modify] https://crrev.com/d5db25071f9cdb10c402e2131f60a46b0c557b74/content/renderer/gpu/render_widget_compositor_delegate.h
[modify] https://crrev.com/d5db25071f9cdb10c402e2131f60a46b0c557b74/content/renderer/gpu/render_widget_compositor_unittest.cc
[modify] https://crrev.com/d5db25071f9cdb10c402e2131f60a46b0c557b74/content/renderer/mus/render_widget_mus_connection.cc
[modify] https://crrev.com/d5db25071f9cdb10c402e2131f60a46b0c557b74/content/renderer/mus/render_widget_mus_connection.h
[modify] https://crrev.com/d5db25071f9cdb10c402e2131f60a46b0c557b74/content/renderer/render_thread_impl.cc
[modify] https://crrev.com/d5db25071f9cdb10c402e2131f60a46b0c557b74/content/renderer/render_thread_impl.h
[modify] https://crrev.com/d5db25071f9cdb10c402e2131f60a46b0c557b74/content/renderer/render_widget.cc
[modify] https://crrev.com/d5db25071f9cdb10c402e2131f60a46b0c557b74/content/renderer/render_widget.h
[modify] https://crrev.com/d5db25071f9cdb10c402e2131f60a46b0c557b74/services/ui/public/cpp/window.cc
[modify] https://crrev.com/d5db25071f9cdb10c402e2131f60a46b0c557b74/services/ui/public/cpp/window_compositor_frame_sink.cc
[modify] https://crrev.com/d5db25071f9cdb10c402e2131f60a46b0c557b74/services/ui/public/cpp/window_compositor_frame_sink.h
[modify] https://crrev.com/d5db25071f9cdb10c402e2131f60a46b0c557b74/ui/aura/mus/window_compositor_frame_sink.cc
[modify] https://crrev.com/d5db25071f9cdb10c402e2131f60a46b0c557b74/ui/aura/mus/window_compositor_frame_sink.h
[modify] https://crrev.com/d5db25071f9cdb10c402e2131f60a46b0c557b74/ui/aura/mus/window_port_mus.cc

Based on discussion last week, it seems like referenced_surfaces in CompositorFrameMetadata could accomplish what we want here, at least in the short term. I have a few concerns:

1. In principle, a parent client could hold onto a surface reference but not embed it anywhere immediately.

2. A parent could embed a surface ID more than once.

3. A parent could crash before submitting a frame with a referenced_surface. Then we would have a dangling reference to the root surface ID.
Even if we decide that TLR (top level root) => child references are temporary, I think attribution of references is important to avoid increasing peak memory usage. Imagine slow renderers or other clients (or in the extreme case, imagine malicious or crashed renderers). Temporary references could stay lingering arbitrarily. If we set a timeout for temporary references then the question is, what is the right timeout? How many temporary references can we keep around at once? Non-determinism generally makes a system harder to reason about.

If we can come up with a protocol that doesn't rely on timeouts then it'll be more robust.
Project Member

Comment 10 by bugdroid1@chromium.org, Jan 18 2017

Blocking:
Components: Internals>Compositing
Project Member

Comment 13 by bugdroid1@chromium.org, Feb 8 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/cec9e67c67ec7872e95917acf6f764656a0c7b98

commit cec9e67c67ec7872e95917acf6f764656a0c7b98
Author: kylechar <kylechar@chromium.org>
Date: Wed Feb 08 16:32:14 2017

Move surface reference code to CompositorFrameSinkSupport.

The code that handled adding/removing surface references was in
GpuCompositorFrameSink which is only used with mustash. We want to
experiment with using surface references with non-mustash Chrome, so
move code to CompositorFrameSinkSupport.

Only add/remove references if SurfaceManager is in surface reference
mode, otherwise behaviour in CompositorFrameSinkSupport is unchanged.

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

Review-Url: https://codereview.chromium.org/2687433002
Cr-Commit-Position: refs/heads/master@{#449010}

[modify] https://crrev.com/cec9e67c67ec7872e95917acf6f764656a0c7b98/cc/surfaces/compositor_frame_sink_support.cc
[modify] https://crrev.com/cec9e67c67ec7872e95917acf6f764656a0c7b98/cc/surfaces/compositor_frame_sink_support.h
[modify] https://crrev.com/cec9e67c67ec7872e95917acf6f764656a0c7b98/cc/surfaces/referenced_surface_tracker.h
[modify] https://crrev.com/cec9e67c67ec7872e95917acf6f764656a0c7b98/cc/surfaces/surface_manager.h
[modify] https://crrev.com/cec9e67c67ec7872e95917acf6f764656a0c7b98/components/display_compositor/gpu_compositor_frame_sink.cc
[modify] https://crrev.com/cec9e67c67ec7872e95917acf6f764656a0c7b98/components/display_compositor/gpu_compositor_frame_sink.h

Project Member

Comment 14 by bugdroid1@chromium.org, Feb 8 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/a398ce6656384abcabbf86fbf06fcb76f67e79dc

commit a398ce6656384abcabbf86fbf06fcb76f67e79dc
Author: jam <jam@chromium.org>
Date: Wed Feb 08 23:17:06 2017

Revert of Move surface reference code to CompositorFrameSinkSupport. (patchset #3 id:60001 of https://codereview.chromium.org/2687433002/ )

Reason for revert:
need to revert this to be able to revert https://codereview.chromium.org/2612083002

Original issue's description:
> Move surface reference code to CompositorFrameSinkSupport.
>
> The code that handled adding/removing surface references was in
> GpuCompositorFrameSink which is only used with mustash. We want to
> experiment with using surface references with non-mustash Chrome, so
> move code to CompositorFrameSinkSupport.
>
> Only add/remove references if SurfaceManager is in surface reference
> mode, otherwise behaviour in CompositorFrameSinkSupport is unchanged.
>
> BUG= 676384 
> CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
>
> Review-Url: https://codereview.chromium.org/2687433002
> Cr-Commit-Position: refs/heads/master@{#449010}
> Committed: https://chromium.googlesource.com/chromium/src/+/cec9e67c67ec7872e95917acf6f764656a0c7b98

TBR=fsamuel@chromium.org,jbauman@chromium.org,kylechar@chromium.org
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG= 676384 

Review-Url: https://codereview.chromium.org/2683953004
Cr-Commit-Position: refs/heads/master@{#449132}

[modify] https://crrev.com/a398ce6656384abcabbf86fbf06fcb76f67e79dc/cc/surfaces/compositor_frame_sink_support.cc
[modify] https://crrev.com/a398ce6656384abcabbf86fbf06fcb76f67e79dc/cc/surfaces/compositor_frame_sink_support.h
[modify] https://crrev.com/a398ce6656384abcabbf86fbf06fcb76f67e79dc/cc/surfaces/referenced_surface_tracker.h
[modify] https://crrev.com/a398ce6656384abcabbf86fbf06fcb76f67e79dc/cc/surfaces/surface_manager.h
[modify] https://crrev.com/a398ce6656384abcabbf86fbf06fcb76f67e79dc/components/display_compositor/gpu_compositor_frame_sink.cc
[modify] https://crrev.com/a398ce6656384abcabbf86fbf06fcb76f67e79dc/components/display_compositor/gpu_compositor_frame_sink.h

Project Member

Comment 15 by bugdroid1@chromium.org, Feb 9 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/178e459f4e385a22b73a4914ca1c290331d8fb0b

commit 178e459f4e385a22b73a4914ca1c290331d8fb0b
Author: kylechar <kylechar@chromium.org>
Date: Thu Feb 09 17:26:10 2017

Move surface reference code to CompositorFrameSinkSupport.

The code that handled adding/removing surface references was in
GpuCompositorFrameSink which is only used with mustash. We want to
experiment with using surface references with non-mustash Chrome, so
move code to CompositorFrameSinkSupport.

Only add/remove references if SurfaceManager is in surface reference
mode, otherwise behaviour in CompositorFrameSinkSupport is unchanged.

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

Review-Url: https://codereview.chromium.org/2687433002
Cr-Original-Commit-Position: refs/heads/master@{#449010}
Committed: https://chromium.googlesource.com/chromium/src/+/cec9e67c67ec7872e95917acf6f764656a0c7b98
Review-Url: https://codereview.chromium.org/2687433002
Cr-Commit-Position: refs/heads/master@{#449336}

[modify] https://crrev.com/178e459f4e385a22b73a4914ca1c290331d8fb0b/cc/surfaces/compositor_frame_sink_support.cc
[modify] https://crrev.com/178e459f4e385a22b73a4914ca1c290331d8fb0b/cc/surfaces/compositor_frame_sink_support.h
[modify] https://crrev.com/178e459f4e385a22b73a4914ca1c290331d8fb0b/cc/surfaces/referenced_surface_tracker.h
[modify] https://crrev.com/178e459f4e385a22b73a4914ca1c290331d8fb0b/cc/surfaces/surface_manager.h
[modify] https://crrev.com/178e459f4e385a22b73a4914ca1c290331d8fb0b/components/display_compositor/gpu_compositor_frame_sink.cc
[modify] https://crrev.com/178e459f4e385a22b73a4914ca1c290331d8fb0b/components/display_compositor/gpu_compositor_frame_sink.h

Owner: kylec...@chromium.org
Since Kyle is actively working on this through FrameSinkManagerHost, I'm reassigning to kyelchar@.
Labels: Type-Feature
Cc: weiliangc@chromium.org
Cc: varkha@chromium.org
Labels: -Pri-3 Pri-2
Blocking: 730193
Blockedon: 657959
Blocking: 657959
Blockedon: -657959
Blockedon: -655231
Blocking: 655231
Status: Started (was: Assigned)
Blocking: -601863
Components: -Internals>MUS
Project Member

Comment 30 by bugdroid1@chromium.org, Jun 28 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/1314c3ca3713b6e774f240d56fba68bb93443363

commit 1314c3ca3713b6e774f240d56fba68bb93443363
Author: kylechar <kylechar@chromium.org>
Date: Wed Jun 28 15:10:01 2017

Change SurfaceReferenceFactory usage in SurfaceManager.

Move the StubSurfaceReferenceFactory implementation to //cc/surfaces and
use it in SurfaceManager when surface references are enabled. Also
update the comment on SurfaceReferencyFactory to reflect that it's only
used with SurfaceSequences and will be deleted eventually, since the
name is misleading at this point.

Bug:  676384 
Cq-Include-Trybots: master.tryserver.blink:linux_trusty_blink_rel
Change-Id: I20e3a43186c67ec3b0e0812bbbc0636b8bc60880
Reviewed-on: https://chromium-review.googlesource.com/550537
Reviewed-by: John Bauman <jbauman@chromium.org>
Reviewed-by: Sadrul Chowdhury <sadrul@chromium.org>
Commit-Queue: kylechar <kylechar@chromium.org>
Cr-Commit-Position: refs/heads/master@{#482986}
[modify] https://crrev.com/1314c3ca3713b6e774f240d56fba68bb93443363/cc/surfaces/BUILD.gn
[add] https://crrev.com/1314c3ca3713b6e774f240d56fba68bb93443363/cc/surfaces/stub_surface_reference_factory.cc
[add] https://crrev.com/1314c3ca3713b6e774f240d56fba68bb93443363/cc/surfaces/stub_surface_reference_factory.h
[modify] https://crrev.com/1314c3ca3713b6e774f240d56fba68bb93443363/cc/surfaces/surface_manager.cc
[modify] https://crrev.com/1314c3ca3713b6e774f240d56fba68bb93443363/cc/surfaces/surface_reference_factory.h
[modify] https://crrev.com/1314c3ca3713b6e774f240d56fba68bb93443363/ui/aura/mus/client_surface_embedder.cc

Project Member

Comment 31 by bugdroid1@chromium.org, Jul 27 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/469eb40b4835d7767f0b6c8f4516d704a0d95595

commit 469eb40b4835d7767f0b6c8f4516d704a0d95595
Author: kylechar <kylechar@chromium.org>
Date: Thu Jul 27 14:29:17 2017

Add surface reference code to HostFrameSinkManager.

Add code to assign temporary references to a parent frame sink to
HostFrameSinkManager. The code can't be turned on in Chrome yet, but
this CL adds tests for the expected behaviour.

Bug:  676384 
Change-Id: Ia727f68eb132e03dd589bbdef05740cf2f06cde3
Reviewed-on: https://chromium-review.googlesource.com/586470
Commit-Queue: kylechar <kylechar@chromium.org>
Reviewed-by: Fady Samuel <fsamuel@chromium.org>
Cr-Commit-Position: refs/heads/master@{#489918}
[modify] https://crrev.com/469eb40b4835d7767f0b6c8f4516d704a0d95595/components/viz/host/DEPS
[modify] https://crrev.com/469eb40b4835d7767f0b6c8f4516d704a0d95595/components/viz/host/host_frame_sink_manager.cc
[modify] https://crrev.com/469eb40b4835d7767f0b6c8f4516d704a0d95595/components/viz/host/host_frame_sink_manager.h
[modify] https://crrev.com/469eb40b4835d7767f0b6c8f4516d704a0d95595/components/viz/host/host_frame_sink_manager_unittests.cc

Project Member

Comment 32 by bugdroid1@chromium.org, Jul 27 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/c1a739abd4ecbad47000408367829035d0e897b8

commit c1a739abd4ecbad47000408367829035d0e897b8
Author: kylechar <kylechar@chromium.org>
Date: Thu Jul 27 23:34:59 2017

Add flag that enables surface references.

This CL wires up the --enable-surface-references flag. When this flag is
enabled we stop using SurfaceSequences and temporary references are
added instead.

The flag is disabled by default so this should be a no-op unless enabled
explicitly. The flag will work with Desktop Chrome and Chrome OS only.

Move StubSurfaceManager from //components/viz/service to
//components/viz/common since it's meant to be used by both the service
and clients.

Bug:  676384 
Cq-Include-Trybots: master.tryserver.chromium.linux:linux_site_isolation
Change-Id: I9de7f14902a546f8d5547c1c8f77e0af056ac8c1
Reviewed-on: https://chromium-review.googlesource.com/532033
Commit-Queue: kylechar <kylechar@chromium.org>
Reviewed-by: Antoine Labour <piman@chromium.org>
Reviewed-by: Sadrul Chowdhury <sadrul@chromium.org>
Reviewed-by: Fady Samuel <fsamuel@chromium.org>
Cr-Commit-Position: refs/heads/master@{#490109}
[modify] https://crrev.com/c1a739abd4ecbad47000408367829035d0e897b8/components/viz/common/BUILD.gn
[rename] https://crrev.com/c1a739abd4ecbad47000408367829035d0e897b8/components/viz/common/surfaces/stub_surface_reference_factory.cc
[rename] https://crrev.com/c1a739abd4ecbad47000408367829035d0e897b8/components/viz/common/surfaces/stub_surface_reference_factory.h
[modify] https://crrev.com/c1a739abd4ecbad47000408367829035d0e897b8/components/viz/service/BUILD.gn
[modify] https://crrev.com/c1a739abd4ecbad47000408367829035d0e897b8/components/viz/service/surfaces/surface_manager.cc
[modify] https://crrev.com/c1a739abd4ecbad47000408367829035d0e897b8/content/browser/browser_main_loop.cc
[modify] https://crrev.com/c1a739abd4ecbad47000408367829035d0e897b8/content/browser/frame_host/render_widget_host_view_child_frame.cc
[modify] https://crrev.com/c1a739abd4ecbad47000408367829035d0e897b8/content/browser/renderer_host/offscreen_canvas_surface_impl.cc
[modify] https://crrev.com/c1a739abd4ecbad47000408367829035d0e897b8/content/browser/renderer_host/render_process_host_impl.cc
[modify] https://crrev.com/c1a739abd4ecbad47000408367829035d0e897b8/content/renderer/child_frame_compositing_helper.cc
[modify] https://crrev.com/c1a739abd4ecbad47000408367829035d0e897b8/content/renderer/child_frame_compositing_helper.h
[modify] https://crrev.com/c1a739abd4ecbad47000408367829035d0e897b8/ui/aura/DEPS
[modify] https://crrev.com/c1a739abd4ecbad47000408367829035d0e897b8/ui/aura/mus/client_surface_embedder.cc
[modify] https://crrev.com/c1a739abd4ecbad47000408367829035d0e897b8/ui/aura/mus/mus_context_factory.h

Project Member

Comment 33 by bugdroid1@chromium.org, Aug 10 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/90ac48ae82f230d3287d8bbd3b808490f9aa48cd

commit 90ac48ae82f230d3287d8bbd3b808490f9aa48cd
Author: kylechar <kylechar@chromium.org>
Date: Thu Aug 10 05:21:31 2017

viz: Mark and delete old temporary references.

There is some potential for temporary references to get "orphaned" and
never deleted. There is a potential race, where a new surface activates
and the expected embedder is assigned a temporary reference to it.
Normally, the embedder would submit a CompositorFrame that references
the new surface and the temporary reference would get deleted. If the
embedder were to instead immediately unembed the frame sink of the new
surface, before submitting a CompositorFrame, the temporary reference
wouldn't get replaced.

This CL adds a check every 10s in SurfaceManager to look for old
temporary references. If a temporary reference has existed for 2 checks
then it is older than 10s and is likely orphaned. We delete the old
temporary reference, which will cause the surface to get garbage
collected if it has been marked as destroyed.

This race is so far purely theoretical and I have not been able to
produce it. So more importantly, this CL adds an UMA stat with the
number of old temporary references. This will allow us to see (1) if the
10s check is necessary at all and (2) if we need to add a more
complicated mechanism to track orphaned temporary references.

Bug:  676384 
Change-Id: I88d95e46f1a52839d5d21eb8e5d54e2895aae08c
Reviewed-on: https://chromium-review.googlesource.com/606709
Commit-Queue: kylechar <kylechar@chromium.org>
Reviewed-by: Fady Samuel <fsamuel@chromium.org>
Reviewed-by: Steven Holte <holte@chromium.org>
Cr-Commit-Position: refs/heads/master@{#493260}
[modify] https://crrev.com/90ac48ae82f230d3287d8bbd3b808490f9aa48cd/components/viz/service/display/surface_aggregator.cc
[modify] https://crrev.com/90ac48ae82f230d3287d8bbd3b808490f9aa48cd/components/viz/service/surfaces/surface_manager.cc
[modify] https://crrev.com/90ac48ae82f230d3287d8bbd3b808490f9aa48cd/components/viz/service/surfaces/surface_manager.h
[modify] https://crrev.com/90ac48ae82f230d3287d8bbd3b808490f9aa48cd/tools/metrics/histograms/histograms.xml

Project Member

Comment 34 by bugdroid1@chromium.org, Aug 10 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/4cb1fff28699cb490c8103a32af42366dbb9b773

commit 4cb1fff28699cb490c8103a32af42366dbb9b773
Author: kylechar <kylechar@chromium.org>
Date: Thu Aug 10 20:28:39 2017

Add HostFrameSinkManager test.

Add a test to HostFrameSinkManagerTests that tests the case where a
frame sink is embedded by two parents so it has two parents in the
hierarchy. Also cleanup naming of some test constants/accessors.

Bug:  676384 
Change-Id: If2d3e909df052a4bc616805027c2315c0749a4ac
Reviewed-on: https://chromium-review.googlesource.com/610844
Commit-Queue: kylechar <kylechar@chromium.org>
Reviewed-by: Saman Sami <samans@chromium.org>
Cr-Commit-Position: refs/heads/master@{#493515}
[modify] https://crrev.com/4cb1fff28699cb490c8103a32af42366dbb9b773/components/viz/host/host_frame_sink_manager_unittests.cc

Project Member

Comment 35 by bugdroid1@chromium.org, Aug 15 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/68156e68656b00d9c963c5b30463a27b9627d14d

commit 68156e68656b00d9c963c5b30463a27b9627d14d
Author: kylechar <kylechar@chromium.org>
Date: Tue Aug 15 13:29:52 2017

Enable surface references by default.

This CL switches from using surface sequences to surface references for
surface lifetime management. This entails adding temporary references
and using the surface reference graph starting at the top-level root
surface and not adding surface sequences everywhere. The
--enable-surface-references flag is switched to be
--disable-surface-references instead.

Surface references are enabled for all platforms except android webview,
which doesn't add any surface sequences anyways.

This also enables surface references for browser_tests and
content_browsertests. One content_browsertests test case is disabled
because it was specifically for surface sequences. There are some
cc_unittests and content_unittests that rely on surface sequences that
will need to be updated after this lands.

There are a couple of risks related to this CL. One big risk is deleting
surfaces we shouldn't. Two UMA stats were added to track the number of
valid and missing surfaces during surface aggregation,
Compositing.SurfaceAggregator.SurfaceDrawQuad.ValidSurface and
Compositing.SurfaceAggregator.SurfaceDrawQuad.MissingSurface. An
increase in missing surface relative to valid surfaces would indicate a
problem with deleting surfaces too early.

Another big risk is never deleting surfaces. Another UMA metric
Compositing.SurfaceManager.NumOldTemporaryReferences was added to track
this.

Bug:  676384 
Cq-Include-Trybots: master.tryserver.chromium.linux:linux_site_isolation
Change-Id: If7a5f423757f290738332640b5f600cd2dc5ccf6
Reviewed-on: https://chromium-review.googlesource.com/610834
Commit-Queue: kylechar <kylechar@chromium.org>
Reviewed-by: Antoine Labour <piman@chromium.org>
Cr-Commit-Position: refs/heads/master@{#494367}
[modify] https://crrev.com/68156e68656b00d9c963c5b30463a27b9627d14d/components/viz/common/switches.cc
[modify] https://crrev.com/68156e68656b00d9c963c5b30463a27b9627d14d/components/viz/common/switches.h
[modify] https://crrev.com/68156e68656b00d9c963c5b30463a27b9627d14d/content/browser/browser_main_loop.cc
[modify] https://crrev.com/68156e68656b00d9c963c5b30463a27b9627d14d/content/browser/frame_host/render_widget_host_view_child_frame_browsertest.cc
[modify] https://crrev.com/68156e68656b00d9c963c5b30463a27b9627d14d/content/browser/renderer_host/compositor_impl_android.cc
[modify] https://crrev.com/68156e68656b00d9c963c5b30463a27b9627d14d/content/browser/renderer_host/render_process_host_impl.cc
[modify] https://crrev.com/68156e68656b00d9c963c5b30463a27b9627d14d/content/renderer/child_frame_compositing_helper.cc

Project Member

Comment 36 by bugdroid1@chromium.org, Aug 15 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/8ecaf46f9f10dfc8d49a61b6f475d8414fa8c886

commit 8ecaf46f9f10dfc8d49a61b6f475d8414fa8c886
Author: kylechar <kylechar@chromium.org>
Date: Tue Aug 15 15:06:39 2017

Switch viz_unittests to use surface references.

This CL switches viz_unittests to use surface references instead of
surface sequences. The only major change is for
CompositorFrameSinkSupportTests. Delete tests that specifically checked
SurfaceSequence related code.

Also make the default CompositorFrameSinkSupport object not be a root
surface. Tests call EvictCurrentSurface() which won't delete a root
surface with surface references. EvictCurrentSurface() is never actually
called on a root surface in non-test code, so don't rely on the
behaviour in tests.

Bug:  676384 
Change-Id: I75a3fd1f14a38ecc21252695e0776f6df78b6339
Reviewed-on: https://chromium-review.googlesource.com/614096
Reviewed-by: Saman Sami <samans@chromium.org>
Commit-Queue: kylechar <kylechar@chromium.org>
Cr-Commit-Position: refs/heads/master@{#494380}
[modify] https://crrev.com/8ecaf46f9f10dfc8d49a61b6f475d8414fa8c886/components/viz/service/display/display_unittest.cc
[modify] https://crrev.com/8ecaf46f9f10dfc8d49a61b6f475d8414fa8c886/components/viz/service/display/surface_aggregator_perftest.cc
[modify] https://crrev.com/8ecaf46f9f10dfc8d49a61b6f475d8414fa8c886/components/viz/service/display/surface_aggregator_pixeltest.cc
[modify] https://crrev.com/8ecaf46f9f10dfc8d49a61b6f475d8414fa8c886/components/viz/service/display/surface_aggregator_unittest.cc
[modify] https://crrev.com/8ecaf46f9f10dfc8d49a61b6f475d8414fa8c886/components/viz/service/frame_sinks/compositor_frame_sink_support_unittest.cc
[modify] https://crrev.com/8ecaf46f9f10dfc8d49a61b6f475d8414fa8c886/components/viz/service/frame_sinks/direct_layer_tree_frame_sink_unittest.cc
[modify] https://crrev.com/8ecaf46f9f10dfc8d49a61b6f475d8414fa8c886/components/viz/service/frame_sinks/frame_sink_manager_unittest.cc

Project Member

Comment 37 by bugdroid1@chromium.org, Aug 16 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/8b3ef6bbac0206feb9ef2df009e991bf14dc9717

commit 8b3ef6bbac0206feb9ef2df009e991bf14dc9717
Author: kylechar <kylechar@chromium.org>
Date: Wed Aug 16 21:31:37 2017

Switch content_unittests to use surface references.

Switch the surface lifetime type to match that of the browser. There are
two tests which verified SurfaceSequences were added at an appropriate
time. The sequences aren't added anymore so remove those assertions.

RenderWidgetHostViewAuraTest.MirrorLayers only has assertions related to
SurfaceSequences. The behaviour for duplicate SurfaceLayers is checked
in SurfaceLayerTests already, so just delete this test.

Bug:  676384 
Cq-Include-Trybots: master.tryserver.chromium.linux:linux_site_isolation
Change-Id: Ic13ac5fc3993516218d1e8c81a7777d6930281bd
Reviewed-on: https://chromium-review.googlesource.com/617329
Reviewed-by: Antoine Labour <piman@chromium.org>
Commit-Queue: kylechar <kylechar@chromium.org>
Cr-Commit-Position: refs/heads/master@{#494957}
[modify] https://crrev.com/8b3ef6bbac0206feb9ef2df009e991bf14dc9717/content/browser/compositor/test/no_transport_image_transport_factory.cc
[modify] https://crrev.com/8b3ef6bbac0206feb9ef2df009e991bf14dc9717/content/browser/frame_host/render_widget_host_view_child_frame_unittest.cc
[modify] https://crrev.com/8b3ef6bbac0206feb9ef2df009e991bf14dc9717/content/browser/frame_host/render_widget_host_view_guest_unittest.cc
[modify] https://crrev.com/8b3ef6bbac0206feb9ef2df009e991bf14dc9717/content/browser/renderer_host/render_widget_host_view_aura_unittest.cc

Project Member

Comment 38 by bugdroid1@chromium.org, Aug 18 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/848a09e19e4417a1d5d3b6289e6716bfaf49086f

commit 848a09e19e4417a1d5d3b6289e6716bfaf49086f
Author: kylechar <kylechar@chromium.org>
Date: Fri Aug 18 17:03:49 2017

Flip default on FrameSinkManagerImpl.

Make FrameSinkManagerImpl by default use surface references. Most
instances of FrameSinkManagerImpl are using surface references now so it
makes the most sense to be the default.

Bug:  676384 
Change-Id: I935e89ec137f1a41f6409e0e2b045a4b6105f2b9
Reviewed-on: https://chromium-review.googlesource.com/617310
Reviewed-by: Bo <boliu@chromium.org>
Reviewed-by: Antoine Labour <piman@chromium.org>
Commit-Queue: kylechar <kylechar@chromium.org>
Cr-Commit-Position: refs/heads/master@{#495587}
[modify] https://crrev.com/848a09e19e4417a1d5d3b6289e6716bfaf49086f/android_webview/browser/surfaces_instance.cc
[modify] https://crrev.com/848a09e19e4417a1d5d3b6289e6716bfaf49086f/components/viz/host/host_frame_sink_manager_unittests.cc
[modify] https://crrev.com/848a09e19e4417a1d5d3b6289e6716bfaf49086f/components/viz/service/display/display_unittest.cc
[modify] https://crrev.com/848a09e19e4417a1d5d3b6289e6716bfaf49086f/components/viz/service/display/surface_aggregator_perftest.cc
[modify] https://crrev.com/848a09e19e4417a1d5d3b6289e6716bfaf49086f/components/viz/service/display/surface_aggregator_pixeltest.cc
[modify] https://crrev.com/848a09e19e4417a1d5d3b6289e6716bfaf49086f/components/viz/service/display/surface_aggregator_unittest.cc
[modify] https://crrev.com/848a09e19e4417a1d5d3b6289e6716bfaf49086f/components/viz/service/frame_sinks/compositor_frame_sink_support_unittest.cc
[modify] https://crrev.com/848a09e19e4417a1d5d3b6289e6716bfaf49086f/components/viz/service/frame_sinks/direct_layer_tree_frame_sink_unittest.cc
[modify] https://crrev.com/848a09e19e4417a1d5d3b6289e6716bfaf49086f/components/viz/service/frame_sinks/frame_sink_manager_impl.cc
[modify] https://crrev.com/848a09e19e4417a1d5d3b6289e6716bfaf49086f/components/viz/service/frame_sinks/frame_sink_manager_impl.h
[modify] https://crrev.com/848a09e19e4417a1d5d3b6289e6716bfaf49086f/components/viz/service/frame_sinks/frame_sink_manager_unittest.cc
[modify] https://crrev.com/848a09e19e4417a1d5d3b6289e6716bfaf49086f/components/viz/service/frame_sinks/surface_references_unittest.cc
[modify] https://crrev.com/848a09e19e4417a1d5d3b6289e6716bfaf49086f/components/viz/service/frame_sinks/surface_synchronization_unittest.cc
[modify] https://crrev.com/848a09e19e4417a1d5d3b6289e6716bfaf49086f/components/viz/service/surfaces/surface_hittest_unittest.cc
[modify] https://crrev.com/848a09e19e4417a1d5d3b6289e6716bfaf49086f/components/viz/service/surfaces/surface_unittest.cc
[modify] https://crrev.com/848a09e19e4417a1d5d3b6289e6716bfaf49086f/content/browser/browser_main_loop.cc
[modify] https://crrev.com/848a09e19e4417a1d5d3b6289e6716bfaf49086f/content/browser/compositor/test/no_transport_image_transport_factory.cc
[modify] https://crrev.com/848a09e19e4417a1d5d3b6289e6716bfaf49086f/content/browser/renderer_host/compositor_impl_android.cc
[modify] https://crrev.com/848a09e19e4417a1d5d3b6289e6716bfaf49086f/content/renderer/android/synchronous_layer_tree_frame_sink.cc
[modify] https://crrev.com/848a09e19e4417a1d5d3b6289e6716bfaf49086f/services/ui/gpu/gpu_main.cc

Project Member

Comment 39 by bugdroid1@chromium.org, Aug 23 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/a7c1d331371db9bdf6ba8ce6f19905346a91eb11

commit a7c1d331371db9bdf6ba8ce6f19905346a91eb11
Author: kylechar <kylechar@chromium.org>
Date: Wed Aug 23 20:23:26 2017

Fix GC for dependencies of live surfaces.

Make sure that if a surface is marked as live then all of it's
dependencies are also kept alive. This was done correctly for surface
sequences, but not surface references. Also add a test for this case.

Bug:  676384 
Change-Id: Id65512724d5ba0c4f9dced50c1296e176c1553d1
Reviewed-on: https://chromium-review.googlesource.com/629258
Reviewed-by: Fady Samuel <fsamuel@chromium.org>
Commit-Queue: kylechar <kylechar@chromium.org>
Cr-Commit-Position: refs/heads/master@{#496786}
[modify] https://crrev.com/a7c1d331371db9bdf6ba8ce6f19905346a91eb11/components/viz/service/frame_sinks/surface_references_unittest.cc
[modify] https://crrev.com/a7c1d331371db9bdf6ba8ce6f19905346a91eb11/components/viz/service/surfaces/surface_manager.cc

Blockedon: 760194
Project Member

Comment 41 by bugdroid1@chromium.org, Aug 29 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/e4b6f0c7c4bb48603d35f9928be8ecc8ca4d73fc

commit e4b6f0c7c4bb48603d35f9928be8ecc8ca4d73fc
Author: kylechar <kylechar@chromium.org>
Date: Tue Aug 29 19:36:50 2017

PDF viewer registers frame sink hierarchy.

We were not registering the frame sink hierarchy for the PDF viewer.
HostFrameSinkManager didn't know what FrameSinkId was expected to embed
new Surfaces from the PDF viewer and would drop the temporary
references. This meant that during resize it was possible to get into a
state where renderer embedded a PDF viewer Surface that had already been
deleted.

RenderWidgetHostViewGuest is built on RenderWidgetHostViewChildFrame
which is supposed to handle registering frame sink hierarchy. The parent
FrameSinkId is only set when using CrossProcessFrameConnector and PDF
viewer is a special case that doesn't use CrossProcessFrameConnector.

When creating RenderWidgetHostViewGuest set the parent FrameSinkId by
getting it from the owner. The owner is a renderer created to embed the
PDF viewer, so there shouldn't be any reparenting concerns.
RenderWidgetHostViewChildFrame will handle registering and unregistering
frame sink hierarchy as expected after this change.

Bug:  760194 ,  676384 
Cq-Include-Trybots: master.tryserver.chromium.linux:linux_site_isolation
Change-Id: I689426b32fb8a18bdd877ef04116777d4870c7a6
Reviewed-on: https://chromium-review.googlesource.com/641654
Reviewed-by: Fady Samuel <fsamuel@chromium.org>
Reviewed-by: Antoine Labour <piman@chromium.org>
Reviewed-by: Ehsan Karamad <ekaramad@chromium.org>
Reviewed-by: Ken Buchanan <kenrb@chromium.org>
Commit-Queue: kylechar <kylechar@chromium.org>
Cr-Commit-Position: refs/heads/master@{#498202}
[modify] https://crrev.com/e4b6f0c7c4bb48603d35f9928be8ecc8ca4d73fc/content/browser/frame_host/render_widget_host_view_guest.cc
[modify] https://crrev.com/e4b6f0c7c4bb48603d35f9928be8ecc8ca4d73fc/content/browser/renderer_host/render_widget_host_view_child_frame.cc
[modify] https://crrev.com/e4b6f0c7c4bb48603d35f9928be8ecc8ca4d73fc/content/browser/renderer_host/render_widget_host_view_child_frame.h

I just did a quick comparison of the Compositing.SurfaceAggregator.SurfaceDrawQuad.MissingSurface metric for surface sequences (stable) vs surface references (canary). The numbers look pretty comparable on average, but drilling down by each platform there is an issue with Android. These numbers are from 2017/09/10:

Non-Android Stable = 0.1458 missing surfaces per million aggregations
Non-Android Canary = 0.2249 missing surfaces per million aggregations

Android Stable = 0.0002 missing per surfaces million aggregations
Android Canary = 2.5706 missing per surfaces million aggregations

There is a problem with how surface references and CompositorImpl work on Android. I'll file a separate bug for it. I think the safest thing would be to turn off surface references on Android for M62 and fix it for M63 at this point?
Blockedon: 764460
Project Member

Comment 44 by bugdroid1@chromium.org, Sep 14 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/530b7dca898a7fd1095f33eb84bbef24a9394f35

commit 530b7dca898a7fd1095f33eb84bbef24a9394f35
Author: kylechar <kylechar@chromium.org>
Date: Thu Sep 14 17:40:22 2017

viz: Don't mark live surfaces as old.

Live surfaces can't be deleted so marking it as old makes the UMA stats
less meaningful. Add test for this behaviour.

Bug:  676384 
Change-Id: Ieb5b578ed3db2be1da5dd35edcfd52bc852e4956
Reviewed-on: https://chromium-review.googlesource.com/667716
Commit-Queue: kylechar <kylechar@chromium.org>
Reviewed-by: Xingyu Zhang <staraz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#501984}
[modify] https://crrev.com/530b7dca898a7fd1095f33eb84bbef24a9394f35/components/viz/service/frame_sinks/surface_references_unittest.cc
[modify] https://crrev.com/530b7dca898a7fd1095f33eb84bbef24a9394f35/components/viz/service/surfaces/surface_manager.cc

Blocking: -730193
Blocking: 732555
Blockedon: 782403
Project Member

Comment 48 by bugdroid1@chromium.org, Jan 5 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/5e03700025c6168eabdadb8d16e0ea0492f2acbb

commit 5e03700025c6168eabdadb8d16e0ea0492f2acbb
Author: kylechar <kylechar@chromium.org>
Date: Fri Jan 05 21:56:59 2018

viz: Fix surface reference flags.

For all non-Android platforms, surface references are being used in
stable and a flag to disable isn't needed. Stop checking flag for
non-Android platforms. Update the flag to be kEnableSurfaceReferences
and use that on Android.

Bug:  676384 
Cq-Include-Trybots: master.tryserver.chromium.android:android_optional_gpu_tests_rel
Change-Id: I196b8868b5f7cf426cdaffcce599ea34e2607a04
Reviewed-on: https://chromium-review.googlesource.com/788455
Reviewed-by: Antoine Labour <piman@chromium.org>
Commit-Queue: kylechar <kylechar@chromium.org>
Cr-Commit-Position: refs/heads/master@{#527402}
[modify] https://crrev.com/5e03700025c6168eabdadb8d16e0ea0492f2acbb/components/viz/common/switches.cc
[modify] https://crrev.com/5e03700025c6168eabdadb8d16e0ea0492f2acbb/components/viz/common/switches.h
[modify] https://crrev.com/5e03700025c6168eabdadb8d16e0ea0492f2acbb/content/browser/browser_main_loop.cc
[modify] https://crrev.com/5e03700025c6168eabdadb8d16e0ea0492f2acbb/content/browser/renderer_host/compositor_impl_android.cc
[modify] https://crrev.com/5e03700025c6168eabdadb8d16e0ea0492f2acbb/content/browser/renderer_host/render_process_host_impl.cc
[modify] https://crrev.com/5e03700025c6168eabdadb8d16e0ea0492f2acbb/content/renderer/child_frame_compositing_helper.cc

Project Member

Comment 49 by bugdroid1@chromium.org, Jan 9 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/78d11e5577a5bdd721b658dd000e8286d09f9019

commit 78d11e5577a5bdd721b658dd000e8286d09f9019
Author: kylechar <kylechar@chromium.org>
Date: Tue Jan 09 20:23:46 2018

viz: Turn on surface references again for Android.

This CL makes surface references the default for Android. The
--enable-surface-references flags is flipped to be
--disable-surface-references instead.

The webvr issue with missing surfaces no longer occurs so I don't see
any obvious problems blocking this change. This will require monitoring
the number of missing surfaces in canary to see if there are
regressions.

Bug:  676384 ,  782403 
Cq-Include-Trybots: master.tryserver.chromium.android:android_optional_gpu_tests_rel
Change-Id: Ia2bea38c2deb2fceaa369539c6d637ed21723be5
Reviewed-on: https://chromium-review.googlesource.com/857339
Reviewed-by: Fady Samuel <fsamuel@chromium.org>
Commit-Queue: kylechar <kylechar@chromium.org>
Cr-Commit-Position: refs/heads/master@{#528087}
[modify] https://crrev.com/78d11e5577a5bdd721b658dd000e8286d09f9019/components/viz/common/switches.cc
[modify] https://crrev.com/78d11e5577a5bdd721b658dd000e8286d09f9019/components/viz/common/switches.h
[modify] https://crrev.com/78d11e5577a5bdd721b658dd000e8286d09f9019/content/browser/renderer_host/compositor_impl_android.cc
[modify] https://crrev.com/78d11e5577a5bdd721b658dd000e8286d09f9019/content/browser/renderer_host/render_process_host_impl.cc
[modify] https://crrev.com/78d11e5577a5bdd721b658dd000e8286d09f9019/content/renderer/child_frame_compositing_helper.cc

Project Member

Comment 50 by bugdroid1@chromium.org, Jan 12 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/db662c7cd30eba687ee54c27633ec1e00088b7f6

commit db662c7cd30eba687ee54c27633ec1e00088b7f6
Author: kylechar <kylechar@chromium.org>
Date: Fri Jan 12 00:53:27 2018

viz: Drop temporary references without client.

When FrameSinkManagerImpl doesn't have |client_| set, there is nothing
to assign owners for temporary references. This case should only occur
in Android WebView (where we don't want temporary references) and tests,
so drop temporary references.

Also add TestFrameSinkManagerClient for tests that has some simple
functionality for setting up FrameSinkId hierarchy and
assigning/dropping temporary references based on this. Update tests in
viz_unittests accordingly.

Bug:  676384 
Cq-Include-Trybots: master.tryserver.chromium.android:android_optional_gpu_tests_rel
Change-Id: I51f66054c83c5b51f170dcecaa36aa91b053add1
Reviewed-on: https://chromium-review.googlesource.com/860749
Commit-Queue: kylechar <kylechar@chromium.org>
Reviewed-by: Saman Sami <samans@chromium.org>
Cr-Commit-Position: refs/heads/master@{#528833}
[modify] https://crrev.com/db662c7cd30eba687ee54c27633ec1e00088b7f6/components/viz/service/frame_sinks/compositor_frame_sink_support_unittest.cc
[modify] https://crrev.com/db662c7cd30eba687ee54c27633ec1e00088b7f6/components/viz/service/frame_sinks/frame_sink_manager_impl.cc
[modify] https://crrev.com/db662c7cd30eba687ee54c27633ec1e00088b7f6/components/viz/service/frame_sinks/surface_references_unittest.cc
[modify] https://crrev.com/db662c7cd30eba687ee54c27633ec1e00088b7f6/components/viz/service/frame_sinks/surface_synchronization_unittest.cc
[modify] https://crrev.com/db662c7cd30eba687ee54c27633ec1e00088b7f6/components/viz/test/BUILD.gn
[add] https://crrev.com/db662c7cd30eba687ee54c27633ec1e00088b7f6/components/viz/test/test_frame_sink_manager_client.cc
[add] https://crrev.com/db662c7cd30eba687ee54c27633ec1e00088b7f6/components/viz/test/test_frame_sink_manager_client.h

Project Member

Comment 51 by bugdroid1@chromium.org, Jan 12 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/cc0ca6441e818883182bed92d7f8c34433575243

commit cc0ca6441e818883182bed92d7f8c34433575243
Author: kylechar <kylechar@chromium.org>
Date: Fri Jan 12 20:22:07 2018

viz: Switch Android WebView to use surface references.

Switch Android WebView to use surface references instead of surface
sequences. WebView didn't actually use the mechanism to add surface
seqeuence dependencies. This mechanism was replaced by temporary
references, which WebView continues to not use, so this should be no-op.

Bug:  676384 
Cq-Include-Trybots: master.tryserver.chromium.android:android_optional_gpu_tests_rel
Change-Id: I2e1c938a3b2ac633a19c06be8ccbe006236c6731
Reviewed-on: https://chromium-review.googlesource.com/860993
Reviewed-by: Fady Samuel <fsamuel@chromium.org>
Reviewed-by: Bo <boliu@chromium.org>
Reviewed-by: Yaron Friedman <yfriedman@chromium.org>
Commit-Queue: kylechar <kylechar@chromium.org>
Cr-Commit-Position: refs/heads/master@{#529045}
[modify] https://crrev.com/cc0ca6441e818883182bed92d7f8c34433575243/android_webview/browser/surfaces_instance.cc
[modify] https://crrev.com/cc0ca6441e818883182bed92d7f8c34433575243/components/viz/service/surfaces/surface_manager.cc
[modify] https://crrev.com/cc0ca6441e818883182bed92d7f8c34433575243/content/renderer/android/synchronous_layer_tree_frame_sink.cc

Project Member

Comment 52 by bugdroid1@chromium.org, Jan 22 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/ee539faa0097eef30896260458f02039b979125b

commit ee539faa0097eef30896260458f02039b979125b
Author: kylechar <kylechar@chromium.org>
Date: Mon Jan 22 17:00:55 2018

viz: Delete all surface sequence code.

All platforms have been converted to use surface references instead of
surface sequences. This CL removes the following code:

1. SurfaceSequence (plus ParamTraits and StructTraits)
2. SurfaceSequenceGenerator
3. SurfaceReferenceFactory (plus many subclasses).
4. Any flags or enums to choose surface references vs surface sequences.
5. Any SurfaceSequence specific IPC messages.

The mojom::OffscreenCanvasSurface interface is left in strange state
where it has no methods but closing the interface controls the offscreen
canvas lifetime. The interface can likely be refactored / removed in a
follow up CL.

Bug:  676384 ,  796700 
Cq-Include-Trybots: master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
Change-Id: I5e01882c18770adbc4a43c570881011d5ed0c396
Reviewed-on: https://chromium-review.googlesource.com/862397
Commit-Queue: kylechar <kylechar@chromium.org>
Reviewed-by: Scott Violet <sky@chromium.org>
Reviewed-by: Kenneth Russell <kbr@chromium.org>
Reviewed-by: Ken Buchanan <kenrb@chromium.org>
Reviewed-by: Antoine Labour <piman@chromium.org>
Reviewed-by: Fady Samuel <fsamuel@chromium.org>
Cr-Commit-Position: refs/heads/master@{#530891}
[modify] https://crrev.com/ee539faa0097eef30896260458f02039b979125b/cc/ipc/cc_param_traits_macros.h
[modify] https://crrev.com/ee539faa0097eef30896260458f02039b979125b/cc/layers/surface_layer.cc
[modify] https://crrev.com/ee539faa0097eef30896260458f02039b979125b/cc/layers/surface_layer.h
[modify] https://crrev.com/ee539faa0097eef30896260458f02039b979125b/cc/layers/surface_layer_unittest.cc
[modify] https://crrev.com/ee539faa0097eef30896260458f02039b979125b/cc/trees/layer_tree_host.cc
[modify] https://crrev.com/ee539faa0097eef30896260458f02039b979125b/cc/trees/layer_tree_host.h
[modify] https://crrev.com/ee539faa0097eef30896260458f02039b979125b/chrome/browser/ui/overlay/overlay_surface_embedder.cc
[modify] https://crrev.com/ee539faa0097eef30896260458f02039b979125b/chrome/browser/ui/overlay/overlay_surface_embedder.h
[modify] https://crrev.com/ee539faa0097eef30896260458f02039b979125b/components/exo/surface.cc
[modify] https://crrev.com/ee539faa0097eef30896260458f02039b979125b/components/viz/common/BUILD.gn
[delete] https://crrev.com/f4387d76f39285fd0741d53a0e1155a3e514661f/components/viz/common/surfaces/sequence_surface_reference_factory.cc
[delete] https://crrev.com/f4387d76f39285fd0741d53a0e1155a3e514661f/components/viz/common/surfaces/sequence_surface_reference_factory.h
[delete] https://crrev.com/f4387d76f39285fd0741d53a0e1155a3e514661f/components/viz/common/surfaces/stub_surface_reference_factory.cc
[delete] https://crrev.com/f4387d76f39285fd0741d53a0e1155a3e514661f/components/viz/common/surfaces/stub_surface_reference_factory.h
[delete] https://crrev.com/f4387d76f39285fd0741d53a0e1155a3e514661f/components/viz/common/surfaces/surface_reference_factory.h
[delete] https://crrev.com/f4387d76f39285fd0741d53a0e1155a3e514661f/components/viz/common/surfaces/surface_reference_owner.h
[delete] https://crrev.com/f4387d76f39285fd0741d53a0e1155a3e514661f/components/viz/common/surfaces/surface_sequence.h
[delete] https://crrev.com/f4387d76f39285fd0741d53a0e1155a3e514661f/components/viz/common/surfaces/surface_sequence_generator.cc
[delete] https://crrev.com/f4387d76f39285fd0741d53a0e1155a3e514661f/components/viz/common/surfaces/surface_sequence_generator.h
[delete] https://crrev.com/f4387d76f39285fd0741d53a0e1155a3e514661f/components/viz/common/surfaces/surface_sequence_generator_unittest.cc
[modify] https://crrev.com/ee539faa0097eef30896260458f02039b979125b/components/viz/common/switches.cc
[modify] https://crrev.com/ee539faa0097eef30896260458f02039b979125b/components/viz/common/switches.h
[modify] https://crrev.com/ee539faa0097eef30896260458f02039b979125b/components/viz/host/host_frame_sink_manager.cc
[modify] https://crrev.com/ee539faa0097eef30896260458f02039b979125b/components/viz/service/BUILD.gn
[modify] https://crrev.com/ee539faa0097eef30896260458f02039b979125b/components/viz/service/display/display_scheduler_unittest.cc
[modify] https://crrev.com/ee539faa0097eef30896260458f02039b979125b/components/viz/service/frame_sinks/frame_sink_manager_impl.cc
[modify] https://crrev.com/ee539faa0097eef30896260458f02039b979125b/components/viz/service/frame_sinks/frame_sink_manager_impl.h
[modify] https://crrev.com/ee539faa0097eef30896260458f02039b979125b/components/viz/service/main/viz_main_impl.cc
[delete] https://crrev.com/f4387d76f39285fd0741d53a0e1155a3e514661f/components/viz/service/surfaces/direct_surface_reference_factory.cc
[delete] https://crrev.com/f4387d76f39285fd0741d53a0e1155a3e514661f/components/viz/service/surfaces/direct_surface_reference_factory.h
[modify] https://crrev.com/ee539faa0097eef30896260458f02039b979125b/components/viz/service/surfaces/surface.cc
[modify] https://crrev.com/ee539faa0097eef30896260458f02039b979125b/components/viz/service/surfaces/surface.h
[modify] https://crrev.com/ee539faa0097eef30896260458f02039b979125b/components/viz/service/surfaces/surface_manager.cc
[modify] https://crrev.com/ee539faa0097eef30896260458f02039b979125b/components/viz/service/surfaces/surface_manager.h
[modify] https://crrev.com/ee539faa0097eef30896260458f02039b979125b/components/viz/service/surfaces/surface_unittest.cc
[modify] https://crrev.com/ee539faa0097eef30896260458f02039b979125b/content/browser/browser_main_loop.cc
[modify] https://crrev.com/ee539faa0097eef30896260458f02039b979125b/content/browser/browser_plugin/browser_plugin_guest.cc
[modify] https://crrev.com/ee539faa0097eef30896260458f02039b979125b/content/browser/browser_plugin/browser_plugin_guest.h
[modify] https://crrev.com/ee539faa0097eef30896260458f02039b979125b/content/browser/frame_host/cross_process_frame_connector.cc
[modify] https://crrev.com/ee539faa0097eef30896260458f02039b979125b/content/browser/frame_host/cross_process_frame_connector.h
[modify] https://crrev.com/ee539faa0097eef30896260458f02039b979125b/content/browser/frame_host/render_widget_host_view_guest.cc
[modify] https://crrev.com/ee539faa0097eef30896260458f02039b979125b/content/browser/frame_host/render_widget_host_view_guest.h
[modify] https://crrev.com/ee539faa0097eef30896260458f02039b979125b/content/browser/frame_host/render_widget_host_view_guest_unittest.cc
[modify] https://crrev.com/ee539faa0097eef30896260458f02039b979125b/content/browser/renderer_host/compositor_impl_android.cc
[modify] https://crrev.com/ee539faa0097eef30896260458f02039b979125b/content/browser/renderer_host/delegated_frame_host.cc
[modify] https://crrev.com/ee539faa0097eef30896260458f02039b979125b/content/browser/renderer_host/delegated_frame_host.h
[modify] https://crrev.com/ee539faa0097eef30896260458f02039b979125b/content/browser/renderer_host/frame_connector_delegate.h
[modify] https://crrev.com/ee539faa0097eef30896260458f02039b979125b/content/browser/renderer_host/offscreen_canvas_surface_impl.cc
[modify] https://crrev.com/ee539faa0097eef30896260458f02039b979125b/content/browser/renderer_host/offscreen_canvas_surface_impl.h
[modify] https://crrev.com/ee539faa0097eef30896260458f02039b979125b/content/browser/renderer_host/render_process_host_impl.cc
[modify] https://crrev.com/ee539faa0097eef30896260458f02039b979125b/content/browser/renderer_host/render_widget_host_view_android.cc
[modify] https://crrev.com/ee539faa0097eef30896260458f02039b979125b/content/browser/renderer_host/render_widget_host_view_child_frame.cc
[modify] https://crrev.com/ee539faa0097eef30896260458f02039b979125b/content/browser/renderer_host/render_widget_host_view_child_frame.h
[modify] https://crrev.com/ee539faa0097eef30896260458f02039b979125b/content/browser/renderer_host/render_widget_host_view_child_frame_browsertest.cc
[modify] https://crrev.com/ee539faa0097eef30896260458f02039b979125b/content/browser/renderer_host/render_widget_host_view_child_frame_unittest.cc
[modify] https://crrev.com/ee539faa0097eef30896260458f02039b979125b/content/common/browser_plugin/browser_plugin_messages.h
[modify] https://crrev.com/ee539faa0097eef30896260458f02039b979125b/content/common/frame_messages.h
[modify] https://crrev.com/ee539faa0097eef30896260458f02039b979125b/content/renderer/browser_plugin/browser_plugin.cc
[modify] https://crrev.com/ee539faa0097eef30896260458f02039b979125b/content/renderer/browser_plugin/browser_plugin.h
[modify] https://crrev.com/ee539faa0097eef30896260458f02039b979125b/content/renderer/child_frame_compositing_helper.cc
[modify] https://crrev.com/ee539faa0097eef30896260458f02039b979125b/content/renderer/child_frame_compositing_helper.h
[modify] https://crrev.com/ee539faa0097eef30896260458f02039b979125b/content/renderer/gpu/render_widget_compositor.cc
[modify] https://crrev.com/ee539faa0097eef30896260458f02039b979125b/content/renderer/mus/mus_embedded_frame.cc
[modify] https://crrev.com/ee539faa0097eef30896260458f02039b979125b/content/renderer/mus/renderer_window_tree_client.cc
[modify] https://crrev.com/ee539faa0097eef30896260458f02039b979125b/content/renderer/render_frame_proxy.cc
[modify] https://crrev.com/ee539faa0097eef30896260458f02039b979125b/content/renderer/render_frame_proxy.h
[modify] https://crrev.com/ee539faa0097eef30896260458f02039b979125b/services/viz/public/cpp/compositing/struct_traits_unittest.cc
[delete] https://crrev.com/f4387d76f39285fd0741d53a0e1155a3e514661f/services/viz/public/cpp/compositing/surface_sequence.typemap
[delete] https://crrev.com/f4387d76f39285fd0741d53a0e1155a3e514661f/services/viz/public/cpp/compositing/surface_sequence_struct_traits.h
[modify] https://crrev.com/ee539faa0097eef30896260458f02039b979125b/services/viz/public/cpp/compositing/typemaps.gni
[modify] https://crrev.com/ee539faa0097eef30896260458f02039b979125b/services/viz/public/interfaces/BUILD.gn
[delete] https://crrev.com/f4387d76f39285fd0741d53a0e1155a3e514661f/services/viz/public/interfaces/compositing/surface_sequence.mojom
[modify] https://crrev.com/ee539faa0097eef30896260458f02039b979125b/third_party/WebKit/Source/platform/graphics/SurfaceLayerBridge.cpp
[modify] https://crrev.com/ee539faa0097eef30896260458f02039b979125b/third_party/WebKit/Source/platform/graphics/SurfaceLayerBridge.h
[modify] https://crrev.com/ee539faa0097eef30896260458f02039b979125b/third_party/WebKit/public/blink_typemaps.gni
[modify] https://crrev.com/ee539faa0097eef30896260458f02039b979125b/third_party/WebKit/public/platform/modules/offscreencanvas/offscreen_canvas_surface.mojom
[modify] https://crrev.com/ee539faa0097eef30896260458f02039b979125b/ui/android/delegated_frame_host_android.cc
[modify] https://crrev.com/ee539faa0097eef30896260458f02039b979125b/ui/android/delegated_frame_host_android.h
[modify] https://crrev.com/ee539faa0097eef30896260458f02039b979125b/ui/aura/local/window_port_local.cc
[modify] https://crrev.com/ee539faa0097eef30896260458f02039b979125b/ui/aura/mus/client_surface_embedder.cc
[modify] https://crrev.com/ee539faa0097eef30896260458f02039b979125b/ui/aura/mus/client_surface_embedder.h
[modify] https://crrev.com/ee539faa0097eef30896260458f02039b979125b/ui/aura/mus/window_port_mus.cc
[modify] https://crrev.com/ee539faa0097eef30896260458f02039b979125b/ui/compositor/compositor.cc
[modify] https://crrev.com/ee539faa0097eef30896260458f02039b979125b/ui/compositor/compositor.h
[modify] https://crrev.com/ee539faa0097eef30896260458f02039b979125b/ui/compositor/layer.cc
[modify] https://crrev.com/ee539faa0097eef30896260458f02039b979125b/ui/compositor/layer.h
[modify] https://crrev.com/ee539faa0097eef30896260458f02039b979125b/ui/compositor/layer_unittest.cc

Blockedon: 804696
This is essentially done, other than fixing a regression on Android caused by a timer.
Status: Fixed (was: Started)
All done.
Cc: -varkha@chromium.org

Sign in to add a comment