Bug 66771 – [Qt][WK2] Drive tiling from the WebProcess and reuse TiledBackingStore.

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Jocelyn Turcotte]

[Qt][WK2] Drive tiling from the WebProcess and reuse TiledBackingStore.

[Jocelyn Turcotte]

Created attachment 104844 [details]
Patch

[Jocelyn Turcotte]

Comment on attachment 104844 [details]
Patch

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

> Source/WebKit2/UIProcess/API/qt/qtouchwebview.cpp:60
>  void QTouchWebViewPrivate::updateViewportState()
>  {
>      QSize availableSize = q->boundingRect().size().toSize();
> +    if (availableSize.isEmpty())
> +        return;

Please ignore that part, it was just to fix an assert and should be done in a different patch.

[Simon Hausmann]

Comment on attachment 104844 [details]
Patch

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

Overall pretty good stuff. The naming issue is cosmetic, but I think that the m_tilesToNodes map handling is buggy. I'm not 100% sure about the QPainter::end() issue, maybe it's not a real problem.

> Source/WebCore/platform/graphics/TiledBackingStore.cpp:100
> +    if (!m_client->tiledBackingStoreIsUpdatingAllowed() || m_contentsFrozen)

I suggest to call this tiledBackingStoreUpdatesAllowed() or tiledBackingStoreRepaintsAllowed()

> Source/WebCore/platform/graphics/TiledBackingStoreClient.h:34
> +    virtual bool tiledBackingStoreIsUpdatingAllowed() { return true; }

Missing const in the declaration?

> Source/WebKit2/UIProcess/API/qt/qtouchwebpage.cpp:79
> +    if (d->sgAgent.isSwapPending())
> +        // The UI thread is currently locked and calling this from the rendering thread didn't crash
> +        // yet, so I'm assuming that this is OK. Else we have to wait until the SG update is over
> +        // and the UI thread returns to the event loop picking our event before it can be sent to
> +        // the web process. This would increase our chances of missing a frame.
> +        d->page->renderNextFrame();

I suggest to add opening curly braces and closing braces around the block, even though to the compiler it's just a one-line body.

> Source/WebKit2/UIProcess/qt/TiledDrawingAreaProxyQt.cpp:79
> +    int nodeID = m_tilesToNodes.get(tileID);

Shouldn't this be m_tilesToNodes.take(tileId) instead of get?

> Source/WebKit2/WebProcess/WebPage/TiledBackingStoreRemoteTile.cpp:87
> +    m_tiledBackingStore->client()->tiledBackingStorePaint(m_localBuffer->context(), m_tiledBackingStore->mapToContents(m_dirtyRect));
> +
> +    RefPtr<ShareableBitmap> bitmap = ShareableBitmap::createShareable(m_rect.size(), ShareableBitmap::SupportsAlpha);
> +    OwnPtr<GraphicsContext> graphicsContext(bitmap->createGraphicsContext());
> +    graphicsContext->drawImageBuffer(m_localBuffer.get(), ColorSpaceDeviceRGB, IntPoint(0, 0));

Between the tiledBackingStorePaint() and the graphicsContext->drawImageBuffer, shouldn't we make sure that the QPainter behind m_localBuffer->context() is end()'ed properly? I might be missing something here :)

[Kenneth Rohde Christiansen]

(In reply to comment #3)
> > Source/WebCore/platform/graphics/TiledBackingStore.cpp:100
> > +    if (!m_client->tiledBackingStoreIsUpdatingAllowed() || m_contentsFrozen)
> 
> I suggest to call this tiledBackingStoreUpdatesAllowed() or tiledBackingStoreRepaintsAllowed()

It has to read as a sentence if you add if in front; that is the general WebKit rule. So yes, the new name is definitely better, but I wonder whether we should add an "Are" in there, but then again we don't do that for "Enabled" methods, so...

> > Source/WebKit2/UIProcess/API/qt/qtouchwebpage.cpp:79
> > +    if (d->sgAgent.isSwapPending())
> > +        // The UI thread is currently locked and calling this from the rendering thread didn't crash
> > +        // yet, so I'm assuming that this is OK. Else we have to wait until the SG update is over
> > +        // and the UI thread returns to the event loop picking our event before it can be sent to
> > +        // the web process. This would increase our chances of missing a frame.
> > +        d->page->renderNextFrame();
> 
> I suggest to add opening curly braces and closing braces around the block, even though to the compiler it's just a one-line body.

This is actually in the style guide.

[Jocelyn Turcotte]

Created attachment 105006 [details]
Patch

- Removed the part I asked to ignore.
- Fixed raised issues, checked for the painter end() issue, it should be alright for Qt and added a comment to clarify how this might be a problem for other platforms.
- Clarified the ChangeLog and comments here and there.

[Kenneth Rohde Christiansen]

Comment on attachment 105006 [details]
Patch

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

I am not capable of reviewing this (yet) but here is a few mini comment while looking at it

> Source/WebCore/ChangeLog:20
> +        - TiledBackingStoreRemoteTile forwards tile creations, update and removals to the proxy.

RemoteTile? Any reasons why this is not called TileProxy or so?

> Source/WebCore/platform/graphics/TiledBackingStore.cpp:206
>  
> +float TiledBackingStore::coverageRatio(const WebCore::IntRect& contentsRect)

Maybe it would be good with a short comment showing what the purpose are of these methods.

> Source/WebKit2/UIProcess/TiledDrawingAreaProxy.cpp:69
> +    // FIXME: Since the visibleContentRect size depends on the scale, they should get to the web process in
> +    //        one single message to avoid unecessary tile rendering because of a possibly very
> +    //        oversized calculated visible rect.

Good idea

> Source/WebKit2/UIProcess/TiledDrawingAreaProxy.cpp:75
> +    page()->process()->send(Messages::DrawingArea::RenderNextFrame(), page()->pageID());

Frame is a pretty overloaded word :-( specially in web/graphics technology

> Source/WebKit2/UIProcess/TiledDrawingAreaProxy.h:105
> +    HashMap<int, int> m_tilesToNodes;

I think it is more common in WebKit to call it someting like tileNodeMap. But maybe just adding Map to the end would make this more clear

> Source/WebKit2/UIProcess/qt/SGAgent.cpp:48
> +struct NodeUpdateSetBackBuffer : public NodeUpdate {

When we do a code camp next time, someone will have to explain to me how this scene graph works :-)

> Source/WebKit2/UIProcess/qt/SGTileNode.h:39
> +    void swapIfNeeded();

swapBuffersIfNeeded?

> Source/WebKit2/WebProcess/WebPage/TiledDrawingArea.cpp:127
> +    return !(m_suspended || m_isWaitingForUIProcess);

I would prefer without the () ... makes the code easier to read

> Source/WebKit2/WebProcess/WebPage/TiledDrawingArea.cpp:132
> +    return IntRect(IntPoint(0, 0), m_webPage->size());

IntPoint::zero() :-)

> Source/WebKit2/WebProcess/WebPage/TiledDrawingArea.cpp:135
> +IntRect TiledDrawingArea::tiledBackingStoreVisibleRect()

const ?

> Source/WebKit2/WebProcess/WebPage/TiledDrawingArea.h:58
> +    virtual void setVisibleContentRect(const WebCore::IntRect&);
> +    virtual void setContentsScale(float);

contentRect but content*S*Scale.

[Jocelyn Turcotte]

Created attachment 105047 [details]
Patch

(In reply to comment #6)
> > Source/WebCore/ChangeLog:20
> > +        - TiledBackingStoreRemoteTile forwards tile creations, update and removals to the proxy.
> 
> RemoteTile? Any reasons why this is not called TileProxy or so?
This is not a proxy itself but only an intermediate that output calls through it's interface. The goal is to use the same class both for tiling in GraphicLayers and in the DrawingArea for a while.

> > Source/WebCore/platform/graphics/TiledBackingStore.cpp:206
> >  
> > +float TiledBackingStore::coverageRatio(const WebCore::IntRect& contentsRect)
> 
> Maybe it would be good with a short comment showing what the purpose are of these methods.
Fixed

> > Source/WebKit2/UIProcess/TiledDrawingAreaProxy.h:105
> > +    HashMap<int, int> m_tilesToNodes;
> 
> I think it is more common in WebKit to call it someting like tileNodeMap. But maybe just adding Map to the end would make this more clear
Fixed

> > Source/WebKit2/UIProcess/qt/SGAgent.cpp:48
> > +struct NodeUpdateSetBackBuffer : public NodeUpdate {
> 
> When we do a code camp next time, someone will have to explain to me how this scene graph works :-)
Sure :)

> > Source/WebKit2/UIProcess/qt/SGTileNode.h:39
> > +    void swapIfNeeded();
> 
> swapBuffersIfNeeded?
Fixed

> > Source/WebKit2/WebProcess/WebPage/TiledDrawingArea.cpp:127
> > +    return !(m_suspended || m_isWaitingForUIProcess);
> 
> I would prefer without the () ... makes the code easier to read
Fixed

> > Source/WebKit2/WebProcess/WebPage/TiledDrawingArea.cpp:132
> > +    return IntRect(IntPoint(0, 0), m_webPage->size());
> 
> IntPoint::zero() :-)
Fixed

> > Source/WebKit2/WebProcess/WebPage/TiledDrawingArea.cpp:135
> > +IntRect TiledDrawingArea::tiledBackingStoreVisibleRect()
> 
> const ?
This is an implemented method from the interface, since it can be more than a simple accessor I'd prefer not to force the const contract on all (possible) implementations.

> > Source/WebKit2/WebProcess/WebPage/TiledDrawingArea.h:58
> > +    virtual void setVisibleContentRect(const WebCore::IntRect&);
> > +    virtual void setContentsScale(float);
> 
> contentRect but content*S*Scale.
visibleContentRect is written everywhere without the s, and contentsScale is written everywhere with the s.
For grep efficiency I'd keep it that way, but it would be nice to clean this up once for all one day.

> > Source/WebKit2/UIProcess/TiledDrawingAreaProxy.cpp:75
> > +    page()->process()->send(Messages::DrawingArea::RenderNextFrame(), page()->pageID());
> 
> Frame is a pretty overloaded word :-( specially in web/graphics technology
Humm, RenderNextDocumentState? RenderNextContentsState? I think RenderNextFrame isn't so ambiguous so I'd keep it, tell me what you would prefer.
The same should be applied to DidRenderFrame if we change it.

[Kenneth Rohde Christiansen]

> > RemoteTile? Any reasons why this is not called TileProxy or so?
> This is not a proxy itself but only an intermediate that output calls through it's interface. The goal is to use the same class both for tiling in GraphicLayers and in the DrawingArea for a while.

Aha, I hope that is possible in a clean way

> > When we do a code camp next time, someone will have to explain to me how this scene graph works :-)
> Sure :)

Looking forward to that!

> > const ?
> This is an implemented method from the interface, since it can be more than a simple accessor I'd prefer not to force the const contract on all (possible) implementations.

Makes sense

> > contentRect but content*S*Scale.
> visibleContentRect is written everywhere without the s, and contentsScale is written everywhere with the s.
> For grep efficiency I'd keep it that way, but it would be nice to clean this up once for all one day.

Yeah, contents vs content story all over. :/

> > Frame is a pretty overloaded word :-( specially in web/graphics technology
> Humm, RenderNextDocumentState? RenderNextContentsState? I think RenderNextFrame isn't so ambiguous so I'd keep it, tell me what you would prefer.
> The same should be applied to DidRenderFrame if we change it.

Just keep it, it is OK, I was just pointing out that it is a bit overloaded :-)

[Tor Arne Vestbø]

Comment on attachment 105047 [details]
Patch

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

Nice stuff!! A few fixes before landing:

> Source/WebKit2/UIProcess/TiledDrawingAreaProxy.h:105
> +    HashMap<int, int> m_tileNodeMap;

Comment please about what the two ints represent

> Source/WebKit2/UIProcess/qt/SGTileNode.cpp:33
> +    , m_material(0)
> +    , m_opaqueMaterial(0)

I think it would be better with a single boolean, as these two are acting as booleans in the code below, and might become dangling pointers if someone calls setMaterial() without updating m_material.

[Jocelyn Turcotte]

Committed r93780: <http://trac.webkit.org/changeset/93780>