Summary: | [Qt][WK2] WebProcess crashes when composited reflections/masks are present | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Noam Rosenthal <noam> | ||||||||||||||
Component: | Layout and Rendering | Assignee: | Nobody <webkit-unassigned> | ||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||
Severity: | Normal | CC: | hausmann, jturcotte, kenneth, webkit.review.bot | ||||||||||||||
Priority: | P2 | Keywords: | Qt | ||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||
OS: | Unspecified | ||||||||||||||||
Attachments: |
|
Description
Noam Rosenthal
2012-01-09 12:30:24 PST
Created attachment 121733 [details]
Patch
Some of the layout tests still don't render correctly, but that can be fixed in a later patch.
Comment on attachment 121733 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=121733&action=review > Source/WebCore/platform/graphics/texmap/TextureMapperNode.cpp:254 > + HashMap<int, ExternallyManagedTile>::iterator mapEnd = m_externallyManagedTiles.end(); > + for (HashMap<int, ExternallyManagedTile>::iterator mapIterator = m_externallyManagedTiles.begin(); mapIterator != mapEnd; ++mapIterator) { I think using 'end' and 'it' would be sufficient. Very common pattern in webkit > Source/WebKit2/WebProcess/WebCoreSupport/WebGraphicsLayer.cpp:382 > + > + if (layer) > + toWebGraphicsLayer(layer)->setLayerTreeTileClient(layerTreeTileClient()); you dont need to setSize here? like in setMaskLayer? (In reply to comment #2) > (From update of attachment 121733 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=121733&action=review > > > Source/WebCore/platform/graphics/texmap/TextureMapperNode.cpp:254 > > + HashMap<int, ExternallyManagedTile>::iterator mapEnd = m_externallyManagedTiles.end(); > > + for (HashMap<int, ExternallyManagedTile>::iterator mapIterator = m_externallyManagedTiles.begin(); mapIterator != mapEnd; ++mapIterator) { > > I think using 'end' and 'it' would be sufficient. Very common pattern in webkit > > > Source/WebKit2/WebProcess/WebCoreSupport/WebGraphicsLayer.cpp:382 > > + > > + if (layer) > > + toWebGraphicsLayer(layer)->setLayerTreeTileClient(layerTreeTileClient()); > > you dont need to setSize here? like in setMaskLayer? No, since reflection layers don't have content (they always draw their parent's content). Created attachment 121843 [details]
Patch
Comment on attachment 121843 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=121843&action=review Most of it makes sense to me, but I have a couple of questions where answers would help me to understand your patch better :) > Source/WebCore/ChangeLog:13 > + (WebCore::TextureMapperNode::collectVisibleContentsRects): Can you elaborate about this part of your change? > Source/WebCore/platform/graphics/texmap/TextureMapperNode.cpp:257 > + if (m_state.tileOwnership == ExternallyManagedTiles) { > + HashMap<int, ExternallyManagedTile>::iterator end = m_externallyManagedTiles.end(); > + for (HashMap<int, ExternallyManagedTile>::iterator mapIterator = m_externallyManagedTiles.begin(); mapIterator != end; ++mapIterator) { > + if (mapIterator->second.frontBuffer.texture) > + return mapIterator->second.frontBuffer.texture.get(); > + } So this simply returns the texture of the first tile that has one, right? > Source/WebKit2/WebProcess/WebCoreSupport/WebGraphicsLayer.cpp:382 > + if (layer) > + toWebGraphicsLayer(layer)->setLayerTreeTileClient(layerTreeTileClient()); I notice that setReplicatedByLayer sets the layer tree client before notifyChange() and setMaskLayer() sets it afterwards. Is there any chance that could make a difference? (seems unlikely to me, but you know the code better, so I thought I'd mention it :) > Source/WebKit2/WebProcess/WebCoreSupport/WebGraphicsLayer.cpp:414 > - if (replicaLayer()) > - replicaLayer()->syncCompositingState(rect); > - if (maskLayer()) > - maskLayer()->syncCompositingState(rect); > + > + if (WebGraphicsLayer* mask = toWebGraphicsLayer(maskLayer())) > + mask->syncCompositingStateForThisLayerOnly(); > + > + if (WebGraphicsLayer* replica = toWebGraphicsLayer(replicaLayer())) > + replica->syncCompositingStateForThisLayerOnly(); Aha, is this an optimization based on the fact that mask or replica layers cannot have children? > Source/WebKit2/WebProcess/WebCoreSupport/WebGraphicsLayer.cpp:536 > - if (m_image) > + if (!drawsContent()) This as well as the removal of the comment are subtle ;). Can you explain these two changes? > > Source/WebCore/ChangeLog:13 > > + (WebCore::TextureMapperNode::collectVisibleContentsRects): > > Can you elaborate about this part of your change? Replicas don't have their own content, so we don't need to check for their visible rect. Masks always have the same visible rect as the masked item, they don't have their own visible rect, since they don't have their own geometry. > > > Source/WebCore/platform/graphics/texmap/TextureMapperNode.cpp:257 > > + } > > So this simply returns the texture of the first tile that has one, right? Yes. > I notice that setReplicatedByLayer sets the layer tree client before notifyChange() and setMaskLayer() sets it afterwards. Is there any chance that could make a difference? (seems unlikely to me, but you know the code better, so I thought I'd mention it :) I don't think so. But I'll make it more consistent. > > > Source/WebKit2/WebProcess/WebCoreSupport/WebGraphicsLayer.cpp:414 > > - if (replicaLayer()) > > - replicaLayer()->syncCompositingState(rect); > > - if (maskLayer()) > > - maskLayer()->syncCompositingState(rect); > > + > > + if (WebGraphicsLayer* mask = toWebGraphicsLayer(maskLayer())) > > + mask->syncCompositingStateForThisLayerOnly(); > > + > > + if (WebGraphicsLayer* replica = toWebGraphicsLayer(replicaLayer())) > > + replica->syncCompositingStateForThisLayerOnly(); > > Aha, is this an optimization based on the fact that mask or replica layers cannot have children? Yes. > > > Source/WebKit2/WebProcess/WebCoreSupport/WebGraphicsLayer.cpp:536 > > - if (m_image) > > + if (!drawsContent()) > > This as well as the removal of the comment are subtle ;). Can you explain these two changes? I think 75882 explains it better. I'll make sure these m_image changes and related comments are all in 75882. Created attachment 122118 [details]
Patch 1
Created attachment 122123 [details]
atch 1: minor improvements to TextureMapperNode
Created attachment 122137 [details]
Patch 2: Fix mask/reflection handling in Qt WebKit2
Comment on attachment 122123 [details] atch 1: minor improvements to TextureMapperNode View in context: https://bugs.webkit.org/attachment.cgi?id=122123&action=review > Source/WebCore/ChangeLog:14 > + 2. Rename TextureMapperNode::texture() to getFirstContentTextureForMask(), and allow the > + texture to come from the externally-managed tiles. isn't the word find* more used for this? like findFirstContentTextureForMask? > Source/WebCore/platform/graphics/texmap/TextureMapper.cpp:34 > + // A texture that's not used would have two references - the reference inside the vector, and the "texture" local variable. unused? but the comment is confusing as you check for > 2 > Source/WebCore/platform/graphics/texmap/TextureMapper.h:129 > - virtual void releaseTextureToPool(BitmapTexture* surface); > virtual PassRefPtr<BitmapTexture> acquireTextureFromPool(const IntSize&); Maybe add a comment here about how they are released. Created attachment 122250 [details]
Patch 1: minor improvements to TextureMapperNode
Addressed Kenneth's comments.
Comment on attachment 122137 [details] Patch 2: Fix mask/reflection handling in Qt WebKit2 Clearing flags on attachment: 122137 Committed r104832: <http://trac.webkit.org/changeset/104832> Comment on attachment 122250 [details]
Patch 1: minor improvements to TextureMapperNode
Filed another bug, 76336
|