Get high resolution frame timebase and interval on compatible systems

Get high resolution frame timebase and interval on compatible systems

Significantly reduces stuttering of animations on Windows caused by rounding errors in the computation of the display refresh rate.
Also made a minor change to how multi-monitor display rates are handled. Now we only use the value from EnumDisplaySettings if DWM compositing is disabled, even if the monitor refresh rate is lower. The previous logic may have erroniously overridden a display refresh of ~60.1Hz or similar with a less accurate 60Hz.

BUG=422000
Committed: https://crrev.com/10b6709e59abc1ef5d3921a38b93acd73a689be8
Cr-Commit-Position: refs/heads/master@{#301230}

[bajones, 2014-10-22 22:43:39]

bajones@chromium.org changed reviewers:
+ brianderson@chromium.org, danakj@chromium.org, kbr@chromium.org,
piman@chromium.org, sievers@chromium.org

[bajones, 2014-10-22 22:43:41]

This doesn't completely solve the problem of occasional jitters that was
introduced by https://codereview.chromium.org/299443020, but it drastically
reduces the majority of it. The rare occasions where you do still get some
jitter it seems to be a single dropped frame now rather than a couple of seconds
of visual jank.

[brianderson, 2014-10-22 23:05:30]

Unlikely, but I wonder if timing_info.qpcRefreshPeriod is noisy because vsync is
disabled. Does it get less noisy is you revert
https://codereview.chromium.org/299443020 ?

https://codereview.chromium.org/668273003/diff/20001/ui/gfx/frame_time.h
File ui/gfx/frame_time.h (left):

https://codereview.chromium.org/668273003/diff/20001/ui/gfx/frame_time.h#oldc...
ui/gfx/frame_time.h:33: // use FrameTime everywhere we need to. See
crbug.com/315334
Note: This comment is a bit missleading. "until we use FrameTime everywhere"
should read "until we use the same timebase everywhere".

IsHighResClockWorking being true means that Now() is equivalent to HighResNow()
now, so we shouldn't have the timebase mismatch issues that we were having
before.

https://codereview.chromium.org/668273003/diff/20001/ui/gl/gl_surface_win.cc
File ui/gl/gl_surface_win.cc (right):

https://codereview.chromium.org/668273003/diff/20001/ui/gl/gl_surface_win.cc#...
ui/gl/gl_surface_win.cc:95:
static_cast<LONGLONG>(timing_info.qpcRefreshPeriod));
+tansell for fyi: Apparently qpcRefreshPeriod is a bit noisy compared to the
Numerator/Denominator calculation below. Would definitely benefit from the vsync
filtering you want to work on.

https://codereview.chromium.org/668273003/diff/20001/ui/gl/gl_surface_win.cc#...
ui/gl/gl_surface_win.cc:98: // Swap the numerator/denominator to convert
frequency to period.
Can you add some comments regarding why you don't use the qpcRefreshPeriod when
we don't have HighRes timestamps.

[bajones, 2014-10-22 23:34:02]

Thanks for the feedback/comments Brian! I've fleshed out the comments in the
code a bit more to explain the logic behind the fallback path better.

[brianderson, 2014-10-23 00:12:29]

lgtm, but I'm not an owner.

[piman, 2014-10-23 03:38:00]

lgtm

[jerryj, 2014-10-24 11:56:20]

Was it intentional to change the behavior of the new monitor code? -- to not use
the new monitor code at all when DWM/Aero is active?  See the second case in
issue 418684 (DWM + 120Hz primary + 60Hz secondary).

Add some comment about the significance of not setting timebase (default zero)
-- that the vsync frequency will be 'closely' matched, but that there will be
period jankiness as a result.

Is "dwm_active" an orphan (set, but never referenced)?

[bajones, 2014-10-24 18:07:30]

On 2014/10/24 at 11:56:20, jerryj wrote:
> Was it intentional to change the behavior of the new monitor code? -- to not
use the new monitor code at all when DWM/Aero is active?  See the second case in
issue 418684 (DWM + 120Hz primary + 60Hz secondary).

This was intentional, yes. I still want to support the scenario you pointed out,
but I'm not convinced that the previous code was the right way to do that after
the rounding issues you pointed out. For now it's safest to disable that
particular edge case.

> Add some comment about the significance of not setting timebase (default zero)
-- that the vsync frequency will be 'closely' matched, but that there will be
period jankiness as a result.

Done.

> Is "dwm_active" an orphan (set, but never referenced)?

Thanks, good catch! I've fixed that in the latest code.

[bajones, 2014-10-24 18:55:13]

bajones@chromium.org changed reviewers:
+ ben@chromium.org, sky@chromium.org

[bajones, 2014-10-24 18:55:14]

the ui/gfx OWNERS file makes it hard to know who needs to review frame_time.h,
so I've added a ben@ and sky@ from one level up. My apologies if there's someone
better suited to the task!

[danakj, 2014-10-24 18:57:41]

rslgtm of frame_time based on brianderson review

[danakj, 2014-10-24 18:57:49]

On 2014/10/24 18:57:41, danakj wrote:
> rslgtm of frame_time based on brianderson review

rs-lgtm.. i mena

[bajones, 2014-10-24 19:53:22]

The CQ bit was checked by bajones@chromium.org

[commit-bot: I haz the power, 2014-10-24 19:54:33]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/668273003/60001

[commit-bot: I haz the power, 2014-10-24 20:01:55]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-10-24 20:01:56]

Try jobs failed on following builders:
  linux_chromium_gn_dbg on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[sky, 2014-10-24 21:26:32]

Rubber stamp LGTM

[bajones, 2014-10-24 22:06:08]

The CQ bit was checked by bajones@chromium.org

[commit-bot: I haz the power, 2014-10-24 22:10:05]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/668273003/80001

[commit-bot: I haz the power, 2014-10-24 22:21:29]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-10-24 22:21:30]

Try jobs failed on following builders:
  android_chromium_gn_compile_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)
  android_dbg_tests_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...)
  linux_chromium_gn_dbg on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[bajones, 2014-10-24 22:23:47]

The CQ bit was checked by bajones@chromium.org

[commit-bot: I haz the power, 2014-10-24 22:25:32]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/668273003/100001

[commit-bot: I haz the power, 2014-10-24 23:48:41]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2014-10-24 23:49:30]

Patchset 6 (id:??) landed as
https://crrev.com/10b6709e59abc1ef5d3921a38b93acd73a689be8
Cr-Commit-Position: refs/heads/master@{#301230}