Bug 90116 – [Qt] Repaint counter for accelerated compositing

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

[Noam Rosenthal]

To debug graphics and performance, we want to enable web developers to see when a composited layer's content has been updated, as that is commonly a cause for choppiness.
The accelerated compositing abstraction already provides the hooks to do that, but we need to implement the Qt/TextureMapper specific parts of it.

[Helder Correia]

Created attachment 151401 [details]
Patch

[Early Warning System Bot]

Comment on attachment 151401 [details]
Patch

Attachment 151401 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/13189010

[Noam Rosenthal]

Comment on attachment 151401 [details]
Patch

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

> Source/WebCore/ChangeLog:17
> +        Note that there is no integration with Preferences. That aproach was
> +        taken initially but revealed complex and overkill for such a
> +        debugging-only functionality. Thus, to disable it simply restart with
> +        the environment variable unset or set to some other value.

Please explain what you've changed in TextureMapper[GL]

> Source/WebKit2/ChangeLog:9
> +

You should note somewhere that this is for WebKit2 only for now.

> Source/WebCore/platform/graphics/texmap/TextureMapperGL.cpp:368
> +    QFont font(QStringLiteral("Monospace"), pointSize, QFont::Bold);

QString::fromUtf8 would work, seems like QStringLiteral doesn't build.

> Source/WebKit2/UIProcess/texmap/LayerBackingStore.cpp:136
> +        static bool showTileDebugVisuals = (qgetenv("QT_WEBKIT_SHOW_COMPOSITING_DEBUG_VISUALS") == "1");
> +        if (!showTileDebugVisuals)
> +            continue;

This should be in its own (static) function

[Helder Correia]

Created attachment 151483 [details]
Patch

[Noam Rosenthal]

Comment on attachment 151483 [details]
Patch

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

Let's put the #if PLATFORM(QT) only when actually needed, this will help other ports (EFL?) do this when they so choose.

> Source/WebCore/platform/graphics/texmap/TextureMapper.h:127
> +#if PLATFORM(QT)
> +    virtual void drawRepaintCounter(int value, int pointSize, const FloatPoint&, const TransformationMatrix& modelViewMatrix = TransformationMatrix()) = 0;
> +#endif

Doesn't need ifdef

> Source/WebCore/platform/graphics/texmap/TextureMapperGL.cpp:364
> +#if PLATFORM(QT)
> +void TextureMapperGL::drawRepaintCounter(int value, int pointSize, const FloatPoint& targetPoint, const TransformationMatrix& modelViewMatrix)

#if should be inside, notImplemented otherwise.

> Source/WebCore/platform/graphics/texmap/TextureMapperGL.h:55
> +#if PLATFORM(QT)
> +    virtual void drawRepaintCounter(int value, int pointSize, const FloatPoint&, const TransformationMatrix& modelViewMatrix = TransformationMatrix()) OVERRIDE;
> +#endif

Doesn't need ifdef

> Source/WebCore/platform/graphics/texmap/TextureMapperImageBuffer.h:57
> +#if PLATFORM(QT)
> +    virtual void drawRepaintCounter(int value, int pointSize, const FloatPoint&, const TransformationMatrix& modelViewMatrix = TransformationMatrix()) OVERRIDE { };
> +#endif

ditto

> Source/WebKit2/UIProcess/texmap/LayerBackingStore.cpp:103
> +#if PLATFORM(QT)
> +static bool shouldShowTileDebugVisuals()
> +{
> +    return (qgetenv("QT_WEBKIT_SHOW_COMPOSITING_DEBUG_VISUALS") == "1");
> +}

You can put the ifdef inside, and return false if not in Qt.

> Source/WebKit2/UIProcess/texmap/LayerBackingStore.cpp:146
> +#if PLATFORM(QT)
> +        static bool shouldDebug = shouldShowTileDebugVisuals();
> +        if (!shouldDebug)
> +            continue;
> +        textureMapper->drawBorder(QColor(Qt::red), 2, tile->rect(), transform);
> +        textureMapper->drawRepaintCounter(static_cast<LayerBackingStoreTile*>(tile)->repaintCount(), 8, tilesToPaint[i]->rect().location(), transform);
> +#endif

Doesn't need the ifdef

> Source/WebKit2/UIProcess/texmap/LayerBackingStore.h:40
> +#if PLATFORM(QT)
> +        , m_repaintCount(0)
> +#endif

Doesn't need the ifdef

> Source/WebKit2/UIProcess/texmap/LayerBackingStore.h:48
> +#if PLATFORM(QT)
> +    inline void incrementRepaintCount() { ++m_repaintCount; }
> +    inline int repaintCount() const { return m_repaintCount; }
> +#endif

Doesn't need the ifdef

> Source/WebKit2/UIProcess/texmap/LayerBackingStore.h:60
> +#if PLATFORM(QT)
> +    int m_repaintCount;
> +#endif

Doesn't need the ifdef

[Helder Correia]

Created attachment 151494 [details]
Patch

[Noam Rosenthal]

Comment on attachment 151494 [details]
Patch

LGTM

[Gustavo Noronha (kov)]

Comment on attachment 151494 [details]
Patch

Attachment 151494 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/13207041

[Helder Correia]

Created attachment 151509 [details]
Fix GTK build

[WebKit Review Bot]

Comment on attachment 151509 [details]
Fix GTK build

Clearing flags on attachment: 151509

Committed r122275: <http://trac.webkit.org/changeset/122275>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.