Bug 12448 – small text which is scaled to be large renders pixelated

• Status: RESOLVED
• Resolution: FIXED
• Component: SVG

[Andreas Neumann]

In the provided example the transformed text looks very ugly. It seems like no or ugly antialiasing is used.

Here is the same example but with animation (scripting): http://www.carto.net/neumann/webkitsvgbugs/Richard4Suzanne.svg

This renders fine in Batik and Opera. Firefox seems to have problems as well.

Thanks for checking on this!

[Eric Seidel (no email)]

I wonder if CG is doing this automatically.

[Eric Seidel (no email)]

Mac OS X will turn off antialiasing in text which is below a specific size.  In this case, font-size='4' on the heart text is causing the text to lose its antialiasing.

I'm not sure how we'd fix this.  You really want that system setting to affect the final rendered size, not the intermediate size.

[Alexey Proskuryakov]

See also: bug 4842 (duplicate?).

[Andreas Neumann]

(In reply to comment #2)
> Mac OS X will turn off antialiasing in text which is below a specific size.  In
> this case, font-size='4' on the heart text is causing the text to lose its
> antialiasing.
> 
> I'm not sure how we'd fix this.  You really want that system setting to affect
> the final rendered size, not the intermediate size.
> 

ah, that explains it. So we need some extra intelligence in text rendering, checking the final rendered size in device pixels ...

[Eric Seidel (no email)]

One could argue this is a CG bug.  CG could be smarter about the "final rendered size".  Maybe it's possible for us to add those smarts to the way we call into CG text rendering.

[Mark Hubbart]

(In reply to comment #3)
> See also: bug 4842 (duplicate?).

This appears to be somewhat linked to bug 14242 as far as I can tell.

There may be one fix for the two issues here:
1. Anti-aliasing is based on the specified point size, rather than the final display size;
2. Point size is rounded to the nearest integer value, regardless of possible later scaling.

I'll upload a testcase...

[Mark Hubbart]

Created attachment 15595 [details]
Both lines of text should render exactly the same.

In this test, both lines of text should be rendered exactly the same. The SVG markup is pretty self explanatory; line one starts at 10pt text with no scaling, and line two starts with 1pt text with 10x scaling.

[Nicholas Shanks]

I hit both of the bugs Mark refers to in comment #6 in my own SVG work. It's very frustrating. Scaling can also expose these problems when zooming the document (Command-Plus/Minus).

<rdar://problem/7521891>

[Renata Hodovan]

Created attachment 66342 [details]
Better scaling for small fonts

[Eric Seidel (no email)]

Comment on attachment 66342 [details]
Better scaling for small fonts

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

I'm confused why Qt doesn't do this all for you.  How do other platforms avoid needing this code?  Why is CoreGraphics (mac) smart enough not to need it?  Or does it have it and I just don't realize?

> WebCore/ChangeLog:48887
> -        (WebCore::HTMLTreeBuilder::insertComment):
> +        (WebCore::HTMLTreeBuilder::insertComment):
>              - Eliminate one unnecessary ref/deref pair.

Huh?

> WebCore/platform/graphics/qt/FontQt.cpp:223
> +    scale = sqrtf(trans.c() * trans.c() + trans.d() * trans.d());

I guess no inline sqr() function. ;)  Does this operation have a name?  One which AffineTransform should support for you?

[Alexey Proskuryakov]

This was originally reported about Mac OS X. We shouldn't making a Qt-only fix in this bug.

[Nikolas Zimmermann]

Comment on attachment 66342 [details]
Better scaling for small fonts

I've discussed this with Reni already, forgot to mark the patch r-.
It needs to be solved cross-platform, but it's very hard to do that with the current design of the SVG text layout engine.
I'm rewriting it at the moment, please stay tuned.

[Nikolas Zimmermann]

New patch coming soon.

[Nikolas Zimmermann]

Created attachment 80736 [details]
Patch v1

Incorporated the nice testcase in my patch. This should finally fix all text problem with small font sizes on all platforms, at least that's the goal.

[Nikolas Zimmermann]

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

[Nikolas Zimmermann]

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

[Nikolas Zimmermann]

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

[Nikolas Zimmermann]

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

[Nikolas Zimmermann]

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

[Nikolas Zimmermann]

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

[Nikolas Zimmermann]

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

[Nikolas Zimmermann]

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

[Nikolas Zimmermann]

Comment on attachment 80736 [details]
Patch v1

Forgot to set r?

[Nikolas Zimmermann]

I left out all LayoutTests/ changes, as it rebases most pixel test results with text, which is huge.

[WebKit Review Bot]

Attachment 80736 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/7682620

[Early Warning System Bot]

Attachment 80736 [details] did not build on qt:
Build output: http://queues.webkit.org/results/7500403

[Dirk Schulze]

Comment on attachment 80736 [details]
Patch v1

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

The patch itself looks good. Please fix the builds as well as the noted issues and upload at least the pixel/DRT tests + results of the new tests.

> Source/WebCore/rendering/svg/SVGInlineTextBox.cpp:153
> +    // If the absolute font size on screen is below x=0.5, 't render anything.

s/'t/don't/

> Source/WebCore/svg/SVGFont.cpp:258
> +            ASSERT(renderObject->parent());

You call renderObject->parent() multiple times (you use it at least 3 times for debug build) can save the pointer please?

> Source/WebCore/svg/SVGFont.cpp:412
> +            ASSERT(renderObject->parent());

ditto.

> Source/WebCore/svg/SVGFont.cpp:502
> +            ASSERT(renderObject->parent());

ditto.

> Source/WebCore/svg/SVGFont.cpp:547
> +                        ASSERT(renderObject);

you asserted this at the beginning already.It might be unnecessary if you store the pointer in a local variable before entering this condition.

[WebKit Review Bot]

Attachment 80736 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/7685531

[Nikolas Zimmermann]

Created attachment 80741 [details]
Patch v2

Fix Dirks issues, fix release builds, include some of the layout tests changes (already almost 2mb).

[Dirk Schulze]

Comment on attachment 80741 [details]
Patch v2

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

> LayoutTests/ChangeLog:18
> +          Since we switched to use floating-point precision metrics, the actual drawn selection may exceed the red rectangle to up to 1px.

a 'to' too much

> LayoutTests/ChangeLog:21
> +          In theory the red re

sentence is incomplete.

> LayoutTests/ChangeLog:22
> +          The red rectangle represents the results of the text query, using getExtentOfChar(), which gives the precise text metrics using floating-point accurancy.
> +          In theory the red re
> +          The selection rects are aligned on integer boundaries.

remove these lines completely

> LayoutTests/ChangeLog:27
> +        - svg/text/selection-(doubleclick|tripleclick).svg:
> +          These tests use DRTs dumpSelectionRect() which draws a red rectangle, highlighting the SelectionController::bounds() obtained through RenderView.
> +          In theory this red rectangle, should be equal to the drawn blue selection rect. Again it isn't, as the bounds() methods uses selectionRectForRepaint()
> +          of RenderText, which returns an enclosingIntRect of the actual selection. The drawn selection starts at y=0.93 in the first testcase, the selectionRectForRepaint()

You should concentrate on the facts, you already wrote about the red rects before :-/

> LayoutTests/ChangeLog:33
> +          the integer variants ascent/descent/height - hopefully we'll see consistent results across platforms for these tests now, if not please comment in the bug report.

There are some minor misspellings, talked on IRC about it. Won't mention these mistakes here.

You're talking about a difference of <1px, but we can see more pixel differences in some cases. We talked about the reason on IRC. But please add a short note about the scaling of hinting that is responsible for this as well.

[WebKit Review Bot]

Attachment 80736 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebCore/rendering/svg/RenderSVGInlineText.cpp:36:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 12 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Review Bot]

Attachment 80741 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/css3..." exit_code: 1

Source/WebCore/rendering/svg/RenderSVGInlineText.cpp:36:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 127 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Nikolas Zimmermann]

Landed in r77485, pixel tests still to be updated.

[Andreas Neumann]

Hey - congratulations on fixing this bug - right in time for Valentines day ;-)

The bug report was made a long time ago (2007). Good to see it wasn't buried and is fixed now!

Andreas

[WebKit Review Bot]

http://trac.webkit.org/changeset/77485 might have broken Qt Linux Release

[Nikolas Zimmermann]

Sorry, it's taking ages, I'm trying to land mac pixel test results since a while, but it keeps gettin stuck, I'm splitting up in even smaller chunks at the moment.

[Nikolas Zimmermann]

Philippe landed gtk baselines in r77490.
Ossy landed qt baselines in r77497.
Dimitri landed cr-win baselines in r77499.

[Csaba Osztrogonác]

Unfortunately we have 2 regressions on Qt:
http://build.webkit.org/results/Qt%20Linux%20Release/r77501%20(27628)/css3/zoom-coords-pretty-diff.html
http://build.webkit.org/results/Qt%20Linux%20Release/r77501%20(27628)/svg/zoom/page/zoom-zoom-coords-pretty-diff.html

Could you check it please?

[Csaba Osztrogonác]

(In reply to comment #39)
> Unfortunately we have 2 regressions on Qt:
> http://build.webkit.org/results/Qt%20Linux%20Release/r77501%20(27628)/css3/zoom-coords-pretty-diff.html
> http://build.webkit.org/results/Qt%20Linux%20Release/r77501%20(27628)/svg/zoom/page/zoom-zoom-coords-pretty-diff.html
> 
> Could you check it please?

I skipped them until fix to make buildbot happy:
http://trac.webkit.org/changeset/77547

[Nikolas Zimmermann]

Dimitri, thanks a lot for the help with the rebaselines!
Unfortunately, I couldn't land any chunk with more than 10+ pngs at once, even that sometimes got stuck - I guess my internet connection had problems yesterday, appologizes again, that it took so long to update the pixel baseline :/

[WebKit Review Bot]

http://trac.webkit.org/changeset/77804 might have broken Qt Linux Release

[Csaba Osztrogonác]

(In reply to comment #42)
> http://trac.webkit.org/changeset/77804 might have broken Qt Linux Release

It is a flakey crash, sorry for the SPAM.

[WebKit Review Bot]

http://trac.webkit.org/changeset/77805 might have broken Qt Linux Release

[WebKit Review Bot]

http://trac.webkit.org/changeset/77806 might have broken Qt Linux Release

[Eric Seidel (no email)]

@ossy: Do we have a bug on the flaky crash?

[Csaba Osztrogonác]

(In reply to comment #46)
> @ossy: Do we have a bug on the flaky crash?

not yet :( I haven't found the exact root of the problem, but I 
suspect this flakiness was caused by GC related changes at last week.

I'm going to file a bug about it as soon as possible.

[Csaba Osztrogonác]

(In reply to comment #46)
> @ossy: Do we have a bug on the flaky crash?

It's too late for me today. :( We do have bug for it: https://bugs.webkit.org/show_bug.cgi?id=53912

I'll add some usable comment and try to make buildbot happier.

[Simon Fraser (smfr)]

svg/text/font-size-below-point-five.svg contains bad English: "Font size should decrease monotonic". It's also failing on Leopard: http://build.webkit.org/results/Leopard%20Intel%20Debug%20(Tests)/r78055%20(27056)/results.html

[Dominik Röttsches (drott)]

Comment on attachment 80741 [details]
Patch v2

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

> Source/WebCore/rendering/svg/SVGInlineTextBox.cpp:593
> +            context->scale(FloatSize(1 / scalingFactor, 1 / scalingFactor));

Unfortunately, temporarily changing the scale/transform of the context breaks using gradient fills for text. The gradients turn out incorrectly scaled. Ideas how to fix this are welcome on bug 64966.