Bug 65528 – [Qt][WK2] Create scene graph nodes for tiles in QTouchWebView instead of using imperative painting.

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

[Jocelyn Turcotte]

[Qt][WK2] Create scene graph nodes for tiles in QTouchWebView instead of using imperative painting.

[Jocelyn Turcotte]

Created attachment 102638 [details]
Patch

[WebKit Review Bot]

Attachment 102638 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit..." exit_code: 1

Source/WebKit2/UIProcess/API/qt/qtouchwebpage.cpp:26:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 15 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Benjamin Poulain]

Comment on attachment 102638 [details]
Patch

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

Quick look, I'll do a review later.

> Source/WebKit2/UIProcess/API/qt/qtouchwebpage.cpp:73
> +    // This takes ownership of itemNode and it's children.

"This" is a bit ambiguous, I have no clue who takes ownership here :)

> Source/WebKit2/UIProcess/qt/SGAgent.cpp:38
> +    enum Type {
> +        CreateTile,
> +        CreateScale,
> +        Remove,
> +        SetTexture
> +    };

I think you also need to implement some kind of barrier to make the queue work, because the operations are not atomic.

If you take this code for example:
 if (!m_nodeID)
     m_nodeID = m_sgAgent->createTileNode(m_parentNodeID);
 ASSERT(m_nodeID);
 QRect sourceRect(0, 0, m_rect.width(), m_rect.height());
 m_sgAgent->setNodeTexture(m_nodeID, m_backBuffer, sourceRect, m_rect);

Without a barrier, nothing prevent the updatePaintNode() thread from only getting CreateTile because SetTexture has not been executed yet.

> Source/WebKit2/UIProcess/qt/TiledDrawingAreaTileQt.cpp:115
> +    ASSERT(false);

ASSERT_NOT_REACHED() :)

> Source/WebKit2/UIProcess/qt/TiledDrawingAreaTileQt.cpp:139
> +    QPainter painter;
> +    painter.begin(&m_backBuffer);
>      IntSize drawPoint = updateChunkRect.location() - m_rect.location();
>      painter.drawImage(QPoint(drawPoint.width(), drawPoint.height()), image);
> +    painter.end();

Maybe just create a scope for a Qainter(&m_backbuffer) to avoid calling end() manually?

[Viatcheslav Ostapenko]

(In reply to comment #1)
> Created an attachment (id=102638) [details]
> Patch

1. You do not need scale nodes. If you call setRect on QSGSimpleTexture node with rect size not equal to texture size, than texture will be scaled.
2. In SGAgent::updatePaintNode why do you call parentNode->prependChildNode ?
3. Wouldn't it be better to replace both front/back buffers with textures and upload update chunks directly to textures?

[Jocelyn Turcotte]

(In reply to comment #3)
Cool thx, then I'll wait for the full review, meanwhile:
> I think you also need to implement some kind of barrier to make the queue work, because the operations are not atomic.
> 
> If you take this code for example:
>  if (!m_nodeID)
>      m_nodeID = m_sgAgent->createTileNode(m_parentNodeID);
>  ASSERT(m_nodeID);
>  QRect sourceRect(0, 0, m_rect.width(), m_rect.height());
>  m_sgAgent->setNodeTexture(m_nodeID, m_backBuffer, sourceRect, m_rect);
> 
> Without a barrier, nothing prevent the updatePaintNode() thread from only getting CreateTile because SetTexture has not been executed yet.

The GUI thread will never run at the same time as updatePaintNode(). The barrier is kind of QSGCanvas::paintEvent and it gets unlocked when the graph's update is over and rendering starts (see QSGCanvasRenderThread::run). We get guaranteed atomicity until we return to the event loop.

> Maybe just create a scope for a Qainter(&m_backbuffer) to avoid calling end() manually?

I thought it to be a bit more readable but safer might be better, I can change it.

[Jocelyn Turcotte]

(In reply to comment #4)
> 1. You do not need scale nodes. If you call setRect on QSGSimpleTexture node with rect size not equal to texture size, than texture will be scaled.
I'd like to keep all tile bound together on an integer coordinate system and scale the whole coordinate system like it is currently with QPainter on the TiledDrawingAreaProxy.
This is to make it simpler to prevent fractional transformed sizes/positions and avoid slight scaling/filtering. I have a pending patch to further enforce that.

> 2. In SGAgent::updatePaintNode why do you call parentNode->prependChildNode ?
To add the node the the scene tree, I use prepend instead of append like explained in the comment to paint complete/older tile sets on top of the newest one (we could have a more flexible mechanism but hardcoding prepend should do for now). I could use append for SGTileNodes, but it doesn't really matter since they don't overlap.

> 3. Wouldn't it be better to replace both front/back buffers with textures and upload update chunks directly to textures?
I tried that but we really need to do the texture upload on the scene graph thread for performance and maintainability reasons, else we need to spawn a shared context between threads and it's not worth it for now.

We need at least to keep one QImage back buffer to receive partial updates from the WebProcess. To copy a texture over another one we need to use use a FBO, it's complex and I think we should keep it simple until we know if we can get the web process to render directly to hardware shared memory (for supported mobile architectures) and pass the shared buffer handle directly to the scene graph to bind it to a texture.

[Jocelyn Turcotte]

Created attachment 102659 [details]
Patch

- Fixed stuff asked in comment #3
- Destroyed any NodeUpdates still in the queue in ~SGAgent
- Removed locks in SGAgent after discussion with Benjamin since the locked GUI thread also protects us there.

[WebKit Review Bot]

Attachment 102659 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit..." exit_code: 1

Source/WebKit2/UIProcess/API/qt/qtouchwebpage.cpp:26:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 15 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Benjamin Poulain]

Comment on attachment 102659 [details]
Patch

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

Little comments before going to bed :)

> Source/WebKit2/UIProcess/API/qt/qtouchwebpage.h:57
> +    virtual QSGNode* updatePaintNode(QSGNode* oldNode, UpdatePaintNodeData*);

oldNode -> itemNode for consistency with the implementation?

> Source/WebKit2/UIProcess/API/qt/qtouchwebpage_p.h:50
> +    QScopedPointer<WebKit::SGAgent> sgAgent;

What about just having SGAgent by value instead of a pointer so it is allocated along QTouchWebPagePrivate?

> Source/WebKit2/UIProcess/qt/SGAgent.cpp:114
> +    int nodeID = nextNodeID++;
> +    nodeUpdatesQueue.append(new NodeUpdateCreateTile(nodeID, parentNodeID));
> +    item->update();
> +    return nodeID;
> +}
> +
> +int SGAgent::createScaleNode(int parentNodeID, float scale)
> +{
> +    int nodeID = nextNodeID++;
> +    nodeUpdatesQueue.append(new NodeUpdateCreateScale(nodeID, parentNodeID, scale));
> +    item->update();
> +    return nodeID;

Ok, call me paranoid, but I am always scared when I see an ID than can wrap around on overflow AND for which one value has a special meaning.
Just to be on the correct side, you could have:
int nextNodeID() const {
        int nodeID = m_nextNodeID++;
        if (!nodeID)
           nodeID = m_nextNodeID++;
        ASSERT(nodeID);
        return nodeID;
}

Too much paranoia? :)

> Source/WebKit2/UIProcess/qt/TiledDrawingAreaTileQt.cpp:52
> +    , m_parentNodeID()

Uh? :)
Missing 0? :)

> Source/WebKit2/UIProcess/qt/TiledDrawingAreaTileQt.cpp:81
>  bool TiledDrawingAreaTile::isReadyToPaint() const
>  {
> -    return m_isFrontBufferValid;
> +    return m_nodeID;

return !!m_nodeID; for clarity?

I have the feeling that is not really the meaning of isReadyToPaint() but I'll check the code another time :)

[Benjamin Poulain]

Comment on attachment 102659 [details]
Patch

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

Little comments before going to bed :)

> Source/WebKit2/UIProcess/API/qt/qtouchwebpage.h:57
> +    virtual QSGNode* updatePaintNode(QSGNode* oldNode, UpdatePaintNodeData*);

oldNode -> itemNode for consistency with the implementation?

> Source/WebKit2/UIProcess/API/qt/qtouchwebpage_p.h:50
> +    QScopedPointer<WebKit::SGAgent> sgAgent;

What about just having SGAgent by value instead of a pointer so it is allocated along QTouchWebPagePrivate?

> Source/WebKit2/UIProcess/qt/SGAgent.cpp:114
> +    int nodeID = nextNodeID++;
> +    nodeUpdatesQueue.append(new NodeUpdateCreateTile(nodeID, parentNodeID));
> +    item->update();
> +    return nodeID;
> +}
> +
> +int SGAgent::createScaleNode(int parentNodeID, float scale)
> +{
> +    int nodeID = nextNodeID++;
> +    nodeUpdatesQueue.append(new NodeUpdateCreateScale(nodeID, parentNodeID, scale));
> +    item->update();
> +    return nodeID;

Ok, call me paranoid, but I am always scared when I see an ID than can wrap around on overflow AND for which one value has a special meaning.
Just to be on the correct side, you could have:
int nextNodeID() const {
        int nodeID = m_nextNodeID++;
        if (!nodeID)
           nodeID = m_nextNodeID++;
        ASSERT(nodeID);
        return nodeID;
}

Too much paranoia? :)

> Source/WebKit2/UIProcess/qt/TiledDrawingAreaTileQt.cpp:52
> +    , m_parentNodeID()

Uh? :)
Missing 0? :)

> Source/WebKit2/UIProcess/qt/TiledDrawingAreaTileQt.cpp:81
>  bool TiledDrawingAreaTile::isReadyToPaint() const
>  {
> -    return m_isFrontBufferValid;
> +    return m_nodeID;

return !!m_nodeID; for clarity?

I have the feeling that is not really the meaning of isReadyToPaint() but I'll check the code another time :)

[Jocelyn Turcotte]

Created attachment 102776 [details]
Patch

[Jocelyn Turcotte]

(In reply to comment #10)
> oldNode -> itemNode for consistency with the implementation?
Renamed itemNode to oldNode, like in QSGItem.

> What about just having SGAgent by value instead of a pointer so it is allocated along QTouchWebPagePrivate?
Changed

> Ok, call me paranoid, but I am always scared when I see an ID than can wrap around on overflow AND for which one value has a special meaning.
> Too much paranoia? :)
From my calculations, if you generate 10 tiles per second non-stop, it would take 10 years :P

> Uh? :)
> Missing 0? :)
Oh, that's embarassing...

> return !!m_nodeID; for clarity?
Changed

> I have the feeling that is not really the meaning of isReadyToPaint() but I'll check the code another time :)
We need it for now for the coverageRatio method, we will have to refactor the tiling soon so we could change the name there.

[Benjamin Poulain]

Comment on attachment 102776 [details]
Patch

Quick comment before lunch :)

> Source/WebKit2/UIProcess/qt/SGTileNode.h:37
> +
> +class SGTileNode : public QSGGeometryNode {
> +public:
> +    SGTileNode();
> +

This is awefully similar to QSGSimpleTextureNode (the way the source rect is handled is different but the end result should be exactly the same as far as I can tell).
Any reason why we cannot use QSGSimpleTextureNode?

[Jocelyn Turcotte]

(In reply to comment #13)
> This is awefully similar to QSGSimpleTextureNode (the way the source rect is handled is different but the end result should be exactly the same as far as I can tell).
> Any reason why we cannot use QSGSimpleTextureNode?

Like just talked, I needed two additional features that QSGSimpleTextureNode doesn't provide:
- Allow a source rect to be specified for edge tiles.
- Own the QSGTexture object for proper destruction on the sg thread after the QSGItem is destroyed on the GUI thread.

The first point could be handled by having textures created for the size of the visible content, but we still need our own node type for the owning part. We can see later if we still need it.

[Benjamin Poulain]

Comment on attachment 102776 [details]
Patch

Yeah, I finally found something meaningfull to complain about :-D

> Source/WebKit2/UIProcess/qt/SGAgent.cpp:99
> +SGAgent::~SGAgent()
> +{
> +    foreach (NodeUpdate* nodeUpdate, nodeUpdatesQueue)
> +        delete nodeUpdate;
> +}

Deque of OwnPtr and you could remove this, see comment later

> Source/WebKit2/UIProcess/qt/SGAgent.h:54
> +    QLinkedList<NodeUpdate*> nodeUpdatesQueue;

QLinkedList is a piece of shit, what about using WTF's deque?
Something else really nice Deque is it has template insertion and accessor. That means you can use it with OwnPtr and have super clean memory management in SGAgent :-D

> Source/WebKit2/UIProcess/qt/SGAgent.h:55
> +    QHash<int, QSGNode*> nodes;

QHash is also a piece of shit :)
WTF's hash table is much better.

[Benjamin Poulain]

Comment on attachment 102776 [details]
Patch

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

> Source/WebKit2/UIProcess/API/qt/qtouchwebpage_p.h:35
>  
> +
>  class QTouchWebPagePrivate {

hum :)

> Source/WebKit2/UIProcess/TiledDrawingAreaProxy.cpp:111
>  
> -
>  static IntPoint innerBottomRight(const IntRect& rect)

Uh :)

>> Source/WebKit2/UIProcess/qt/SGAgent.cpp:99
>> +SGAgent::~SGAgent()
>> +{
>> +    foreach (NodeUpdate* nodeUpdate, nodeUpdatesQueue)
>> +        delete nodeUpdate;
>> +}
> 
> Deque of OwnPtr and you could remove this, see comment later

Deque of OwnPtr and you could remove this, see comment later

> Source/WebKit2/UIProcess/qt/SGAgent.cpp:136
> +            SGTileNode* tileNode  = new SGTileNode;

One space too many before the equal sign.

> Source/WebKit2/UIProcess/qt/SGAgent.cpp:137
> +            QSGNode* parentNode = createTileUpdate->parentNodeID ? nodes.value(createTileUpdate->parentNodeID) : itemNode;

Wouldn't that mean something went horribly wrong if createTileUpdate->parentNodeID is 0?
I guess the tile should always be in the scale node of the tileset.

> Source/WebKit2/UIProcess/qt/SGAgent.cpp:144
> +            QSGTransformNode* scaleNode  = new QSGTransformNode;

One space too many before the equal sign.

> Source/WebKit2/UIProcess/qt/SGAgent.cpp:172
> +            ASSERT(false);

ASSERT_NOT_REACHED();

[Jocelyn Turcotte]

Committed r92376: <http://trac.webkit.org/changeset/92376>

[Jocelyn Turcotte]

Committed a fix for a memory leak introduced by this patch in
r93620: <http://trac.webkit.org/changeset/93620>