Bug 91694 – [chromium] Add droppedFrameCount to renderingStats.

• Component: New Bugs
• Status: RESOLVED FIXED

[Dave Tu]

[chromium] Add timeSpentPainting to renderingStats and rename other fields.

[Dave Tu]

Created attachment 153136 [details]
Patch

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[Dave Tu]

Question: the name changes here are incompatible with the Chromium side. How do you reconcile patches like this that require a simultaneous WebKit/Chromium change?

Also fun bit of history; dropped frame count was discussed months ago here: https://bugs.webkit.org/show_bug.cgi?id=73454

[Nat Duca]

Comment on attachment 153136 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=153136&action=review

> Source/Platform/chromium/public/WebRenderingStats.h:33
> +    int screenFrameCount;

As you note, by renaming these, you're going to break chrome when this lands. So, I suggest you put a comment saying // FIXME: when we land crbug.com/137790, rename this to animationFrameCount. Your focus here is on adding the droppedFrameCount in a hurry 'cause we really really desperately need it. We çan fix up naming and the complexities of renaming as that mentioned bug #

> Source/WebCore/platform/graphics/chromium/cc/CCFrameRateCounter.cpp:37
> +const double CCFrameRateCounter::kDroppedFrameTime = 1.0 / 50.0;

So, whats the rationale behind 20ms? Is this what tonyg uses in the perf.py test?

You should file a followup bug that determines this based on the monitor refresh rate. I'm okay with it being fixed for v0. We're adding this value to CCThreadProxy very very soon. If FPSCounter was constructed with a "displayRefreshRate" value and had a mutation method setDisplayRefreshRate, the proxy could set it. But again, dont do it now, do it later. :)

http://code.google.com/p/chromium/issues/detail?id=129674

> Source/WebCore/platform/graphics/chromium/cc/CCFrameRateCounter.cpp:45
> +inline double CCFrameRateCounter::frameInterval(int frameNumber) const

This is elegant. Nicely done.

> Source/WebCore/platform/graphics/chromium/cc/CCFrameRateCounter.cpp:-46
> -    return safeMod(frame, kTimeStampHistorySize);

what if frameNumber == 0?

> Source/WebCore/platform/graphics/chromium/cc/CCFrameRateCounter.cpp:70
> +    if (!isBadFrameInterval(delta) && delta > kDroppedFrameTime)

Why are we checking isBadFrameInteravl()? Those are defined as really short frames, iirw? That wouldnt' count as a dropped frame anyways...

[Dave Tu]

Created attachment 153356 [details]
Patch

[Dave Tu]

Comment on attachment 153136 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=153136&action=review

>> Source/Platform/chromium/public/WebRenderingStats.h:33
>> +    int screenFrameCount;
> 
> As you note, by renaming these, you're going to break chrome when this lands. So, I suggest you put a comment saying // FIXME: when we land crbug.com/137790, rename this to animationFrameCount. Your focus here is on adding the droppedFrameCount in a hurry 'cause we really really desperately need it. We çan fix up naming and the complexities of renaming as that mentioned bug #

Okay.

>> Source/WebCore/platform/graphics/chromium/cc/CCFrameRateCounter.cpp:37
>> +const double CCFrameRateCounter::kDroppedFrameTime = 1.0 / 50.0;
> 
> So, whats the rationale behind 20ms? Is this what tonyg uses in the perf.py test?
> 
> You should file a followup bug that determines this based on the monitor refresh rate. I'm okay with it being fixed for v0. We're adding this value to CCThreadProxy very very soon. If FPSCounter was constructed with a "displayRefreshRate" value and had a mutation method setDisplayRefreshRate, the proxy could set it. But again, dont do it now, do it later. :)
> 
> http://code.google.com/p/chromium/issues/detail?id=129674

There's some noise in the frame times, so it should be slightly higher than 16.67 (which is what is in perf.py). The actual number is arbitrary, though. 1/55? 1/59?

Okay, sounds good; I'll add a FIXME here as well.

>> Source/WebCore/platform/graphics/chromium/cc/CCFrameRateCounter.cpp:45
>> +inline double CCFrameRateCounter::frameInterval(int frameNumber) const
> 
> This is elegant. Nicely done.

Thanks!

>> Source/WebCore/platform/graphics/chromium/cc/CCFrameRateCounter.cpp:-46
>> -    return safeMod(frame, kTimeStampHistorySize);
> 
> what if frameNumber == 0?

safeMod works on -1, see comment in line 39.

>> Source/WebCore/platform/graphics/chromium/cc/CCFrameRateCounter.cpp:70
>> +    if (!isBadFrameInterval(delta) && delta > kDroppedFrameTime)
> 
> Why are we checking isBadFrameInteravl()? Those are defined as really short frames, iirw? That wouldnt' count as a dropped frame anyways...

Both really short frames and really long frames, as in when rendering goes idle. Those idle frames should be ignored.

[Nat Duca]

Comment on attachment 153356 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=153356&action=review

I like this. @enne for review plus nits.

> Source/WebCore/platform/graphics/chromium/cc/CCFrameRateCounter.h:76
> +    // FIXME: After crbug.com/129674 lands, determine this threshold based on monitor refresh rate.

Did you file a bug? Reference the bug rather than the blocking bug.

> Source/WebCore/platform/graphics/chromium/cc/CCRenderingStats.h:31
> +    // FIXME: After landing crbug.com/137790, rename these to animationFrameCount and screenFrameCount.

File a bug and reference the bug, rather than reference the blocking bug.

[Adrienne Walker]

Comment on attachment 153356 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=153356&action=review

> Source/WebCore/platform/graphics/chromium/cc/CCSingleThreadProxy.cpp:32
> +#include "cc/CCFrameRateCounter.h"

It's kind of weird to me that CCSingleThreadProxy has to directly know about the fps counter.  Can you just wrap sourceAnimationFrameNumber and droppedFrameCount in CCLTHI member functions?

[Nat Duca]

Comment on attachment 153356 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=153356&action=review

>> Source/WebCore/platform/graphics/chromium/cc/CCSingleThreadProxy.cpp:32
>> +#include "cc/CCFrameRateCounter.h"
> 
> It's kind of weird to me that CCSingleThreadProxy has to directly know about the fps counter.  Can you just wrap sourceAnimationFrameNumber and droppedFrameCount in CCLTHI member functions?

Good poitn. David, lets add a method on m_layerTreeHostImpl->renderingStats(CCRenderingStats&) and have the proxies just call through to that. Then we can have one place where we fill in the impl-side stats.

[Dave Tu]

Created attachment 153925 [details]
Patch

[Dave Tu]

Comment on attachment 153356 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=153356&action=review

>> Source/WebCore/platform/graphics/chromium/cc/CCFrameRateCounter.h:76
>> +    // FIXME: After crbug.com/129674 lands, determine this threshold based on monitor refresh rate.
> 
> Did you file a bug? Reference the bug rather than the blocking bug.

Done.

>> Source/WebCore/platform/graphics/chromium/cc/CCRenderingStats.h:31
>> +    // FIXME: After landing crbug.com/137790, rename these to animationFrameCount and screenFrameCount.
> 
> File a bug and reference the bug, rather than reference the blocking bug.

Done.

>>> Source/WebCore/platform/graphics/chromium/cc/CCSingleThreadProxy.cpp:32
>>> +#include "cc/CCFrameRateCounter.h"
>> 
>> It's kind of weird to me that CCSingleThreadProxy has to directly know about the fps counter.  Can you just wrap sourceAnimationFrameNumber and droppedFrameCount in CCLTHI member functions?
> 
> Good poitn. David, lets add a method on m_layerTreeHostImpl->renderingStats(CCRenderingStats&) and have the proxies just call through to that. Then we can have one place where we fill in the impl-side stats.

Done.

[Adrienne Walker]

Comment on attachment 153925 [details]
Patch

R=me.  Looks great, thanks.

[WebKit Review Bot]

Comment on attachment 153925 [details]
Patch

Clearing flags on attachment: 153925

Committed r123421: <http://trac.webkit.org/changeset/123421>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.