Created attachment 8117 [details]
This is a sample of the output of the text-rotate test

The text-rotate test shows nearly-correct behaviour in the top left, but
incorrect behaviour in the bottom right.

Created attachment 8118 [details] [review]
Patch to correct the text-rotate behaviour

Investigating the problem I found that when calculating the height of the font
after scaling, instead of taking the magnitude of a scaled unit vector, the
code used the y-component of a scaled sqrt(2) magnitude vector (ie (1,1)). I've
made the obvious fix in this patch, however taking the font size from the ctm
is dangerous at best; if the transform stretches the font it will give the
correct answer. So later the code should be changed to pass in the desired font
size.

Created attachment 8119 [details]
Output of text-rotate after the patch

This output is still incorrect. The rectangle drawn, taken from the extents of
the drawn glyphs, is too large. What is done to produce this seems ok; the only
issue I could find is the use of ATSGlyphIdealMetrics instead of
ATSGlyphScreenMetrics. The Ideal metrics don't include the height of the glyph,
so this is currently simulated by laying out the text vertically and getting
the ideal metrics of the vertical glyphs. ATSGlyphScreenMetrics produces more
information, and may match FT output better.

This doesn't pass make test yet. The text in the rest of the failures appears
to be offset - I'll investigate that next, but it may just be related to the
incorrect extents as above. Some errors are expected due to the lack of support
for mask in the quartz backend.

Brian, I believe you should use _cairo_matrix_compute_scale_factors().  Check
the ft backend for example.

As for Ideal vs Screen metrics, I agree.  Our extents are "ink" extents, so
Screen should be better.

Can you attach two patches?

Created attachment 8143 [details] [review]
compute font size with  _cairo_matrix_compute_scale_factor

As requested. I'm not so keen on the "CGAffineTransformMakeWithScaleFactors"
name introduced here, since there's another function already there called
"CGAffineTransformMakeWithCairoFontScale". Mine makes the transform with just
the scale factors, the other also uses the rotation. Suggestions?

I don't have any strong feelings about the function name.  So, should I commit
this?  How about ATSGlyphScreenMetrics?

I'd say commit it. ATSUI is still marked as experimental, and there will need to
be a cleanup pass before it gets marked as supported (theres lots of ignored
error conditions in there). Its more important at this stage to get it closer to
working.

As for the screen metrics: that's a no-go. I tried it and it wasn't producing
the inked extents for me, and in any case it produces device-space extents when
the ctm_inverse isn't available (making it available means calculating the
inverse or breaking the cairo api). These apis do produce the inked extents: 

ATSUGetGlyphBounds - needs the text layout for the glyphs. we can't get hold of
this, theres no api for producing a layout from a glyph.
ATSUMeasureTextImage - needs the text, not the glyphs. Again, no use.

Also both of these work in device space. The approach I'm working on is getting
the stroked extents of the path for the glyph - that's where 9378 appeared from.
I hope to have the measurements working later tonight in some form. 

In ATSUI, fetching text extents would have to be considered part of the 'toy'
api. You can do this much more directly outside of cairo.

Created attachment 8152 [details] [review]
fix the extents to use screen metrics instead of ideal metrics.

Well, after me saying this approach doesn't work, it does. I was just applying
the wrong scaling to get the measurements back into user space. I'm not 100%
sure of the y bearing calculation, and there are probably some off-by-one
errors, corrections for antialiasing etc that should be done. But this is
getting towards being 'good enough'.

Created attachment 8153 [details]
output after all the atsui patches so far. note extent boxes are tighter now.

Remaining in the scope of this bug: improving tests to allow per-backend
reference images. Also coverage of tests needs to be improved: the current set
don't scale the device space to be different from user space for fonts, and
don't display any text with descenders (which would have made the extents
problem more obvious).

Created attachment 8154 [details] [review]
replacement for the extents fix

Typo in previous patch - good catch Behdad. Here's an update.

Committed: f3b9f486cd763c7805ec041319817cfb8c51128a.

Behdad: can you please roll back f3b9f486cd763c7805ec041319817cfb8c51128a.

It looks much better on the test cases, but thats because the test cases aren't
testing enough. Theres no descenders in there, and if you change the test text
to eg 'cairg', the descender hangs outside of the extents. So its not really
improving things any, and in fact contains a typo which causes a warning, so
just nuke it.

The current method of calculating glyph sizes does not and cannot work. There is
in fact no way to calculate the y-bearing in ATSUI without starting with a text
layout, or measuring the paths directly.

The old code suckered me into thinking that it must've been close and just
needed tinkering, but its not. The y-bearing in both Ideal and Screen vertical
metrics are from the origin of a vertical layout,  which is at the top-centre,
to the  top of the glyph, . The y-bearing in an horizontal layout  is from the
baseline to the top of the glyph. The two can only be related if we also know
the distance from the origin of a vertical layout to the baseline of the glyph.

However we can't get this figure from ATSUI, and we'd need to, because its
making it up internally. Vera Sans doesn't store any vertical metrics, so its
not coming from the font.

The width and height we're calculating are also wrong (in that they are
typographic bounds). Width and height could be calculated from the stroke_extent
of the path of the glyph, and can also give us the y-bearing (just do this
bounding box calculation with the path origin at 0,0). 

In practice, people using ATSUI should not attempt to get glyph extents from
cairo, but should use ATSUGetGlyphBounds (which provides dev-space bounding
trapezoids, but only up to 31 at a time), or ATSUMeasureTextImage, which
measures the dev-space bbox of whole lines of text. Possibly we could expose
some ATSUI-specific api to fill extents in the cache from the glyph bounds, but
I'd rather leave that until someone has a use case.

In the meantime, we can measure the bounding box of the paths with something like:
typedef struct cairo_atsui_path_ref {
   CGMutablePathRef path;
   CGAffineTransform ctm;
}
//...
cairo_atsui_path_ref_t callbackData;
callbackData.path = CGPathCreateMutable();
callbacData.ctm = CGAffineTransformMake(...); // use appropriate cairo bits
status = ATSUGlyphGetCubicPaths (style, glyphID, 
   // these procs are all one-liners calling eg CGPathAddCurveToPoint 
   move_to_proc, line_to_proc, curve_to_proc, close_path_proc, 
   &callbackData, &status);
if (!status) {
	CGRect rect = CGPathGetBoundingBox(path);
	// etc
}
CGPathRelease(path);

I'll come up with a patch based on this approach, this time tested on a wider
variety of text (eg using c-cedilla, A-circumflex, to get ink above the
capheight and below the baseline).

Sorry for the churn. 

So, none of the patch is useful?

As for getting extents from cairo versus ATSUI, I don't agree.  The entire point
of cairo is to let people deal with one API.  Anyway, if you don't have a better
fix, I'll just revert this.

Created attachment 8190 [details] [review]
measure glyphs using path bounding boxes. 

I don't disagree on the single api Behdad, and I'm trying to make it work! I'm
just observing that at the point where an atsui user would get glyphs for
themselves (ie they aren't using the toy api) then they have all the
information needed to make one call to measure their text directly using atsui.


In any case: this latest patch fixes the issue properly, as I described above.
I've created it to be applied directly without the revert. A few test images
follow to show what changed.

NB since this uses paths to get the glyph extents, which means no bitmap fonts.
AFAIK bitmap fonts are unsupported in ATSUI.

Created attachment 8191 [details]
shows the broken behaviour with descenders before this patch

The descenders poke outside of the bounding box in this image.

Created attachment 8192 [details]
showing fixed descender handling

Created attachment 8193 [details]
Demonstrating that the 'cairo' rendering didn't regress.

Created attachment 8194 [details] [review]
Same patch, without the compile warning. oops.

Committed. 261d6b805c77dacb27d29d054e48ed548f1fbcc5

Comment on attachment 8143 [details] [review]
compute font size with  _cairo_matrix_compute_scale_factor

Committed first patch as d30b1bf157126668c4309731022b2ded2ad09571

This bug has outlived its usefulness. Most of the issues it describes are now patched, with one exception - the tests can't be made to pass with freetype-based images. This is covered in bug 9465.