Use DwmGetCompositionTimingInfo to get vsync info on Windows

Use DwmGetCompositionTimingInfo to get vsync info on Windows


BUG=


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=150395

[jbates, 2012-07-28 01:35:30]

https://chromiumcodereview.appspot.com/10825053/diff/11003/ui/surface/acceler...
File ui/surface/accelerated_surface_win.cc (right):

https://chromiumcodereview.appspot.com/10825053/diff/11003/ui/surface/acceler...
ui/surface/accelerated_surface_win.cc:846: return;
2 space indent

https://chromiumcodereview.appspot.com/10825053/diff/11003/ui/surface/acceler...
File ui/surface/accelerated_surface_win.h (right):

https://chromiumcodereview.appspot.com/10825053/diff/11003/ui/surface/acceler...
ui/surface/accelerated_surface_win.h:73: void
GetPresentationStats(base::TimeTicks *timebase,
comments + n/d abbrev.

[jbates, 2012-07-30 18:29:21]

LGTM

I added owners reviewers for base/ and ui/:
jar: base/OWNERS
sky: ui/OWNERS

[jbates, 2012-07-30 18:37:51]

https://chromiumcodereview.appspot.com/10825053/diff/11/ui/surface/accelerate...
File ui/surface/accelerated_surface_win.h (right):

https://chromiumcodereview.appspot.com/10825053/diff/11/ui/surface/accelerate...
ui/surface/accelerated_surface_win.h:77: void
GetPresentationStats(base::TimeTicks *timebase,
- comments should explicitly say how to calculate the frame interval and give
example values, because it's actually the opposite of what is expected. eg:
interval_in_seconds = interval_denominator/interval_numerator
example values: denominator = 1001, numerator = 60000.
- pointer style is typically TimeTicks* timebase in chromium.

[jbates, 2012-07-30 19:26:59]

https://chromiumcodereview.appspot.com/10825053/diff/11/ui/surface/accelerate...
File ui/surface/accelerated_surface_win.h (right):

https://chromiumcodereview.appspot.com/10825053/diff/11/ui/surface/accelerate...
ui/surface/accelerated_surface_win.h:77: void
GetPresentationStats(base::TimeTicks *timebase,
On 2012/07/30 18:37:51, jbates wrote:
> - comments should explicitly say how to calculate the frame interval and give
> example values, because it's actually the opposite of what is expected. eg:
> interval_in_seconds = interval_denominator/interval_numerator
> example values: denominator = 1001, numerator = 60000.

Err, except in reverse since you already swap them :)

> - pointer style is typically TimeTicks* timebase in chromium.

[brianderson, 2012-07-30 19:38:13]

https://chromiumcodereview.appspot.com/10825053/diff/11/base/time.h
File base/time.h (right):

https://chromiumcodereview.appspot.com/10825053/diff/11/base/time.h#newcode68
base/time.h:68: static TimeDelta FromQPCValue(LONGLONG qpcValue);
jar, is it ok that we add functions to convert native qpc values to TimeDeltas
and TimeTicks?  We are receiving qpc values from a windows API call and need
this functionality.

https://chromiumcodereview.appspot.com/10825053/diff/11/base/time_win.cc
File base/time_win.cc (right):

https://chromiumcodereview.appspot.com/10825053/diff/11/base/time_win.cc#newc...
base/time_win.cc:417: double ticks_per_microsecond_;  // 0 indicates QPF failed
and we're broken.
jar, is changing this to a double ok?

https://chromiumcodereview.appspot.com/10825053/diff/11/ui/surface/accelerate...
File ui/surface/accelerated_surface_win.h (right):

https://chromiumcodereview.appspot.com/10825053/diff/11/ui/surface/accelerate...
ui/surface/accelerated_surface_win.h:77: void
GetPresentationStats(base::TimeTicks *timebase,
Yeah, I just swapped them in a recent commit.  Will update comments.

[apatrick_chromium, 2012-07-30 19:43:31]

https://chromiumcodereview.appspot.com/10825053/diff/11/base/time.h
File base/time.h (right):

https://chromiumcodereview.appspot.com/10825053/diff/11/base/time.h#newcode68
base/time.h:68: static TimeDelta FromQPCValue(LONGLONG qpcValue);
Google style is qpc_value.

https://chromiumcodereview.appspot.com/10825053/diff/11/base/time.h#newcode516
base/time.h:516: static TimeTicks FromQPCValue(LONGLONG qpcValue);
Same here.

https://chromiumcodereview.appspot.com/10825053/diff/11/base/time_win.cc
File base/time_win.cc (right):

https://chromiumcodereview.appspot.com/10825053/diff/11/base/time_win.cc#newc...
base/time_win.cc:376: static_cast<int64>(qpcValue / ticks_per_microsecond_));
nit: indent 4 spaces.

https://chromiumcodereview.appspot.com/10825053/diff/11/base/time_win.cc#newc...
base/time_win.cc:455: return TimeTicks() +
why not TimeTicks::FromInternalValue()?

https://chromiumcodereview.appspot.com/10825053/diff/11/base/time_win.cc#newc...
base/time_win.cc:456:
HighResNowSingleton::GetInstance()->FromQPCValue(qpcValue);
nit: indent 4 spaces

https://chromiumcodereview.appspot.com/10825053/diff/11/ui/surface/accelerate...
File ui/surface/accelerated_surface_win.cc (right):

https://chromiumcodereview.appspot.com/10825053/diff/11/ui/surface/accelerate...
ui/surface/accelerated_surface_win.cc:842: DWM_TIMING_INFO timingInfo;
timing_info

https://chromiumcodereview.appspot.com/10825053/diff/11/ui/surface/accelerate...
ui/surface/accelerated_surface_win.cc:844: HRESULT gctiResult =
DwmGetCompositionTimingInfo(window_, &timingInfo);
I'd just call this result. otherwise gcti_result.

[brianderson, 2012-07-30 20:16:39]

https://chromiumcodereview.appspot.com/10825053/diff/11/base/time_win.cc
File base/time_win.cc (right):

https://chromiumcodereview.appspot.com/10825053/diff/11/base/time_win.cc#newc...
base/time_win.cc:455: return TimeTicks() +
Copied this trick from TimeTicks::HighResNow() a few lines up.  I could use
FromInternalValue if I changed HighResNowSingleton::FromQPCValue to be
QPCValueToMicroseconds.  That will be more straightforward.  Will change.

https://chromiumcodereview.appspot.com/10825053/diff/11/ui/surface/accelerate...
File ui/surface/accelerated_surface_win.cc (right):

https://chromiumcodereview.appspot.com/10825053/diff/11/ui/surface/accelerate...
ui/surface/accelerated_surface_win.cc:844: HRESULT gctiResult =
DwmGetCompositionTimingInfo(window_, &timingInfo);
Artifact of previous code that had multiple result types.  Fixed.

[jar (doing other things), 2012-07-30 21:31:52]

Please correct nits below before landing... and then...

base code LGTM.

https://chromiumcodereview.appspot.com/10825053/diff/6009/base/time_win.cc
File base/time_win.cc (right):

https://chromiumcodereview.appspot.com/10825053/diff/6009/base/time_win.cc#ne...
base/time_win.cc:376: static_cast<int64>(qpcValue / ticks_per_microsecond_));
nit: only use an indent of 4 beyond the start of the word "return".  

Note for instance, when you wrap a super long line that you can't align with the
paren, you don't add extra indentation beyond the 4 spaces.  Good example is
given in:

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Functi...

https://chromiumcodereview.appspot.com/10825053/diff/6009/base/time_win.cc#ne...
base/time_win.cc:381: : ticks_per_microsecond_(0.0),
nit: colon should be indent 4 characters from the "H"

https://chromiumcodereview.appspot.com/10825053/diff/6009/base/time_win.cc#ne...
base/time_win.cc:456:
HighResNowSingleton::GetInstance()->FromQPCValue(qpcValue);
nit: 4 character indent off start of "return"

[apatrick_chromium, 2012-07-30 21:36:49]

lgtm

[jar (doing other things), 2012-07-30 21:41:33]

https://chromiumcodereview.appspot.com/10825053/diff/11/base/time_win.cc
File base/time_win.cc (right):

https://chromiumcodereview.appspot.com/10825053/diff/11/base/time_win.cc#newc...
base/time_win.cc:417: double ticks_per_microsecond_;  // 0 indicates QPF failed
and we're broken.
On 2012/07/30 19:38:13, Brian Anderson wrote:
> jar, is changing this to a double ok?

<Doh>.... I hadn't noticed this. <woops!>

Can you motivate why you need this level of precision?

I don't see how this would have a positive impact, but will cause more
processing time.

[brianderson, 2012-07-30 22:37:31]

https://chromiumcodereview.appspot.com/10825053/diff/11/base/time_win.cc
File base/time_win.cc (right):

https://chromiumcodereview.appspot.com/10825053/diff/11/base/time_win.cc#newc...
base/time_win.cc:417: double ticks_per_microsecond_;  // 0 indicates QPF failed
and we're broken.
Dividing a 64-bit integer by the 32-bit float could cause problems.  The 64-bit
integer now.QuadPart will be converted to a 32-bit float for the division.  The
precision of the division will be lower for larger magnitudes of now.QuadPart.

For example, suppose ticks_per_microsecond = 1e3 (~=2^10) => 1GHz.  That's
approximately 10 bits of precision in the divisor.  If now.QuadPart is close to
wrapping around, conversion to a 32bit float lops off more than the bottom 32
bits of precision.  That's at least a 2^(32-10) => 2^22 delta between the
magnitude of the LSB of the numerator and the MSB of the denominator.  Which
means we could only detect deltas of 2^22 microseconds (~4 seconds).

Is there anything wrong with my math?

In reality though, the counter probably always starts at 0 and would take many
many years to reach that point.  Are there any platforms that do not start the
counter at 0?

[brianderson, 2012-07-31 20:59:55]

Does anyone have any other input regarding float vs double usage here?  Double
is definitely safer, but may not be necessary if the QPC always starts at 0 and
we don't care about years of continuous uptime.

Do we have any performance critical code that calls TimeTicks::HighResNow()
enough times for float vs double performance to be noticeable?

[nduca, 2012-07-31 21:08:42]

How about making the change as a standalone patch? I think it makes sense but
this way we can easily roll it out if its a problem.

http://www.agner.org/optimize/instruction_tables.pdf

Fdiv has latency 7 on float and 27 on double acording to that. And, remember,
obtaining the friggin' clock takes thousands of non-latent clocks.

That is to say, this is going to be free.

[brianderson, 2012-07-31 21:26:03]

Reverted to using floats.  Will make doubles a separate patch.

[sky, 2012-07-31 23:20:17]

ui.gyp LGTM

[commit-bot: I haz the power, 2012-08-07 18:04:44]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/brianderson@chromium.org/10825053/18001

[commit-bot: I haz the power, 2012-08-07 21:03:39]

Change committed as 150395