Input data and BeginFrames need to be synchronized |
|||||||||||||
Issue descriptionAfter I started using MojoCompositorFrameSink in the renderer (crrev.com/2774373002), I noticed that main_thread_scroll_latency regressed as much as %100 in some cases. This is a blocker for my CL and has to be addressed before landing. After some digging, I noticed that we used to always send input data before BeginFrame and now that we send BeginFrames using mojo, the ordering can no longer be guaranteed. See RenderWidgetHostViewAura::OnBeginFrame and notice that host_->FlushInput() is called before host_->Send(BeginFrame(…)). I confirmed that switching the order of these two lines causes regressions similar to what we saw after using mojo (See crrev.com/2804183003). Probably this happens because if BeginFrame arrives first, then we’ll start generating frame that does not reflect the latest input data, basically wasting one frame. Therefore we have to keep the order if I don’t want my CL to get reverted… == Immediate solution == I think I can get away with sending BeginFrames over Chrome IPC and using Mojo for everything else (submitting frames, returning resources, …). == What to do after we have mus == I’ve been thinking about what this means in the mus world. Currently BeginFrames are sent from mus-gpu using MojoCompositorFrameSinkClient and input events are sent from mus-ws and there is no synchronization between them. I was thinking maybe we should also send the BeginFrames from mus-ws so that some synchronization is possible. Instead of mus-gpu sending BeginFrames to all clients, it should only send a BeginFrame to mus-ws and then mus-ws can notify the clients after flushing input data.
,
Apr 8 2017
,
Apr 8 2017
,
Apr 10 2017
,
Apr 12 2017
Adding relevant folks: piman@, enne@, aelias@, tdresser@, dtapuska@
,
Apr 12 2017
This is simulation only. Flush input is only used for synthetic gestures. Could we not generated the input at a different offset instead?
,
Apr 12 2017
What do you mean simulation only? I don't quite follow what FlushInput does...
,
Apr 12 2017
I did also suspect that FlushInput only matters for telemetry but I was not quite sure. Can we just switch to mojo and accept the new numbers as the new norm?
,
Apr 12 2017
fsamuel I think flush input is used for synthetic gestures only and synthetic gestures, which its name kinda suggests, is used for tests.
,
Apr 12 2017
Yes flush input generates an input event according to the frame signal for telemetry. Normally on other hardware the events come in at whatever the hardware sends them as (except Android). So we could schedule this flushinput at whatever time we want. We aligned it to the frame signal ab ok it 6 months ago but before that it was just a 60hz timer.
,
Apr 17 2017
Assigning to samans@ to address. I don't have a good solution in mind but we definitely need to solve this for Mushrome/Mus+Ash.
,
Apr 19 2017
,
May 3 2017
,
May 8 2017
,
May 23 2017
,
May 26 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/f26257b9ee4b7e722cc05468ceecbcf3f920be22 commit f26257b9ee4b7e722cc05468ceecbcf3f920be22 Author: samans <samans@chromium.org> Date: Fri May 26 16:37:45 2017 Send BeginFrames for the renderer using Mojo It used to be the case that synthetic gestures were sent along with the BeginFrame to the renderer. It would make perf tests noisy if we sent BeginFrames using mojo which does not guarantee ordering with legacy IPC. Now after crrev.com/2886263002 BeginFrames and input events are sent independently and it's now safe to switch to mojo. The same perf test that was showing noise/regression before (smoothness.top_25_smooth, mostly mean_main_thread_scroll_latency) is no longer indicating any issues. BUG= 709689 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation Review-Url: https://codereview.chromium.org/2890063002 Cr-Commit-Position: refs/heads/master@{#475030} [modify] https://crrev.com/f26257b9ee4b7e722cc05468ceecbcf3f920be22/content/browser/frame_host/render_widget_host_view_child_frame.cc [modify] https://crrev.com/f26257b9ee4b7e722cc05468ceecbcf3f920be22/content/browser/renderer_host/browser_compositor_view_mac.h [modify] https://crrev.com/f26257b9ee4b7e722cc05468ceecbcf3f920be22/content/browser/renderer_host/browser_compositor_view_mac.mm [modify] https://crrev.com/f26257b9ee4b7e722cc05468ceecbcf3f920be22/content/browser/renderer_host/delegated_frame_host.cc [modify] https://crrev.com/f26257b9ee4b7e722cc05468ceecbcf3f920be22/content/browser/renderer_host/delegated_frame_host.h [modify] https://crrev.com/f26257b9ee4b7e722cc05468ceecbcf3f920be22/content/browser/renderer_host/delegated_frame_host_client_aura.cc [modify] https://crrev.com/f26257b9ee4b7e722cc05468ceecbcf3f920be22/content/browser/renderer_host/delegated_frame_host_client_aura.h [modify] https://crrev.com/f26257b9ee4b7e722cc05468ceecbcf3f920be22/content/browser/renderer_host/render_widget_host_view_android.cc [modify] https://crrev.com/f26257b9ee4b7e722cc05468ceecbcf3f920be22/content/browser/renderer_host/render_widget_host_view_aura.cc [modify] https://crrev.com/f26257b9ee4b7e722cc05468ceecbcf3f920be22/content/browser/renderer_host/render_widget_host_view_aura.h [modify] https://crrev.com/f26257b9ee4b7e722cc05468ceecbcf3f920be22/content/browser/renderer_host/render_widget_host_view_mac.h [modify] https://crrev.com/f26257b9ee4b7e722cc05468ceecbcf3f920be22/content/browser/renderer_host/render_widget_host_view_mac.mm [modify] https://crrev.com/f26257b9ee4b7e722cc05468ceecbcf3f920be22/content/renderer/gpu/renderer_compositor_frame_sink.cc [modify] https://crrev.com/f26257b9ee4b7e722cc05468ceecbcf3f920be22/content/renderer/gpu/renderer_compositor_frame_sink.h
,
Jun 13 2017
Marking as FIXED
,
Jun 13 2017
,
Feb 26 2018
|
|||||||||||||
►
Sign in to add a comment |
|||||||||||||
Comment 1 by samans@chromium.org
, Apr 8 2017