Bug 107942 – [texmap] Implement frames-per-second debug counter

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Bruno Abinader (history only)]

Currently we do have a repaint counter implemented (enabled w/ WEBKIT_SHOW_COMPOSITING_DEBUG_VISUALS=1) implemented on EFL, however this is not sufficient in situations where you need to benchmark w/o adding JS code (ie. requestAnimationFrame() calls) to provide a FPS counter. My plan is to provide this either by reusing the same envvar (set to =2, for instance), or add a new envvar (ie. WEBKIT_SHOW_FPS_COUNTER=1). What do you guys think? I do have a sample patch which I'm going to upload soon as initial approach.

[Dongseong Hwang]

That's a good proposal to view FPS. It is really needed.

However, I'm not sure adding new env is good direction. I want to listen other folks' opinion.
FYI, I did not introduce WEBKIT_SHOW_COMPOSITING_DEBUG_VISUALS. This env had been long time, and I just cleaned up recently.

I wish your implementation would be platform independent and other folks will use easily.

Thank you for your effort!

[Bruno Abinader (history only)]

(In reply to comment #1)
> That's a good proposal to view FPS. It is really needed.
> 
> However, I'm not sure adding new env is good direction. I want to listen other folks' opinion.
> FYI, I did not introduce WEBKIT_SHOW_COMPOSITING_DEBUG_VISUALS. This env had been long time, and I just cleaned up recently.
> 
> I wish your implementation would be platform independent and other folks will use easily.
> 
> Thank you for your effort!

I agree with you on the envvar setup, using a different value for FPS counter (i.e. =2) sounds good for me as well. As for the implementation, as commonly used by JS-based FPS counters, we can increment a frameCounter on every call to requestAnimationFrame() and update the FPS value every 1 or more seconds (we can even set up the refresh interval as parameter to the envvar). I believe this is pretty well supported on all platforms and won't require any platform-specific adjustments. What do you guys think?

[Bruno Abinader (history only)]

Created attachment 186992 [details]
Patch

Proposed patch. I've thought about using WEBKIT_SHOW_COMPOSITING_DEBUG_VISUALS and Settings/Preferences support, however the first didn't quite met the requirements to customize FPS timer interval, and the latter was just too complex to such a simple implementation, keeping it as low-profile as possible.

[Noam Rosenthal]

Comment on attachment 186992 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=186992&action=review

This is not a good way to show FPS :)
Drawing text in every frame into a buffer and then uploading that buffer is very inefficient, and would probably result in reduced FPS.
I think this should be part of WebInspector.

> Source/WebCore/ChangeLog:8
> +        Added TextureMapper' FPS counter via WEBKIT_SHOW_FPS=<interval> envvar,

Spell out envvar -> environment variable

> Source/WebCore/ChangeLog:12
> +        Visual debugging feature, no need to tests.

no need to tests -> no need for new testes

> Source/WebCore/platform/graphics/texmap/TextureMapperLayer.cpp:92
> +    // FIXME: Using drawRepaintCounter() from TextureMapper.

Why is that a FIXME?

> Source/WebCore/platform/graphics/texmap/coordinated/CoordinatedGraphicsScene.cpp:88
> +    if (ok && m_fpsInterval) {

Early return please

> Source/WebCore/platform/graphics/texmap/coordinated/CoordinatedGraphicsScene.cpp:91
> +        if (layer) {

In which case would layer equal false?

[Bruno Abinader (history only)]

Created attachment 187068 [details]
Patch

Updated patch with fixes reviewed by No'am.

[Bruno Abinader (history only)]

(In reply to comment #4)
> (From update of attachment 186992 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=186992&action=review
> 
> This is not a good way to show FPS :)
> Drawing text in every frame into a buffer and then uploading that buffer is very inefficient, and would probably result in reduced FPS.
> I think this should be part of WebInspector.

I agree with you, however this is currently how compositing debug visuals are being shown on screen (see repaint counter [1], debug border [2]). Chromium also shows a FPS counter on screen w/ an associated chart of past seconds measurements, so I followed current approach on this aspect. To optimize drawing, I thought about moving the rendering of this to the backing store, what do you think about this approach?

Links:
[1] http://trac.webkit.org/browser/trunk/Source/WebCore/platform/graphics/texmap/TextureMapperLayer.cpp#L128
[2] http://trac.webkit.org/browser/trunk/Source/WebCore/platform/graphics/texmap/TextureMapperLayer.cpp#L135

[Bruno Abinader (history only)]

Created attachment 187086 [details]
Patch

Updated patch with FPS drawing function now called from CoordinatedGraphicsScene.

[Antonio Gomes]

Comment on attachment 187086 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=187086&action=review

> Source/WebCore/platform/graphics/texmap/coordinated/CoordinatedGraphicsScene.cpp:97
> +    ensureRootLayer();

why is that?

> Source/WebCore/platform/graphics/texmap/coordinated/CoordinatedGraphicsScene.cpp:723
> +    m_lastFPS = (int)(m_paintCount / m_fpsInterval);

no c-cast, please.

[Bruno Abinader (history only)]

Created attachment 187094 [details]
Patch

Removed leftover from previous approach.

[Bruno Abinader (history only)]

Created attachment 187099 [details]
Patch

Using non-C cast as reviewed by Antonio.

[Noam Rosenthal]

Comment on attachment 187099 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=187099&action=review

> Source/WebCore/ChangeLog:12
> +        Added FPS counter via WEBKIT_SHOW_FPS=<interval> environment variable,
> +        where <interval> means the period in seconds (i.e. =1.5) between FPS
> +        updates on screen.
> +
> +        Visual debugging feature, no need for new tests.

Insufficient changelog. Please explain how the FPS counter works.

> Source/WebCore/platform/graphics/texmap/coordinated/CoordinatedGraphicsScene.cpp:48
> +const TransformationMatrix CoordinatedGraphicsScene::s_matrix;
> +const Color CoordinatedGraphicsScene::s_fpsBackgroundColor = Color::black;
> +const FloatPoint CoordinatedGraphicsScene::s_fpsPoint;

I don't see how saving these as static variables help. retrieving Color::black from memory is about as expensive as constructing it from scratch anyway.

> Source/WebCore/platform/graphics/texmap/coordinated/CoordinatedGraphicsScene.cpp:132
> +    m_textureMapper->beginClip(s_matrix, clipRect);
>  
>      if (m_setDrawsBackground) {
>          RGBA32 rgba = makeRGBA32FromFloats(m_backgroundColor.red(),
>              m_backgroundColor.green(), m_backgroundColor.blue(),
>              m_backgroundColor.alpha() * opacity);
> -        m_textureMapper->drawSolidColor(clipRect, TransformationMatrix(), Color(rgba));
> +        m_textureMapper->drawSolidColor(clipRect, s_matrix, Color(rgba));

How is this related to FPS counting?

> Source/WebCore/platform/graphics/texmap/coordinated/CoordinatedGraphicsScene.cpp:143
> +    if (m_fpsTimer.isActive())
> +        m_textureMapper->drawRepaintCounter(m_lastFPS, s_fpsBackgroundColor, s_fpsPoint, matrix);

Conditionals based on a timer's active state are somewhat implicit and hard to debug.

> Source/WebCore/platform/graphics/texmap/coordinated/CoordinatedGraphicsScene.cpp:722
> +void CoordinatedGraphicsScene::fpsTimerFired(SceneTimer*)
> +{
> +    m_lastFPS = int(m_paintCount / m_fpsInterval);
> +    m_paintCount = 0;
> +}

I don't see why you would need a timer here at all.
Every paint you can check the current time vs. the previous time the FPS was shown, and only display the FPS if it has reached the interval.

[Bruno Abinader (history only)]

Created attachment 187636 [details]
Patch

Refactory based on review by No'am.

[Noam Rosenthal]

Comment on attachment 187636 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=187636&action=review

LGTM with nitpicks.

> Source/WebCore/ChangeLog:9
> +        where <interval> means the period in seconds (i.e. =1.5) between FPS

means -> is

> Source/WebCore/ChangeLog:10
> +        updates on screen. It is measured by counting CGScene::paintTo* calls

CGScene -> CoordinatedGraphicsScene

> Source/WebCore/platform/graphics/texmap/coordinated/CoordinatedGraphicsScene.cpp:718
> +    const double delta = WTF::currentTime() - m_fpsTimestamp;

No need for const

[Bruno Abinader (history only)]

Created attachment 187667 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 187667 [details]
Patch for landing

Clearing flags on attachment: 187667

Committed r142524: <http://trac.webkit.org/changeset/142524>