| Summary: | [chromium] Don't let invalidation for next frame prevent tile upload | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Dana Jansens <danakj> | ||||||||
| Component: | New Bugs | Assignee: | Dana Jansens <danakj> | ||||||||
| Status: | RESOLVED FIXED | ||||||||||
| Severity: | Normal | CC: | backer, cc-bugs, enne, jamesr, piman, reveman, shawnsingh, vangelis, webkit.review.bot | ||||||||
| Priority: | P2 | ||||||||||
| Version: | 528+ (Nightly build) | ||||||||||
| Hardware: | Unspecified | ||||||||||
| OS: | Unspecified | ||||||||||
| Attachments: |
|
||||||||||
|
Description
Dana Jansens
2012-02-28 15:33:28 PST
Created attachment 129344 [details]
Patch
I'll make tests if you like this idea. Comment on attachment 129344 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=129344&action=review I think this approach will work well. Definitely want tests > Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:78 > + m_dirtyRect.unite(m_nextFrameDirtyRect); should be safe to just be m_dirtyRect = m_nextFrame... right? m_dirtyRect should always be empty here unless some other layer has damaged us maybe ASSERT() and unite() just to be careful @reveman seems okay for threaded compositor/async commit/etc? We're assuming WebKit will only ever invalidate the layer it is painting, not other layers. The following code seems to show that WebKit can and will invalidate a different layer than it is currently painting (tested with asserts):
<!DOCTYPE html>
<html lang="en_US">
<head>
<title>Missing tiles</title>
</head>
<body style="background-color:red; margin:0; overflow:hidden">
<div style="background-color:white;">
<img src="http://www.google.com/logos/2012/hertz-2011-hp.gif"/>
</div>
<div style="background-color:white; height:1000px; -webkit-transform:translateZ(0)">
<img src="http://www.google.com/logos/2012/hertz-2011-hp.gif"/>
</div>
</body>
</html>
I save the layer being painted before calling prepareToUpdate, and in TLC::invalidateRect(), I asserted that the layer being painted is NULL or equal to the layer being invalidated. This assert fails.
Could we check tile->m_updateRect in pushPropertiesTo instead? For background: The problem currently is that a layer may have tiles that are dirty but not visible. These tiles may have arbitrary contents in them. If the layout were to change and the tiles became visible, we don't want them to exist in the impl thread before they are repainted. Thus there is a check on tile->isDirty() that prevents these tiles from existing on impl thread. However, it assumes that tiles are never dirty after they have been painted. But WebKit can set dirty on any layer during the paint of any other layer (including itself). (In reply to comment #6) > Could we check tile->m_updateRect in pushPropertiesTo instead? m_updateRect does not differentiate between tiles that were not painted because they had valid contents, and tiles that were not painted because they were not visible. Created attachment 129364 [details]
Patch
Testing m_updateRect along with m_dirtyRect works, as long as we reset m_updateRect every frame (not just when >= 1 tile ends up getting painted).
Comment on attachment 129364 [details]
Patch
Unofficially looks good to me. Nice work finding a way to determine if a tile is dirty this frame without using any extra memory to track it.
Thanks for the background! Latest patch looks good to me. Created attachment 129447 [details]
Patch
Added some tests for invalidation on layers other than the one being painted
Comment on attachment 129447 [details]
Patch
R=me
Comment on attachment 129447 [details] Patch Clearing flags on attachment: 129447 Committed r109263: <http://trac.webkit.org/changeset/109263> All reviewed patches have been landed. Closing bug. |
