RESOLVED FIXED 58186
[chromium] Add lowpass filter and graph to fps indicator
https://bugs.webkit.org/show_bug.cgi?id=58186
Summary [chromium] Add lowpass filter and graph to fps indicator
Nat Duca
Reported 2011-04-08 23:00:29 PDT
[chromium] Add lowpass filter and graph to fps indicator
Attachments
Patch (8.42 KB, patch)
2011-04-08 23:00 PDT, Nat Duca
no flags
Patch (7.88 KB, patch)
2011-04-08 23:06 PDT, Nat Duca
no flags
Reduce filtering amount a bit. (7.96 KB, patch)
2011-04-11 15:28 PDT, Nat Duca
no flags
Patch (8.99 KB, patch)
2011-04-13 10:21 PDT, Nat Duca
no flags
Style fixes (9.03 KB, patch)
2011-04-14 10:08 PDT, Nat Duca
no flags
Nat Duca
Comment 1 2011-04-08 23:00:56 PDT
Nat Duca
Comment 2 2011-04-08 23:06:19 PDT
Nat Duca
Comment 3 2011-04-11 15:28:16 PDT
Created attachment 89101 [details] Reduce filtering amount a bit.
Darin Fisher (:fishd, Google)
Comment 4 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.
Nat Duca
Comment 5 2011-04-13 10:21:16 PDT
Darin Fisher (:fishd, Google)
Comment 6 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
Nat Duca
Comment 7 2011-04-14 10:08:01 PDT
Created attachment 89601 [details] Style fixes
WebKit Commit Bot
Comment 8 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>
WebKit Commit Bot
Comment 9 2011-04-14 22:13:35 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.