improvements of the FPS counter in the HUDLayer

this change improves the following points of the FPS counter:
-cleaner design: less colors, text arranged above the graph, deviation number right aligned 
-added transparency: FPS counter does not completely cover the webpage
-less frequent number updates: makes the numbers easier to read
-indicator line at 60fps

comparison images: https://docs.google.com/folder/d/0B8Y78t3tjy1XZk1xdWx6VjN5aFE/edit

Please download the patch and provide a screenshot from your system to test the layout.

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=166041

[egraether, 2012-10-26 00:06:52]

I started working on improving the FPS counter. This is a first version that
provides improved look and usability.

Please have a look at the comparison pictures or test the patch on your system,
if you want to provide some feedback.

https://docs.google.com/folder/d/0B8Y78t3tjy1XZk1xdWx6VjN5aFE/edit

https://codereview.chromium.org/11272042/diff/1/cc/font_atlas.cc
File cc/font_atlas.cc (right):

https://codereview.chromium.org/11272042/diff/1/cc/font_atlas.cc#newcode63
cc/font_atlas.cc:63: IntSize FontAtlas::textSize(const std::string& text)
This new method calculates the screen text size of a text. It is used to right
align the stdDeviation value in the FPSCounter. I just realized that maybe a
drawTextRightAligned method would have been better.

https://codereview.chromium.org/11272042/diff/1/cc/frame_rate_counter.cc
File cc/frame_rate_counter.cc (right):

https://codereview.chromium.org/11272042/diff/1/cc/frame_rate_counter.cc#newc...
cc/frame_rate_counter.cc:84: void
FrameRateCounter::getAverageFPSAndStandardDeviation(double& averageFPS, double&
standardDeviation) const
I refactored this method, because it was hard to read.

https://codereview.chromium.org/11272042/diff/1/cc/frame_rate_counter.cc#newc...
cc/frame_rate_counter.cc:121: if (frameCount)
This is a fix for a divide by zero error. It made the stdDeviation show up as
nan sometimes.

https://codereview.chromium.org/11272042/diff/1/cc/frame_rate_counter.h
File cc/frame_rate_counter.h (right):

https://codereview.chromium.org/11272042/diff/1/cc/frame_rate_counter.h#newco...
cc/frame_rate_counter.h:54: static const int kTimeStampHistorySize = 170;
I increased the number of samples in the graph so the text fits above the graph.

https://codereview.chromium.org/11272042/diff/1/cc/heads_up_display_layer_imp...
File cc/heads_up_display_layer_impl.cc (right):

https://codereview.chromium.org/11272042/diff/1/cc/heads_up_display_layer_imp...
cc/heads_up_display_layer_impl.cc:151: platformLayerTreeTop =
drawFPSCounter(canvas, layerTreeHostImpl()->fpsCounter());
The drawFPSCounter method now returns the height, so that the platformLayerTree
is painted below.

https://codereview.chromium.org/11272042/diff/1/cc/heads_up_display_layer_imp...
cc/heads_up_display_layer_impl.cc:179: paint.setColor(SkColorSetARGB(200, 0, 0,
0));
The background is drawn transparent so that the covered part of the webpage is
partly visible.

https://codereview.chromium.org/11272042/diff/1/cc/heads_up_display_layer_imp...
cc/heads_up_display_layer_impl.cc:194: if
(base::TimeDelta(fpsCounter->timeStampOfRecentFrame(0) -
textUpdateTime).InSecondsF() > 0.25) {
The fps and deviation numbers used to be hard to read, because they were updated
every frame. Now they get updated only 4 times per second.

[jamesr, 2012-10-30 21:28:09]

lgtm

[commit-bot: I haz the power, 2012-10-30 22:55:40]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/egraether@chromium.org/11272042/1

[commit-bot: I haz the power, 2012-10-30 22:55:43]

Failed to apply patch for cc/heads_up_display_layer_impl.h:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file cc/heads_up_display_layer_impl.h
  Hunk #1 FAILED at 7.
  1 out of 2 hunks FAILED -- saving rejects to file
cc/heads_up_display_layer_impl.h.rej

Patch:       cc/heads_up_display_layer_impl.h
Index: cc/heads_up_display_layer_impl.h
diff --git a/cc/heads_up_display_layer_impl.h b/cc/heads_up_display_layer_impl.h
index
28b438368303fd919f08a76de2606d6b61515533..d30eb793dc09ba3ec82295f28e2f1ca45f8d9baf
100644
--- a/cc/heads_up_display_layer_impl.h
+++ b/cc/heads_up_display_layer_impl.h
@@ -7,10 +7,13 @@
 
 #include "CCFontAtlas.h"
 #include "base/memory/scoped_ptr.h"
+#include "base/time.h"
 #include "cc/layer_impl.h"
 #include "cc/scoped_texture.h"
 
 class SkCanvas;
+class SkPaint;
+class SkRect;
 
 namespace cc {
 
@@ -43,13 +46,19 @@ private:
     virtual const char* layerTypeAsString() const OVERRIDE;
 
     void drawHudContents(SkCanvas*);
-    void drawFPSCounter(SkCanvas*, FrameRateCounter*, int top, int height);
-    void drawFPSCounterText(SkCanvas*, FrameRateCounter*, int top, int width,
int height);
+    int drawFPSCounter(SkCanvas*, FrameRateCounter*);
+    void drawFPSCounterText(SkCanvas*, SkPaint&, FrameRateCounter*, SkRect);
+    void drawFPSCounterGraph(SkCanvas*, SkPaint&, FrameRateCounter*, SkRect);
     void drawDebugRects(SkCanvas*, DebugRectHistory*);
 
     scoped_ptr<FontAtlas> m_fontAtlas;
     scoped_ptr<ScopedTexture> m_hudTexture;
     scoped_ptr<SkCanvas> m_hudCanvas;
+
+    double m_averageFPS;
+    double m_stdDeviation;
+
+    base::TimeTicks textUpdateTime;
 };
 
 }  // namespace cc

[jamesr, 2012-10-30 22:59:13]

On 2012/10/30 22:55:43, I haz the power (commit-bot) wrote:
> Failed to apply patch for cc/heads_up_display_layer_impl.h:

This means the patch didn't apply cleanly to ToT, probably because of some of
the renames/refactors we've done.  What you should do is update the patch to
apply then re-upload it and let me know so I can click the commit checkbox
again.

[egraether, 2012-10-31 17:36:42]

While updating I found some bugs in the fps counter graph display when threaded
compositing was active. The graph showed always a zero value at the left,
because of a negative interval. At start the graph showed a really high
framerate, because of 0 intervals from the history initialization not being
ignored.

https://codereview.chromium.org/11272042/diff/8001/cc/frame_rate_counter.cc
File cc/frame_rate_counter.cc (right):

https://codereview.chromium.org/11272042/diff/8001/cc/frame_rate_counter.cc#n...
cc/frame_rate_counter.cc:75: bool intervalTooFast = schedulerAllowsDoubleFrames
? delta < kFrameTooFast : delta <= 0.0;
In threaded compositing the intervalTooFast check was not active, because
double-frames are not possible. This however made negative and zero intervals
possible, which this fix prevents now.

https://codereview.chromium.org/11272042/diff/8001/cc/heads_up_display_layer_...
File cc/heads_up_display_layer_impl.cc (right):

https://codereview.chromium.org/11272042/diff/8001/cc/heads_up_display_layer_...
cc/heads_up_display_layer_impl.cc:234: for (int i = 1; i <
fpsCounter->timeStampHistorySize() - 1; ++i) {
The loop starts now with 1 because the graph drawing happens within
FrameRateCounter::markBeginningOfFrame and FrameRateCounter::markEndOfFrame
calls where a new timestamp has already been set, but the member
m_currentFrameNumber was not incrmented yet. This leads to
FrameRateCounter::timeStampOfRecentFrame(0) returning the most recent instead of
the oldest timestamp. This case is avoided now.

[commit-bot: I haz the power, 2012-11-01 19:22:13]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/egraether@chromium.org/11272042/21001

[commit-bot: I haz the power, 2012-11-01 19:22:17]

Failed to apply patch for cc/heads_up_display_layer_impl.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file cc/heads_up_display_layer_impl.cc
  Hunk #3 FAILED at 159.
  1 out of 3 hunks FAILED -- saving rejects to file
cc/heads_up_display_layer_impl.cc.rej

Patch:       cc/heads_up_display_layer_impl.cc
Index: cc/heads_up_display_layer_impl.cc
diff --git a/cc/heads_up_display_layer_impl.cc
b/cc/heads_up_display_layer_impl.cc
index
381fff569178f1bd01fadcd38180f3e4fd87cad4..11b955f0a4f8edeccef1b4e74945c3c1a7d46e64
100644
--- a/cc/heads_up_display_layer_impl.cc
+++ b/cc/heads_up_display_layer_impl.cc
@@ -43,6 +43,8 @@ static inline SkPaint createPaint()
 
 HeadsUpDisplayLayerImpl::HeadsUpDisplayLayerImpl(int id)
     : LayerImpl(id)
+    , m_averageFPS(0)
+    , m_stdDeviation(0)
 {
 }
 
@@ -143,17 +145,10 @@ void HeadsUpDisplayLayerImpl::drawHudContents(SkCanvas*
canvas)
         canvas->drawRect(SkRect::MakeXYWH(0, 0, bounds().width(),
bounds().height()), paint);
     }
 
-    int fpsCounterHeight = 40;
-    int fpsCounterTop = 2;
-    int platformLayerTreeTop;
+    int platformLayerTreeTop = 0;
 
     if (settings.showFPSCounter)
-        platformLayerTreeTop = fpsCounterTop + fpsCounterHeight;
-    else
-        platformLayerTreeTop = 0;
-
-    if (settings.showFPSCounter)
-        drawFPSCounter(canvas, layerTreeHostImpl()->fpsCounter(),
fpsCounterTop, fpsCounterHeight);
+        platformLayerTreeTop = drawFPSCounter(canvas,
layerTreeHostImpl()->fpsCounter());
 
     if (settings.showPlatformLayerTree && m_fontAtlas.get()) {
         std::string layerTree = layerTreeHostImpl()->layerTreeAsText();
@@ -164,77 +159,108 @@ void HeadsUpDisplayLayerImpl::drawHudContents(SkCanvas*
canvas)
         drawDebugRects(canvas, layerTreeHostImpl()->debugRectHistory());
 }
 
-void HeadsUpDisplayLayerImpl::drawFPSCounter(SkCanvas* canvas,
FrameRateCounter* fpsCounter, int top, int height)
+int HeadsUpDisplayLayerImpl::drawFPSCounter(SkCanvas* canvas, FrameRateCounter*
fpsCounter)
 {
-    float textWidth = 170; // so text fits on linux.
-    float graphWidth = fpsCounter->timeStampHistorySize();
+    const int left = 2;
+    const int top = 2;
 
-    // Draw the FPS text.
-    drawFPSCounterText(canvas, fpsCounter, top, textWidth, height);
+    const int padding = 4;
 
-    // Draw FPS graph.
+    const int fontHeight = m_fontAtlas.get() ? m_fontAtlas->fontHeight() : 0;
+    const int graphWidth = fpsCounter->timeStampHistorySize() - 3;
+    const int graphHeight = 40;
+
+    const int width = graphWidth + 2 * padding;
+    const int height = fontHeight + graphHeight + 4 * padding + 2;
+
+    SkPaint paint = createPaint();
+
+    // Draw background.
+    paint.setColor(SkColorSetARGB(215, 17, 17, 17));
+    canvas->drawRect(SkRect::MakeXYWH(left, top, width, height), paint);
+
+    SkRect textBounds = SkRect::MakeXYWH(left + padding, top + padding,
graphWidth, fontHeight);
+    SkRect graphBounds = SkRect::MakeXYWH(left + padding, textBounds.bottom() +
2 * padding, graphWidth, graphHeight);
+
+    drawFPSCounterText(canvas, paint, fpsCounter, textBounds);
+    drawFPSCounterGraph(canvas, paint, fpsCounter, graphBounds);
+
+    return top + height;
+}
+
+void HeadsUpDisplayLayerImpl::drawFPSCounterText(SkCanvas* canvas, SkPaint&
paint, FrameRateCounter* fpsCounter, SkRect bounds)
+{
+    // Update FPS text - not every frame so text is readable
+    if (base::TimeDelta(fpsCounter->timeStampOfRecentFrame(0) -
textUpdateTime).InSecondsF() > 0.25) {
+        fpsCounter->getAverageFPSAndStandardDeviation(m_averageFPS,
m_stdDeviation);
+        textUpdateTime = fpsCounter->timeStampOfRecentFrame(0);
+    }
+
+    // Draw FPS text.
+    if (m_fontAtlas.get()) {
+        std::string fpsText = base::StringPrintf("FPS:%5.1f", m_averageFPS);
+        std::string deviationText = base::StringPrintf("+/-%4.1f",
m_stdDeviation);
+
+        int deviationWidth = m_fontAtlas->textSize(deviationText).width();
+        IntSize textArea = IntSize(bounds.width(), bounds.height());
+
+        paint.setColor(SK_ColorRED);
+        m_fontAtlas->drawText(canvas, paint, fpsText, gfx::Point(bounds.left(),
bounds.top()), textArea);
+        m_fontAtlas->drawText(canvas, paint, deviationText,
gfx::Point(bounds.right() - deviationWidth, bounds.top()), textArea);
+    }
+}
+
+void HeadsUpDisplayLayerImpl::drawFPSCounterGraph(SkCanvas* canvas, SkPaint&
paint, FrameRateCounter* fpsCounter, SkRect bounds)
+{
     const double loFPS = 0;
     const double hiFPS = 80;
-    SkPaint paint = createPaint();
-    paint.setColor(SkColorSetRGB(154, 205, 50));
-    canvas->drawRect(SkRect::MakeXYWH(2 + textWidth, top, graphWidth, height /
2), paint);
 
-    paint.setColor(SkColorSetRGB(255, 250, 205));
-    canvas->drawRect(SkRect::MakeXYWH(2 + textWidth, top + height / 2,
graphWidth, height / 2), paint);
+    paint.setStyle(SkPaint::kStroke_Style);
+    paint.setStrokeWidth(1);
+
+    // Draw top and bottom line.
+    paint.setColor(SkColorSetRGB(150, 150, 150));
+    canvas->drawLine(bounds.left(), bounds.top() - 1, bounds.right(),
bounds.top() - 1, paint);
+    canvas->drawLine(bounds.left(), bounds.bottom(), bounds.right(),
bounds.bottom(), paint);
 
-    int graphLeft = static_cast<int>(textWidth + 3);
+    // Draw 60fps line.
+    paint.setColor(SkColorSetRGB(100, 100, 100));
+    canvas->drawLine(bounds.left(), bounds.top() + bounds.height() / 4,
bounds.right(), bounds.top() + bounds.height() / 4, paint);
+
+    // Draw FPS graph.
     int x = 0;
-    double h = static_cast<double>(height - 2);
     SkPath path;
-    for (int i = 0; i < fpsCounter->timeStampHistorySize() - 1; ++i) {
-        int j = i + 1;
-        base::TimeDelta delta = fpsCounter->timeStampOfRecentFrame(j) -
fpsCounter->timeStampOfRecentFrame(i);
+
+    for (int i = 1; i < fpsCounter->timeStampHistorySize() - 1; ++i) {
+        base::TimeDelta delta = fpsCounter->timeStampOfRecentFrame(i + 1) -
fpsCounter->timeStampOfRecentFrame(i);
 
         // Skip plotting this particular instantaneous frame rate if it is not
likely to have been valid.
-        if (fpsCounter->isBadFrameInterval(delta)) {
-            x += 1;
-            continue;
+        if (!fpsCounter->isBadFrameInterval(delta)) {
+            double fps = 1.0 / delta.InSecondsF();
+
+            // Clamp the FPS to the range we want to plot visually.
+            double p = 1 - ((fps - loFPS) / (hiFPS - loFPS));
+            if (p < 0)
+                p = 0;
+            if (p > 1)
+                p = 1;
+
+            // Plot this data point.
+            SkPoint cur = SkPoint::Make(bounds.left() + x, bounds.top() + p *
bounds.height());
+            if (path.isEmpty())
+                path.moveTo(cur);
+            else
+                path.lineTo(cur);
         }
 
-        double fps = 1.0 / delta.InSecondsF();
-
-        // Clamp the FPS to the range we want to plot visually.
-        double p = 1 - ((fps - loFPS) / (hiFPS - loFPS));
-        if (p < 0)
-            p = 0;
-        if (p > 1)
-            p = 1;
-
-        // Plot this data point.
-        SkPoint cur = SkPoint::Make(graphLeft + x, 1 + top + p*h);
-        if (path.isEmpty())
-            path.moveTo(cur);
-        else
-            path.lineTo(cur);
         x += 1;
     }
-    paint.setColor(SK_ColorRED);
-    paint.setStyle(SkPaint::kStroke_Style);
-    paint.setStrokeWidth(1);
+
     paint.setAntiAlias(true);
+    paint.setColor(SK_ColorRED);
     canvas->drawPath(path, paint);
 }
 
-void HeadsUpDisplayLayerImpl::drawFPSCounterText(SkCanvas* canvas,
FrameRateCounter* fpsCounter, int top, int width, int height)
-{
-    double averageFPS, stdDeviation;
-    fpsCounter->getAverageFPSAndStandardDeviation(averageFPS, stdDeviation);
-
-    // Draw background.
-    SkPaint paint = createPaint();
-    paint.setColor(SK_ColorBLACK);
-    canvas->drawRect(SkRect::MakeXYWH(2, top, width, height), paint);
-
-    // Draw FPS text.
-    if (m_fontAtlas.get())
-        m_fontAtlas->drawText(canvas, createPaint(), base::StringPrintf("FPS:
%4.1f +/- %3.1f", averageFPS, stdDeviation), gfx::Point(10, height / 3),
IntSize(width, height));
-}
-
 void HeadsUpDisplayLayerImpl::drawDebugRects(SkCanvas* canvas,
DebugRectHistory* debugRectHistory)
 {
     const std::vector<DebugRect>& debugRects = debugRectHistory->debugRects();

[nduca, 2012-11-04 20:18:02]

lgtm

[commit-bot: I haz the power, 2012-11-05 18:50:32]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/egraether@chromium.org/11272042/18009

[commit-bot: I haz the power, 2012-11-05 21:48:16]

Change committed as 166041