Bug 80199

Summary: [chromium] Make compositeAndReadback and damage tracking play nicely together
Product: WebKit Reporter: Shawn Singh <shawnsingh>
Component: Layout and RenderingAssignee: Shawn Singh <shawnsingh>
Severity: Normal CC: backer, cc-bugs, jamesr, nduca, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 76668    
Description Flags
contextLost() check placed before swap none

Description Shawn Singh 2012-03-02 15:40:08 PST
Issue reported in: http://code.google.com/p/chromium/issues/detail?id=116055

compositeAndReadback causes the composited rendering path to proceed, including damage tracking, but in single-threaded mode, compositeAndReadback does not actually affect the front buffer (which is the purpose of damage tracking).  As a result, damage tracking gets out-of-sync with what really changed on the front buffer, and some repaints do not actually show up on the screen when partial swap is enabled.

Jonathan, when you have a chance, can you please try to reproduce with --enable-threaded-compositing?  (remember, HUD and rect visualization won't show up in multi threaded mode yet)  The error should be un-reproducible, because as I understand, swapBuffers is actually performed on compositeAndReadback in multi-threaded mode.  In my own linux osmesa attempts, I could not reproduce the problem in multi-threaded mode, and I could not reproduce the problem when I hacked a swapBuffers in single-threaded mode.

There are 3 options I see to fix this problem.  Nat can you please suggest which fix you think is best?  I think option #3 is correct, and the other two are hacky.  Thanks in advance.
  1. do a swapBuffers on compositeAndReadback for single threaded mode.
  2. bypass damage tracking in composite-and-readback single threaded mode.
  3. (seems like the right thing to do...) change damage tracking to *accumulate* dirtiness until swapbuffers is actually performed.  Eventually, when we can support per-surface scissoring, we can reset the dirty-accumulation after each surface is updated.

Let me know then I will proceed ... thanks!
Comment 1 Shawn Singh 2012-03-06 13:51:10 PST
Created attachment 130436 [details]

The fix is trivial, but testing it is not so obvious.  Testing this fix requires observing front-buffer changes. As I understand, this could be done via chromium browser gpu pixel tests, but it won't happen in layout tests or webkit_unit_tests.  Is it worth going through that trouble?  ... also, I think context lost issues are already addressed by the existing code in compositeAndReadback (exits early if doComposite returns false).
Comment 2 Shawn Singh 2012-03-06 13:55:32 PST
For clarity - Nat and I had discussed offline, and for now we're considering option #1.  Thanks
Comment 3 Nat Duca 2012-03-06 13:57:57 PST
Comment on attachment 130436 [details]

LGTM but copy the check for context lost at :85 up to :82 before calling swap. That has an assertion inside it that will pop if you call it with a lost context.
Comment 4 Shawn Singh 2012-03-06 14:14:15 PST
Created attachment 130442 [details]
contextLost() check placed before swap
Comment 5 James Robinson 2012-03-06 14:43:16 PST
Comment on attachment 130442 [details]
contextLost() check placed before swap

Seems good, R=me
Comment 6 Shawn Singh 2012-03-06 14:50:42 PST
Comment on attachment 130442 [details]
contextLost() check placed before swap

Cool, thanks for reviewing.
Comment 7 WebKit Review Bot 2012-03-06 15:09:54 PST
Comment on attachment 130442 [details]
contextLost() check placed before swap

Clearing flags on attachment: 130442

Committed r109965: <http://trac.webkit.org/changeset/109965>
Comment 8 WebKit Review Bot 2012-03-06 15:09:58 PST
All reviewed patches have been landed.  Closing bug.