Bug 102900 – Coordinated Graphics: refactor LayerTreeRenderer::syncRemoteContent().

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

[Dongseong Hwang]

Currently, QQuickWebPage::updatePaintNode() calls
LayerTreeRenderer::syncRemoteContent() with locking the main thread.
syncRemoteContent() is heavy, so we should not call syncRemoteContent() in
updatePaintNode() because syncRemoteContent() does not access any data
structures, which the main thread use.

After this patch, syncRemoteContent() is used only internally, so
syncRemoteContent() becomes private.

[Dongseong Hwang]

Created attachment 175376 [details]
Patch

[Dongseong Hwang]

After this patch, I can feel reducing lag due to texture uploading.
For example, snowstack. http://www.satine.org/research/webkit/snowleopard/snowstack.html

[Dongseong Hwang]

I decided to post this patch after reading Slava's mail. Thank Slava for explanation!

Qt WK2 port already runs layer tree painting on separate thread.
Layer tree renderer is created on main thread, but rendering is performed only on paint thread.
Qt5 creates paint nodes for painting and QQuickWebPage paint node keeps threaded reference on LayerTreeRenderer. Messages to LayerTreeRenderer are delivered through dispatchUpdate/bind/renderQueue .
All updates from renderQueue to renderer are applied in QQuickWebPage::updatePaintNode call. QQuickItem::updatePaintNode is very special call. During this call main thread is locked and it is safe to access main thread objects from paint thread.
http://doc-snapshot.qt-project.org/5.0/qquickitem.html#updatePaintNode

So, the only thing that needs to be implemented for GTK is replacement for Qt paint node and sync point similar to QQuickItem::updatePaintNode .

[Noam Rosenthal]

Comment on attachment 175376 [details]
Patch

Good catch!

[WebKit Review Bot]

Comment on attachment 175376 [details]
Patch

Clearing flags on attachment: 175376

Committed r135401: <http://trac.webkit.org/changeset/135401>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Viatcheslav Ostapenko]

Comment on attachment 175376 [details]
Patch

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

> Source/WebKit2/UIProcess/CoordinatedGraphics/LayerTreeRenderer.cpp:125
> +    syncRemoteContent();

Actually, I doubt that it improves something and I would suspect regression here. 
QQuickWebPage::updatePaintNode is called only when update is required (like new tiles coming). LayerTreeRenderer::paintToCurrentGLContext is called for every paint, so with this patch it will be called more often and that's why individual tiles might come faster, because paint node update is called only from didRenderFrame. If faster displaying of incoming tile updates is desired this could be achieved by calling updateViewPort more often in LayerTreeCoordinatorProxy (like, for every incoming tile update), but this could cause visual partial update artifacts.
Also, syncRemoteContent originally was also called from paint and I removed it intentionally because this function uses renderQueue which is accessible both from main and paint threads. Previously renderQueue had no locking and it worked because Qt5 guarantees that main thread is locked during updatePaintNode call. Now this might work because of locking of renderQueue, but it will crash it locking would be removed.

[Dongseong Hwang]

(In reply to comment #7)
> (From update of attachment 175376 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=175376&action=review
> 
> > Source/WebKit2/UIProcess/CoordinatedGraphics/LayerTreeRenderer.cpp:125
> > +    syncRemoteContent();
> 
> Actually, I doubt that it improves something and I would suspect regression here. 

Thanks for helpful explanation! I could really feel reducing lag due to texture uploading.

> QQuickWebPage::updatePaintNode is called only when update is required (like new tiles coming). LayerTreeRenderer::paintToCurrentGLContext is called for every paint, so with this patch it will be called more often and that's why individual tiles might come faster, because paint node update is called only from didRenderFrame. If faster displaying of incoming tile updates is desired this could be achieved by calling updateViewPort more often in LayerTreeCoordinatorProxy (like, for every incoming tile update), but this could cause visual partial update artifacts.

Before I had changed, I hesitated due to your reason: updatePaintNode is called if needed. After taking some time to think, I arrived at the conclusion: it is safe.
It is because we update tiles when handling didRenderFrame in that calling flushLayerChanges(), especially commitTileOperations(). LayerTreeRenderer::paintToCurrentGLContext can causes calling updateTile() but actual update is deferred until flushLayerChanges() is called. Furthermore, Web Process still wait to progress until receiving a RenderNextFrame message.

void LayerTreeRenderer::flushLayerChanges()
{
    m_renderedContentsScrollPosition = m_pendingRenderedContentsScrollPosition;

    // Since the frame has now been rendered, we can safely unlock the animations until the next layout.
    setAnimationsLocked(false);

    m_rootLayer->flushCompositingState(FloatRect());
    commitTileOperations();
    removeReleasedImageBackingsIfNeeded();

    // The pending tiles state is on its way for the screen, tell the web process to render the next one.
    dispatchOnMainThread(bind(&LayerTreeRenderer::renderNextFrame, this));
}

> Also, syncRemoteContent originally was also called from paint and I removed it intentionally because this function uses renderQueue which is accessible both from main and paint threads. Previously renderQueue had no locking and it worked because Qt5 guarantees that main thread is locked during updatePaintNode call. Now this might work because of locking of renderQueue, but it will crash it locking would be removed.

Yes, your work ensuring atomic operation encouraged me to make the patch. syncRemoteContent is not called from external clients anymore and LayerTreeRenderer::paintToGraphicsContext also calls syncRemoteContent, so I think it is natural for LayerTreeRenderer::paintToCurrentGLContext to call syncRemoteContent now. However, when another requirement exists, syncRemoteContent can be public again. I'm ok.

Please let me know if there is gap with your opinion.