cc: Improve frame/commit accounting

Makes CCFrameCounter vsync aware to more accurately account for dropped frames.

Adds the following counters:
vsyncCount: The maximum number of times the compositor could have synced.
implAnimationFrameCount: The number impl-side animation frames.

Uses int64_t instead of size_t or int.
Fixes dropped_percentage calculation.
Adds commit_efficiency calculation.
    
BUG=138641
BUG=138642
BUG=151456

[brianderson, 2012-10-04 01:16:32]

Here are some more metrics to help us measure texture upload performance
improvements.

[dtu, 2012-10-04 01:51:36]

https://codereview.chromium.org/11028021/diff/1/cc/CCRenderingStats.h
File cc/CCRenderingStats.h (right):

https://codereview.chromium.org/11028021/diff/1/cc/CCRenderingStats.h#newcode16
cc/CCRenderingStats.h:16: size_t numAnimationFramesImpl;
Can we use thingCount instead of numThings? Also, why size_t?

[reveman, 2012-10-04 02:15:56]

https://codereview.chromium.org/11028021/diff/1/cc/CCFrameRateController.cpp
File cc/CCFrameRateController.cpp (right):

https://codereview.chromium.org/11028021/diff/1/cc/CCFrameRateController.cpp#...
cc/CCFrameRateController.cpp:216: return;
nit: unnecessary return statement

[brianderson, 2012-10-04 20:11:59]

https://codereview.chromium.org/11028021/diff/1/cc/CCFrameRateController.cpp
File cc/CCFrameRateController.cpp (right):

https://codereview.chromium.org/11028021/diff/1/cc/CCFrameRateController.cpp#...
cc/CCFrameRateController.cpp:216: return;
On 2012/10/04 02:15:56, David Reveman wrote:
> nit: unnecessary return statement

Done.

https://codereview.chromium.org/11028021/diff/1/cc/CCRenderingStats.h
File cc/CCRenderingStats.h (right):

https://codereview.chromium.org/11028021/diff/1/cc/CCRenderingStats.h#newcode16
cc/CCRenderingStats.h:16: size_t numAnimationFramesImpl;
On 2012/10/04 01:51:36, Dave Tu wrote:
> Can we use thingCount instead of numThings?
Sure, will make it consistent.

> Also, why size_t?
Should probably use uint64_t instead.  I used size_t so we could get very high
counts, but size_t is platform dependent and isn't always a 64 bit unsigned int.

[dtu, 2012-10-04 21:41:09]

lgtm with nits

https://chromiumcodereview.appspot.com/11028021/diff/4002/cc/CCFrameRateContr...
File cc/CCFrameRateController.cpp (right):

https://chromiumcodereview.appspot.com/11028021/diff/4002/cc/CCFrameRateContr...
cc/CCFrameRateController.cpp:216: return;
nit: the return is still here.

https://chromiumcodereview.appspot.com/11028021/diff/4002/cc/CCRenderingStats.h
File cc/CCRenderingStats.h (right):

https://chromiumcodereview.appspot.com/11028021/diff/4002/cc/CCRenderingStats...
cc/CCRenderingStats.h:11: uint64_t compositorSyncCount;
nit: Sorry, int64_t is preferred to uint64_t in this scenario.
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Intege...

[nduca, 2012-10-05 00:02:39]

Can you explain csync and what it means a bit? I'm not sure I grok.

[brianderson, 2012-10-05 00:17:32]

I renamed cSync to compositorSync to make it more explicit.  compositorSync is a
vsyncTick in the CCFrameRateController.  I am trying to decouple the concepts of
the hardware vsync vs the compositor tick.

https://chromiumcodereview.appspot.com/11028021/diff/4002/cc/CCFrameRateContr...
File cc/CCFrameRateController.cpp (right):

https://chromiumcodereview.appspot.com/11028021/diff/4002/cc/CCFrameRateContr...
cc/CCFrameRateController.cpp:216: return;
On 2012/10/04 21:41:09, Dave Tu wrote:
> nit: the return is still here.

Doh, will get it in next patch

https://chromiumcodereview.appspot.com/11028021/diff/4002/cc/CCRenderingStats.h
File cc/CCRenderingStats.h (right):

https://chromiumcodereview.appspot.com/11028021/diff/4002/cc/CCRenderingStats...
cc/CCRenderingStats.h:11: uint64_t compositorSyncCount;
On 2012/10/04 21:41:09, Dave Tu wrote:
> nit: Sorry, int64_t is preferred to uint64_t in this scenario.
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Intege...
Ok, will change to int64_t.

[brianderson, 2012-10-17 01:48:18]

Dave, David, can you guys take another look at this patch? I moved the
accounting logic to the CCFrameRateCounter so that we could get the numbers in
single threaded mode as well.

[reveman, 2012-10-17 19:32:30]

https://codereview.chromium.org/11028021/diff/15001/cc/frame_rate_counter.cc
File cc/frame_rate_counter.cc (right):

https://codereview.chromium.org/11028021/diff/15001/cc/frame_rate_counter.cc#...
cc/frame_rate_counter.cc:57: base::TimeTicks(),
base::TimeDelta::FromMicroseconds(base::Time::kMicrosecondsPerSecond / 60));
this assumes that 60FPS is the initial frame rate, right? can you instead pass
timebase and interval to the constructor so we don't have to worry about keeping
this in sync with thread_proxy.cc?

or if you know that setTimebaseAndInterval will always be called before these
values are used you can just initialize them to some unique values and ASSERT if
that condition is not met.

https://codereview.chromium.org/11028021/diff/15001/cc/frame_rate_counter.cc#...
cc/frame_rate_counter.cc:150: stats->vsyncCount += (now - m_intervalChangedTime
+ (m_interval/2)) / m_interval;
this same logic is used in setTimebaseAndInterval. can you move this into
utility function rather than duplicating it.

https://codereview.chromium.org/11028021/diff/15001/cc/frame_rate_counter.cc#...
cc/frame_rate_counter.cc:153: stats->droppedFrameCount = stats->vsyncCount -
(m_currentFrameNumber - 1);
Looks like droppedFrameCount is unnecessary. Should we remove this field and
have our test code compute it instead?

[brianderson, 2012-10-17 22:34:48]

https://codereview.chromium.org/11028021/diff/15001/cc/frame_rate_counter.cc
File cc/frame_rate_counter.cc (right):

https://codereview.chromium.org/11028021/diff/15001/cc/frame_rate_counter.cc#...
cc/frame_rate_counter.cc:57: base::TimeTicks(),
base::TimeDelta::FromMicroseconds(base::Time::kMicrosecondsPerSecond / 60));
On 2012/10/17 19:32:30, David Reveman wrote:
> this assumes that 60FPS is the initial frame rate, right? can you instead pass
> timebase and interval to the constructor so we don't have to worry about
keeping
> this in sync with thread_proxy.cc?
> 
> or if you know that setTimebaseAndInterval will always be called before these
> values are used you can just initialize them to some unique values and ASSERT
if
> that condition is not met.

Done.

https://codereview.chromium.org/11028021/diff/15001/cc/frame_rate_counter.cc#...
cc/frame_rate_counter.cc:150: stats->vsyncCount += (now - m_intervalChangedTime
+ (m_interval/2)) / m_interval;
On 2012/10/17 19:32:30, David Reveman wrote:
> this same logic is used in setTimebaseAndInterval. can you move this into
> utility function rather than duplicating it.

Done.

https://codereview.chromium.org/11028021/diff/15001/cc/frame_rate_counter.cc#...
cc/frame_rate_counter.cc:153: stats->droppedFrameCount = stats->vsyncCount -
(m_currentFrameNumber - 1);
I think it would be nice to keep it as a separate field in case we ever want to
calculate it in a different way.

[reveman, 2012-10-18 00:03:00]

lgtm

[dtu, 2012-10-18 01:46:30]

lgtm

[reveman, 2014-03-24 18:43:02]

can this be closed?