Bug 76805 – [chromium] Add support to force full damage in CCDamageTracker

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Shawn Singh]

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.

[Shawn Singh]

Created attachment 123513 [details]
Will add unit test if this change looks OK

[James Robinson]

Do we always lose our frontbuffer when switching tabs?

[Shawn Singh]

(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?

[Jonathan Backer]

> 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

[Nat Duca]

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)

[Shawn Singh]

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

[Shawn Singh]

Created attachment 125442 [details]
Patch with unit test

By the way, Nat and I discussed offline, and we agreed this approach is reasonable.

[James Robinson]

Comment on attachment 125442 [details]
Patch with unit test

R=me

[WebKit Review Bot]

Comment on attachment 125442 [details]
Patch with unit test

Clearing flags on attachment: 125442

Committed r106754: <http://trac.webkit.org/changeset/106754>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.