Bug 73591 – [Qt] [WK2] Webkit should release TextureMapper GL objects if page paint node is deallocated.

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Qt

[Viatcheslav Ostapenko]

1. If paint node is deallocated it means that view&page were removed from QSGCanvas and can be inserted into different canvas where existing GL object will be invalid.
2. Can be used as memory saving API for mobile devices (remove invisible view/page from canvas to remove page backing stores).

[Viatcheslav Ostapenko]

Created attachment 117497 [details]
patch

[Noam Rosenthal]

Comment on attachment 117497 [details]
patch

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

See comments.
Question, what are we doing with directly-composited images? Would that come in a different patch?

> Source/WebCore/ChangeLog:3
> +        [Qt] [WK2] Webkit should TextureMapper GL objects if page paint node is deallocated.

I think this is missing a verb (release?).

> Source/WebKit2/WebProcess/WebCoreSupport/WebGraphicsLayer.cpp:579
> +void WebGraphicsLayer::reCreateTiledBackingStore()

Let's call it recreateTiledBackingStoreIfNeeded

> Source/WebKit2/WebProcess/WebCoreSupport/WebGraphicsLayer.cpp:591
> +}

Don't you need to regenerate the content as well, e.g. setNeedsDisplay ?

> Source/WebKit2/WebProcess/WebPage/LayerTreeHost.h:87
> +    virtual void needTileBuffersRecreate() { }

purgeTiledBackingStores (?)

> Source/WebKit2/WebProcess/WebPage/qt/LayerTreeHostQt.cpp:69
> +    , m_needTileBuffersRecreate(false)

m_shouldRecreateTileBuffers

[Viatcheslav Ostapenko]

(In reply to comment #2)
> (From update of attachment 117497 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=117497&action=review
> 
> See comments.
> Question, what are we doing with directly-composited images? 
> Would that come in a different patch?

No.
For some reason I thought that tiles are used for directly-composited images also. I'll figure it out.

> > Source/WebKit2/WebProcess/WebCoreSupport/WebGraphicsLayer.cpp:591
> > +}
> Don't you need to regenerate the content as well, e.g. setNeedsDisplay ?

IMHO it called somewhere else on page getting visible, but I think it doesn't harm to call it here also.

[Viatcheslav Ostapenko]

Created attachment 118504 [details]
Added cleaning of direct composited images

[Noam Rosenthal]

Comment on attachment 118504 [details]
Added cleaning of direct composited images

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

You're using too many names to describe the same thing.
How about:
purgeNodeTexturesRecursive
PurgeBackingStores
purgeGLResouces
recreateBackingStoreIfNeeded
purgeBackingStore
m_shouldRecreateBackingStore

> Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:74
> +    // Trigger setting of correct visibility flags after everything was allocated and initialized

Period at end of sentence.

> Source/WebKit2/WebProcess/WebCoreSupport/WebGraphicsLayer.cpp:591
> +    if (m_layerInfo.imageBackingStoreID) {
> +        layerTreeTileClient()->releaseImageBackingStore(m_layerInfo.imageBackingStoreID);
> +        m_layerInfo.imageBackingStoreID = 0;
> +    }

Early return

> Source/WebKit2/WebProcess/WebCoreSupport/WebGraphicsLayer.cpp:596
> +

Newline unnecessary

> Source/WebKit2/WebProcess/WebPage/qt/LayerTreeHostQt.cpp:440
> +    if (m_shouldRecreateTileBuffers) {

Early return.

[Viatcheslav Ostapenko]

Created attachment 118522 [details]
Names and style fixed.

[Noam Rosenthal]

Comment on attachment 118522 [details]
Names and style fixed.

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

Please fix typo, and regenerate Changelogs :)

> Source/WebKit2/UIProcess/LayerTreeHostProxy.h:58
> +    void purgeGLResouces();

you mean purgeGLResources?
(typo)

[Viatcheslav Ostapenko]

(In reply to comment #7)
> (From update of attachment 118522 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=118522&action=review
> 
> Please fix typo, and regenerate Changelogs :)
> 
> > Source/WebKit2/UIProcess/LayerTreeHostProxy.h:58
> > +    void purgeGLResouces();
> 
> you mean purgeGLResources?
> (typo)

Uff!
I hate copy&paste ;)

[Noam Rosenthal]

> > > Source/WebKit2/UIProcess/LayerTreeHostProxy.h:58
> > > +    void purgeGLResouces();
> > 
> > you mean purgeGLResources?
> > (typo)
> 
> Uff!
> I hate copy&paste ;)

Just checking that you're alert...

[Viatcheslav Ostapenko]

Created attachment 118527 [details]
Resouces fixed

[WebKit Review Bot]

Comment on attachment 118527 [details]
Resouces fixed

Clearing flags on attachment: 118527

Committed r102435: <http://trac.webkit.org/changeset/102435>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.