Bug 76805

Summary: [chromium] Add support to force full damage in CCDamageTracker
Product: WebKit Reporter: Shawn Singh <shawnsingh>
Component: Layout and RenderingAssignee: Shawn Singh <shawnsingh>
Status: RESOLVED FIXED    
Severity: Normal CC: backer, cc-bugs, jamesr, mmocny, nduca, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 76668, 77555    
Attachments:
Description Flags
Will add unit test if this change looks OK
none
Patch with unit test none

Description Shawn Singh 2012-01-22 20:43:09 PST
Steps to reproduce the issue on linux:

1. build osmesa target in addition to chromium
2. run chromium with the following flags: --use-gl=osmesa --enable-partial-swap
3. navigate to the poster circle css demo, which should trigger accelerated compositing
4. open a new tab
5. switch back to the poster circle demo.

Because of partial swap, there will be leftover junk from the previous tab.  The solution is that tab switching should force-damage the entire viewport.
Comment 1 Shawn Singh 2012-01-22 20:48:07 PST
Created attachment 123513 [details]
Will add unit test if this change looks OK
Comment 2 James Robinson 2012-01-22 20:52:14 PST
Do we always lose our frontbuffer when switching tabs?
Comment 3 Shawn Singh 2012-01-22 20:55:49 PST
(In reply to comment #2)
> Do we always lose our frontbuffer when switching tabs?

Good point - 

There is a boolean layerRendererCapabilities().contextHasCachedFrontBuffer

However, that's not enough to assume the front buffer is still cached, it may have been evicted, right?

I did not see any other hook that currently exists on the Impl thread to detect if the front buffer was lost... can you please point me to it, if it exists?
Comment 4 Jonathan Backer 2012-01-23 05:26:33 PST
> However, that's not enough to assume the front buffer is still cached, it may have been evicted, right?

mmocny@ is working on evicting front buffers. Currently they are never evicted. So it's safe to use |layerRendererCapabilities().contextHasCachedFrontBuffer|. 

mmocny@ is plumbing a signal to the renderer to drop the front buffer (which the renderer will forward to the GPU process). So we should be able to invalidate. I've CC'ed him so that he knows this is an issue.

 
> I did not see any other hook that currently exists on the Impl thread to detect if the front buffer was lost... can you please point me to it, if it exists?

Not yet. mmocny's WIP is here: https://bugs.webkit.org/show_bug.cgi?id=76634
Comment 5 Nat Duca 2012-01-23 10:21:13 PST
Comment on attachment 123513 [details]
Will add unit test if this change looks OK

View in context: https://bugs.webkit.org/attachment.cgi?id=123513&action=review

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:345
> +        rootLayer()->renderSurface()->damageTracker()->forceFullDamageNextUpdate();

Its not clear to me that the damage tracker should understand this concept? It seems awefully like "taking a damage tracker and bringing it back to its initial state."

At which point, why not just delete the damage tracker and recreate it? E.g. renderSurface()->resetDamageTracker()? Or delete the rendersurface and recreate it?

E.g. where reset is deleting the old damage tracker and creating a new one?

Or, for that matter, delete the rendersurface? And then make sure it gets created on the next update?

I thought the point of putting damage trackers on rendersurfaces was for this sort of thing.

Using the layerrenderercaps structure for now makes sense on setVisible(false)
Comment 6 Shawn Singh 2012-02-03 15:46:39 PST
Changing name of this bug, to reflect the new purpose of this bug.  Patch for review will be coming shortly.

The original tab-switch issue will be fixed by two patches: this one, followed by https://bugs.webkit.org/show_bug.cgi?id=77555
Comment 7 Shawn Singh 2012-02-03 15:59:36 PST
Created attachment 125442 [details]
Patch with unit test

By the way, Nat and I discussed offline, and we agreed this approach is reasonable.
Comment 8 James Robinson 2012-02-03 17:19:47 PST
Comment on attachment 125442 [details]
Patch with unit test

R=me
Comment 9 WebKit Review Bot 2012-02-04 22:14:07 PST
Comment on attachment 125442 [details]
Patch with unit test

Clearing flags on attachment: 125442

Committed r106754: <http://trac.webkit.org/changeset/106754>
Comment 10 WebKit Review Bot 2012-02-04 22:14:28 PST
All reviewed patches have been landed.  Closing bug.