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

Issue 792192 link

Starred by 1 user
Status: Fixed
Owner:
kylec...@chromium.org
Closed: Dec 2017
Cc:
█ fsam...@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Task



Sign in to add a comment

Cleanup viz frame sink code

Project Member Reported by kylec...@chromium.org, Dec 5 2017 
The FrameSinkManagerClient class isn't necessary and it has a number of downsides.

1. FrameSinkManagerImpl already knows about CompositorFrameSinkSupports, which are the only implementation of FrameSinkManagerClient, so FrameSinkManagerImpl has two pointers back to the same object.
2. The virtual SetBeingFrameSource() is the only reason that CompositorFrameSinkSupport::Create() is needed.
3. There is another class with an almost identical name, viz::mojom::FrameSinkManagerClient.

It would be better if this class didn't exist anymore and FrameSinkManagerImpl was cleaned up.
Project Member

Comment 1 by bugdroid1@chromium.org, Dec 6 2017 

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

commit 8390d6521342c17eb17ec090e24641e74b995e28
Author: kylechar <kylechar@chromium.org>
Date: Wed Dec 06 00:29:42 2017

Use CompositorFrameSinkSupport in tests.

This CL changes FrameSinkManagerTest to use CompositorFrameSinkSupport
instead of FrameSinkManagerClient. This will allow for
FrameSinkManagerClient to be deleted in a follow up CL and
FrameSinkManagerImpls internal data structures to be cleaned up.

Also rename SurfaceManagerOrderingParamTest to
FrameSinkManagerOrderingParamTest. This wasn't updated previously.

Bug:  792192 
Cq-Include-Trybots: master.tryserver.chromium.android:android_optional_gpu_tests_rel
Change-Id: I0ad3f1e8f8530dccf001fe81c6c9191176dbaddf
Reviewed-on: https://chromium-review.googlesource.com/809614
Reviewed-by: Fady Samuel <fsamuel@chromium.org>
Commit-Queue: kylechar <kylechar@chromium.org>
Cr-Commit-Position: refs/heads/master@{#521912}
[modify] https://crrev.com/8390d6521342c17eb17ec090e24641e74b995e28/components/viz/service/frame_sinks/compositor_frame_sink_support.h
[modify] https://crrev.com/8390d6521342c17eb17ec090e24641e74b995e28/components/viz/service/frame_sinks/frame_sink_manager_unittest.cc
Project Member

Comment 2 by bugdroid1@chromium.org, Dec 7 2017 

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

commit d582339afac55fdbe01a8694c9e5b07449132b6e
Author: kylechar <kylechar@chromium.org>
Date: Thu Dec 07 03:05:36 2017

Delete viz::FrameSinkManagerClient and cleanup.

This CL deletes viz::FrameSinkManagerClient. FrameSinkManagerImpl has a
pointer to each CompositorFrameSinkSupport directly now since this is
needed anyways. The FrameSinkManagerImpl::clients_ map is removed as a
result.

SinkAndSupport will always contain a CompositorFrameSinkSupport* while
the CompositorFrameSinkSupport object exists. SinkAndSupport::sink will
be non-null when FrameSinkManagerImpl owns the compositor frame sink.

CompositorFrameSinkSupport::Init()/Create() are also no longer needed
since SetBeginFrameSource() is no longer virtual. Remove Init() and make
the constructor public. Create() is left since it's widely used for now
but is marked deprecated.

Also remove some old/wrong forward definitions and fix some style guide
violations.

Bug:  792192 
Cq-Include-Trybots: master.tryserver.chromium.android:android_optional_gpu_tests_rel
Change-Id: I14f9662c9db485ce18edf1d358493648997d3648
Reviewed-on: https://chromium-review.googlesource.com/810866
Reviewed-by: Fady Samuel <fsamuel@chromium.org>
Reviewed-by: Yuri Wiitala <miu@chromium.org>
Commit-Queue: kylechar <kylechar@chromium.org>
Cr-Commit-Position: refs/heads/master@{#522323}
[modify] https://crrev.com/d582339afac55fdbe01a8694c9e5b07449132b6e/components/viz/service/BUILD.gn
[modify] https://crrev.com/d582339afac55fdbe01a8694c9e5b07449132b6e/components/viz/service/frame_sinks/compositor_frame_sink_support.cc
[modify] https://crrev.com/d582339afac55fdbe01a8694c9e5b07449132b6e/components/viz/service/frame_sinks/compositor_frame_sink_support.h
[delete] https://crrev.com/ea6e12b3635b8f77966cd4bacc02c31b914e95c4/components/viz/service/frame_sinks/frame_sink_manager_client.h
[modify] https://crrev.com/d582339afac55fdbe01a8694c9e5b07449132b6e/components/viz/service/frame_sinks/frame_sink_manager_impl.cc
[modify] https://crrev.com/d582339afac55fdbe01a8694c9e5b07449132b6e/components/viz/service/frame_sinks/frame_sink_manager_impl.h
Project Member

Comment 3 by bugdroid1@chromium.org, Dec 7 2017 

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

commit 5000787d004e79605d5ecc1b59298345afb133cf
Author: kylechar <kylechar@chromium.org>
Date: Thu Dec 07 22:21:24 2017

Fix FrameSinkManagerImpl member variables.

This CL fixes some issues with FrameSinkManagerImpl:
1. We weren't always deleting things from |frame_sink_source_map_| and
   useless entries were building up. Fix this by deleting entries if
   they contain no data even if there is a registered
   CompositorFrameSinkSupport. We will just recreate the map entries
   without any data if they are needed later. Also fix tests to ensure
   this doesn't reoccur.
2. Use flat_map/flat_set where appropriate. We have a small number of
   infrequently changing FrameSinkIds so flat data structures should
   provide good performance and reduced memory usage.
3. Cleanup some logic in FrameSinkManagerImpl to make it more
   understandable. Fix one potential problem with iterator invalidation.

Bug:  792192 
Cq-Include-Trybots: master.tryserver.chromium.android:android_optional_gpu_tests_rel
Change-Id: Ied85bd52721f935ac243383350753c8f88ff27c1
Reviewed-on: https://chromium-review.googlesource.com/814217
Reviewed-by: Fady Samuel <fsamuel@chromium.org>
Commit-Queue: kylechar <kylechar@chromium.org>
Cr-Commit-Position: refs/heads/master@{#522573}
[modify] https://crrev.com/5000787d004e79605d5ecc1b59298345afb133cf/components/viz/service/frame_sinks/frame_sink_manager_impl.cc
[modify] https://crrev.com/5000787d004e79605d5ecc1b59298345afb133cf/components/viz/service/frame_sinks/frame_sink_manager_impl.h
[modify] https://crrev.com/5000787d004e79605d5ecc1b59298345afb133cf/components/viz/service/frame_sinks/frame_sink_manager_unittest.cc

Comment 4 by kylec...@chromium.org, Dec 8 2017

Status: Fixed (was: Started)

Comment 5 by kylec...@chromium.org, Dec 14 2017

Status: Started (was: Fixed)
Summary: Cleanup viz frame sink code (was: Delete viz::FrameSinkManagerClient)
Project Member

Comment 6 by bugdroid1@chromium.org, Dec 14 2017 

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

commit deac2fada00576e6edb77e8ce410dcb67155dc05
Author: kylechar <kylechar@chromium.org>
Date: Thu Dec 14 17:43:32 2017

viz: Remove CompositorFrameSinkSupport::Create().

Create directly with std::make_unique instead.

TBR: boliu@chromium.org, tedchoc@chromium.org
Bug:  792192 
Cq-Include-Trybots: master.tryserver.chromium.android:android_optional_gpu_tests_rel
Change-Id: I97442118fef4efb6dd0be4e64b5f140499a469f2
Reviewed-on: https://chromium-review.googlesource.com/826428
Reviewed-by: Bo <boliu@chromium.org>
Reviewed-by: danakj <danakj@chromium.org>
Commit-Queue: kylechar <kylechar@chromium.org>
Cr-Commit-Position: refs/heads/master@{#524105}
[modify] https://crrev.com/deac2fada00576e6edb77e8ce410dcb67155dc05/android_webview/browser/hardware_renderer.cc
[modify] https://crrev.com/deac2fada00576e6edb77e8ce410dcb67155dc05/android_webview/browser/surfaces_instance.cc
[modify] https://crrev.com/deac2fada00576e6edb77e8ce410dcb67155dc05/components/viz/host/host_frame_sink_manager.cc
[modify] https://crrev.com/deac2fada00576e6edb77e8ce410dcb67155dc05/components/viz/service/display/display_unittest.cc
[modify] https://crrev.com/deac2fada00576e6edb77e8ce410dcb67155dc05/components/viz/service/display/surface_aggregator_perftest.cc
[modify] https://crrev.com/deac2fada00576e6edb77e8ce410dcb67155dc05/components/viz/service/display/surface_aggregator_pixeltest.cc
[modify] https://crrev.com/deac2fada00576e6edb77e8ce410dcb67155dc05/components/viz/service/display/surface_aggregator_unittest.cc
[modify] https://crrev.com/deac2fada00576e6edb77e8ce410dcb67155dc05/components/viz/service/frame_sinks/compositor_frame_sink_impl.cc
[modify] https://crrev.com/deac2fada00576e6edb77e8ce410dcb67155dc05/components/viz/service/frame_sinks/compositor_frame_sink_support.cc
[modify] https://crrev.com/deac2fada00576e6edb77e8ce410dcb67155dc05/components/viz/service/frame_sinks/compositor_frame_sink_support.h
[modify] https://crrev.com/deac2fada00576e6edb77e8ce410dcb67155dc05/components/viz/service/frame_sinks/compositor_frame_sink_support_unittest.cc
[modify] https://crrev.com/deac2fada00576e6edb77e8ce410dcb67155dc05/components/viz/service/frame_sinks/direct_layer_tree_frame_sink.h
[modify] https://crrev.com/deac2fada00576e6edb77e8ce410dcb67155dc05/components/viz/service/frame_sinks/direct_layer_tree_frame_sink_unittest.cc
[modify] https://crrev.com/deac2fada00576e6edb77e8ce410dcb67155dc05/components/viz/service/frame_sinks/frame_sink_manager_unittest.cc
[modify] https://crrev.com/deac2fada00576e6edb77e8ce410dcb67155dc05/components/viz/service/frame_sinks/root_compositor_frame_sink_impl.cc
[modify] https://crrev.com/deac2fada00576e6edb77e8ce410dcb67155dc05/components/viz/service/frame_sinks/surface_references_unittest.cc
[modify] https://crrev.com/deac2fada00576e6edb77e8ce410dcb67155dc05/components/viz/service/frame_sinks/surface_synchronization_unittest.cc
[modify] https://crrev.com/deac2fada00576e6edb77e8ce410dcb67155dc05/components/viz/service/frame_sinks/video_detector_unittest.cc
[modify] https://crrev.com/deac2fada00576e6edb77e8ce410dcb67155dc05/components/viz/service/surfaces/surface_hittest_unittest.cc
[modify] https://crrev.com/deac2fada00576e6edb77e8ce410dcb67155dc05/components/viz/service/surfaces/surface_unittest.cc
[modify] https://crrev.com/deac2fada00576e6edb77e8ce410dcb67155dc05/components/viz/test/test_layer_tree_frame_sink.cc
[modify] https://crrev.com/deac2fada00576e6edb77e8ce410dcb67155dc05/content/renderer/android/synchronous_layer_tree_frame_sink.cc

Comment 7 by kylec...@chromium.org, Dec 14 2017

Status: Fixed (was: Started)
►

Sign in to add a comment