cc: RetroFrames can cause high latency |
||||||
Issue description
I'm assuming the intention of retro frames is the opposite.
But there are two things about it that I saw it put us into higher-than-necessary latency mode on Android:
When we go idle (BFS::RemoveObserver) it's plausible that we end up with a leftover |begin_retro_frame_args_| since this pattern is plausible:
a) BeginFrame -> no changes -> DEADLINE_MODE_LATE (and nothing changes seems a likely cause of going idle)
b) next BeginFrame:
bool should_defer_begin_frame =
!begin_retro_frame_args_.empty() ||
!begin_retro_frame_task_.IsCancelled() ||
!observing_begin_frame_source_ ||
(state_machine_.begin_impl_frame_state() !=
SchedulerStateMachine::BEGIN_IMPL_FRAME_STATE_IDLE); // we are
DEADLINE_MODE_LATE
if (should_defer_begin_frame) {
begin_retro_frame_args_.push_back(adjusted_args);
c) remove scheduler from BFS
d) become nonidle
e) BeginFrame -> would normally be immediate, but see above, we have to queue these args now too
f) Scheduler::BeginRetroFrame():
base::TimeTicks expiration_time = args.deadline;
if (now <= expiration_time) // that's pretty strict, so we might discard our most recent BF even, and now we have to wait until next vsync
break;
Maybe clearing |begin_retro_frame_args_| BeginImplFrameNotExpectedSoon() is a good idea.
,
Apr 12 2016
I'm making attempt #N at removing retro frames. Here's what I'm proposing: 1. The scheduler rejects begin frames while its deadline is pending. This means that the scheduler's LastUsedBeginFrameArgs will not update. 2. When the scheduler finishes its deadline it calls DidFinishFrame on its BFS. The BFS should send it a missed frame like it does in AddObserver. How the missed frame is calculated and if it's actually sent is up to the BFS implementation e.g. SyntheticBFS calculates it from the time source, other BFS cache the last vsync information that they got, etc. 3. The scheduler does not treat MISSED begin frames in any special way. We discard these right now if the deadline (as specified by the begin frame args) has passed. There's no good reason to do this. I think this works for all cases where retro frames have been useful: 1. late deadlines due to waiting for commit i.e. deadline = frame_time + interval. 2. race between regular deadline and new begin frame - not likely to happen but needs to be guarded against. 3. no deadline - browser commit to active tree mode, in this mode the scheduler waits forever until the ready to draw signal but we also don't want to have an extra frame of latency due to waiting for the next vsync.
,
Apr 12 2016
NOTE: The above proposal does not imply that the a BeginFrameObserver has to provide backpressure via DidFinishFrame, only that it can choose to reject frames in OnBeginFrame (by not updating its LastUsedBeginFrameArgs) and then call DidFinishFrame if it wants a missed frame. A lot of BeginFrameObservers such as those that essentially exist for proxying (e.g. DelegatedFrameHost, something something mus, OOPIF, etc.) will just accept all frames and never call DidFinishFrame.
,
Apr 13 2016
,
Apr 21 2016
,
Apr 22 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/02d0cfecc62ac64f61863f647234c562398e1d5c commit 02d0cfecc62ac64f61863f647234c562398e1d5c Author: sunnyps <sunnyps@chromium.org> Date: Fri Apr 22 19:04:43 2016 cc: Remove unnecessary state from scheduler state machine. BEGIN_IMPL_FRAME_STATE_BEGIN_FRAME_STARTING was used for animation and webview invalidate. This is unnecessary now given that animation has been moved to LTHI::WillBeginImplFrame. BUG= 602485 R=enne@chromium.org CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel Review URL: https://codereview.chromium.org/1904333002 Cr-Commit-Position: refs/heads/master@{#389195} [modify] https://crrev.com/02d0cfecc62ac64f61863f647234c562398e1d5c/cc/scheduler/scheduler.cc [modify] https://crrev.com/02d0cfecc62ac64f61863f647234c562398e1d5c/cc/scheduler/scheduler_state_machine.cc [modify] https://crrev.com/02d0cfecc62ac64f61863f647234c562398e1d5c/cc/scheduler/scheduler_state_machine.h [modify] https://crrev.com/02d0cfecc62ac64f61863f647234c562398e1d5c/cc/scheduler/scheduler_state_machine_unittest.cc
,
Apr 22 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/6414a4526b77dd087581d8a9c1cec50affc66333 commit 6414a4526b77dd087581d8a9c1cec50affc66333 Author: sievers <sievers@chromium.org> Date: Fri Apr 22 21:33:20 2016 Android: Browser-side scheduling latency tweaks - Emit an immediate 'missed' BeginFrame when an BFObserver is added instead of waiting for next vsync - Remove unneeded requestRender() (=SetNeedsAnimate()) call The former helps both the browser compositor and display scheduler (now that it uses the same BeginFrameSource). Regarding the browser compositor: When the omnibox is hiding (or selection handles are showing) we need to browser composite with the correct scroll offset for a given renderer frame. Without this patch we run: ScrollBegin VSync SwapCompositorFrame VSync BrowserComposite Now we do: ScrollBegin VSync SwapCF MissedBF BrowserComposite VSync In the next interval, this works because if nothing changed yet, the scheduler will defer the BeginMainFrame until there is another change in the browser compositor tree (MODE_LATE). Calling SetNeedsAnimate()) from CompositorViewHolder when the top control offset changes is unnecessary, since this is a renderer-driven animation (causes an invalidation in the browser compositor tree), so will already cause the necessary UpdateLayerTreeHost(). Requesting SetNeedsAnimate() causes an extra BeginMainFrame() (SingleThreadProxy+scheduler are not particularly good at tracking these between needs_commit and needs_animate) and puts us into high latency mode otherwise. Fileed bug crbug.com/602489 to make this more robust in SingleThreadProxy and scheduler. TBR=dtrainor@chromium.org BUG= 591725 , 602489 , 602485 , 604007 Review URL: https://codereview.chromium.org/1879833002 Cr-Commit-Position: refs/heads/master@{#389248} [modify] https://crrev.com/6414a4526b77dd087581d8a9c1cec50affc66333/chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorViewHolder.java [modify] https://crrev.com/6414a4526b77dd087581d8a9c1cec50affc66333/content/browser/renderer_host/compositor_impl_android.cc
,
Jul 14 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/bf10654bfdc17a91703deff61ac808adb18dbf97 commit bf10654bfdc17a91703deff61ac808adb18dbf97 Author: sunnyps <sunnyps@chromium.org> Date: Thu Jul 14 00:22:38 2016 cc: Do not use deadline state for scheduler unittests. Scheduler unittests should not rely on the deadline state which is an internal implementation detail of the scheduler. Checking the state from the perspective of the client is better. This is a prerequisite for refactoring the deadline logic in the scheduler and removing retro frames. R=brianderson@chromium.org BUG= 602485 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_blink_rel Review-Url: https://codereview.chromium.org/2131103003 Cr-Commit-Position: refs/heads/master@{#405361} [modify] https://crrev.com/bf10654bfdc17a91703deff61ac808adb18dbf97/cc/scheduler/scheduler.cc [modify] https://crrev.com/bf10654bfdc17a91703deff61ac808adb18dbf97/cc/scheduler/scheduler.h [modify] https://crrev.com/bf10654bfdc17a91703deff61ac808adb18dbf97/cc/scheduler/scheduler_unittest.cc
,
Sep 9 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/559280b26cc5672f5f054e8ac35281e804c14d78 commit 559280b26cc5672f5f054e8ac35281e804c14d78 Author: sunnyps <sunnyps@chromium.org> Date: Fri Sep 09 23:34:34 2016 cc: Remove frame queuing from the scheduler. CC scheduler has a frame queuing mechanism called "retro frames". This has been responsible for a lot of complexity and hard to fix bugs. The original intent for adding retro frames was to allow the scheduler to handle multiple frames in flight but that goal doesn't seem feasible or even desirable any more. This CL removes the retro frames queue and instead makes the Scheduler end the previous frame when it receives a BeginFrame message. One surprising behavior was that SyntheticBFS MISSED frames would be queued as retro frames and this would convert the synchronous begin frame call (inside Scheduler::ProcessScheduledActions) to an asynchronous retro frame PostTask. To work around this the Scheduler keeps track of a single CancelableClosure that's used for MISSED frames. The above behavior was also causing the BeginFramesNotFromClient tests to work even though there was an extra MISSED frame in the queue. This is more elegantly solved in another way by using frame number to advance the task runner instead of just running pending tasks. Lastly SchedulerStateMachine is modified so that it's possible to end the previous frame and still have the same behavior as before in the commit to active tree (browser compositor) mode. R=brianderson@chromium.org,enne@chromium.org,danakj@chromium.org BUG= 602485 , 644992 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel Review-Url: https://codereview.chromium.org/2323063004 Cr-Commit-Position: refs/heads/master@{#417764} [modify] https://crrev.com/559280b26cc5672f5f054e8ac35281e804c14d78/cc/scheduler/scheduler.cc [modify] https://crrev.com/559280b26cc5672f5f054e8ac35281e804c14d78/cc/scheduler/scheduler.h [modify] https://crrev.com/559280b26cc5672f5f054e8ac35281e804c14d78/cc/scheduler/scheduler_state_machine.cc [modify] https://crrev.com/559280b26cc5672f5f054e8ac35281e804c14d78/cc/scheduler/scheduler_state_machine.h [modify] https://crrev.com/559280b26cc5672f5f054e8ac35281e804c14d78/cc/scheduler/scheduler_state_machine_unittest.cc [modify] https://crrev.com/559280b26cc5672f5f054e8ac35281e804c14d78/cc/scheduler/scheduler_unittest.cc [modify] https://crrev.com/559280b26cc5672f5f054e8ac35281e804c14d78/cc/test/scheduler_test_common.h
,
Sep 12 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/95beb47e50065959ee2f5b43cf431431e32e14cd commit 95beb47e50065959ee2f5b43cf431431e32e14cd Author: sammc <sammc@chromium.org> Date: Mon Sep 12 08:35:45 2016 Revert of cc: Remove frame queuing from the scheduler. (patchset #3 id:40001 of https://codereview.chromium.org/2323063004/ ) Reason for revert: Broke ChromeScreenshotGrabberTest.TakeScreenshot on Linux ChromiumOS Tests (dbg)(1): https://build.chromium.org/p/chromium.chromiumos/builders/Linux%20ChromiumOS%20Tests%20%28dbg%29%281%29/builds/18015. Original issue's description: > cc: Remove frame queuing from the scheduler. > > CC scheduler has a frame queuing mechanism called "retro frames". This > has been responsible for a lot of complexity and hard to fix bugs. The > original intent for adding retro frames was to allow the scheduler to > handle multiple frames in flight but that goal doesn't seem feasible or > even desirable any more. This CL removes the retro frames queue and > instead makes the Scheduler end the previous frame when it receives a > BeginFrame message. > > One surprising behavior was that SyntheticBFS MISSED frames would be > queued as retro frames and this would convert the synchronous begin > frame call (inside Scheduler::ProcessScheduledActions) to an > asynchronous retro frame PostTask. To work around this the Scheduler > keeps track of a single CancelableClosure that's used for MISSED frames. > > The above behavior was also causing the BeginFramesNotFromClient tests > to work even though there was an extra MISSED frame in the queue. This > is more elegantly solved in another way by using frame number to advance > the task runner instead of just running pending tasks. > > Lastly SchedulerStateMachine is modified so that it's possible to end > the previous frame and still have the same behavior as before in the > commit to active tree (browser compositor) mode. > > R=brianderson@chromium.org,enne@chromium.org,danakj@chromium.org > BUG= 602485 , 644992 > CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel > > Committed: https://crrev.com/559280b26cc5672f5f054e8ac35281e804c14d78 > Cr-Commit-Position: refs/heads/master@{#417764} TBR=enne@chromium.org,brianderson@chromium.org,danakj@chromium.org,sunnyps@chromium.org # Not skipping CQ checks because original CL landed more than 1 days ago. BUG= 602485 , 644992 Review-Url: https://codereview.chromium.org/2336493002 Cr-Commit-Position: refs/heads/master@{#417895} [modify] https://crrev.com/95beb47e50065959ee2f5b43cf431431e32e14cd/cc/scheduler/scheduler.cc [modify] https://crrev.com/95beb47e50065959ee2f5b43cf431431e32e14cd/cc/scheduler/scheduler.h [modify] https://crrev.com/95beb47e50065959ee2f5b43cf431431e32e14cd/cc/scheduler/scheduler_state_machine.cc [modify] https://crrev.com/95beb47e50065959ee2f5b43cf431431e32e14cd/cc/scheduler/scheduler_state_machine.h [modify] https://crrev.com/95beb47e50065959ee2f5b43cf431431e32e14cd/cc/scheduler/scheduler_state_machine_unittest.cc [modify] https://crrev.com/95beb47e50065959ee2f5b43cf431431e32e14cd/cc/scheduler/scheduler_unittest.cc [modify] https://crrev.com/95beb47e50065959ee2f5b43cf431431e32e14cd/cc/test/scheduler_test_common.h
,
Sep 12 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/19b19d83112b03eccad58c1b8d4c41b9f2d5515c commit 19b19d83112b03eccad58c1b8d4c41b9f2d5515c Author: danakj <danakj@chromium.org> Date: Mon Sep 12 19:55:03 2016 Revert of Disable LayerTreeHostTilesTestPartialInvalidation.{Full,Partial}Raster_SingleThread_OneCopy tests o… (patchset #1 id:1 of https://codereview.chromium.org/2334693002/ ) Reason for revert: I think this was fixed by https://codereview.chromium.org/2336493002 judging from the tryflakes stopping after it. Original issue's description: > Disable LayerTreeHostTilesTestPartialInvalidation.{Full,Partial}Raster_SingleThread_OneCopy tests on Linux ASan > > They're flaky on Linux ASan. TBR=khushalsagar@chromium.org,kjellander@chromium.org NOPRESUBMIT=true NOTREECHECKS=true NOTRY=true BUG= 645898 , 602485 , 644992 Review-Url: https://codereview.chromium.org/2334883002 Cr-Commit-Position: refs/heads/master@{#418011} [modify] https://crrev.com/19b19d83112b03eccad58c1b8d4c41b9f2d5515c/cc/trees/layer_tree_host_pixeltest_tiles.cc
,
Sep 12 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/19b19d83112b03eccad58c1b8d4c41b9f2d5515c commit 19b19d83112b03eccad58c1b8d4c41b9f2d5515c Author: danakj <danakj@chromium.org> Date: Mon Sep 12 19:55:03 2016 Revert of Disable LayerTreeHostTilesTestPartialInvalidation.{Full,Partial}Raster_SingleThread_OneCopy tests o… (patchset #1 id:1 of https://codereview.chromium.org/2334693002/ ) Reason for revert: I think this was fixed by https://codereview.chromium.org/2336493002 judging from the tryflakes stopping after it. Original issue's description: > Disable LayerTreeHostTilesTestPartialInvalidation.{Full,Partial}Raster_SingleThread_OneCopy tests on Linux ASan > > They're flaky on Linux ASan. TBR=khushalsagar@chromium.org,kjellander@chromium.org NOPRESUBMIT=true NOTREECHECKS=true NOTRY=true BUG= 645898 , 602485 , 644992 Review-Url: https://codereview.chromium.org/2334883002 Cr-Commit-Position: refs/heads/master@{#418011} [modify] https://crrev.com/19b19d83112b03eccad58c1b8d4c41b9f2d5515c/cc/trees/layer_tree_host_pixeltest_tiles.cc
,
Sep 12 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/19b19d83112b03eccad58c1b8d4c41b9f2d5515c commit 19b19d83112b03eccad58c1b8d4c41b9f2d5515c Author: danakj <danakj@chromium.org> Date: Mon Sep 12 19:55:03 2016 Revert of Disable LayerTreeHostTilesTestPartialInvalidation.{Full,Partial}Raster_SingleThread_OneCopy tests o… (patchset #1 id:1 of https://codereview.chromium.org/2334693002/ ) Reason for revert: I think this was fixed by https://codereview.chromium.org/2336493002 judging from the tryflakes stopping after it. Original issue's description: > Disable LayerTreeHostTilesTestPartialInvalidation.{Full,Partial}Raster_SingleThread_OneCopy tests on Linux ASan > > They're flaky on Linux ASan. TBR=khushalsagar@chromium.org,kjellander@chromium.org NOPRESUBMIT=true NOTREECHECKS=true NOTRY=true BUG= 645898 , 602485 , 644992 Review-Url: https://codereview.chromium.org/2334883002 Cr-Commit-Position: refs/heads/master@{#418011} [modify] https://crrev.com/19b19d83112b03eccad58c1b8d4c41b9f2d5515c/cc/trees/layer_tree_host_pixeltest_tiles.cc
,
Sep 13 2016
So here's my understanding of the problem: With the retro frame removal, if browser UI raster is spread across multiple frame intervals we start a new impl frame for each begin frame. This means that the frame count keeps increasing and when we draw, we also do an extra prepare tiles that wasn't happening before. This in itself is not interesting but it looks like it pushed the display scheduler's missed frame task out a bit and it receives a normal begin frame before the missed frame task gets to run. This new frame has a later deadline than the previous (missed) frame which means that the display scheduler doesn't draw until much later. It is incorrect for the browser compositor to do a commit before display scheduler gets to draw the previous frame. This is because we lock resources during commit (by submitting an empty compositor frame to the surface) and that causes the copy output request to fail. But there never was any condition in the scheduler that prevents this from happening so maybe we just got lucky? I have a fix that makes the browser cc scheduler wait for swap ack before commit. Also we need to delay the draw callback for the browser (root) surface to happen after the display is done drawing, for other (child) surfaces we want the draw callback to happen before the drawing to reduce latency.
,
Sep 14 2016
Oooh ya it does sounds like we just got lucky :x These fixes sounds important.
,
Sep 28 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/864a70f6f93a87ff374bf2aea2494d4d7d0150d7 commit 864a70f6f93a87ff374bf2aea2494d4d7d0150d7 Author: sunnyps <sunnyps@chromium.org> Date: Tue Sep 27 18:10:26 2016 Reland of cc: Remove frame queuing from the scheduler. (patchset #1 id:1 of https://codereview.chromium.org/2336493002/ ) Reason for revert: Reland after fixing screenshot grabber test and perf issues. Original issue's description: > Revert of cc: Remove frame queuing from the scheduler. (patchset #3 id:40001 of https://codereview.chromium.org/2323063004/ ) > > Reason for revert: > Broke ChromeScreenshotGrabberTest.TakeScreenshot on Linux ChromiumOS Tests (dbg)(1): https://build.chromium.org/p/chromium.chromiumos/builders/Linux%20ChromiumOS%20Tests%20%28dbg%29%281%29/builds/18015. > > Original issue's description: > > cc: Remove frame queuing from the scheduler. > > > > CC scheduler has a frame queuing mechanism called "retro frames". This > > has been responsible for a lot of complexity and hard to fix bugs. The > > original intent for adding retro frames was to allow the scheduler to > > handle multiple frames in flight but that goal doesn't seem feasible or > > even desirable any more. This CL removes the retro frames queue and > > instead makes the Scheduler end the previous frame when it receives a > > BeginFrame message. > > > > One surprising behavior was that SyntheticBFS MISSED frames would be > > queued as retro frames and this would convert the synchronous begin > > frame call (inside Scheduler::ProcessScheduledActions) to an > > asynchronous retro frame PostTask. To work around this the Scheduler > > keeps track of a single CancelableClosure that's used for MISSED frames. > > > > The above behavior was also causing the BeginFramesNotFromClient tests > > to work even though there was an extra MISSED frame in the queue. This > > is more elegantly solved in another way by using frame number to advance > > the task runner instead of just running pending tasks. > > > > Lastly SchedulerStateMachine is modified so that it's possible to end > > the previous frame and still have the same behavior as before in the > > commit to active tree (browser compositor) mode. > > > > R=brianderson@chromium.org,enne@chromium.org,danakj@chromium.org > > BUG= 602485 , 644992 > > CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel > > > > Committed: https://crrev.com/559280b26cc5672f5f054e8ac35281e804c14d78 > > Cr-Commit-Position: refs/heads/master@{#417764} > > TBR=enne@chromium.org,brianderson@chromium.org,danakj@chromium.org,sunnyps@chromium.org > # Not skipping CQ checks because original CL landed more than 1 days ago. > BUG= 602485 , 644992 > > Committed: https://crrev.com/95beb47e50065959ee2f5b43cf431431e32e14cd > Cr-Commit-Position: refs/heads/master@{#417895} TBR=enne@chromium.org,brianderson@chromium.org,danakj@chromium.org,sammc@chromium.org # Not skipping CQ checks because original CL landed more than 1 days ago. BUG= 602485 , 644992 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel Review-Url: https://codereview.chromium.org/2339633003 Cr-Commit-Position: refs/heads/master@{#421268} [modify] https://crrev.com/864a70f6f93a87ff374bf2aea2494d4d7d0150d7/cc/scheduler/scheduler.cc [modify] https://crrev.com/864a70f6f93a87ff374bf2aea2494d4d7d0150d7/cc/scheduler/scheduler.h [modify] https://crrev.com/864a70f6f93a87ff374bf2aea2494d4d7d0150d7/cc/scheduler/scheduler_state_machine.cc [modify] https://crrev.com/864a70f6f93a87ff374bf2aea2494d4d7d0150d7/cc/scheduler/scheduler_state_machine.h [modify] https://crrev.com/864a70f6f93a87ff374bf2aea2494d4d7d0150d7/cc/scheduler/scheduler_state_machine_unittest.cc [modify] https://crrev.com/864a70f6f93a87ff374bf2aea2494d4d7d0150d7/cc/scheduler/scheduler_unittest.cc [modify] https://crrev.com/864a70f6f93a87ff374bf2aea2494d4d7d0150d7/cc/surfaces/direct_compositor_frame_sink.cc [modify] https://crrev.com/864a70f6f93a87ff374bf2aea2494d4d7d0150d7/cc/surfaces/display.cc [modify] https://crrev.com/864a70f6f93a87ff374bf2aea2494d4d7d0150d7/cc/test/scheduler_test_common.h [modify] https://crrev.com/864a70f6f93a87ff374bf2aea2494d4d7d0150d7/cc/test/test_compositor_frame_sink.cc [modify] https://crrev.com/864a70f6f93a87ff374bf2aea2494d4d7d0150d7/cc/test/test_compositor_frame_sink.h [modify] https://crrev.com/864a70f6f93a87ff374bf2aea2494d4d7d0150d7/cc/trees/proxy_impl.cc [modify] https://crrev.com/864a70f6f93a87ff374bf2aea2494d4d7d0150d7/cc/trees/single_thread_proxy.cc
,
Oct 3 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/5a28cd7dd0ac324e02428174ed3333dbe88edfcd commit 5a28cd7dd0ac324e02428174ed3333dbe88edfcd Author: sunnyps <sunnyps@chromium.org> Date: Mon Oct 03 23:28:09 2016 Revert of cc: Remove frame queuing from the scheduler. (patchset #14 id:400001 of https://codereview.chromium.org/2339633003/ ) Reason for revert: Highly likely that this is causing the flakes we are seeing in http://crbug.com/645736 Original issue's description: > Reland of cc: Remove frame queuing from the scheduler. (patchset #1 id:1 of https://codereview.chromium.org/2336493002/ ) > > Reason for revert: > Reland after fixing screenshot grabber test and perf issues. > > Original issue's description: > > Revert of cc: Remove frame queuing from the scheduler. (patchset #3 id:40001 of https://codereview.chromium.org/2323063004/ ) > > > > Reason for revert: > > Broke ChromeScreenshotGrabberTest.TakeScreenshot on Linux ChromiumOS Tests (dbg)(1): https://build.chromium.org/p/chromium.chromiumos/builders/Linux%20ChromiumOS%20Tests%20%28dbg%29%281%29/builds/18015. > > > > Original issue's description: > > > cc: Remove frame queuing from the scheduler. > > > > > > CC scheduler has a frame queuing mechanism called "retro frames". This > > > has been responsible for a lot of complexity and hard to fix bugs. The > > > original intent for adding retro frames was to allow the scheduler to > > > handle multiple frames in flight but that goal doesn't seem feasible or > > > even desirable any more. This CL removes the retro frames queue and > > > instead makes the Scheduler end the previous frame when it receives a > > > BeginFrame message. > > > > > > One surprising behavior was that SyntheticBFS MISSED frames would be > > > queued as retro frames and this would convert the synchronous begin > > > frame call (inside Scheduler::ProcessScheduledActions) to an > > > asynchronous retro frame PostTask. To work around this the Scheduler > > > keeps track of a single CancelableClosure that's used for MISSED frames. > > > > > > The above behavior was also causing the BeginFramesNotFromClient tests > > > to work even though there was an extra MISSED frame in the queue. This > > > is more elegantly solved in another way by using frame number to advance > > > the task runner instead of just running pending tasks. > > > > > > Lastly SchedulerStateMachine is modified so that it's possible to end > > > the previous frame and still have the same behavior as before in the > > > commit to active tree (browser compositor) mode. > > > > > > R=brianderson@chromium.org,enne@chromium.org,danakj@chromium.org > > > BUG= 602485 , 644992 > > > CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel > > > > > > Committed: https://crrev.com/559280b26cc5672f5f054e8ac35281e804c14d78 > > > Cr-Commit-Position: refs/heads/master@{#417764} > > > > TBR=enne@chromium.org,brianderson@chromium.org,danakj@chromium.org,sunnyps@chromium.org > > # Not skipping CQ checks because original CL landed more than 1 days ago. > > BUG= 602485 , 644992 > > > > Committed: https://crrev.com/95beb47e50065959ee2f5b43cf431431e32e14cd > > Cr-Commit-Position: refs/heads/master@{#417895} > > TBR=enne@chromium.org,brianderson@chromium.org,danakj@chromium.org,sammc@chromium.org > # Not skipping CQ checks because original CL landed more than 1 days ago. > BUG= 602485 , 644992 > CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel > > Committed: https://crrev.com/864a70f6f93a87ff374bf2aea2494d4d7d0150d7 > Cr-Commit-Position: refs/heads/master@{#421268} TBR=enne@chromium.org,danakj@chromium.org,brianderson@chromium.org BUG= 602485 , 644992 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel Review-Url: https://codereview.chromium.org/2386183003 Cr-Commit-Position: refs/heads/master@{#422595} [modify] https://crrev.com/5a28cd7dd0ac324e02428174ed3333dbe88edfcd/cc/scheduler/scheduler.cc [modify] https://crrev.com/5a28cd7dd0ac324e02428174ed3333dbe88edfcd/cc/scheduler/scheduler.h [modify] https://crrev.com/5a28cd7dd0ac324e02428174ed3333dbe88edfcd/cc/scheduler/scheduler_state_machine.cc [modify] https://crrev.com/5a28cd7dd0ac324e02428174ed3333dbe88edfcd/cc/scheduler/scheduler_state_machine.h [modify] https://crrev.com/5a28cd7dd0ac324e02428174ed3333dbe88edfcd/cc/scheduler/scheduler_state_machine_unittest.cc [modify] https://crrev.com/5a28cd7dd0ac324e02428174ed3333dbe88edfcd/cc/scheduler/scheduler_unittest.cc [modify] https://crrev.com/5a28cd7dd0ac324e02428174ed3333dbe88edfcd/cc/surfaces/direct_compositor_frame_sink.cc [modify] https://crrev.com/5a28cd7dd0ac324e02428174ed3333dbe88edfcd/cc/test/scheduler_test_common.h [modify] https://crrev.com/5a28cd7dd0ac324e02428174ed3333dbe88edfcd/cc/test/test_compositor_frame_sink.cc [modify] https://crrev.com/5a28cd7dd0ac324e02428174ed3333dbe88edfcd/cc/test/test_compositor_frame_sink.h [modify] https://crrev.com/5a28cd7dd0ac324e02428174ed3333dbe88edfcd/cc/trees/proxy_impl.cc [modify] https://crrev.com/5a28cd7dd0ac324e02428174ed3333dbe88edfcd/cc/trees/single_thread_proxy.cc
,
Oct 8 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/da928f0adc826e2e5e32ba08da7019eb9a0ed912 commit da928f0adc826e2e5e32ba08da7019eb9a0ed912 Author: sunnyps <sunnyps@chromium.org> Date: Sat Oct 08 00:32:53 2016 Reland of cc: Remove frame queuing from the scheduler. Original issue's description: > cc: Remove frame queuing from the scheduler. > > CC scheduler has a frame queuing mechanism called "retro frames". This > has been responsible for a lot of complexity and hard to fix bugs. The > original intent for adding retro frames was to allow the scheduler to > handle multiple frames in flight but that goal doesn't seem feasible or > even desirable any more. This CL removes the retro frames queue and > instead makes the Scheduler end the previous frame when it receives a > BeginFrame message. > > One surprising behavior was that SyntheticBFS MISSED frames would be > queued as retro frames and this would convert the synchronous begin > frame call (inside Scheduler::ProcessScheduledActions) to an > asynchronous retro frame PostTask. To work around this the Scheduler > keeps track of a single CancelableClosure that's used for MISSED frames. > > The above behavior was also causing the BeginFramesNotFromClient tests > to work even though there was an extra MISSED frame in the queue. This > is more elegantly solved in another way by using frame number to advance > the task runner instead of just running pending tasks. > > Lastly SchedulerStateMachine is modified so that it's possible to end > the previous frame and still have the same behavior as before in the > commit to active tree (browser compositor) mode. R=brianderson@chromium.org,danakj@chromium.org,enne@chromium.org BUG= 602485 , 644992 , 651387 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel Review-Url: https://codereview.chromium.org/2392113003 Cr-Commit-Position: refs/heads/master@{#424027} [modify] https://crrev.com/da928f0adc826e2e5e32ba08da7019eb9a0ed912/cc/scheduler/scheduler.cc [modify] https://crrev.com/da928f0adc826e2e5e32ba08da7019eb9a0ed912/cc/scheduler/scheduler.h [modify] https://crrev.com/da928f0adc826e2e5e32ba08da7019eb9a0ed912/cc/scheduler/scheduler_state_machine.cc [modify] https://crrev.com/da928f0adc826e2e5e32ba08da7019eb9a0ed912/cc/scheduler/scheduler_state_machine.h [modify] https://crrev.com/da928f0adc826e2e5e32ba08da7019eb9a0ed912/cc/scheduler/scheduler_state_machine_unittest.cc [modify] https://crrev.com/da928f0adc826e2e5e32ba08da7019eb9a0ed912/cc/scheduler/scheduler_unittest.cc [modify] https://crrev.com/da928f0adc826e2e5e32ba08da7019eb9a0ed912/cc/surfaces/direct_compositor_frame_sink.cc [modify] https://crrev.com/da928f0adc826e2e5e32ba08da7019eb9a0ed912/cc/test/scheduler_test_common.h [modify] https://crrev.com/da928f0adc826e2e5e32ba08da7019eb9a0ed912/cc/test/test_compositor_frame_sink.cc [modify] https://crrev.com/da928f0adc826e2e5e32ba08da7019eb9a0ed912/cc/test/test_compositor_frame_sink.h [modify] https://crrev.com/da928f0adc826e2e5e32ba08da7019eb9a0ed912/cc/trees/proxy_impl.cc [modify] https://crrev.com/da928f0adc826e2e5e32ba08da7019eb9a0ed912/cc/trees/single_thread_proxy.cc
,
Feb 17 2017
Retro frame logic was deleted some time ago but some issues still remain. See issue 678119 . |
||||||
►
Sign in to add a comment |
||||||
Comment 1 by sunn...@chromium.org
, Apr 12 2016