Bug 72578 - [Chromium] Add flag to allow bypassing root surface clear
Summary: [Chromium] Add flag to allow bypassing root surface clear
Status: RESOLVED WONTFIX
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Daniel Sievers
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-11-16 18:58 PST by Daniel Sievers
Modified: 2013-04-12 07:24 PDT (History)
10 users (show)

See Also:


Attachments
Patch (2.30 KB, patch)
2011-11-16 19:00 PST, Daniel Sievers
no flags Details | Formatted Diff | Diff
Patch (2.38 KB, patch)
2011-11-16 19:05 PST, Daniel Sievers
no flags Details | Formatted Diff | Diff
Patch (3.36 KB, patch)
2011-12-02 18:18 PST, Daniel Sievers
no flags Details | Formatted Diff | Diff
Patch (3.37 KB, patch)
2011-12-02 19:41 PST, Daniel Sievers
jamesr: review-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Daniel Sievers 2011-11-16 18:58:25 PST
[Chromium] Restrict clearing render surface on each frame to debug builds
Comment 1 Daniel Sievers 2011-11-16 19:00:17 PST
Created attachment 115513 [details]
Patch
Comment 2 Daniel Sievers 2011-11-16 19:05:42 PST
Created attachment 115514 [details]
Patch
Comment 3 WebKit Review Bot 2011-11-17 04:41:01 PST
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
Comment 4 Sami Kyostila 2011-11-29 12:30:48 PST
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
Comment 5 James Robinson 2011-11-29 16:27:06 PST
(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?
Comment 6 Nat Duca 2011-12-02 11:16:34 PST
(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.
Comment 7 Sami Kyostila 2011-12-02 11:30:21 PST
(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.
Comment 8 James Robinson 2011-12-02 12:48:16 PST
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.
Comment 9 Nat Duca 2011-12-02 15:38:28 PST
(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.
Comment 10 Daniel Sievers 2011-12-02 18:18:15 PST
Created attachment 117726 [details]
Patch
Comment 11 Daniel Sievers 2011-12-02 18:19:36 PST
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.
Comment 12 James Robinson 2011-12-02 18:21:20 PST
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.
Comment 13 Nat Duca 2011-12-02 18:25:21 PST
(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.
Comment 14 Nat Duca 2011-12-02 18:30:12 PST
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."
Comment 15 Antoine Labour 2011-12-02 18:48:25 PST
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?
Comment 16 Nat Duca 2011-12-02 18:50:38 PST
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.
Comment 17 Nat Duca 2011-12-02 18:51:23 PST
(In reply to comment #16)
> Set it from WebViewImpl and WebCompositorImpl.
Even better, set the optimization level in CCSettings constructor
Comment 18 Antoine Labour 2011-12-02 18:58:10 PST
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.
Comment 19 Daniel Sievers 2011-12-02 19:41:13 PST
Created attachment 117735 [details]
Patch
Comment 20 Nat Duca 2011-12-02 20:29:47 PST
(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.
Comment 21 Nat Duca 2012-01-04 11:29:24 PST
Where do we stand on this bug?
Comment 22 James Robinson 2012-02-21 20:23:03 PST
Comment on attachment 117735 [details]
Patch

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