Bug 73591 - [Qt] [WK2] Webkit should release TextureMapper GL objects if page paint node is deallocated.
Summary: [Qt] [WK2] Webkit should release TextureMapper GL objects if page paint node ...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Qt (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P3 Normal
Assignee: Viatcheslav Ostapenko
URL:
Keywords: Qt
Depends on: 74176
Blocks:
  Show dependency treegraph
 
Reported: 2011-12-01 14:36 PST by Viatcheslav Ostapenko
Modified: 2011-12-09 04:33 PST (History)
4 users (show)

See Also:


Attachments
patch (15.35 KB, patch)
2011-12-01 15:00 PST, Viatcheslav Ostapenko
noam: review-
noam: commit-queue-
Details | Formatted Diff | Diff
Added cleaning of direct composited images (33.74 KB, patch)
2011-12-08 17:54 PST, Viatcheslav Ostapenko
noam: review-
noam: commit-queue-
Details | Formatted Diff | Diff
Names and style fixed. (30.14 KB, patch)
2011-12-08 20:51 PST, Viatcheslav Ostapenko
noam: review-
noam: commit-queue-
Details | Formatted Diff | Diff
Resouces fixed (33.65 KB, patch)
2011-12-08 21:19 PST, Viatcheslav Ostapenko
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Viatcheslav Ostapenko 2011-12-01 14:36:33 PST
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).
Comment 1 Viatcheslav Ostapenko 2011-12-01 15:00:33 PST
Created attachment 117497 [details]
patch
Comment 2 Noam Rosenthal 2011-12-01 15:26:28 PST
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
Comment 3 Viatcheslav Ostapenko 2011-12-01 17:45:16 PST
(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.
Comment 4 Viatcheslav Ostapenko 2011-12-08 17:54:24 PST
Created attachment 118504 [details]
Added cleaning of direct composited images
Comment 5 Noam Rosenthal 2011-12-08 18:39:07 PST
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.
Comment 6 Viatcheslav Ostapenko 2011-12-08 20:51:17 PST
Created attachment 118522 [details]
Names and style fixed.
Comment 7 Noam Rosenthal 2011-12-08 20:57:02 PST
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)
Comment 8 Viatcheslav Ostapenko 2011-12-08 20:58:27 PST
(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 ;)
Comment 9 Noam Rosenthal 2011-12-08 20:59:48 PST
> > > Source/WebKit2/UIProcess/LayerTreeHostProxy.h:58
> > > +    void purgeGLResouces();
> > 
> > you mean purgeGLResources?
> > (typo)
> 
> Uff!
> I hate copy&paste ;)

Just checking that you're alert...
Comment 10 Viatcheslav Ostapenko 2011-12-08 21:19:53 PST
Created attachment 118527 [details]
Resouces fixed
Comment 11 WebKit Review Bot 2011-12-08 22:54:29 PST
Comment on attachment 118527 [details]
Resouces fixed

Clearing flags on attachment: 118527

Committed r102435: <http://trac.webkit.org/changeset/102435>
Comment 12 WebKit Review Bot 2011-12-08 22:54:34 PST
All reviewed patches have been landed.  Closing bug.