Bug 80199 - [chromium] Make compositeAndReadback and damage tracking play nicely together
Summary: [chromium] Make compositeAndReadback and damage tracking play nicely together
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Shawn Singh
URL:
Keywords:
Depends on:
Blocks: 76668
  Show dependency treegraph
 
Reported: 2012-03-02 15:40 PST by Shawn Singh
Modified: 2012-03-06 15:09 PST (History)
5 users (show)

See Also:


Attachments
Patch (1.62 KB, patch)
2012-03-06 13:51 PST, Shawn Singh
no flags Details | Formatted Diff | Diff
contextLost() check placed before swap (1.69 KB, patch)
2012-03-06 14:14 PST, Shawn Singh
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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]
Patch

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]
Patch

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.