Bug 76675 - [chromium] Remove setLayerTreeHost nonsense on lost context
Summary: [chromium] Remove setLayerTreeHost nonsense on lost context
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: James Robinson
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-01-19 17:19 PST by James Robinson
Modified: 2012-01-26 14:47 PST (History)
7 users (show)

See Also:


Attachments
Patch (1.62 KB, patch)
2012-01-19 17:20 PST, James Robinson
kbr: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description James Robinson 2012-01-19 17:19:09 PST
[chromium] Remove setLayerTreeHost nonsense on lost context
Comment 1 James Robinson 2012-01-19 17:20:30 PST
Created attachment 123226 [details]
Patch
Comment 2 James Robinson 2012-01-19 17:22:38 PST
Pretty simple.  This codepath used to do more back when layers managed their own textures, but since everything either uses managed textures (content/image/video layers) or has its own context lost management (webgl/plugin) we don't have to do anything special when losing a context.

setLayerTreeHost() is now only called when:
*) A layer enters a tree
*) A layer leaves a tree
*) A layer moves from one tree to another (for example moves from being inside a tab to a window.open()'d popup)
Comment 3 James Robinson 2012-01-19 17:23:33 PST
Tested by:

platform/chromium/compositing/*lost-context*

manually opening poster circle, a page with html5 video, and a webgl sample and killing the GPU process
Comment 4 Nat Duca 2012-01-19 17:23:39 PST
Comment on attachment 123226 [details]
Patch

Looks highly amusing to me. Which should rhyme with LGTM but doesnt.
Comment 5 Vangelis Kokkevis 2012-01-19 22:59:46 PST
Comment on attachment 123226 [details]
Patch

Do we end up calling evictAndDeleteAllTextures and if so are we trying to delete textures in a context that no longer exists?
Comment 6 James Robinson 2012-01-20 11:08:29 PST
(In reply to comment #5)
> (From update of attachment 123226 [details])
> Do we end up calling evictAndDeleteAllTextures and if so are we trying to delete textures in a context that no longer exists?

We do and we are (relying on all GL calls on a lost context being no-ops, which they are).  We could add a path that goes through the normal eviction path but doesn't actually issue any GL calls if we were concerned about this.
Comment 7 Adrienne Walker 2012-01-20 11:47:07 PST
LGTM2.  Now that all layer types are using ManagedTexture, this should be safe to do.
Comment 8 Alok Priyadarshi 2012-01-26 13:38:32 PST
(In reply to comment #2)
> Pretty simple.  This codepath used to do more back when layers managed their own textures, but since everything either uses managed textures (content/image/video layers) or has its own context lost management (webgl/plugin) we don't have to do anything special when losing a context.
> 
> setLayerTreeHost() is now only called when:
> *) A layer enters a tree
> *) A layer leaves a tree
> *) A layer moves from one tree to another (for example moves from being inside a tab to a window.open()'d popup)

This is a good change. I just wanted point out a slightly related bug: https://bugs.webkit.org/show_bug.cgi?id=77135

setLayerTreeHost() also get called even when a layer remains in the same tree:
1. Moving a layer within the tree. See WebCore::RenderLayer::removeOnlyThisLayer().
2. GraphicsLayer::setChildren() removes-adds the layers common in the current child-list and the new child-list

Since setLayerTreeHost(0) also triggers resource cleanup, these two cases sometimes severely affect performance due to unnecessary work.
Comment 9 Kenneth Russell 2012-01-26 14:42:58 PST
Comment on attachment 123226 [details]
Patch

r=me based on enne's review.
Comment 10 James Robinson 2012-01-26 14:47:09 PST
Committed r106048: <http://trac.webkit.org/changeset/106048>