WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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
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
Show Obsolete
(4)
View All
Add attachment
proposed patch, testcase, etc.
Nat Duca
Comment 1
2011-04-08 23:00:56 PDT
Created
attachment 88919
[details]
Patch
Nat Duca
Comment 2
2011-04-08 23:06:19 PDT
Created
attachment 88920
[details]
Patch
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
Created
attachment 89396
[details]
Patch
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.
Top of Page
Format For Printing
XML
Clone This Bug