Bug 70748 – [skia] replace offscreen technique with native support for antialiased clipping

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Mike Reed]

[skia] replace offscreen technique with native support for antialiased clipping

[Mike Reed]

Created attachment 112222 [details]
Patch

[Mike Reed]

Created attachment 112226 [details]
Patch

[WebKit Review Bot]

Comment on attachment 112226 [details]
Patch

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

New failing tests:
fast/borders/only-one-border-with-width.html
fast/backgrounds/gradient-background-leakage.html
fast/replaced/border-radius-clip.html
fast/backgrounds/background-leakage.html
platform/gtk/fast/css/rect-shadow-tiled.html
fast/box-shadow/shadow-tiling-artifact.html
svg/W3C-SVG-1.1/masking-path-05-f.svg

[Mike Reed]

Created attachment 112239 [details]
Patch

[WebKit Review Bot]

Comment on attachment 112239 [details]
Patch

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

New failing tests:
fast/borders/only-one-border-with-width.html
fast/replaced/border-radius-clip.html
fast/backgrounds/background-leakage.html
platform/gtk/fast/css/rect-shadow-tiled.html
fast/box-shadow/shadow-tiling-artifact.html
svg/W3C-SVG-1.1/masking-path-05-f.svg

[Mike Reed]

trying to repro the crashes...

[Mike Reed]

Created attachment 112390 [details]
Patch

[Mike Reed]

Created attachment 112401 [details]
Patch

[Stephen White]

Comment on attachment 112401 [details]
Patch

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

> LayoutTests/platform/chromium/test_expectations.txt:1117
> +BUGWK70748 WIN LINUX : svg/W3C-SVG-1.1/masking-path-05-f.svg = IMAGE
> +

Won't these also fail on MAC (since it's now Skia)?

[Mike Reed]

mac-skia is not on yet, and when it it, it will have a separate line for each of these, since it will likely get rebaselined at a different time than linux,win. Cary/Elliot are aware of this change, and will manage it when they reenable mac-skia.

[Stephen White]

(In reply to comment #10)
> mac-skia is not on yet, and when it it, it will have a separate line for each of these, since it will likely get rebaselined at a different time than linux,win. Cary/Elliot are aware of this change, and will manage it when they reenable mac-skia.

Although Chrome is not using skia yet, the Mac WebKit canaries are running with Skia enabled, and it's my understanding that the gardeners are attempting to keep them green.  The last time I let a change which broke them, there were complaints to that effect.  I think you need to suppress the failures on Mac as well.  

Other than that, this change looks fine, assuming the underlying Skia code is solid.

[Mike Reed]

Created attachment 112496 [details]
Patch

[Mike Reed]

changed expectations from WIN LINUX to CPU, which should cover windows, linux, and skia-mac, but not cg-mac

[Adam Langley]

From a brief look at the patch I would have expected PlatformContextSkia::applyAntiAliasedClipPaths (and calls to it) to also be removed.

[Mike Reed]

Created attachment 112500 [details]
Patch

[Mike Reed]

good catch. applyAntiAliasedClipPaths removed.

[Adam Langley]

LGTM (Note: I am not a WebKit reviewer. You need an r+ from a real WK reviewer before landing.)

[Stephen White]

Comment on attachment 112500 [details]
Patch

I didn't know that MAC-CG was excluded from CPU.  Strange, but ok.  r=me

(In reply to comment #18)
> (From update of attachment 112500 [details])
> I didn't know that MAC-CG was excluded from CPU.  Strange, but ok.  r=me

Yup, on Mac, "CPU" means CPU-Skia (as opposed to "CPU-CG", which means CG).  Similar for GPU / GPU-CG.

The sordid history of this can be found in https://bugs.webkit.org/show_bug.cgi?id=66705

[WebKit Review Bot]

Comment on attachment 112500 [details]
Patch

Clearing flags on attachment: 112500

Committed r98486: <http://trac.webkit.org/changeset/98486>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

I think this patch broke lint checks on test_expectations.  I'm surprised this didn't turn red in the style trybot...


/Volumes/MacintoshHD2/epoger/src/webkit/lion-timeouts/WebKit$ Tools/Scripts/new-run-webkit-tests --chromium --lint
FAILURES FOR <leopard, x86, debug, cpu>
Line:2682 Entries on line 1048 and line 2682 match overlapping sets of configurations fast/borders/border-radius-split-inline.html
Line:3104 Entries on line 1077 and line 3104 match overlapping sets of configurations fast/transforms/shadows.html
Line:3773 More specific entry on line 3773 overrides line 1085 media/audio-repaint.html

FAILURES FOR <leopard, x86, release, cpu>
Line:2682 Entries on line 1048 and line 2682 match overlapping sets of configurations fast/borders/border-radius-split-inline.html
Line:3104 Entries on line 1077 and line 3104 match overlapping sets of configurations fast/transforms/shadows.html

FAILURES FOR <snowleopard, x86, debug, cpu>
Line:3104 Entries on line 1077 and line 3104 match overlapping sets of configurations fast/transforms/shadows.html
Line:3773 More specific entry on line 3773 overrides line 1085 media/audio-repaint.html

FAILURES FOR <snowleopard, x86, release, cpu>
Line:3104 Entries on line 1077 and line 3104 match overlapping sets of configurations fast/transforms/shadows.html

FAILURES FOR <lion, x86, debug, cpu>
Line:3104 Entries on line 1077 and line 3104 match overlapping sets of configurations fast/transforms/shadows.html
Line:3773 More specific entry on line 3773 overrides line 1085 media/audio-repaint.html

FAILURES FOR <lion, x86, release, cpu>
Line:3104 Entries on line 1077 and line 3104 match overlapping sets of configurations fast/transforms/shadows.html

FAILURES FOR <xp, x86, debug, cpu>
Line:2305 Entries on line 1073 and line 2305 match overlapping sets of configurations fast/repaint/shadow-multiple-horizontal.html
Line:2306 Entries on line 1074 and line 2306 match overlapping sets of configurations fast/repaint/shadow-multiple-strict-horizontal.html
Line:2307 Entries on line 1075 and line 2307 match overlapping sets of configurations fast/repaint/shadow-multiple-strict-vertical.html
Line:2308 Entries on line 1076 and line 2308 match overlapping sets of configurations fast/repaint/shadow-multiple-vertical.html
Line:2311 Entries on line 1077 and line 2311 match overlapping sets of configurations fast/transforms/shadows.html
Line:3772 Entries on line 1085 and line 3772 match overlapping sets of configurations media/audio-repaint.html

FAILURES FOR <xp, x86, release, cpu>
Line:1522 Entries on line 1085 and line 1522 match overlapping sets of configurations media/audio-repaint.html
Line:2305 Entries on line 1073 and line 2305 match overlapping sets of configurations fast/repaint/shadow-multiple-horizontal.html
Line:2306 Entries on line 1074 and line 2306 match overlapping sets of configurations fast/repaint/shadow-multiple-strict-horizontal.html
Line:2307 Entries on line 1075 and line 2307 match overlapping sets of configurations fast/repaint/shadow-multiple-strict-vertical.html
Line:2308 Entries on line 1076 and line 2308 match overlapping sets of configurations fast/repaint/shadow-multiple-vertical.html
Line:2311 Entries on line 1077 and line 2311 match overlapping sets of configurations fast/transforms/shadows.html

FAILURES FOR <vista, x86, debug, cpu>
Line:2305 Entries on line 1073 and line 2305 match overlapping sets of configurations fast/repaint/shadow-multiple-horizontal.html
Line:2306 Entries on line 1074 and line 2306 match overlapping sets of configurations fast/repaint/shadow-multiple-strict-horizontal.html
Line:2307 Entries on line 1075 and line 2307 match overlapping sets of configurations fast/repaint/shadow-multiple-strict-vertical.html
Line:2308 Entries on line 1076 and line 2308 match overlapping sets of configurations fast/repaint/shadow-multiple-vertical.html
Line:2311 Entries on line 1077 and line 2311 match overlapping sets of configurations fast/transforms/shadows.html
Line:3772 Entries on line 1085 and line 3772 match overlapping sets of configurations media/audio-repaint.html

FAILURES FOR <vista, x86, release, cpu>
Line:1522 Entries on line 1085 and line 1522 match overlapping sets of configurations media/audio-repaint.html
Line:2305 Entries on line 1073 and line 2305 match overlapping sets of configurations fast/repaint/shadow-multiple-horizontal.html
Line:2306 Entries on line 1074 and line 2306 match overlapping sets of configurations fast/repaint/shadow-multiple-strict-horizontal.html
Line:2307 Entries on line 1075 and line 2307 match overlapping sets of configurations fast/repaint/shadow-multiple-strict-vertical.html
Line:2308 Entries on line 1076 and line 2308 match overlapping sets of configurations fast/repaint/shadow-multiple-vertical.html
Line:2311 Entries on line 1077 and line 2311 match overlapping sets of configurations fast/transforms/shadows.html

FAILURES FOR <win7, x86, debug, cpu>
Line:2305 Entries on line 1073 and line 2305 match overlapping sets of configurations fast/repaint/shadow-multiple-horizontal.html
Line:2306 Entries on line 1074 and line 2306 match overlapping sets of configurations fast/repaint/shadow-multiple-strict-horizontal.html
Line:2307 Entries on line 1075 and line 2307 match overlapping sets of configurations fast/repaint/shadow-multiple-strict-vertical.html
Line:2308 Entries on line 1076 and line 2308 match overlapping sets of configurations fast/repaint/shadow-multiple-vertical.html
Line:2311 Entries on line 1077 and line 2311 match overlapping sets of configurations fast/transforms/shadows.html
Line:3772 Entries on line 1085 and line 3772 match overlapping sets of configurations media/audio-repaint.html

FAILURES FOR <win7, x86, release, cpu>
Line:1522 Entries on line 1085 and line 1522 match overlapping sets of configurations media/audio-repaint.html
Line:2305 Entries on line 1073 and line 2305 match overlapping sets of configurations fast/repaint/shadow-multiple-horizontal.html
Line:2306 Entries on line 1074 and line 2306 match overlapping sets of configurations fast/repaint/shadow-multiple-strict-horizontal.html
Line:2307 Entries on line 1075 and line 2307 match overlapping sets of configurations fast/repaint/shadow-multiple-strict-vertical.html
Line:2308 Entries on line 1076 and line 2308 match overlapping sets of configurations fast/repaint/shadow-multiple-vertical.html
Line:2311 Entries on line 1077 and line 2311 match overlapping sets of configurations fast/transforms/shadows.html

Lint failed.

[Julien Chaffraix]

Reverted r98486 for reason:

Broke Chromium's test_expectation.txt

Committed r98527: <http://trac.webkit.org/changeset/98527>

[Julien Chaffraix]

(In reply to comment #22)
> I think this patch broke lint checks on test_expectations.

For the record, such patch should either be fixed or reverted soon as it prevents the bots from running. Because of today's SVN situation, I couldn't figure out the situation until several hours after the commit and I had to wait even more for a window to roll it out.

>  I'm surprised this didn't turn red in the style trybot...

The style bot does not check that AFAICT. I would have expected the cr-linux to catch that up though as they run the layout tests. Eric / Adam, is this a known issue?

[Mike Reed]

Created attachment 112665 [details]
Patch

[Mike Reed]

ran lint locally to look for overlaps in expectations (not caught by cr-linux)

[Stephen White]

(In reply to comment #24)
> (In reply to comment #22)
> > I think this patch broke lint checks on test_expectations.
> 
> For the record, such patch should either be fixed or reverted soon as it prevents the bots from running. Because of today's SVN situation, I couldn't figure out the situation until several hours after the commit and I had to wait even more for a window to roll it out.
> 
> >  I'm surprised this didn't turn red in the style trybot...
> 
> The style bot does not check that AFAICT. I would have expected the cr-linux to catch that up though as they run the layout tests. Eric / Adam, is this a known issue?

Each platform bot only catches lint errors for that platform.  (e.g., the cr-linux bot only fails out of there are LINUX duplicates.)

I think the lint check used to be part of the presubmit check on Chrome when test_expectations.txt was modified, but I don't think it has found its way to webkit-patch (and the style bot) yet.

[Stephen White]

Comment on attachment 112665 [details]
Patch

OK.  r=me

[WebKit Review Bot]

Comment on attachment 112665 [details]
Patch

Clearing flags on attachment: 112665

Committed r98596: <http://trac.webkit.org/changeset/98596>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Ryosuke Niwa]

This patch appears to have caused the bug 75276.

[Ryosuke Niwa]

When you're adding a test expectation like this, please rebsaseline or remove expectations as soon as the patch is landed. Leaving failing test expectation like we did here results in regressions slipping in. At this point, we can't even tell whether the bug 75276 is caused by this patch or subsequent changes.

Our obsession with keeping bots green is biting us back. If didn't add test expectations in the original patch, gardeners at the time would have noticed the change and he/she would have been able to detect the bug 75276 as soon as it was introduced.

[Ryosuke Niwa]

Rebaselines done in
http://trac.webkit.org/changeset/103724
http://trac.webkit.org/changeset/103725
http://trac.webkit.org/changeset/103733
http://trac.webkit.org/changeset/103736
http://trac.webkit.org/changeset/103737
http://trac.webkit.org/changeset/103743

[Ryosuke Niwa]

The following 3 tests are not rebaselined and the failures are tracked by the bug 75276:
fast/repaint/shadow-multiple-horizontal.html
fast/repaint/shadow-multiple-strict-horizontal.html
fast/repaint/shadow-multiple-strict-vertical.html

(In reply to comment #32)
> When you're adding a test expectation like this, please rebsaseline or remove expectations as soon as the patch is landed. Leaving failing test expectation like we did here results in regressions slipping in. At this point, we can't even tell whether the bug 75276 is caused by this patch or subsequent changes.
> 
> Our obsession with keeping bots green is biting us back. If didn't add test expectations in the original patch, gardeners at the time would have noticed the change and he/she would have been able to detect the bug 75276 as soon as it was introduced.

We would love to rebaseline the tests right away, but at last check we still did not have guidance on how to properly generate new baselines.  See https://bugs.webkit.org/show_bug.cgi?id=72746 , where I used "webkit-patch rebaseline-expectations" but was told that its results were somehow incorrect.

Ryosuke:
1. How did you generate the new baselines in http://trac.webkit.org/changeset/103724 and friends?
2. How did you compare the new baseline images to the old ones they replaced and confirm that the changes are as expected?

[Ryosuke Niwa]

(In reply to comment #35)
> We would love to rebaseline the tests right away, but at last check we still did not have guidance on how to properly generate new baselines.  See https://bugs.webkit.org/show_bug.cgi?id=72746 , where I used "webkit-patch rebaseline-expectations" but was told that its results were somehow incorrect.

The problem appears that the script at the time had some bugs that resulted in incorrect rebaselines. Also be aware that 10.5 bots are very slow and are usually 5-6 hours behind ToT. So you need to wait until 5-6 hours until those bots catch up before you can run the script.

Now, once bots have caught up, you can run "Tools/Scripts/rebaseline-chromium-webkit-tests --use-deprecated-script" instead of webkit-patch rebaseline-expectations (rebaseline-expectations unfortunately doesn't take Linux GPU into account; see the bug 75282).

> 1. How did you generate the new baselines in http://trac.webkit.org/changeset/103724 and friends?
> 2. How did you compare the new baseline images to the old ones they replaced and confirm that the changes are as expected?

I went through every single diff on http://test-results.appspot.com/dashboards/flakiness_dashboard.html before running scripts.