Bug 77478 – [chromium] Disable root layer clears on release builds.

• Component: New Bugs
• Status: RESOLVED FIXED

[Jonathan Backer]

[chromium] Disable root layer clears on select release builds.

[Jonathan Backer]

Created attachment 124821 [details]
Patch

[Jonathan Backer]

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.

[Jonathan Backer]

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.

[Nat Duca]

Comment on attachment 124821 [details]
Patch

s/clearBeforeComposite/clearRenderSurfaceBeforeDraw/?

Neat.

[WebKit Review Bot]

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

[Shawn Singh]

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. =)

[Jonathan Backer]

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

[Dana Jansens]

Its the skia off by one bug baked into the expectations again I think.

[James Robinson]

Have you run the layout tests with this patch on a mac? I'm more worried about weird transparency issues there than here.

[Jonathan Backer]

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

[Jonathan Backer]

Created attachment 125326 [details]
Patch

[WebKit Review Bot]

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

[Jonathan Backer]

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.

[Dana Jansens]

You probably want mac_layout_rel, etc for release builders.

[Jonathan Backer]

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

[Jonathan Backer]

Created attachment 125694 [details]
Patch

[James Robinson]

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

[Jonathan Backer]

Created attachment 126119 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 126119 [details]
Patch for landing

Rejecting attachment 126119 [details] from commit-queue.

[e-mail] 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.

[W. James MacLean]

Created attachment 126123 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 126123 [details]
Patch for landing

Clearing flags on attachment: 126123

Committed r107120: <http://trac.webkit.org/changeset/107120>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Tony Chang]

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?

[Jonathan Backer]

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

[James Robinson]

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