|
|
Created:
3 years, 7 months ago by stanisc Modified:
3 years, 6 months ago 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. |
DescriptionFix 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 #
Messages
Total messages: 43 (34 generated)
Description was changed from ========== Fix for missed args in GpuVSyncBeginFrameSource BUG= ========== to ========== Fix for missed args in GpuVSyncBeginFrameSource BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
The CQ bit was checked by stanisc@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Patchset #2 (id:20001) has been deleted
The CQ bit was checked by stanisc@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by stanisc@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
The CQ bit was checked by stanisc@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Fix for missed args in GpuVSyncBeginFrameSource BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== 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 ==========
Description was changed from ========== 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 ========== to ========== 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 ==========
Description was changed from ========== 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 ========== to ========== 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 ==========
stanisc@chromium.org changed reviewers: + jbauman@chromium.org, sunnyps@chromium.org
sunnyps@chromium.org: Please review cc/scheduler and if possible content/browser/compositor too. jbauman@chromium.org: Please review content/browser/compositor.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
lgtm
Sunny, please take a look.
lgtm % nits https://codereview.chromium.org/2897263003/diff/80001/cc/scheduler/begin_fram... File cc/scheduler/begin_frame_source.cc (right): https://codereview.chromium.org/2897263003/diff/80001/cc/scheduler/begin_fram... cc/scheduler/begin_frame_source.cc:288: if (GetMissedBeginFrameArgs(obs, &missed_args)) nit: Can you add a DCHECK_EQ(missed_args.type, BeginFrameArgs::MISSED) here? https://codereview.chromium.org/2897263003/diff/80001/cc/scheduler/begin_fram... cc/scheduler/begin_frame_source.cc:298: last_begin_frame_args_ = BeginFrameArgs(); nit: Can you remove this line: last_begin_frame_args_ = BeginFrameArgs() ? This variable used to be called missed_begin_frame_args_ but I changed it a while back to actually be the same as last_begin_frame_args_. I should've removed this line. This is the reason for the special workaround in GpuVsyncBFS::GetMissedBeginFrameArgs. https://codereview.chromium.org/2897263003/diff/80001/cc/scheduler/begin_fram... File cc/scheduler/begin_frame_source.h (right): https://codereview.chromium.org/2897263003/diff/80001/cc/scheduler/begin_fram... cc/scheduler/begin_frame_source.h:281: virtual bool GetMissedBeginFrameArgs(BeginFrameObserver* obs, nit: Can you make this return BeginFrameArgs? The BFS can look at BeginFrameArgs::IsValid and decide if needs to be forwarded to the observer as a missed frame. https://codereview.chromium.org/2897263003/diff/80001/cc/scheduler/begin_fram... cc/scheduler/begin_frame_source.h:284: BeginFrameArgs last_begin_frame_args_; nit: Can you add a method to get last_begin_frame_args_ instead of accessing it directly in the subclass? https://codereview.chromium.org/2897263003/diff/80001/content/browser/composi... File content/browser/compositor/gpu_vsync_begin_frame_source.cc (right): https://codereview.chromium.org/2897263003/diff/80001/content/browser/composi... content/browser/compositor/gpu_vsync_begin_frame_source.cc:52: if (!last_observer_args.IsValid()) nit: Remove this workaround after removing the "last_begin_frame_args_ = BeginFrameArgs" line from ExternalBFS. https://codereview.chromium.org/2897263003/diff/80001/content/browser/composi... File content/browser/compositor/gpu_vsync_begin_frame_source.h (right): https://codereview.chromium.org/2897263003/diff/80001/content/browser/composi... content/browser/compositor/gpu_vsync_begin_frame_source.h:25: cc::ExternalBeginFrameSourceClient { nit: public cc::ExternalBeginFrameSourceClient https://codereview.chromium.org/2897263003/diff/80001/content/browser/composi... File content/browser/compositor/gpu_vsync_begin_frame_source_unittest.cc (right): https://codereview.chromium.org/2897263003/diff/80001/content/browser/composi... content/browser/compositor/gpu_vsync_begin_frame_source_unittest.cc:15: TestBeginFrameObserver() {} nit: Can you = default the ctors/dtors instead? https://codereview.chromium.org/2897263003/diff/80001/content/browser/composi... content/browser/compositor/gpu_vsync_begin_frame_source_unittest.cc:25: void OnBeginFrameSourcePausedChanged(bool paused) override{}; nit: space between override and {} in accordance with the style guide https://codereview.chromium.org/2897263003/diff/80001/content/browser/composi... content/browser/compositor/gpu_vsync_begin_frame_source_unittest.cc:54: void TearDown() override {} nit: Can you remove SetUp and TearDown if you're not using them? https://codereview.chromium.org/2897263003/diff/80001/content/browser/composi... content/browser/compositor/gpu_vsync_begin_frame_source_unittest.cc:110: EXPECT_EQ(frame_time, args.frame_time); nit: Should the BFS also adjust the frame time like it adjusts the deadline? https://codereview.chromium.org/2897263003/diff/80001/content/browser/composi... content/browser/compositor/gpu_vsync_begin_frame_source_unittest.cc:132: begin_frame_source.AddObserver(&observer1); nit: Can you add an EXPECT that the observer doesn't get a missed frame here? https://codereview.chromium.org/2897263003/diff/80001/content/browser/composi... content/browser/compositor/gpu_vsync_begin_frame_source_unittest.cc:137: begin_frame_source.OnVSync(now - base::TimeDelta::FromSeconds(5), interval); nit: Can you add EXPECTs for the observer's begin frame? https://codereview.chromium.org/2897263003/diff/80001/content/browser/composi... content/browser/compositor/gpu_vsync_begin_frame_source_unittest.cc:151: EXPECT_EQ(timestamp1, args1.frame_time); nit: EXPECT for the deadline? https://codereview.chromium.org/2897263003/diff/80001/content/browser/composi... content/browser/compositor/gpu_vsync_begin_frame_source_unittest.cc:163: EXPECT_EQ(timestamp1, args2.frame_time); nit: deadline?
https://codereview.chromium.org/2897263003/diff/80001/cc/scheduler/begin_fram... File cc/scheduler/begin_frame_source.cc (right): https://codereview.chromium.org/2897263003/diff/80001/cc/scheduler/begin_fram... cc/scheduler/begin_frame_source.cc:288: if (GetMissedBeginFrameArgs(obs, &missed_args)) On 2017/05/30 22:13:16, sunnyps wrote: > nit: Can you add a DCHECK_EQ(missed_args.type, BeginFrameArgs::MISSED) here? Done. https://codereview.chromium.org/2897263003/diff/80001/cc/scheduler/begin_fram... cc/scheduler/begin_frame_source.cc:298: last_begin_frame_args_ = BeginFrameArgs(); On 2017/05/30 22:13:17, sunnyps wrote: > nit: Can you remove this line: last_begin_frame_args_ = BeginFrameArgs() ? > > This variable used to be called missed_begin_frame_args_ but I changed it a > while back to actually be the same as last_begin_frame_args_. I should've > removed this line. This is the reason for the special workaround in > GpuVsyncBFS::GetMissedBeginFrameArgs. Done. https://codereview.chromium.org/2897263003/diff/80001/cc/scheduler/begin_fram... File cc/scheduler/begin_frame_source.h (right): https://codereview.chromium.org/2897263003/diff/80001/cc/scheduler/begin_fram... cc/scheduler/begin_frame_source.h:281: virtual bool GetMissedBeginFrameArgs(BeginFrameObserver* obs, On 2017/05/30 22:13:17, sunnyps wrote: > nit: Can you make this return BeginFrameArgs? The BFS can look at > BeginFrameArgs::IsValid and decide if needs to be forwarded to the observer as a > missed frame. Returning BeginFrameArgs would be slightly less optimal because that would require an extra struct copy in some cases. But that probably doesn't matter much considering that this is called infrequently. Done. https://codereview.chromium.org/2897263003/diff/80001/cc/scheduler/begin_fram... cc/scheduler/begin_frame_source.h:284: BeginFrameArgs last_begin_frame_args_; On 2017/05/30 22:13:17, sunnyps wrote: > nit: Can you add a method to get last_begin_frame_args_ instead of accessing it > directly in the subclass? I am reluctant to do this because this member is already protected which means it is intended to be accessed directly in derived classes. And the pattern so far has been to access these protected members directly from derived classes. https://codereview.chromium.org/2897263003/diff/80001/content/browser/composi... File content/browser/compositor/gpu_vsync_begin_frame_source.cc (right): https://codereview.chromium.org/2897263003/diff/80001/content/browser/composi... content/browser/compositor/gpu_vsync_begin_frame_source.cc:52: if (!last_observer_args.IsValid()) On 2017/05/30 22:13:17, sunnyps wrote: > nit: Remove this workaround after removing the "last_begin_frame_args_ = > BeginFrameArgs" line from ExternalBFS. Done. https://codereview.chromium.org/2897263003/diff/80001/content/browser/composi... File content/browser/compositor/gpu_vsync_begin_frame_source.h (right): https://codereview.chromium.org/2897263003/diff/80001/content/browser/composi... content/browser/compositor/gpu_vsync_begin_frame_source.h:25: cc::ExternalBeginFrameSourceClient { On 2017/05/30 22:13:17, sunnyps wrote: > nit: public cc::ExternalBeginFrameSourceClient This inheritance doesn't need to be public because users of GpuVSyncBeginFrameSource don't need to see ExternalBeginFrameSourceClient::OnNeedsBeginFrames or even know that GpuVSyncBeginFrameSource derives from that class. https://codereview.chromium.org/2897263003/diff/80001/content/browser/composi... File content/browser/compositor/gpu_vsync_begin_frame_source_unittest.cc (right): https://codereview.chromium.org/2897263003/diff/80001/content/browser/composi... content/browser/compositor/gpu_vsync_begin_frame_source_unittest.cc:15: TestBeginFrameObserver() {} On 2017/05/30 22:13:17, sunnyps wrote: > nit: Can you = default the ctors/dtors instead? Done. https://codereview.chromium.org/2897263003/diff/80001/content/browser/composi... content/browser/compositor/gpu_vsync_begin_frame_source_unittest.cc:25: void OnBeginFrameSourcePausedChanged(bool paused) override{}; On 2017/05/30 22:13:17, sunnyps wrote: > nit: space between override and {} in accordance with the style guide It is interesting that presubmit complains when this space is here and 'git cl format' removes it. I've added it back anyway. https://codereview.chromium.org/2897263003/diff/80001/content/browser/composi... content/browser/compositor/gpu_vsync_begin_frame_source_unittest.cc:54: void TearDown() override {} On 2017/05/30 22:13:17, sunnyps wrote: > nit: Can you remove SetUp and TearDown if you're not using them? Done. https://codereview.chromium.org/2897263003/diff/80001/content/browser/composi... content/browser/compositor/gpu_vsync_begin_frame_source_unittest.cc:110: EXPECT_EQ(frame_time, args.frame_time); On 2017/05/30 22:13:17, sunnyps wrote: > nit: Should the BFS also adjust the frame time like it adjusts the deadline? frame time is passed to RAF so I think it shouldn't be adjusted. https://codereview.chromium.org/2897263003/diff/80001/content/browser/composi... content/browser/compositor/gpu_vsync_begin_frame_source_unittest.cc:132: begin_frame_source.AddObserver(&observer1); On 2017/05/30 22:13:17, sunnyps wrote: > nit: Can you add an EXPECT that the observer doesn't get a missed frame here? Done. https://codereview.chromium.org/2897263003/diff/80001/content/browser/composi... content/browser/compositor/gpu_vsync_begin_frame_source_unittest.cc:137: begin_frame_source.OnVSync(now - base::TimeDelta::FromSeconds(5), interval); On 2017/05/30 22:13:17, sunnyps wrote: > nit: Can you add EXPECTs for the observer's begin frame? This is covered by BasicTest. I've added a simple check for the number of BeginFrame received and the arg type. https://codereview.chromium.org/2897263003/diff/80001/content/browser/composi... content/browser/compositor/gpu_vsync_begin_frame_source_unittest.cc:151: EXPECT_EQ(timestamp1, args1.frame_time); On 2017/05/30 22:13:17, sunnyps wrote: > nit: EXPECT for the deadline? Done. https://codereview.chromium.org/2897263003/diff/80001/content/browser/composi... content/browser/compositor/gpu_vsync_begin_frame_source_unittest.cc:163: EXPECT_EQ(timestamp1, args2.frame_time); On 2017/05/30 22:13:17, sunnyps wrote: > nit: deadline? Done.
The CQ bit was checked by stanisc@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2897263003/diff/80001/content/browser/composi... File content/browser/compositor/gpu_vsync_begin_frame_source.h (right): https://codereview.chromium.org/2897263003/diff/80001/content/browser/composi... content/browser/compositor/gpu_vsync_begin_frame_source.h:25: cc::ExternalBeginFrameSourceClient { On 2017/05/31 01:17:53, stanisc wrote: > On 2017/05/30 22:13:17, sunnyps wrote: > > nit: public cc::ExternalBeginFrameSourceClient > > This inheritance doesn't need to be public because users of > GpuVSyncBeginFrameSource don't need to see > ExternalBeginFrameSourceClient::OnNeedsBeginFrames or even know that > GpuVSyncBeginFrameSource derives from that class. google style guide prohibits private inheritance: https://google.github.io/styleguide/cppguide.html#Inheritance
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2897263003/diff/80001/content/browser/composi... File content/browser/compositor/gpu_vsync_begin_frame_source.h (right): https://codereview.chromium.org/2897263003/diff/80001/content/browser/composi... content/browser/compositor/gpu_vsync_begin_frame_source.h:25: cc::ExternalBeginFrameSourceClient { On 2017/05/31 01:26:02, sunnyps wrote: > On 2017/05/31 01:17:53, stanisc wrote: > > On 2017/05/30 22:13:17, sunnyps wrote: > > > nit: public cc::ExternalBeginFrameSourceClient > > > > This inheritance doesn't need to be public because users of > > GpuVSyncBeginFrameSource don't need to see > > ExternalBeginFrameSourceClient::OnNeedsBeginFrames or even know that > > GpuVSyncBeginFrameSource derives from that class. > > google style guide prohibits private inheritance: > https://google.github.io/styleguide/cppguide.html#Inheritance Thanks to pointing out to that! Done.
The CQ bit was checked by stanisc@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...) linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...) linux_chromium_headless_rel on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by stanisc@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sunnyps@chromium.org, jbauman@chromium.org Link to the patchset: https://codereview.chromium.org/2897263003/#ps120001 (title: "Addressed feedback")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch. Bot data: {"patchset_id": 120001, "attempt_start_ts": 1496254947090110, "parent_rev": "0a8618b26560b57be00a6d18e0af01aeac89681f", "commit_rev": "c9365f582c63efd4f812d4dd56fdfed84af29837"}
Message was sent while issue was closed.
Description was changed from ========== 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 ========== to ========== 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/+/c9365f582c63efd4f812d4dd56fd... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:120001) as https://chromium.googlesource.com/chromium/src/+/c9365f582c63efd4f812d4dd56fd... |