Bug 77478 - [chromium] Disable root layer clears on release builds.
Summary: [chromium] Disable root layer clears on release builds.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Jonathan Backer
URL:
Keywords:
Depends on: 77667
Blocks:
  Show dependency treegraph
 
Reported: 2012-01-31 14:25 PST by Jonathan Backer
Modified: 2012-02-13 13:44 PST (History)
7 users (show)

See Also:


Attachments
Patch (2.77 KB, patch)
2012-01-31 14:26 PST, Jonathan Backer
no flags Details | Formatted Diff | Diff
Patch (3.99 KB, patch)
2012-02-03 07:46 PST, Jonathan Backer
no flags Details | Formatted Diff | Diff
Patch (719.14 KB, patch)
2012-02-06 12:45 PST, Jonathan Backer
no flags Details | Formatted Diff | Diff
Patch for landing (715.26 KB, patch)
2012-02-08 11:02 PST, Jonathan Backer
no flags Details | Formatted Diff | Diff
Patch for landing (717.63 KB, patch)
2012-02-08 11:10 PST, W. James MacLean
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Backer 2012-01-31 14:25:47 PST
[chromium] Disable root layer clears on select release builds.
Comment 1 Jonathan Backer 2012-01-31 14:26:46 PST
Created attachment 124821 [details]
Patch
Comment 2 Jonathan Backer 2012-01-31 14:29:14 PST
Chromium bug: http://crbug.com/111739

Related WebKit bug: https://bugs.webkit.org/show_bug.cgi?id=72578

Doing some perf measurements now. Will update with results.
Comment 3 Jonathan Backer 2012-01-31 14:41:12 PST
Some measurements on a simple scrolling test (vsync disabled).

With clears: 5121 ms / 257 frames = 19.9 ms
Without clears: 5071 ms / 278 frames = 18.2 ms

for a 10% speed improvement on a Samsung Chromebook.
Comment 4 Nat Duca 2012-01-31 14:44:01 PST
Comment on attachment 124821 [details]
Patch

s/clearBeforeComposite/clearRenderSurfaceBeforeDraw/?

Neat.
Comment 5 WebKit Review Bot 2012-01-31 15:09:32 PST
Comment on attachment 124821 [details]
Patch

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

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/direct-image-compositing.html
compositing/geometry/tall-page-composited.html
compositing/masks/simple-composited-mask.html
compositing/scaling/tiled-layer-recursion.html
Comment 6 Shawn Singh 2012-01-31 15:20:35 PST
Comment on attachment 124821 [details]
Patch

(1) we should probably also re-name clearSurfaceForDebug.   However, "clearSurface" worries me, since it may not actually clear the surface because of those new conditions...

(2) the old FIXME could be removed

otherwise looks good to me, too. =)
Comment 7 Jonathan Backer 2012-02-02 10:46:43 PST
(In reply to comment #5)
> (From update of attachment 124821 [details])
> Attachment 124821 [details] did not pass chromium-ews (chromium-xvfb):
> Output: http://queues.webkit.org/results/11393137
> 
> 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/direct-image-compositing.html
> compositing/geometry/tall-page-composited.html
> compositing/masks/simple-composited-mask.html
> compositing/scaling/tiled-layer-recursion.html

I dug into these: the rounded corners of the scroll bars on Linux have 1-2 pixel differences. I modified ImageDiff.cpp to give me a dump and we're off by a value of 1 in the blue channel.

This gave me a hunch, that we're baking the clear blue into the test expectations. To test this hypothesis, I moved to master and changed the clear blue to a clear black. The same set of tests failed in the same way --- 1-2 pixels on the scroll bar corners because we're off by 1 in the blue channel.
Comment 8 Dana Jansens 2012-02-02 12:36:49 PST
Its the skia off by one bug baked into the expectations again I think.
Comment 9 James Robinson 2012-02-02 16:14:02 PST
Have you run the layout tests with this patch on a mac? I'm more worried about weird transparency issues there than here.
Comment 10 Jonathan Backer 2012-02-03 07:12:16 PST
(In reply to comment #9)
> Have you run the layout tests with this patch on a mac? I'm more worried about weird transparency issues there than here.

I have run --platform=chromium-gpu layout tests on the Mac (with and without specifying LayoutTests/compositing) and I see no difference in output whether I enable root layer clears or disable them.

I have also tested on Windows. DRT is much flakier here (I'm getting a lot of CRASHES). But I am also not seeing differences.

I am going upload a version of the patch that disables root layer clears for all Release builds and see what the EWS bots say.
Comment 11 Jonathan Backer 2012-02-03 07:46:27 PST
Created attachment 125326 [details]
Patch
Comment 12 WebKit Review Bot 2012-02-03 08:20:11 PST
Comment on attachment 125326 [details]
Patch

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

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/direct-image-compositing.html
compositing/geometry/tall-page-composited.html
compositing/masks/simple-composited-mask.html
compositing/scaling/tiled-layer-recursion.html
Comment 13 Jonathan Backer 2012-02-03 08:31:41 PST
Du'oh. Just been informed that cr-linux is the only chromium EWS bot. I am now trying on Chromium bots mac_layout, win_layout, linux_layout.
Comment 14 Dana Jansens 2012-02-03 08:55:45 PST
You probably want mac_layout_rel, etc for release builders.
Comment 15 Jonathan Backer 2012-02-03 11:42:52 PST
No new failing layout tests on mac_layout_rel: http://build.chromium.org/p/tryserver.chromium/builders/mac_layout_rel/builds/187

Unfortunately, my compiles keep timing out on win_layout_rel.

How about rebaselining Linux for these scrollbars? When we resize a buffer in GLSurfaceOSMesa::Resize we memset to 0. So although we'll be backing in some error (from a Skia off-by-one alpha channel), we won't be introducing flake
Comment 16 Jonathan Backer 2012-02-06 12:45:16 PST
Created attachment 125694 [details]
Patch
Comment 17 James Robinson 2012-02-06 14:15:25 PST
Comment on attachment 125694 [details]
Patch

R=me on the code change, but please don't land the = IMAGE for WIN/MAC in test_expectations.txt
Comment 18 Jonathan Backer 2012-02-08 11:02:58 PST
Created attachment 126119 [details]
Patch for landing
Comment 19 WebKit Review Bot 2012-02-08 11:04:06 PST
Comment on attachment 126119 [details]
Patch for landing

Rejecting attachment 126119 [details] from commit-queue.

backer@chromium.org does not have committer permissions according to http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/committers.py.

- If you do not have committer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags.

- If you have committer rights please correct the error in Tools/Scripts/webkitpy/common/config/committers.py by adding yourself to the file (no review needed).  The commit-queue restarts itself every 2 hours.  After restart the commit-queue will correctly respect your committer rights.
Comment 20 W. James MacLean 2012-02-08 11:10:20 PST
Created attachment 126123 [details]
Patch for landing
Comment 21 WebKit Review Bot 2012-02-08 12:35:30 PST
Comment on attachment 126123 [details]
Patch for landing

Clearing flags on attachment: 126123

Committed r107120: <http://trac.webkit.org/changeset/107120>
Comment 22 WebKit Review Bot 2012-02-08 12:35:38 PST
All reviewed patches have been landed.  Closing bug.
Comment 23 Tony Chang 2012-02-13 12:29:36 PST
fast/canvas/canvas-text-alignment.html has been failing on Linux GPU debug after this change landed.  Looking at the flakiness dashboard, this is due to a couple pixels in the scrollbar being painted differently:
http://build.chromium.org/f/chromium/layout_test_results/Webkit_Linux__dbg__-_GPU/results/layout-test-results/fast/canvas/canvas-text-alignment-diff.png

http://test-results.appspot.com/dashboards/flakiness_dashboard.html#showExpectations=true&group=%40ToT%20GPU%20Mesa%20-%20chromium.org&tests=fast%2Fcanvas%2Fcanvas-text-alignment.html

Should I disable the test on Linux debug?
Comment 24 Jonathan Backer 2012-02-13 12:40:45 PST
(In reply to comment #23)
> fast/canvas/canvas-text-alignment.html has been failing on Linux GPU debug after this change landed.  Looking at the flakiness dashboard, this is due to a couple pixels in the scrollbar being painted differently:
> http://build.chromium.org/f/chromium/layout_test_results/Webkit_Linux__dbg__-_GPU/results/layout-test-results/fast/canvas/canvas-text-alignment-diff.png
> 
> http://test-results.appspot.com/dashboards/flakiness_dashboard.html#showExpectations=true&group=%40ToT%20GPU%20Mesa%20-%20chromium.org&tests=fast%2Fcanvas%2Fcanvas-text-alignment.html
> 
> Should I disable the test on Linux debug?

Thanks for tracking this down. Debug and Release builds now have slightly different behaviours. The difference in the scrollbars is a most likely an off by one Skia rendering bug. I think that jamesr@ moved to using mock scrollbars for the compositing layout tests to avoid this sort of problem.

@jamesr: Is it possible to use mock scrollbars for these tests as well?
Comment 25 James Robinson 2012-02-13 13:44:25 PST
(In reply to comment #24)
> (In reply to comment #23)
> > fast/canvas/canvas-text-alignment.html has been failing on Linux GPU debug after this change landed.  Looking at the flakiness dashboard, this is due to a couple pixels in the scrollbar being painted differently:
> > http://build.chromium.org/f/chromium/layout_test_results/Webkit_Linux__dbg__-_GPU/results/layout-test-results/fast/canvas/canvas-text-alignment-diff.png
> > 
> > http://test-results.appspot.com/dashboards/flakiness_dashboard.html#showExpectations=true&group=%40ToT%20GPU%20Mesa%20-%20chromium.org&tests=fast%2Fcanvas%2Fcanvas-text-alignment.html
> > 
> > Should I disable the test on Linux debug?
> 
> Thanks for tracking this down. Debug and Release builds now have slightly different behaviours. The difference in the scrollbars is a most likely an off by one Skia rendering bug. I think that jamesr@ moved to using mock scrollbars for the compositing layout tests to avoid this sort of problem.
> 
> @jamesr: Is it possible to use mock scrollbars for these tests as well?

We could enable mock scrollbars for everything in the GPU config, but that means that we can't share results at all for the GPU and CPU configurations which I think is pretty unfortunate.

I'd suggest marking the test as failing with IMAGE but only for LINUX DEBUG GPU.