Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(478)

Issue 2897263003: Fix Omnibox.CharTypedToRepaintLatency regression with D3DVsync experiment (Closed)

Created:
3 years, 7 months ago by stanisc
Modified:
3 years, 6 months ago
Reviewers:
sunnyps, jbauman
CC:
chromium-reviews, jam, jbauman+watch_chromium.org, darin-cc_chromium.org, piman+watch_chromium.org, kalyank, cc-bugs_chromium.org, scheduler-bugs_chromium.org, danakj+watch_chromium.org
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

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} Committed: https://chromium.googlesource.com/chromium/src/+/c9365f582c63efd4f812d4dd56fdfed84af29837

Patch Set 1 #

Patch Set 2 : Added unittest file #

Patch Set 3 : Fixed build error on linux/android #

Patch Set 4 : Address the test flakiness #

Total comments: 30

Patch Set 5 : Addressed feedback. #

Patch Set 6 : Addressed feedback #

Unified diffs Side-by-side diffs Delta from patch set Stats (+268 lines, -20 lines) Patch
M cc/scheduler/begin_frame_source.h View 1 2 3 4 1 chunk +4 lines, -0 lines 0 comments Download
M cc/scheduler/begin_frame_source.cc View 1 2 3 4 3 chunks +25 lines, -16 lines 0 comments Download
M content/browser/compositor/gpu_vsync_begin_frame_source.h View 1 2 3 4 5 2 chunks +12 lines, -3 lines 0 comments Download
M content/browser/compositor/gpu_vsync_begin_frame_source.cc View 1 2 3 4 2 chunks +40 lines, -1 line 0 comments Download
A content/browser/compositor/gpu_vsync_begin_frame_source_unittest.cc View 1 2 3 4 1 chunk +185 lines, -0 lines 0 comments Download
M content/test/BUILD.gn View 1 2 2 chunks +2 lines, -0 lines 0 comments Download

Messages

Total messages: 43 (34 generated)
stanisc
sunnyps@chromium.org: Please review cc/scheduler and if possible content/browser/compositor too. jbauman@chromium.org: Please review content/browser/compositor.
3 years, 6 months ago (2017-05-26 22:25:34 UTC) #21
jbauman
lgtm
3 years, 6 months ago (2017-05-27 00:18:44 UTC) #24
stanisc
Sunny, please take a look.
3 years, 6 months ago (2017-05-30 18:18:02 UTC) #25
sunnyps
lgtm % nits https://codereview.chromium.org/2897263003/diff/80001/cc/scheduler/begin_frame_source.cc File cc/scheduler/begin_frame_source.cc (right): https://codereview.chromium.org/2897263003/diff/80001/cc/scheduler/begin_frame_source.cc#newcode288 cc/scheduler/begin_frame_source.cc:288: if (GetMissedBeginFrameArgs(obs, &missed_args)) nit: Can you ...
3 years, 6 months ago (2017-05-30 22:13:17 UTC) #26
stanisc
https://codereview.chromium.org/2897263003/diff/80001/cc/scheduler/begin_frame_source.cc File cc/scheduler/begin_frame_source.cc (right): https://codereview.chromium.org/2897263003/diff/80001/cc/scheduler/begin_frame_source.cc#newcode288 cc/scheduler/begin_frame_source.cc:288: if (GetMissedBeginFrameArgs(obs, &missed_args)) On 2017/05/30 22:13:16, sunnyps wrote: > ...
3 years, 6 months ago (2017-05-31 01:17:53 UTC) #27
sunnyps
https://codereview.chromium.org/2897263003/diff/80001/content/browser/compositor/gpu_vsync_begin_frame_source.h File content/browser/compositor/gpu_vsync_begin_frame_source.h (right): https://codereview.chromium.org/2897263003/diff/80001/content/browser/compositor/gpu_vsync_begin_frame_source.h#newcode25 content/browser/compositor/gpu_vsync_begin_frame_source.h:25: cc::ExternalBeginFrameSourceClient { On 2017/05/31 01:17:53, stanisc wrote: > On ...
3 years, 6 months ago (2017-05-31 01:26:02 UTC) #30
stanisc
https://codereview.chromium.org/2897263003/diff/80001/content/browser/compositor/gpu_vsync_begin_frame_source.h File content/browser/compositor/gpu_vsync_begin_frame_source.h (right): https://codereview.chromium.org/2897263003/diff/80001/content/browser/compositor/gpu_vsync_begin_frame_source.h#newcode25 content/browser/compositor/gpu_vsync_begin_frame_source.h:25: cc::ExternalBeginFrameSourceClient { On 2017/05/31 01:26:02, sunnyps wrote: > On ...
3 years, 6 months ago (2017-05-31 17:33:12 UTC) #33
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2897263003/120001
3 years, 6 months ago (2017-05-31 18:23:07 UTC) #40
commit-bot: I haz the power
3 years, 6 months ago (2017-05-31 21:07:04 UTC) #43
Message was sent while issue was closed.
Committed patchset #6 (id:120001) as
https://chromium.googlesource.com/chromium/src/+/c9365f582c63efd4f812d4dd56fd...

Powered by Google App Engine
This is Rietveld 408576698