Summary: | [skia] remove legacy draw-text-as-paths code, as skia now draws all text as text | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Mike Reed <reed> | ||||||||||
Component: | New Bugs | Assignee: | Mike Reed <reed> | ||||||||||
Status: | RESOLVED WONTFIX | ||||||||||||
Severity: | Normal | CC: | brettw, enne, jamesr, kbr, schenney, senorblanco, webkit.review.bot | ||||||||||
Priority: | P2 | ||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||
Hardware: | Unspecified | ||||||||||||
OS: | Unspecified | ||||||||||||
Attachments: |
|
Description
Mike Reed
2011-07-12 10:45:09 PDT
Created attachment 100522 [details]
Patch
Created attachment 100534 [details]
Patch
It looks like you have merge conflicts in test_expectations.txt, so the EWS/etc bots can't run. Could you reupload the patch with these resolved? test_expectations.txt changes rapidly so your odds of conflicting are much lower if you make your edits somewhere other than the very end of the file (I normally pick a spot a few hundred lines from the bottom). Can you post some before/after images from windows for comparison? Comment on attachment 100534 [details]
Patch
Patch doesn't apply so bots can't run
Created attachment 100540 [details]
Patch
Created attachment 100548 [details]
sample screen shots w/ patch applied
What about the pixel output from DRT? We explicitly turn AA off for that test (see http://trac.webkit.org/browser/trunk/LayoutTests/platform/chromium-win/svg/css/text-shadow-multiple-expected.png) Comment on attachment 100540 [details]
Patch
no review yet
*** Bug 59817 has been marked as a duplicate of this bug. *** Reviewing the image diffs, the big difference is actually a surprise: antialiasing. DRT (on windows) explicitly disables all font smoothing, and so all GDI and skia-drawn text comes out BW. However, the legacy path (which this CL removes) drew paths with antialiasing, so that text (typically when rotated) is still drawn antialiased... and this has been captured in our expected images! Thus, the diffs are between expected (antialiased) and actual (unantialiased) rotated text. Note: when I view each test in a browser with the patch, of course everything is nicely antialiased, but this time via Skia's text drawing, rather than paths. The sped difference between paths and text drawing is something like 10-20x faster for text (and the delta is even greater when drawing via the gpu). The reason AA was disabled for these tests was that the algorithm was presumed to be numerically unstable, do you know if that's still the case? If not then we could just turn it back on. Do you know how many tests are affected? Sounds like the biggest headache with this patch will be dealing with all the baselines. I've uploaded the text_expectations.txt file. There are approx 200 images to rebaseline. I can't speak to the stability question. Certainly there is no perfect way to perform blending, and we occasionally want to tweak the math for performance, sometimes even between C and SSE2 and SSE3 (not to mention gpu). When we do rebaseline, we need to wait for Skia rev. 1850 or later. Hoping to DEPS-roll that in tomorrow. (In reply to comment #12) > I've uploaded the text_expectations.txt file. There are approx 200 images to rebaseline. > > I can't speak to the stability question. Certainly there is no perfect way to perform blending, and we occasionally want to tweak the math for performance, sometimes even between C and SSE2 and SSE3 (not to mention gpu). OK, 200 tests is definitely not outrageous. skia 1850 is in. ready for review/commit/rebaseline Comment on attachment 100540 [details]
Patch
Looks good to me (I like it when code goes away). Take care when rebaselining any tests which have Japanese/Thai/Indic fonts -- these tend to produce different results on WinXP/Vista/7, and require separate baselines.
Leaving r? for James to take a look.
(In reply to comment #13) > When we do rebaseline, we need to wait for Skia rev. 1850 or later. Hoping to DEPS-roll that in tomorrow. BTW, looks like WebKit is still at skia r1831. Needs a chromium-in-Webkit roll, I think. The newer skia is only needed for the rebaseline, not to allow this CL to land. I presume we can land it now, and just wait for the other DEPS roll, and then initiate the rebaselining. Comment on attachment 100540 [details]
Patch
Cool! Please wait for the skia change to roll in before landing so we can start rebaselining as soon as the bots cycle.
The skia DEPS in webkit has rolled, so re-requesting the commit+ Comment on attachment 100540 [details] Patch Clearing flags on attachment: 100540 Committed r91069: <http://trac.webkit.org/changeset/91069> All reviewed patches have been landed. Closing bug. Committed r91074: <http://trac.webkit.org/changeset/91074> Reopening because the test_expectations file was marked with this as the bug for rebaselining. These additional two tests were failing as a result of this patch: http://test-results.appspot.com/dashboards/flakiness_dashboard.html#showExpectations=true&group=%40ToT%20GPU%20Mesa%20-%20chromium.org&tests=compositing%2Fshadows%2Fshadow-drawing.html%2Cfast%2Fcanvas%2Fcanvas-text-baseline.html This patch causes errors in test_expectations Line:3577 Duplicate or ambiguous expectation. svg/W3C-I18N/tspan-dirNone-ubOverride-in-default-context.svg Line:3578 Duplicate or ambiguous expectation. svg/W3C-I18N/tspan-dirNone-ubOverride-in-ltr-context.svg Line:3579 Duplicate or ambiguous expectation. svg/W3C-I18N/tspan-dirLTR-ubOverride-in-default-context.svg Line:3580 Duplicate or ambiguous expectation. svg/W3C-I18N/tspan-dirLTR-ubOverride-in-ltr-context.svg Line:3581 Duplicate or ambiguous expectation. svg/text/bidi-text-query.svg Line:3832 Duplicate or ambiguous expectation. svg/custom/use-detach.svg Please run Tools/Scripts/new-run-webkit-tests --lint-test-files Committed r91105: <http://trac.webkit.org/changeset/91105> |