Bug 10067 - quartz surface does not use device glyph extents
Summary: quartz surface does not use device glyph extents
Alias: None
Product: cairo
Classification: Unclassified
Component: quartz backend (show other bugs)
Version: 1.3.15
Hardware: Other All
: medium blocker
Assignee: Carl Worth
QA Contact: cairo-bugs mailing list
Depends on:
Reported: 2007-02-22 16:58 UTC by Brian Ewins
Modified: 2007-03-25 17:58 UTC (History)
2 users (show)

See Also:
i915 platform:
i915 features:

correct scaling using glyph device extents. (2.19 KB, patch)
2007-03-06 16:53 UTC, Brian Ewins
Details | Splinter Review
test svg (240 bytes, image/svg+xml)
2007-03-15 05:38 UTC, Brian Ewins
ff2 rendering (16.12 KB, image/png)
2007-03-15 05:41 UTC, Brian Ewins
minefield, 22 Feb (15.12 KB, image/png)
2007-03-15 05:57 UTC, Brian Ewins
fix the problem by patching _cairo_atsui_font_text_to_glyph (1.73 KB, patch)
2007-03-15 18:59 UTC, Brian Ewins
Details | Splinter Review

Description Brian Ewins 2007-02-22 16:58:35 UTC
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,
	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 */

	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.
Comment 1 Brian Ewins 2007-02-26 03:36:38 UTC
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.
Comment 2 Brian Ewins 2007-03-06 16:53:28 UTC
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).
Comment 3 Robert O'Callahan 2007-03-15 03:19:46 UTC
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?
Comment 4 Robert O'Callahan 2007-03-15 03:26:48 UTC
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.
Comment 5 Brian Ewins 2007-03-15 05:36:38 UTC
(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 
... 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. 
Comment 6 Brian Ewins 2007-03-15 05:38:02 UTC
Created attachment 9153 [details]
test svg

this is a test case provided to me for use with moz-svg. renderings follow.
Comment 7 Brian Ewins 2007-03-15 05:41:32 UTC
Created attachment 9154 [details]
ff2 rendering

This is rendered with firefox 2 mac - moz svg was using the old quartz/atsui code.
Comment 8 Brian Ewins 2007-03-15 05:57:28 UTC
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.
Comment 9 Robert O'Callahan 2007-03-15 11:14:19 UTC
BTW we adjust the offsets not just for kerning, but also for CSS word-spacing, letter-spacing and justification.
Comment 10 Robert O'Callahan 2007-03-15 11:20:18 UTC
BTW doing pixel-snapping of glyphs will significantly degrade text quality. OS X does aggressive subpixel glyph positioning and rendering.
Comment 11 Brian Ewins 2007-03-15 14:47:30 UTC
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.
Comment 12 Brian Ewins 2007-03-15 18:59:42 UTC
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.

Comment 13 Brian Ewins 2007-03-16 03:29:51 UTC
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.

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.