| Summary: | small text which is scaled to be large renders pixelated | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Andreas Neumann <a.neumann> | ||||||||||
| Component: | SVG | Assignee: | Nikolas Zimmermann <zimmermann> | ||||||||||
| Status: | RESOLVED FIXED | ||||||||||||
| Severity: | Normal | CC: | abarth, ap, aroben, baker, bugzilla, ctruta, dglazkov, edoardo, eric, jeffschiller, krit, marcus.erronius, mitz, nickshanks, ossy, rhodovan.u-szeged, simon.fraser, webkit-ews, webkit.review.bot, zherczeg, zimmermann | ||||||||||
| Priority: | P2 | Keywords: | InRadar | ||||||||||
| Version: | 420+ | ||||||||||||
| Hardware: | Mac | ||||||||||||
| OS: | OS X 10.4 | ||||||||||||
| URL: | http://www.carto.net/neumann/webkitsvgbugs/svghearts_static.svg | ||||||||||||
| Attachments: |
|
||||||||||||
|
Description
Andreas Neumann
2007-01-28 04:38:24 PST
I wonder if CG is doing this automatically. 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. (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 ... 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. (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... 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.
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). Created attachment 66342 [details]
Better scaling for small fonts
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? This was originally reported about Mac OS X. We shouldn't making a Qt-only fix in this bug. 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.
New patch coming soon. 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.
*** Bug 14242 has been marked as a duplicate of this bug. *** *** Bug 17053 has been marked as a duplicate of this bug. *** *** Bug 19393 has been marked as a duplicate of this bug. *** *** Bug 20192 has been marked as a duplicate of this bug. *** *** Bug 21774 has been marked as a duplicate of this bug. *** *** Bug 49846 has been marked as a duplicate of this bug. *** *** Bug 50313 has been marked as a duplicate of this bug. *** *** Bug 50528 has been marked as a duplicate of this bug. *** Comment on attachment 80736 [details]
Patch v1
Forgot to set r?
I left out all LayoutTests/ changes, as it rebases most pixel test results with text, which is huge. Attachment 80736 [details] did not build on chromium: Build output: http://queues.webkit.org/results/7682620 Attachment 80736 [details] did not build on qt: Build output: http://queues.webkit.org/results/7500403 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. Attachment 80736 [details] did not build on chromium: Build output: http://queues.webkit.org/results/7685531 Created attachment 80741 [details]
Patch v2
Fix Dirks issues, fix release builds, include some of the layout tests changes (already almost 2mb).
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. 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.
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.
Landed in r77485, pixel tests still to be updated. 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 http://trac.webkit.org/changeset/77485 might have broken Qt Linux Release 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. Philippe landed gtk baselines in r77490. Ossy landed qt baselines in r77497. Dimitri landed cr-win baselines in r77499. 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? (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 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 :/ http://trac.webkit.org/changeset/77804 might have broken Qt Linux Release (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. http://trac.webkit.org/changeset/77805 might have broken Qt Linux Release http://trac.webkit.org/changeset/77806 might have broken Qt Linux Release @ossy: Do we have a bug on the flaky crash? (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. (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. 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 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. |
