Layer property changes during paintContent() can leave the layer in an invalid state as paintContentsIfDirty() has been called without a matching updateCompositorResources() call. Removing conditionals around these calls ensure they are balanced. This change can result in unnecessary uploads but that can be properly solved using this change: https://bugs.webkit.org/show_bug.cgi?id=72192
Created attachment 115620 [details] Patch
Comment on attachment 115620 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=115620&action=review I wish there were some better way to assert that this never happened in practice. Do you think it'd be a good idea to store the compositor frame that is being painted in TiledLayerChromium::prepareToUpdate and assert/early-out if the frame number is different during TiledLayerChromium::updateCompositorResources? > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:-339 > - if (layer->drawsContent()) > - layer->paintContentsIfDirty(); Can you leave the drawsContent() check both here and in updateCompositorResources? Otherwise, you're going to trip asserts like the ASSERT(drawsContent()) in VideoLayerChromium::updateCompositorResources.
Created attachment 115642 [details] Patch
(In reply to comment #2) > (From update of attachment 115620 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=115620&action=review > > I wish there were some better way to assert that this never happened in practice. Do you think it'd be a good idea to store the compositor frame that is being painted in TiledLayerChromium::prepareToUpdate and assert/early-out if the frame number is different during TiledLayerChromium::updateCompositorResources? > > > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:-339 > > - if (layer->drawsContent()) > > - layer->paintContentsIfDirty(); > > Can you leave the drawsContent() check both here and in updateCompositorResources? Otherwise, you're going to trip asserts like the ASSERT(drawsContent()) in VideoLayerChromium::updateCompositorResources. Realized this just after uploading the previous patch. I think my new patch handles this properly. We need to make sure that if one of these calls are made the other needs to be made as well. It will be easy to avoid both calls when 72192 has landed.
Comment on attachment 115642 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=115642&action=review > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:338 > static void paintContentsIfDirty(LayerChromium* layer) > { > - if (layer->drawsContent()) > - layer->paintContentsIfDirty(); > + layer->paintContentsIfDirty(); this helper isn't useful any more, can you just inline it?
Does https://bugs.webkit.org/show_bug.cgi?id=71038 simplify this?
Created attachment 115671 [details] Patch
(In reply to comment #5) > (From update of attachment 115642 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=115642&action=review > > > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:338 > > static void paintContentsIfDirty(LayerChromium* layer) > > { > > - if (layer->drawsContent()) > > - layer->paintContentsIfDirty(); > > + layer->paintContentsIfDirty(); > > this helper isn't useful any more, can you just inline it? Fixed.
(In reply to comment #6) > Does https://bugs.webkit.org/show_bug.cgi?id=71038 simplify this? That change is great as it makes this fix less likely to cause a performance regression.
Comment on attachment 115671 [details] Patch Looks great, R=me. Could you let Enne check it over before landing?
Comment on attachment 115671 [details] Patch LGTM
Comment on attachment 115671 [details] Patch Clearing flags on attachment: 115671 Committed r100702: <http://trac.webkit.org/changeset/100702>
All reviewed patches have been landed. Closing bug.