76675 2012-01-19 17:19:09 -0800 [chromium] Remove setLayerTreeHost nonsense on lost context 2012-01-26 14:47:09 -0800 1 1 1 Unclassified WebKit New Bugs 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED P2 Normal --- 1 jamesr jamesr alokp cc-bugs enne nduca shawnsingh vangelis webkit.review.bot oldest_to_newest 539170 0 jamesr 2012-01-19 17:19:09 -0800 [chromium] Remove setLayerTreeHost nonsense on lost context 539171 1 123226 jamesr 2012-01-19 17:20:30 -0800 Created attachment 123226 Patch 539173 2 jamesr 2012-01-19 17:22:38 -0800 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) 539177 3 jamesr 2012-01-19 17:23:33 -0800 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 539178 4 123226 nduca 2012-01-19 17:23:39 -0800 Comment on attachment 123226 Patch Looks highly amusing to me. Which should rhyme with LGTM but doesnt. 539291 5 123226 vangelis 2012-01-19 22:59:46 -0800 Comment on attachment 123226 Patch Do we end up calling evictAndDeleteAllTextures and if so are we trying to delete textures in a context that no longer exists? 539628 6 jamesr 2012-01-20 11:08:29 -0800 (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. 539660 7 enne 2012-01-20 11:47:07 -0800 LGTM2. Now that all layer types are using ManagedTexture, this should be safe to do. 543439 8 alokp 2012-01-26 13:38:32 -0800 (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. 543503 9 123226 kbr 2012-01-26 14:42:58 -0800 Comment on attachment 123226 Patch r=me based on enne's review. 543506 10 jamesr 2012-01-26 14:47:09 -0800 Committed r106048: <http://trac.webkit.org/changeset/106048> 123226 2012-01-19 17:20:30 -0800 2012-01-26 14:42:58 -0800 Patch bug-76675-20120119172029.patch text/plain 1662 jamesr U3VidmVyc2lvbiBSZXZpc2lvbjogMTA1NDU5CmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9D aGFuZ2VMb2cgYi9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXggZDdjOTE0NjIxZmI2OTE3 Yzg3NDhmYTYzZDc3Y2VkNjExYzg2MTcwMi4uYTdkMDljNTkxOGM5NTZkNzAzNjg1YmYxODI0NmI3 YWE0ZTU1NDE1NyAxMDA3NTUKLS0tIGEvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCisrKyBiL1Nv dXJjZS9XZWJDb3JlL0NoYW5nZUxvZwpAQCAtMSwzICsxLDE2IEBACisyMDEyLTAxLTE5ICBKYW1l cyBSb2JpbnNvbiAgPGphbWVzckBjaHJvbWl1bS5vcmc+CisKKyAgICAgICAgW2Nocm9taXVtXSBS ZW1vdmUgc2V0TGF5ZXJUcmVlSG9zdCBub25zZW5zZSBvbiBsb3N0IGNvbnRleHQKKyAgICAgICAg aHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTc2Njc1CisKKyAgICAgICAg UmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAgICAgVGhpcyBjb2RlIGlzbid0IG5l ZWRlZCBhbnkgbW9yZS4gT24gYSBsb3N0IGNvbnRleHQgZXZlbnQsIHdlIGRyb3AgYWxsIFRleHR1 cmVNYW5hZ2VyLW1hbmFnZWQgdGV4dHVyZXMgdGhyb3VnaAorICAgICAgICB0aGUgcHJveHkgYW5k IG5vIGxheWVyIHR5cGVzIG5lZWQgc3BlY2lhbCBsb3N0IGNvbnRleHQgaGFuZGxpbmcuCisKKyAg ICAgICAgKiBwbGF0Zm9ybS9ncmFwaGljcy9jaHJvbWl1bS9jYy9DQ0xheWVyVHJlZUhvc3QuY3Bw OgorICAgICAgICAoV2ViQ29yZTo6Q0NMYXllclRyZWVIb3N0OjpkaWRSZWNyZWF0ZUdyYXBoaWNz Q29udGV4dCk6CisKIDIwMTItMDEtMTkgIEdyZWcgQmlsbG9jayAgPGdiaWxsb2NrQGdvb2dsZS5j b20+CiAKICAgICAgICAgW0NvdmVyaXR5XSBGaXggdW5pbml0aWFsaXplZCBjb25zdHJ1Y3RvciBk ZWZlY3RzIGluIC4uLi9odG1sCmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9wbGF0Zm9ybS9n cmFwaGljcy9jaHJvbWl1bS9jYy9DQ0xheWVyVHJlZUhvc3QuY3BwIGIvU291cmNlL1dlYkNvcmUv cGxhdGZvcm0vZ3JhcGhpY3MvY2hyb21pdW0vY2MvQ0NMYXllclRyZWVIb3N0LmNwcAppbmRleCA4 NjYzZWMwYWMyY2RmMzZiMjQ3MjI4MWNmZmFlYTE5NDAyMjVhMjNjLi40ODM4NmFhNDc5NDEwNWY4 MzZmMGQ0ZGY1MDk1Zjc1ZjJhNjI0MTljIDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViQ29yZS9wbGF0 Zm9ybS9ncmFwaGljcy9jaHJvbWl1bS9jYy9DQ0xheWVyVHJlZUhvc3QuY3BwCisrKyBiL1NvdXJj ZS9XZWJDb3JlL3BsYXRmb3JtL2dyYXBoaWNzL2Nocm9taXVtL2NjL0NDTGF5ZXJUcmVlSG9zdC5j cHAKQEAgLTE4MSwxMCArMTgxLDYgQEAgUGFzc093blB0cjxDQ0xheWVyVHJlZUhvc3RJbXBsPiBD Q0xheWVyVHJlZUhvc3Q6OmNyZWF0ZUxheWVyVHJlZUhvc3RJbXBsKENDTGF5ZXIKIAogdm9pZCBD Q0xheWVyVHJlZUhvc3Q6OmRpZFJlY3JlYXRlR3JhcGhpY3NDb250ZXh0KGJvb2wgc3VjY2VzcykK IHsKLSAgICBpZiAobV9yb290TGF5ZXIpIHsKLSAgICAgICAgbV9yb290TGF5ZXItPnNldExheWVy VHJlZUhvc3QoMCk7Ci0gICAgICAgIG1fcm9vdExheWVyLT5zZXRMYXllclRyZWVIb3N0KHRoaXMp OwotICAgIH0KICAgICBtX2NsaWVudC0+ZGlkUmVjcmVhdGVHcmFwaGljc0NvbnRleHQoc3VjY2Vz cyk7CiB9CiAK