Summary: | [chromium] Add lowpass filter and graph to fps indicator | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Nat Duca <nduca> | ||||||||||||
Component: | New Bugs | Assignee: | 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
Nat Duca
2011-04-08 23:00:29 PDT
Created attachment 88919 [details]
Patch
Created attachment 88920 [details]
Patch
Created attachment 89101 [details]
Reduce filtering amount a bit.
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. Created attachment 89396 [details]
Patch
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 Created attachment 89601 [details]
Style fixes
Comment on attachment 89601 [details] Style fixes Clearing flags on attachment: 89601 Committed r83944: <http://trac.webkit.org/changeset/83944> All reviewed patches have been landed. Closing bug. |