Bug 79841

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:

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

Attachments:

Description	Flags
Patch	none
Patch	none
Patch	none

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	Details Formatted Diff Diff
Patch (7.66 KB, patch) 2012-02-28 17:49 PST, Dana Jansens	no flags	Details Formatted Diff Diff
Patch (11.01 KB, patch) 2012-02-29 07:20 PST, Dana Jansens	no flags	Details Formatted Diff Diff
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.