Omnibox.CharTypedToRepaintLatency regression with D3DVsync experiment |
|||
Issue description
Omnibox.CharTypedToRepaintLatency curve looks worse for D3DVsync experiment vs. the control group. Both curves join at about 16 ms delay. My theory is that this is caused by a difference in implementation of BeginFrameSource (BFS) between D3D v-Sync and the default delay based v-sync implementation.
DelayBased BFS makes sure to generate the proper MISSED BeginFrameArgs in AddObserver method even if it was inactive before:
base::TimeTicks last_or_missed_tick_time =
time_source_->NextTickTime() - time_source_->Interval();
In contrast GpuVsyncDelayBasedSource is derived from External BFS and it simply skips sending a MISSED BeginFrame notification in AddObserver if it was inactive before and doesn't have a previous frame BeginFrameArgs.
Perhaps GpuVsyncDelayBasedSource could try to project the MISSED frame timestamp based on the last known timestamp and interval. That should be more or less in line with what DelayBased BFS does.
,
May 24 2017
,
May 31 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/c9365f582c63efd4f812d4dd56fdfed84af29837 commit c9365f582c63efd4f812d4dd56fdfed84af29837 Author: stanisc <stanisc@chromium.org> Date: Wed May 31 21:06:51 2017 Fix Omnibox.CharTypedToRepaintLatency regression with D3DVsync experiment The regression was caused by a subtle difference in how MISSED BeginFrameArgs is generated between DelayBasedBeginFrameSource and GpuVSyncBeginFrameSource (which is based on external BFS). External BFS would only generate a MISSED args if it already has a previous args (which is the case only when it already has at least one other observer). So in many real cases it would skip issuing a MISSED args. In contrast, DelayBasedBeginFrameSource uses its DelayBasedTimeSource to find the last missed frame time regardless of whether it had another observer before. I've slightly refactored ExternalBeginFrameSource to make it possible to override generation of a MISSED args in a derived class. Now GpuVSyncBeginFrameSource is able to project the last missed frame time based on a previous args even if it was issued some time ago. And because GpuVSyncBeginFrameSource is now more complex I included a unit test to cover all the special logic that it adds on top of ExternalBeginFrameSource. BUG= 723935 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel Review-Url: https://codereview.chromium.org/2897263003 Cr-Commit-Position: refs/heads/master@{#476022} [modify] https://crrev.com/c9365f582c63efd4f812d4dd56fdfed84af29837/cc/scheduler/begin_frame_source.cc [modify] https://crrev.com/c9365f582c63efd4f812d4dd56fdfed84af29837/cc/scheduler/begin_frame_source.h [modify] https://crrev.com/c9365f582c63efd4f812d4dd56fdfed84af29837/content/browser/compositor/gpu_vsync_begin_frame_source.cc [modify] https://crrev.com/c9365f582c63efd4f812d4dd56fdfed84af29837/content/browser/compositor/gpu_vsync_begin_frame_source.h [add] https://crrev.com/c9365f582c63efd4f812d4dd56fdfed84af29837/content/browser/compositor/gpu_vsync_begin_frame_source_unittest.cc [modify] https://crrev.com/c9365f582c63efd4f812d4dd56fdfed84af29837/content/test/BUILD.gn
,
Jun 5 2017
,
Aug 4 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/47e6660d4a2e273a3735582976088c5c34eb752d commit 47e6660d4a2e273a3735582976088c5c34eb752d Author: Stanislav Chiknavaryan <stanisc@chromium.org> Date: Fri Aug 04 21:31:31 2017 Reland "Fix Omnibox.CharTypedToRepaintLatency regression with D3DVsync experiment" This reverts commit d76957c122db1f7cf7d60f5663b6926a6dc34285. The previous fix had to be reverted due to occasionally hitting a DCHECK in ExternalBeginFrameSource::GetMissedBeginFrameArgs which turned out to be unrelated to my change and was addressed here: https://chromium-review.googlesource.com/c/569448/ This change is pretty much the same as the original one except that some of the code has been moved to viz and some merge conflicts had to be resolved. Here is the original description from https://codereview.chromium.org/2897263003: The regression was caused by a subtle difference in how MISSED BeginFrameArgs is generated between DelayBasedBeginFrameSource and GpuVSyncBeginFrameSource (which is based on external BFS). External BFS would only generate a MISSED args if it already has a previous args (which is the case only when it already has at least one other observer). So in many real cases it would skip issuing a MISSED args. In contrast, DelayBasedBeginFrameSource uses its DelayBasedTimeSource to find the last missed frame time regardless of whether it had another observer before. I've slightly refactored ExternalBeginFrameSource to make it possible to override generation of a MISSED args in a derived class. Now GpuVSyncBeginFrameSource is able to project the last missed frame time based on a previous args even if it was issued some time ago. And because GpuVSyncBeginFrameSource is now more complex I included a unit test to cover all the special logic that it adds on top of ExternalBeginFrameSource. BUG= 723935 Change-Id: I2336407b6e51bd74aef6bf4a080eee1be50988c8 Reviewed-on: https://chromium-review.googlesource.com/586358 Commit-Queue: Stanislav Chiknavaryan <stanisc@chromium.org> Reviewed-by: Sunny Sachanandani <sunnyps@chromium.org> Reviewed-by: danakj <danakj@chromium.org> Cr-Commit-Position: refs/heads/master@{#492126} [modify] https://crrev.com/47e6660d4a2e273a3735582976088c5c34eb752d/components/viz/common/frame_sinks/begin_frame_source.cc [modify] https://crrev.com/47e6660d4a2e273a3735582976088c5c34eb752d/components/viz/common/frame_sinks/begin_frame_source.h [modify] https://crrev.com/47e6660d4a2e273a3735582976088c5c34eb752d/components/viz/common/frame_sinks/begin_frame_source_unittest.cc [modify] https://crrev.com/47e6660d4a2e273a3735582976088c5c34eb752d/content/browser/compositor/gpu_vsync_begin_frame_source.cc [modify] https://crrev.com/47e6660d4a2e273a3735582976088c5c34eb752d/content/browser/compositor/gpu_vsync_begin_frame_source.h [add] https://crrev.com/47e6660d4a2e273a3735582976088c5c34eb752d/content/browser/compositor/gpu_vsync_begin_frame_source_unittest.cc [modify] https://crrev.com/47e6660d4a2e273a3735582976088c5c34eb752d/content/test/BUILD.gn |
|||
►
Sign in to add a comment |
|||
Comment 1 by stanisc@chromium.org
, May 24 2017