RESOLVED FIXED 79841
[chromium] Don't let invalidation for next frame prevent tile upload 
https://bugs.webkit.org/show_bug.cgi?id=79841
Summary [chromium] Don't let invalidation for next frame prevent tile upload
Dana Jansens
Reported 2012-02-28 15:33:28 PST
[chromium] Don't let invalidation for next frame prevent tile upload
Attachments
Patch (3.02 KB, patch)
2012-02-28 15:34 PST, Dana Jansens 		no flags
DetailsFormatted DiffDiff
Patch (7.66 KB, patch)
2012-02-28 17:49 PST, Dana Jansens 		no flags
DetailsFormatted DiffDiff
Patch (11.01 KB, patch)
2012-02-29 07:20 PST, Dana Jansens 		no flags
DetailsFormatted DiffDiff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.
Dana Jansens
Comment 1 2012-02-28 15:34:00 PST
Created attachment 129344 [details] Patch
Dana Jansens
Comment 2 2012-02-28 15:34:23 PST
I'll make tests if you like this idea.
James Robinson
Comment 3 2012-02-28 15:39:54 PST
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
Dana Jansens
Comment 4 2012-02-28 15:50:04 PST
@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.
Dana Jansens
Comment 5 2012-02-28 16:28:11 PST
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.
David Reveman
Comment 6 2012-02-28 16:29:07 PST
Could we check tile->m_updateRect in pushPropertiesTo instead?
Dana Jansens
Comment 7 2012-02-28 16:43:36 PST
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).
Dana Jansens
Comment 8 2012-02-28 16:47:55 PST
(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.
Dana Jansens
Comment 9 2012-02-28 17:49:31 PST
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).
Adrienne Walker
Comment 10 2012-02-28 18:04:50 PST
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.
David Reveman
Comment 11 2012-02-28 19:39:31 PST
Thanks for the background! Latest patch looks good to me.
Dana Jansens
Comment 12 2012-02-29 07:20:50 PST
Created attachment 129447 [details] Patch Added some tests for invalidation on layers other than the one being painted
James Robinson
Comment 13 2012-02-29 11:24:10 PST
Comment on attachment 129447 [details] Patch R=me
WebKit Review Bot
Comment 14 2012-02-29 14:35:08 PST
Comment on attachment 129447 [details] Patch Clearing flags on attachment: 129447 Committed r109263: <http://trac.webkit.org/changeset/109263>
WebKit Review Bot
Comment 15 2012-02-29 14:35:16 PST
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.