Bug 58186 - [chromium] Add lowpass filter and graph to fps indicator
Summary: [chromium] Add lowpass filter and graph to fps indicator
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other OS X 10.5
: P2 Normal
Assignee: Nat Duca
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-04-08 23:00 PDT by Nat Duca
Modified: 2011-04-14 22:13 PDT (History)
3 users (show)

See Also:


Attachments
Patch (8.42 KB, patch)
2011-04-08 23:00 PDT, Nat Duca
no flags Details | Formatted Diff | Diff
Patch (7.88 KB, patch)
2011-04-08 23:06 PDT, Nat Duca
no flags Details | Formatted Diff | Diff
Reduce filtering amount a bit. (7.96 KB, patch)
2011-04-11 15:28 PDT, Nat Duca
no flags Details | Formatted Diff | Diff
Patch (8.99 KB, patch)
2011-04-13 10:21 PDT, Nat Duca
no flags Details | Formatted Diff | Diff
Style fixes (9.03 KB, patch)
2011-04-14 10:08 PDT, Nat Duca
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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.