Mus+Ash: Mus should signal clients with BeginFrame |
|||||||||||||
Issue descriptionCurrently, in Mus+Ash, each client ticks on its own. We should make sure that all clients receive BeginFrames from Mus instead. ServerWindowSurface is a SurfaceFactoryClient and so it will receive "SetBeginFrameSource" once its FrameSinkId is registered in SurfaceManager. When a ServerWindowSurface is created it should register as a child of it's parent window's surface via SurfaceManager::RegisterFrameSinkHierarchy. That will cause ServerWindowSurface::SetBeginFrameSource to get called. If the client has said it needs BeginFrames, then we should install an observer (ServerWindowSurface can be that observer). See RenderWidgetHostViewAura::UpdateNeedsBeginFramesInternal as an example. ServerWindowSurface should send an IPC back to ui::CompositorFrameSink about the BeginFrame along with BeginFrameArgs via SurfaceClient (this interface should probably be called FrameSource at some point in the future). ui::CompositorFrameSink should hold an ExternalBeginFrameSource and should be ticked in response to that IPC.
,
Oct 9 2016
Yes, basically. +sky@ Mus-ws schedules new frames in an adhoc way. Currently, there's a ServerWindowDelegate::OnScheduleWindowPaint mechanism that ultimately triggers FrameGenerator::Draw. Once FrameGenerator is ticked by BeginFrameSource, "SubmitCompositorFrame" should automatically request a BeginFrame, I think, and so this "ScheduleWindowPaint" mechanism can go away.
,
Oct 14 2016
If --use-mus-in-renderer then we likely need a special case for a Mus/Mojo triggered ExternalBeginFrameSource. This will likely require a change in RenderThreadImpl::CreateExternalBeginFrameSource.
,
Oct 14 2016
FrameGenerator should probably hold a SyntheticBeginFrame source which is updated based on vsync parameters. Every ServerWindowSurface is a child of FrameGenerator and so it should also get the BeginFrameSource automagically if we "SurfaceManager::RegisterFrameSinkHierarchy". When root window is created, FrameGenerator is its parent. If FrameGenerator is created after root window, then we assign hierarchy at creation. When a child surface created, we "RegisterFrameSinkHierarchy" with the parent surface of the same type. That will propagate the BeginFrameSource down the window hierarchy. We need to respond OnBeginFrame inside the "ServerWindowSurface" or "FrameGenerator" by sending an IPC or calling DrawWindowTree respectively. In other words, FrameGenerator::OnBeginFrame calls FrameGenerator::Draw. ServerWindowSurface::OnBeginFrame sends an IPC out through WindowTreeClient interface that signals BeginFrame to ui::CompositorFrameSink.
,
Oct 14 2016
There was some point in time where FrameGenerator owned a DisplayCompositor which owned a cc::Display which owned a SyntheticBeginFrameSource already, but it looks like that's no longer the case. Agreed with the rest of this, in terms of needing to register the hierarchy of ServerWindowSurfaces, and then IPCing to the browser process. There was some awkwardness in terms of what owned the begin frame source in the browser process side of things and what type it is, that's mostly due to the fact that GpuProcessTransportFactory appears to create a cc::Display over on the browser side still. I had a wip patch for this at some point, but got pulled off by a bunch of other regressions as this didn't seem to be that critical.
,
Nov 8 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/a6ce960a00e4b8bf5608d51b6d95eb4963271887 commit a6ce960a00e4b8bf5608d51b6d95eb4963271887 Author: fsamuel <fsamuel@chromium.org> Date: Tue Nov 08 03:36:08 2016 Mus+Ash: Unified BeginFrame Skeleton This CL implements a skeleton ExternalBeginFrameSource in Mus+Ash. The following are the notable changes in this patch: 1. MojoCompositorFrameSinkClient gets an OnBeginFrame method. 2. A MojoCompositorFrameSinkPrivate interface is introduced with methods to add and remove child FrameSinkIds for BeginFrame propagation. 3. ServerWindowCOmpositorFrameSink implements MojoCompositorFrameSinkPrivate. 4. ServerWindowCompositorFrameSinkManager no longer retains direct ownership of ServerWindowCompositorFrameSink, but rather creates a strong binding to the MojoCompositorFrameSinkPrivate messagepipe and holds one end of the pipe. In a future CL, ServerWindowCompositorFrameSinkManager will no longer create ServerWindowCompositorFrameSink directly but through a mojo interface on DisplayCompositor. 5. All ServerWindowCompositorFrameSinks have the root window's frame sink as their BeginFrame parent. This is temporary until Surface ID propagation details are all worked out. This will not work properly on multi-monitor yet. 6. ScheduleWindowPaint is gone throughout the window server. Instead, for now, FrameGenerator always requests a BeginFrame and calls DrawWindowTree when it receives a BeginFrame. 7. ServerWindowCompositorFrameSink no longer needs to know about ServerWindow or ServerWindowCompositorFrameSinkManager which takes us a step closer to the mus-ws/mus-gpu split. 8. WindowCompositorFrameSink holds an ExternalBeginFrameSource that is updated from Mus. BUG= 653895 TBR=junov@chromium.org for trivial offscreen canvas change. CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel Review-Url: https://codereview.chromium.org/2474113002 Cr-Commit-Position: refs/heads/master@{#430504} [modify] https://crrev.com/a6ce960a00e4b8bf5608d51b6d95eb4963271887/cc/ipc/BUILD.gn [modify] https://crrev.com/a6ce960a00e4b8bf5608d51b6d95eb4963271887/cc/ipc/begin_frame_args_struct_traits.h [modify] https://crrev.com/a6ce960a00e4b8bf5608d51b6d95eb4963271887/cc/ipc/mojo_compositor_frame_sink.mojom [modify] https://crrev.com/a6ce960a00e4b8bf5608d51b6d95eb4963271887/services/ui/public/cpp/window_compositor_frame_sink.cc [modify] https://crrev.com/a6ce960a00e4b8bf5608d51b6d95eb4963271887/services/ui/public/cpp/window_compositor_frame_sink.h [modify] https://crrev.com/a6ce960a00e4b8bf5608d51b6d95eb4963271887/services/ui/ws/display.cc [modify] https://crrev.com/a6ce960a00e4b8bf5608d51b6d95eb4963271887/services/ui/ws/display.h [modify] https://crrev.com/a6ce960a00e4b8bf5608d51b6d95eb4963271887/services/ui/ws/display_manager.cc [modify] https://crrev.com/a6ce960a00e4b8bf5608d51b6d95eb4963271887/services/ui/ws/display_manager.h [modify] https://crrev.com/a6ce960a00e4b8bf5608d51b6d95eb4963271887/services/ui/ws/frame_generator.cc [modify] https://crrev.com/a6ce960a00e4b8bf5608d51b6d95eb4963271887/services/ui/ws/frame_generator.h [modify] https://crrev.com/a6ce960a00e4b8bf5608d51b6d95eb4963271887/services/ui/ws/platform_display.cc [modify] https://crrev.com/a6ce960a00e4b8bf5608d51b6d95eb4963271887/services/ui/ws/platform_display.h [modify] https://crrev.com/a6ce960a00e4b8bf5608d51b6d95eb4963271887/services/ui/ws/server_window.cc [modify] https://crrev.com/a6ce960a00e4b8bf5608d51b6d95eb4963271887/services/ui/ws/server_window_compositor_frame_sink.cc [modify] https://crrev.com/a6ce960a00e4b8bf5608d51b6d95eb4963271887/services/ui/ws/server_window_compositor_frame_sink.h [modify] https://crrev.com/a6ce960a00e4b8bf5608d51b6d95eb4963271887/services/ui/ws/server_window_compositor_frame_sink_manager.cc [modify] https://crrev.com/a6ce960a00e4b8bf5608d51b6d95eb4963271887/services/ui/ws/server_window_compositor_frame_sink_manager.h [modify] https://crrev.com/a6ce960a00e4b8bf5608d51b6d95eb4963271887/services/ui/ws/server_window_delegate.h [modify] https://crrev.com/a6ce960a00e4b8bf5608d51b6d95eb4963271887/services/ui/ws/test_server_window_delegate.cc [modify] https://crrev.com/a6ce960a00e4b8bf5608d51b6d95eb4963271887/services/ui/ws/test_server_window_delegate.h [modify] https://crrev.com/a6ce960a00e4b8bf5608d51b6d95eb4963271887/services/ui/ws/test_utils.cc [modify] https://crrev.com/a6ce960a00e4b8bf5608d51b6d95eb4963271887/services/ui/ws/window_server.cc [modify] https://crrev.com/a6ce960a00e4b8bf5608d51b6d95eb4963271887/services/ui/ws/window_server.h [modify] https://crrev.com/a6ce960a00e4b8bf5608d51b6d95eb4963271887/third_party/WebKit/Source/platform/graphics/OffscreenCanvasFrameDispatcherImpl.cpp [modify] https://crrev.com/a6ce960a00e4b8bf5608d51b6d95eb4963271887/third_party/WebKit/Source/platform/graphics/OffscreenCanvasFrameDispatcherImpl.h [modify] https://crrev.com/a6ce960a00e4b8bf5608d51b6d95eb4963271887/third_party/WebKit/public/blink_typemaps.gni [modify] https://crrev.com/a6ce960a00e4b8bf5608d51b6d95eb4963271887/ui/aura/mus/window_compositor_frame_sink.cc [modify] https://crrev.com/a6ce960a00e4b8bf5608d51b6d95eb4963271887/ui/aura/mus/window_compositor_frame_sink.h
,
Nov 27 2016
This is not yet complete because all CompositorFrameSinks have the root as their parent. We need to revisit this bug once Mash transitions to Aura.
,
Dec 1 2016
,
Dec 1 2016
,
Dec 9 2016
Issue 589634 has been merged into this issue.
,
Jan 22 2017
,
Jan 26 2017
,
Feb 14 2017
,
Feb 17 2017
The complexity here stems from the SurfaceManager::RegisterBeginFrameSource needing a valid FrameSinkId. This isn't strictly a necessary requirement. All ServerWindows have a unique FrameSinkId, but not all ServerWindows have corresponding CompositorFrames. If we only registered BeginFrame hierarchies based on valid FrameSinkIds from the perspective of SurfaceManager, then we'd have to know a priori which Windows will submit CompositorFrames and WAIT for them to create CompositorFrameSinks which complicates Mus-Gpu restartability somewhat. Speaking to Enne, it sounds like we can simply drop these DCHECKs, they're nice-to-haves but not strictly necessary. This also makes reparenting windows MUCH MUCH easier in the window server as we don't need to worry about having to update BeginFrame hierarchies to correspond to the nearest Surface based ancestor on reparenting. I think RegisterFrameSinkHierarchy and UnregisterFrameSinkHierarchy could move off MojoCompositorFrameSinkPrivate onto DisplayCompositor in this case. This greatly simplifies ServerWindowCompositorFrameSinkManager and perhaps ServerWindowCompositorFrameSinkManager can go away entirely.
,
Feb 23 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/974e4a0d982d7502fe46f0db7621b390b38e1efc commit 974e4a0d982d7502fe46f0db7621b390b38e1efc Author: fsamuel <fsamuel@chromium.org> Date: Thu Feb 23 19:58:21 2017 Remove DCHECKs in SurfaceManager::RegisterFrameSinkHierarchy SurfaceManager::RegisterFrameSinkHierarchy needs a valid parent and child FrameSinkId. In other words, a hierarchy cannot be established until the parent and child have SurfaceFactories. This isn't strictly a necessary requirement. In Mus+Ash, all ServerWindows have a unique FrameSinkId, but not all ServerWindows have corresponding CompositorFrames. If we only registered BeginFrame hierarchies based on valid FrameSinkIds from the perspective of SurfaceManager, then we'd have to know a priori which Windows will submit CompositorFrames and WAIT for them to create CompositorFrameSinks which complicates Mus-Gpu restartability somewhat. Speaking to Enne, it sounds like we can simply drop these DCHECKs, they're nice-to-haves but not strictly necessary. This also makes reparenting windows MUCH MUCH easier in the window server as we don't need to worry about having to update BeginFrame hierarchies to correspond to the nearest Surface based ancestor on reparenting. In a subsequent CL, AddChildFrame and RemoveChildFrameSink will move off MojoCompositorFrameSinkPrivate onto DisplayCompositor. BUG= 653895 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel Review-Url: https://codereview.chromium.org/2714553005 Cr-Commit-Position: refs/heads/master@{#452596} [modify] https://crrev.com/974e4a0d982d7502fe46f0db7621b390b38e1efc/cc/surfaces/surface_manager.cc
,
Feb 24 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/60b01374443cd1e14598ed8fd554109c697a096f commit 60b01374443cd1e14598ed8fd554109c697a096f Author: fsamuel <fsamuel@chromium.org> Date: Fri Feb 24 23:21:50 2017 Move FrameSink hierarchy registration to DisplayCompositor interface Previously, SurfaceManager tied FrameSink hierarchy registration with validity of FrameSinkIds. This expectation has been removed. Thus, we can now register the entire ServerWindow tree with the display compositor. This greatly simplifies window reparenting and addresses correctness issues for reparenting across monitors. BUG= 653895 TBR=jbauman@chromium.org CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel Review-Url: https://codereview.chromium.org/2711913006 Cr-Commit-Position: refs/heads/master@{#452989} [modify] https://crrev.com/60b01374443cd1e14598ed8fd554109c697a096f/cc/ipc/display_compositor.mojom [modify] https://crrev.com/60b01374443cd1e14598ed8fd554109c697a096f/cc/ipc/mojo_compositor_frame_sink.mojom [modify] https://crrev.com/60b01374443cd1e14598ed8fd554109c697a096f/cc/surfaces/compositor_frame_sink_support.cc [modify] https://crrev.com/60b01374443cd1e14598ed8fd554109c697a096f/cc/surfaces/compositor_frame_sink_support.h [modify] https://crrev.com/60b01374443cd1e14598ed8fd554109c697a096f/components/display_compositor/gpu_compositor_frame_sink.cc [modify] https://crrev.com/60b01374443cd1e14598ed8fd554109c697a096f/components/display_compositor/gpu_compositor_frame_sink.h [modify] https://crrev.com/60b01374443cd1e14598ed8fd554109c697a096f/components/display_compositor/gpu_root_compositor_frame_sink.cc [modify] https://crrev.com/60b01374443cd1e14598ed8fd554109c697a096f/components/display_compositor/gpu_root_compositor_frame_sink.h [modify] https://crrev.com/60b01374443cd1e14598ed8fd554109c697a096f/services/ui/surfaces/display_compositor.cc [modify] https://crrev.com/60b01374443cd1e14598ed8fd554109c697a096f/services/ui/surfaces/display_compositor.h [modify] https://crrev.com/60b01374443cd1e14598ed8fd554109c697a096f/services/ui/ws/server_window.cc [modify] https://crrev.com/60b01374443cd1e14598ed8fd554109c697a096f/services/ui/ws/server_window.h [modify] https://crrev.com/60b01374443cd1e14598ed8fd554109c697a096f/services/ui/ws/server_window_compositor_frame_sink_manager.cc [modify] https://crrev.com/60b01374443cd1e14598ed8fd554109c697a096f/services/ui/ws/server_window_compositor_frame_sink_manager.h [modify] https://crrev.com/60b01374443cd1e14598ed8fd554109c697a096f/services/ui/ws/window_server.cc
,
Feb 24 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/60b01374443cd1e14598ed8fd554109c697a096f commit 60b01374443cd1e14598ed8fd554109c697a096f Author: fsamuel <fsamuel@chromium.org> Date: Fri Feb 24 23:21:50 2017 Move FrameSink hierarchy registration to DisplayCompositor interface Previously, SurfaceManager tied FrameSink hierarchy registration with validity of FrameSinkIds. This expectation has been removed. Thus, we can now register the entire ServerWindow tree with the display compositor. This greatly simplifies window reparenting and addresses correctness issues for reparenting across monitors. BUG= 653895 TBR=jbauman@chromium.org CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel Review-Url: https://codereview.chromium.org/2711913006 Cr-Commit-Position: refs/heads/master@{#452989} [modify] https://crrev.com/60b01374443cd1e14598ed8fd554109c697a096f/cc/ipc/display_compositor.mojom [modify] https://crrev.com/60b01374443cd1e14598ed8fd554109c697a096f/cc/ipc/mojo_compositor_frame_sink.mojom [modify] https://crrev.com/60b01374443cd1e14598ed8fd554109c697a096f/cc/surfaces/compositor_frame_sink_support.cc [modify] https://crrev.com/60b01374443cd1e14598ed8fd554109c697a096f/cc/surfaces/compositor_frame_sink_support.h [modify] https://crrev.com/60b01374443cd1e14598ed8fd554109c697a096f/components/display_compositor/gpu_compositor_frame_sink.cc [modify] https://crrev.com/60b01374443cd1e14598ed8fd554109c697a096f/components/display_compositor/gpu_compositor_frame_sink.h [modify] https://crrev.com/60b01374443cd1e14598ed8fd554109c697a096f/components/display_compositor/gpu_root_compositor_frame_sink.cc [modify] https://crrev.com/60b01374443cd1e14598ed8fd554109c697a096f/components/display_compositor/gpu_root_compositor_frame_sink.h [modify] https://crrev.com/60b01374443cd1e14598ed8fd554109c697a096f/services/ui/surfaces/display_compositor.cc [modify] https://crrev.com/60b01374443cd1e14598ed8fd554109c697a096f/services/ui/surfaces/display_compositor.h [modify] https://crrev.com/60b01374443cd1e14598ed8fd554109c697a096f/services/ui/ws/server_window.cc [modify] https://crrev.com/60b01374443cd1e14598ed8fd554109c697a096f/services/ui/ws/server_window.h [modify] https://crrev.com/60b01374443cd1e14598ed8fd554109c697a096f/services/ui/ws/server_window_compositor_frame_sink_manager.cc [modify] https://crrev.com/60b01374443cd1e14598ed8fd554109c697a096f/services/ui/ws/server_window_compositor_frame_sink_manager.h [modify] https://crrev.com/60b01374443cd1e14598ed8fd554109c697a096f/services/ui/ws/window_server.cc
,
Feb 24 2017
With the patch that just landed, I'm fairly confident in our BeginFrame implementation in Mus. If this sticks, I'm ready to mark this bug as FIXED. Yaaay.
,
Feb 27 2017
,
Feb 27 2017
I've seen no evidence of breakage so far so I'm marking this bug as fixed.
,
Jun 13 2017
,
Feb 26 2018
|
|||||||||||||
►
Sign in to add a comment |
|||||||||||||
Comment 1 by sadrul@chromium.org
, Oct 7 2016