Bug 64368 – [skia] remove legacy draw-text-as-paths code, as skia now draws all text as text

• Component: New Bugs
• Status: RESOLVED WONTFIX

[Mike Reed]

[skia] remove legacy draw-text-as-paths code, as skia now draws all text as text

[Mike Reed]

Created attachment 100522 [details]
Patch

[Mike Reed]

Created attachment 100534 [details]
Patch

[James Robinson]

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?

[James Robinson]

Comment on attachment 100534 [details]
Patch

Patch doesn't apply so bots can't run

[Mike Reed]

Created attachment 100540 [details]
Patch

[Mike Reed]

Created attachment 100548 [details]
sample screen shots w/ patch applied

[James Robinson]

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)

[Mike Reed]

Comment on attachment 100540 [details]
Patch

no review yet

[Mike Reed]

*** Bug 59817 has been marked as a duplicate of this bug. ***

[Mike Reed]

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

[James Robinson]

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.

[Mike Reed]

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

[Mike Reed]

When we do rebaseline, we need to wait for Skia rev. 1850 or later. Hoping to DEPS-roll that in tomorrow.

[James Robinson]

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

[Mike Reed]

skia 1850 is in. ready for review/commit/rebaseline

[Stephen White]

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.

[Stephen White]

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

[Mike Reed]

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.

[James Robinson]

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.

[Mike Reed]

The skia DEPS in webkit has rolled, so re-requesting the commit+

[WebKit Review Bot]

Comment on attachment 100540 [details]
Patch

Clearing flags on attachment: 100540

Committed r91069: <http://trac.webkit.org/changeset/91069>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Adrienne Walker]

Committed r91074: <http://trac.webkit.org/changeset/91074>

[Adrienne Walker]

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

[Vincent Scheib]

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

[Adrienne Walker]

Committed r91105: <http://trac.webkit.org/changeset/91105>