Bug 58186 – [chromium] Add lowpass filter and graph to fps indicator

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Nat Duca]

[chromium] Add lowpass filter and graph to fps indicator

[Nat Duca]

Created attachment 88919 [details]
Patch

[Nat Duca]

Created attachment 88920 [details]
Patch

[Nat Duca]

Created attachment 89101 [details]
Reduce filtering amount a bit.

[Darin Fisher (:fishd, Google)]

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.

[Nat Duca]

Created attachment 89396 [details]
Patch

[Darin Fisher (:fishd, Google)]

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

[Nat Duca]

Created attachment 89601 [details]
Style fixes

[WebKit Commit Bot]

Comment on attachment 89601 [details]
Style fixes

Clearing flags on attachment: 89601

Committed r83944: <http://trac.webkit.org/changeset/83944>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.