Bug 64368

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 BugsAssignee: 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 Flags
Patch
none
Patch
none
Patch
none
sample screen shots w/ patch applied none

Description Mike Reed 2011-07-12 10:45:09 PDT
[skia] remove legacy draw-text-as-paths code, as skia now draws all text as text
Comment 1 Mike Reed 2011-07-12 10:59:44 PDT
Created attachment 100522 [details]
Patch
Comment 2 Mike Reed 2011-07-12 11:47:45 PDT
Created attachment 100534 [details]
Patch
Comment 3 James Robinson 2011-07-12 12:19:34 PDT
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 4 James Robinson 2011-07-12 12:19:54 PDT
Comment on attachment 100534 [details]
Patch

Patch doesn't apply so bots can't run
Comment 5 Mike Reed 2011-07-12 12:40:51 PDT
Created attachment 100540 [details]
Patch
Comment 6 Mike Reed 2011-07-12 13:00:33 PDT
Created attachment 100548 [details]
sample screen shots w/ patch applied
Comment 7 James Robinson 2011-07-12 13:04:50 PDT
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 8 Mike Reed 2011-07-12 13:58:14 PDT
Comment on attachment 100540 [details]
Patch

no review yet
Comment 9 Mike Reed 2011-07-13 13:02:30 PDT
*** Bug 59817 has been marked as a duplicate of this bug. ***
Comment 10 Mike Reed 2011-07-13 13:06:31 PDT
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).
Comment 11 James Robinson 2011-07-13 13:12:23 PDT
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.
Comment 12 Mike Reed 2011-07-13 13:23:58 PDT
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).
Comment 13 Mike Reed 2011-07-13 13:24:41 PDT
When we do rebaseline, we need to wait for Skia rev. 1850 or later. Hoping to DEPS-roll that in tomorrow.
Comment 14 James Robinson 2011-07-13 14:52:44 PDT
(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.
Comment 15 Mike Reed 2011-07-14 07:56:31 PDT
skia 1850 is in. ready for review/commit/rebaseline
Comment 16 Stephen White 2011-07-14 08:23:34 PDT
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.
Comment 17 Stephen White 2011-07-14 08:40:42 PDT
(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.
Comment 18 Mike Reed 2011-07-14 08:57:53 PDT
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 19 James Robinson 2011-07-14 11:17:03 PDT
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.
Comment 20 Mike Reed 2011-07-15 05:42:11 PDT
The skia DEPS in webkit has rolled, so re-requesting the commit+
Comment 21 WebKit Review Bot 2011-07-15 08:27:43 PDT
Comment on attachment 100540 [details]
Patch

Clearing flags on attachment: 100540

Committed r91069: <http://trac.webkit.org/changeset/91069>
Comment 22 WebKit Review Bot 2011-07-15 08:27:49 PDT
All reviewed patches have been landed.  Closing bug.
Comment 23 Adrienne Walker 2011-07-15 09:51:13 PDT
Committed r91074: <http://trac.webkit.org/changeset/91074>
Comment 24 Adrienne Walker 2011-07-15 09:53:00 PDT
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
Comment 25 Vincent Scheib 2011-07-15 09:54:04 PDT
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
Comment 26 Adrienne Walker 2011-07-15 13:50:07 PDT
Committed r91105: <http://trac.webkit.org/changeset/91105>