| Summary: | quartz surface does not use device glyph extents | ||
|---|---|---|---|
| Product: | cairo | Reporter: | Brian Ewins <Brian.Ewins> |
| Component: | quartz backend | Assignee: | Carl Worth <cworth> |
| Status: | RESOLVED FIXED | QA Contact: | cairo-bugs mailing list <cairo-bugs> |
| Severity: | blocker | ||
| Priority: | medium | CC: | jwalden+bfo, roc |
| Version: | 1.3.15 | ||
| Hardware: | Other | ||
| OS: | All | ||
| Whiteboard: | |||
| i915 platform: | i915 features: | ||
| Attachments: |
correct scaling using glyph device extents.
test svg ff2 rendering minefield, 22 Feb fix the problem by patching _cairo_atsui_font_text_to_glyph |
||
This patch unfortunately makes use of the 'slow' glyph calculations (using the glyph path information) inevitable, when the only thing we need here is the device advance (from ATSUGlyphGetScreenMetrics). Although, in fact we take the slow path for every glyph right now anyway because of cairo-scaled-font.c:1449 - "info |= CAIRO_SCALED_GLYPH_INFO_METRICS;" (on the info that is requested when we init a scaled glyph). The options for making this faster are: - expose enough information about the transforms involved that the advances can be recovered from the glyph x, y coordinates (thats the approach taken in bug 9568, and it's not pretty) - expose enough information from the atsui scaled font that ATSUGlyphGetScreenMetrics can be called here. This bypasses the glyph cache. - add eg CAIRO_SCALED_GLYPH_INFO_ADVANCES as an option to _cairo_scaled_glyph_lookup, for glyph info that doesn't need the paths; the quartz code should still be pretty much the same as above, and there should be no impact on other font backends; just some small changes to cairo-scaled-font and cairo-atsui-font. The third option looks the best to me, I'll try to get some perf results for comparison. I have to wonder if there is a similar fast/slow path issue with FT, since the advances are stored in the font, while the inked extents must be calculated from the glyph outlines at some point. Created attachment 9002 [details] [review] correct scaling using glyph device extents. Patch as described, with better error recovery. I'm more confident that this patch should just be applied now. The same approach is used on the xlib, glitz backends; also the patch has been used in anger in joost in combination with pending atsui patches. Finally, I dug into the issue of whether the atsui approach of using paths to find extents was excessive, and found that it's exactly the same in freetype - the CBox is used for the extents (the bounding box of the glyph path control points). I'm confused. You seem to be throwing away the glyph x/ys provided, which may have been chosen by the application to be something other than what you get by adding up the advances. And haven't the incoming glyph advances been transformed to device units here? http://lxr.mozilla.org/seamonkey/source/gfx/cairo/cairo/src/cairo-gstate.c#1498 I guess I'm just ignorant about what's going on with transforms here, but I still don't see how you can ignore the passed-in x/ys. (In reply to comment #4) > I guess I'm just ignorant about what's going on with transforms here, but I > still don't see how you can ignore the passed-in x/ys. > Fair point. I'd say you're right, I'm wrong here. The xlib backend for example gets the x/ys at http://lxr.mozilla.org/seamonkey/source/gfx/cairo/cairo/src/cairo-scaled-font.c#878 ... my patch doesn't, so I'm throwing away any kerning you've done. Also worth mentioning that that is doing pixel-snapping; the equivalent in the quartz code would be cg_advances[i-1].width = _cairo_lround(glyphs[i].x) - xprev; however its doing that because (IIRC) the x/y stored there are used when rendering with glyph surfaces, so that they don't get antialiased twice. But still, with the code before the patch, the rendering I got when the device and user units aren't identical was junk. I'll attach the problem code/renderings from when the bug was reported to me (directly via mail, unfortunately), so you can see what I mean. Created attachment 9153 [details]
test svg
this is a test case provided to me for use with moz-svg. renderings follow.
Created attachment 9154 [details]
ff2 rendering
This is rendered with firefox 2 mac - moz svg was using the old quartz/atsui code.
Created attachment 9155 [details] minefield, 22 Feb This is the minefield rendering, 22 Feb with the patch in https://bugzilla.mozilla.org/show_bug.cgi?id=327522#c24 applied. The wrong font is showing here because moz-svg is using the toy API, and trying to select Tahoma. However its the glyph spacings that are the issue here. If you apply the patch in this bug, or remove viewBox="0 0 158 18" from the testcase, it renders correctly. I'd been holding off on a lot of these font scaling patches because theres been a lack of review on them, and I didn't want to mess up 1.4, had been meaning to look at this one again tonight. From your comments it looks like the error is elsewhere, most likely in getting the glyph extents. BTW we adjust the offsets not just for kerning, but also for CSS word-spacing, letter-spacing and justification. BTW doing pixel-snapping of glyphs will significantly degrade text quality. OS X does aggressive subpixel glyph positioning and rendering. On the pixel snapping - yes, its only something you'd do with glyph surfaces. I've confirmed that the bug is in _cairo_atsui_font_text_to_glyphs, I printed out what the x coord is set to and the x advance; summing these should give the next x (roughly). The x advances are being calculated from the glyph metrics, independently of the positions, and summing the advances is how the ft font backend calculates the positions; so we should get comparable numbers. This results look correct in the text-antialias-none test printing "cairo": 0 + 8 = 8 (this should be rougly the same value as the start of next line) 7.61719 + 3 = 10.6172 10.9512 + 8 = 18.9512 18.3047 + 7 = 25.3047 24.9023 + 7 = 31.9023 but in the text-pattern test, with a non-identity ctm, its all gone wrong: 3.2 + 7 = 10.2 17.275 + 8 = 25.275 32.9625 + 3.5 = 36.4625 40.075 + 5.5 = 45.575 50.0375 + 8 = 58.0375 In this case I've scaled x 2, but it looks like the x positions on the left have the advance added twice. Scaling x4: 1.6 + 3.5 = 5.1 (1.6 + 4 * 3.5 = 15.6 - matches next line) 15.675 + 4 = 19.675 (15.675 + 4 * 4 = 31.675, ditto) 31.3625 + 1.75 = 33.1125 38.475 + 2.75 = 41.225 48.4375 + 4 = 52.4375 ... so the scale causing us to see multiples of the advance. Most likely the style includes the normally-identity ctm somewhere. Created attachment 9172 [details] [review] fix the problem by patching _cairo_atsui_font_text_to_glyph The answer is really in the last comment. The positions we were calculating were the same no matter what the ctm scale was; I spent time scratching my head looking for broken scaling code but the problem was just that the there was *no* scaling code. Oops. The patch changes _cairo_atsui_font_text_to_glyph to use the same method as cairo-scaled-font.c uses to calculate the glyph positions. This isn't using the real positioning information from ATSUDirectGetLayoutDataArrayPtrFromTextLayout. I could transform those results, but this code is good enough, I think - its only hit by the toy api. Comments? Annoyingly this turns out to be something I'd previously fixed as part of a patch in bug 9568 (which fixes the same problem by scaling the coordinates, not using the advances). That patch needs updated - its been broken since the font size != 1.0 fix - but it should produce better output than this one. the corrected patches for this were pushed see http://gitweb.freedesktop.org/?p=cairo;a=commit;h=12feb1f98627637bf83ba70f739bb5b2699085aa and http://gitweb.freedesktop.org/?p=cairo;a=commit;h=986b0ff83813b68a19490fe8629bfdaeed68cbe2 and were released in 1.4.2. |
Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.
To reproduce, edit the text-pattern test case so that a plain rgb fill is used instead of a pattern (this prevents (n)quartz falling back to atsui for this test) The bug is this, in show_glyphs: cg_advances[i-1].width = glyphs[i].x - xprev; cg_advances[i-1].height = glyphs[i].y - yprev; lhs is in device units, rhs is in user units. Here's the fixed loop: _cairo_scaled_font_freeze_cache (scaled_font); for (i = 0; i < num_glyphs; i++) { status = _cairo_scaled_glyph_lookup (scaled_font, glyphs[i].index, CAIRO_SCALED_GLYPH_INFO_METRICS, &scaled_glyph); if (status) { _cairo_scaled_font_thaw_cache (scaled_font); /* XXX suspect I should call _cairo_nquartz_teardown_source */ CGContextRestoreGState (surface->cgContext); /* and this is not good - there is a fallback but this is wrong err */ return CAIRO_INT_STATUS_UNSUPPORTED; } cg_glyphs[i] = glyphs[i].index; cg_advances[i].width = scaled_glyph->metrics.x_advance; cg_advances[i].height = scaled_glyph->metrics.y_advance; } _cairo_scaled_font_thaw_cache (scaled_font); I'll turn this into a patch shortly. See also bug 9568, which is the exact same bug in the fallback code; that patch was always a mess, this appears to be the best way to fix that problem too.