Bug 90116

Summary: [Qt] Repaint counter for accelerated compositing
Product: WebKit Reporter: Noam Rosenthal <noam>
Component: Layout and RenderingAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: gustavo, noam, webkit.review.bot, xan.lopez
Priority: P2 Keywords: Qt
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Fix GTK build none

Description Noam Rosenthal 2012-06-27 17:41:31 PDT
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.
Comment 1 Helder Correia 2012-07-09 22:54:00 PDT
Created attachment 151401 [details]
Patch
Comment 2 Early Warning System Bot 2012-07-09 23:23:44 PDT
Comment on attachment 151401 [details]
Patch

Attachment 151401 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/13189010
Comment 3 Noam Rosenthal 2012-07-10 06:26:03 PDT
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
Comment 4 Helder Correia 2012-07-10 10:42:57 PDT
Created attachment 151483 [details]
Patch
Comment 5 Noam Rosenthal 2012-07-10 10:46:36 PDT
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
Comment 6 Helder Correia 2012-07-10 11:36:47 PDT
Created attachment 151494 [details]
Patch
Comment 7 Noam Rosenthal 2012-07-10 11:43:01 PDT
Comment on attachment 151494 [details]
Patch

LGTM
Comment 8 Gustavo Noronha (kov) 2012-07-10 11:51:09 PDT
Comment on attachment 151494 [details]
Patch

Attachment 151494 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/13207041
Comment 9 Helder Correia 2012-07-10 13:41:10 PDT
Created attachment 151509 [details]
Fix GTK build
Comment 10 WebKit Review Bot 2012-07-10 16:06:31 PDT
Comment on attachment 151509 [details]
Fix GTK build

Clearing flags on attachment: 151509

Committed r122275: <http://trac.webkit.org/changeset/122275>
Comment 11 WebKit Review Bot 2012-07-10 16:06:35 PDT
All reviewed patches have been landed.  Closing bug.