Summary: | [Chromium] Calls to paintContentsIfDirty() and updateCompositorResources() should be balanced. | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | David Reveman <reveman> | ||||||||
Component: | WebCore Misc. | Assignee: | David Reveman <reveman> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | cc-bugs, enne, jamesr, nduca, webkit.review.bot | ||||||||
Priority: | P2 | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
Bug Depends on: | |||||||||||
Bug Blocks: | 71388 | ||||||||||
Attachments: |
|
Description
David Reveman
2011-11-17 10:15:32 PST
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. |