Bug 72578 – [Chromium] Add flag to allow bypassing root surface clear

• Component: New Bugs
• Status: RESOLVED WONTFIX

[Daniel Sievers]

[Chromium] Restrict clearing render surface on each frame to debug builds

[Daniel Sievers]

Created attachment 115513 [details]
Patch

[Daniel Sievers]

Created attachment 115514 [details]
Patch

[WebKit Review Bot]

Comment on attachment 115514 [details]
Patch

Attachment 115514 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10312137

New failing tests:
platform/chromium/compositing/layout-width-change.html
compositing/geometry/fixed-in-composited.html
compositing/masks/multiple-masks.html
compositing/masks/masked-ancestor.html
compositing/geometry/ancestor-overflow-change.html
compositing/geometry/video-fixed-scrolling.html
compositing/direct-image-compositing.html
compositing/geometry/tall-page-composited.html
compositing/masks/simple-composited-mask.html

[Sami Kyostila]

We should also use EXT_discard_framebuffer where available to make the "clear" even more efficient:
    http://www.khronos.org/registry/gles/extensions/EXT/EXT_discard_framebuffer.txt

[James Robinson]

(In reply to comment #4)
> We should also use EXT_discard_framebuffer where available to make the "clear" even more efficient:
>     http://www.khronos.org/registry/gles/extensions/EXT/EXT_discard_framebuffer.txt

Do you know where this is implemented?

[Nat Duca]

(In reply to comment #5)
> We should also use EXT_discard_framebuffer where available to make the "clear" even more efficient:

We should make that a separate bug.

Lets get this patch landed.

[Sami Kyostila]

(In reply to comment #6)
> (In reply to comment #5)
> > We should also use EXT_discard_framebuffer where available to make the "clear" even more efficient:
> 
> We should make that a separate bug.
> 
> Lets get this patch landed.

Definitely agreed, that's a future improvement and not a blocker for this one.

I've opened https://bugs.webkit.org/show_bug.cgi?id=73679.

[James Robinson]

Comment on attachment 115514 [details]
Patch

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

We still want the clear to blue to happen for release builds since we're getting reports from the wild where we fail to draw parts of the screen and blue is a lot easier to debug than corruption.

Maybe instead we make this a per-platform setting, so we can leave it on for desktop platforms while we chase these issues down?

> Source/WebCore/ChangeLog:9
> +        Reviewed by James Robinson.

This is wrong, please take it out. I haven't review+'d this patch.

[Nat Duca]

(In reply to comment #8)
> (From update of attachment 115514 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=115514&action=review
> 
> We still want the clear to blue to happen for release builds since we're getting reports from the wild where we fail to draw parts of the screen and blue is a lot easier to debug than corruption.
> 
> Maybe instead we make this a per-platform setting, so we can leave it on for desktop platforms while we chase these issues down?

Lets do this and kill a few birds with one stone:

enum OptimizationLevel {
  OPTIMIZATION_LEVEL_0 = 0,    // default
  OPTIMIZATION_LEVEL_1 = 1,    // cros
  OPTIMIZATION_LEVEL_2 = 2,    // android
}

Put an OptimizationLevel on CCSettings. Use that to drive the clears.

[Daniel Sievers]

Created attachment 117726 [details]
Patch

[Daniel Sievers]

What about keeping it more specific for now as in the attached patch?

(In reply to comment #9)
> (In reply to comment #8)
> > (From update of attachment 115514 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=115514&action=review
> > 
> > We still want the clear to blue to happen for release builds since we're getting reports from the wild where we fail to draw parts of the screen and blue is a lot easier to debug than corruption.
> > 
> > Maybe instead we make this a per-platform setting, so we can leave it on for desktop platforms while we chase these issues down?
> 
> Lets do this and kill a few birds with one stone:
> 
> enum OptimizationLevel {
>   OPTIMIZATION_LEVEL_0 = 0,    // default
>   OPTIMIZATION_LEVEL_1 = 1,    // cros
>   OPTIMIZATION_LEVEL_2 = 2,    // android
> }
> 
> Put an OptimizationLevel on CCSettings. Use that to drive the clears.

[James Robinson]

Comment on attachment 117726 [details]
Patch

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

> Source/WebCore/ChangeLog:9
> +        Reviewed by <your name here>.

Just leave this line alone. The script doesn't know what to do with this format, it looks very specifically for the string generated by the tools which is "Reviewed by NOBODY (OOPS!)" Without that line we can't use the commit queue to land this patch.

[Nat Duca]

(In reply to comment #11)
> What about keeping it more specific for now as in the attached patch?
Because that's too specific. CCSettings is our public api for settings. At the end of the day, I do not see a point in propagating details about our optimizaiton levels all the way out to webkit or to chromium. From the point of the end user, 

Please please, just an optimization level? The 0,1,2 is intentional because I'm going to set kmaxswaps to 1 for 0 and 2 for 1+. You'll set clear for 2 only. It works.

[Nat Duca]

Correcting some typos:
> I do not see a point in propagating details about our optimizaiton algorithms all the way out to webkit or to chromium. From the point of the end user, all this complexity boils down to "i'm on a desktop, netbook or laptop." That's 0, 1, and 2. And that gives us a ton of flexibility internally to "do the right thing."

[Antoine Labour]

Comment on attachment 117726 [details]
Patch

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

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.h:85
> +    static const bool clearRootSurface;

I personally don't like to have this static. Statics always end up biting me.
What if I want a different setting for the UI vs the web contents?

[Nat Duca]

Comment on attachment 117726 [details]
Patch

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

>> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.h:85
>> +    static const bool clearRootSurface;
> 
> I personally don't like to have this static. Statics always end up biting me.
> What if I want a different setting for the UI vs the web contents?

Why is this static? This should be just straight up bool.
Set it from WebViewImpl and WebCompositorImpl.

[Nat Duca]

(In reply to comment #16)
> Set it from WebViewImpl and WebCompositorImpl.
Even better, set the optimization level in CCSettings constructor

[Antoine Labour]

Also, I don't think we can make a straight up decision clear vs not clear based on a linear "optimization level".
Tiling architectures will benefit from a clear. Some traditional architecture also benefit from a clear because it resets acceleration hardware (e.g. frame buffer compression). But some don't (it sucks bandwidth). It's fairly orthogonal to the raw speed of the GPU.

[Daniel Sievers]

Created attachment 117735 [details]
Patch

[Nat Duca]

(In reply to comment #18)
> Also, I don't think we can make a straight up decision clear vs not clear based on a linear "optimization level".
> Tiling architectures will benefit from a clear. Some traditional architecture also benefit from a clear because it resets acceleration hardware (e.g. frame buffer compression). But some don't (it sucks bandwidth). It's fairly orthogonal to the raw speed of the GPU.

Sure, in the long term, you're right. But, we don't have the code maturity for a renderer-string-based probe either. I think for now we can get by establishing some basic precedent that the per-optimization determinations are internal and embedders just give broad hints. We need this for memory managment too. In the long term, I suspect we're headed toward yet-another "device->settings" table based on probing. :/

I'll also keep my eyes on android perf and do something smarter if I see render times drop.

[Nat Duca]

Where do we stand on this bug?

[James Robinson]

Comment on attachment 117735 [details]
Patch

The root layer clear is disabled on ToT in release builds, so I think this is unnecessary now.