Bug 58186

Summary: [chromium] Add lowpass filter and graph to fps indicator
Product: WebKit Reporter: Nat Duca <nduca>
Component: New BugsAssignee: Nat Duca <nduca>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, fishd, nduca
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Other   
OS: OS X 10.5   
Attachments:
Description Flags
Patch
none
Patch
none
Reduce filtering amount a bit.
none
Patch
none
Style fixes none

Description Nat Duca 2011-04-08 23:00:29 PDT
[chromium] Add lowpass filter and graph to fps indicator
Comment 1 Nat Duca 2011-04-08 23:00:56 PDT
Created attachment 88919 [details]
Patch
Comment 2 Nat Duca 2011-04-08 23:06:19 PDT
Created attachment 88920 [details]
Patch
Comment 3 Nat Duca 2011-04-11 15:28:16 PDT
Created attachment 89101 [details]
Reduce filtering amount a bit.
Comment 4 Darin Fisher (:fishd, Google) 2011-04-12 23:08:33 PDT
Comment on attachment 89101 [details]
Reduce filtering amount a bit.

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

looks good overall

> Source/WebCore/platform/graphics/chromium/cc/CCHeadsUpDisplay.cpp:138
> +    const float attack = 0.1;

"attack" is an interesting choice of variable name.  is that standard
jargon in digital signal processing?  i might have chosen "alpha" (to
match literature) or maybe "sensitivity" (to describe effect).

> Source/WebCore/platform/graphics/chromium/cc/CCHeadsUpDisplay.cpp:141
> +            printf("FFT: %f\n", 1.0 / secForLastFrame);

did you mean to leave this printf here?

> Source/WebCore/platform/graphics/chromium/cc/CCHeadsUpDisplay.cpp:178
> +        // fps graph

this function is getting long.  how about some helper methods?  drawFPS(), drawGraph()?

> Source/WebCore/platform/graphics/chromium/cc/CCHeadsUpDisplay.h:37
> +#define CC_HUD_PRESENT_HISTORY_SIZE 64

webkit style is to avoid macros for declaring constants.  instead, please
use a c++ constant.  you could make it scoped to the CCHeadsUpDisplay class.
Comment 5 Nat Duca 2011-04-13 10:21:16 PDT
Created attachment 89396 [details]
Patch
Comment 6 Darin Fisher (:fishd, Google) 2011-04-14 09:35:11 PDT
Comment on attachment 89396 [details]
Patch

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

> Source/WebCore/platform/graphics/chromium/cc/CCHeadsUpDisplay.cpp:156
> +            m_presentTimeHistoryInSec[(m_currentFrameNumber + kPresentHistorySize - 2) % kPresentHistorySize];

this might read better if this second line were indented so that you get:

  double secForLastFrame = m_presentTimeHistoryInSec[...
                           m_presentTimeHistoryInSec[...

> Source/WebCore/platform/graphics/chromium/cc/CCHeadsUpDisplay.cpp:165
> +

nit: kill the extra new line here

> Source/WebCore/platform/graphics/chromium/cc/CCHeadsUpDisplay.cpp:188
> +    int graphLeft = (int)(textWidth + 3);

nit, sorry... you should use C++ style static_cast instead of C-style casts.

> Source/WebCore/platform/graphics/chromium/cc/CCHeadsUpDisplay.cpp:191
> +    double h = (double)height - 2;

ditto for the cast
Comment 7 Nat Duca 2011-04-14 10:08:01 PDT
Created attachment 89601 [details]
Style fixes
Comment 8 WebKit Commit Bot 2011-04-14 22:13:29 PDT
Comment on attachment 89601 [details]
Style fixes

Clearing flags on attachment: 89601

Committed r83944: <http://trac.webkit.org/changeset/83944>
Comment 9 WebKit Commit Bot 2011-04-14 22:13:35 PDT
All reviewed patches have been landed.  Closing bug.