Bug 70748 - [skia] replace offscreen technique with native support for antialiased clipping
Summary: [skia] replace offscreen technique with native support for antialiased clipping
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: Mike Reed
URL:
Keywords:
Depends on:
Blocks: 75276
  Show dependency treegraph
 
Reported: 2011-10-24 11:52 PDT by Mike Reed
Modified: 2011-12-30 10:48 PST (History)
11 users (show)

See Also:


Attachments
Patch (4.85 KB, patch)
2011-10-24 11:56 PDT, Mike Reed
no flags Details | Formatted Diff | Diff
Patch (12.79 KB, patch)
2011-10-24 11:59 PDT, Mike Reed
no flags Details | Formatted Diff | Diff
Patch (13.02 KB, patch)
2011-10-24 13:32 PDT, Mike Reed
no flags Details | Formatted Diff | Diff
Patch (14.44 KB, patch)
2011-10-25 13:29 PDT, Mike Reed
no flags Details | Formatted Diff | Diff
Patch (14.13 KB, patch)
2011-10-25 14:15 PDT, Mike Reed
no flags Details | Formatted Diff | Diff
Patch (13.50 KB, patch)
2011-10-26 06:23 PDT, Mike Reed
no flags Details | Formatted Diff | Diff
Patch (16.13 KB, patch)
2011-10-26 06:56 PDT, Mike Reed
no flags Details | Formatted Diff | Diff
Patch (19.14 KB, patch)
2011-10-27 05:26 PDT, Mike Reed
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mike Reed 2011-10-24 11:52:52 PDT
[skia] replace offscreen technique with native support for antialiased clipping
Comment 1 Mike Reed 2011-10-24 11:56:01 PDT
Created attachment 112222 [details]
Patch
Comment 2 Mike Reed 2011-10-24 11:59:14 PDT
Created attachment 112226 [details]
Patch
Comment 3 WebKit Review Bot 2011-10-24 12:36:57 PDT
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
Comment 4 Mike Reed 2011-10-24 13:32:52 PDT
Created attachment 112239 [details]
Patch
Comment 5 WebKit Review Bot 2011-10-24 13:57:42 PDT
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
Comment 6 Mike Reed 2011-10-24 14:36:18 PDT
trying to repro the crashes...
Comment 7 Mike Reed 2011-10-25 13:29:29 PDT
Created attachment 112390 [details]
Patch
Comment 8 Mike Reed 2011-10-25 14:15:10 PDT
Created attachment 112401 [details]
Patch
Comment 9 Stephen White 2011-10-25 15:05:36 PDT
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)?
Comment 10 Mike Reed 2011-10-26 04:58:43 PDT
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.
Comment 11 Stephen White 2011-10-26 05:44:16 PDT
(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.
Comment 12 Mike Reed 2011-10-26 06:23:29 PDT
Created attachment 112496 [details]
Patch
Comment 13 Mike Reed 2011-10-26 06:24:46 PDT
changed expectations from WIN LINUX to CPU, which should cover windows, linux, and skia-mac, but not cg-mac
Comment 14 Adam Langley 2011-10-26 06:43:26 PDT
From a brief look at the patch I would have expected PlatformContextSkia::applyAntiAliasedClipPaths (and calls to it) to also be removed.
Comment 15 Mike Reed 2011-10-26 06:56:50 PDT
Created attachment 112500 [details]
Patch
Comment 16 Mike Reed 2011-10-26 06:57:15 PDT
good catch. applyAntiAliasedClipPaths removed.
Comment 17 Adam Langley 2011-10-26 07:03:59 PDT
LGTM (Note: I am not a WebKit reviewer. You need an r+ from a real WK reviewer before landing.)
Comment 18 Stephen White 2011-10-26 08:00:27 PDT
Comment on attachment 112500 [details]
Patch

I didn't know that MAC-CG was excluded from CPU.  Strange, but ok.  r=me
Comment 19 epoger 2011-10-26 08:31:12 PDT
(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
Comment 20 WebKit Review Bot 2011-10-26 08:49:07 PDT
Comment on attachment 112500 [details]
Patch

Clearing flags on attachment: 112500

Committed r98486: <http://trac.webkit.org/changeset/98486>
Comment 21 WebKit Review Bot 2011-10-26 08:49:13 PDT
All reviewed patches have been landed.  Closing bug.
Comment 22 epoger 2011-10-26 09:30:52 PDT
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.
Comment 23 Julien Chaffraix 2011-10-26 17:21:51 PDT
Reverted r98486 for reason:

Broke Chromium's test_expectation.txt

Committed r98527: <http://trac.webkit.org/changeset/98527>
Comment 24 Julien Chaffraix 2011-10-26 17:30:15 PDT
(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?
Comment 25 Mike Reed 2011-10-27 05:26:27 PDT
Created attachment 112665 [details]
Patch
Comment 26 Mike Reed 2011-10-27 05:27:07 PDT
ran lint locally to look for overlaps in expectations (not caught by cr-linux)
Comment 27 Stephen White 2011-10-27 06:54:16 PDT
(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.
Comment 28 Stephen White 2011-10-27 09:57:47 PDT
Comment on attachment 112665 [details]
Patch

OK.  r=me
Comment 29 WebKit Review Bot 2011-10-27 10:19:45 PDT
Comment on attachment 112665 [details]
Patch

Clearing flags on attachment: 112665

Committed r98596: <http://trac.webkit.org/changeset/98596>
Comment 30 WebKit Review Bot 2011-10-27 10:19:51 PDT
All reviewed patches have been landed.  Closing bug.
Comment 31 Ryosuke Niwa 2011-12-27 16:01:35 PST
This patch appears to have caused the bug 75276.
Comment 32 Ryosuke Niwa 2011-12-27 16:05:54 PST
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.
Comment 34 Ryosuke Niwa 2011-12-27 17:03:15 PST
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
Comment 35 epoger 2011-12-28 06:38:35 PST
(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?
Comment 36 Ryosuke Niwa 2011-12-30 10:48:13 PST
(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.