Bug 9530

(In reply to comment #0)
> I'm not sure what the fix is but the problem can be found at least in
> 
> cairo_scaled_font_glyph_extents()
> 
> where the result from _cairo_scaled_glyph_lookup() is used as a cairo_status_t.

Hmm... the only non cairo_status_t value expected from
_cairo_scaled_glyph_lookup() would be CAIRO_INT_STATUS_UNSUPPORTED. But that's
never supposed to be returned from glyph_lookup when only asking for metrics,
(it's only when asking for something like a path from a bitmap font that
glyph_lookup can return unsupported).

Can you provide more information about what cairo_int_status_t value you're
getting and where it's coming from? (Sure would love that future feature of
"reversible debugging" from gdb right now...)

-Carl

Status is indeed UNSUPPORTED, as you say.

Here's a trace if that helps:

#0  cairo_scaled_font_glyph_extents (scaled_font=0x29abba0, glyphs=0xbfffed8c,
num_glyphs=1, extents=0xbfffed30) at cairo-scaled-font.c:722
#1  0x000c7f38 in pango_cairo_atsui_font_get_glyph_extents (font=0x153a4280,
glyph=65535, ink_rect=0x0, logical_rect=0xbfffede0) at pangocairo-atsuifont.c:182
#2  0x154a8bbd in set_glyph (font=0x3e9, glyphs=0x154a77d0, i=29442144,
offset=14, glyph=65535) at basic-atsui.c:94
#3  0x154a8df5 in basic_engine_shape (engine=0x153a3e20, font=0x153a4280,
text=0x29a8320 "#include \"config.h\"", length=19, analysis=0x28d1bf4,
glyphs=0x154a77d0) at basic-atsui.c:167
#4  0x004276a9 in pango_shape (text=0x29a8320 "#include \"config.h\"",
length=19, analysis=0x28d1bf4, glyphs=0x154a77d0) at shape.c:51
#5  0x0041ab29 in shape_run (line=0x1fffad0, state=0xbffff100, item=0x28d1be8)
at pango-layout.c:2920
#6  0x0041dbf4 in process_item (layout=0x28cd0d0, line=0x1fffad0,
state=0xbffff100, force_fit=1, no_break_at_end=0) at pango-layout.c:3012
#7  0x0041e101 in pango_layout_check_lines (layout=0x28cd0d0) at pango-layout.c:3247
#8  0x0041ea61 in pango_layout_get_extents_internal (layout=0x28cd0d0,
ink_rect=0x0, logical_rect=0xbffff330, line_extents=0x0) at pango-layout.c:2198
#9  0x011b1080 in gtk_text_layout_get_line_display (layout=0xfd1a0,
line=0x2988370, size_only=1) at gtktextlayout.c:2107
#10 0x011b2082 in gtk_text_layout_real_wrap (layout=0xfd1a0, line=0x2988370,
line_data=0x0) at gtktextlayout.c:1043
#11 0x011959e8 in _gtk_text_btree_validate_line (tree=0x2959e70, line=0x2988370,
view_id=0xfd1a0) at gtktextbtree.c:5366

I suspect there is a bug in the atsui code in pango that triggers this, but I
guess that the unsupported error still shouldn't be passed to
_cairo_scaled_font_set_error().

Created attachment 8293 [details] [review]
shows the atsu style tags that will trigger this bug

My bad. I needed to use paths to get hold of the inked extents that cairo
expects, atsui only gives inked extents for text or typographic extents for
glyphs. It will return CAIRO_INT_STATUS_UNSUPPORTED if it cannot produce paths
for metrics.

It should be possible to recover the metrics from the glyph surface if I
generate that, but there are circumstances where this will not work - eg if
italic/bold are selected from the toy api, or in future if we allow arbitrary
ATSUStyles instead of an ATSUFontID. (the attached patch shows the conditions
under which we can't even get a glyph surface without paths)

The root of the problem is the inability to draw styled glyphs using
CGContextShowGlyph*, which was previously being used in cairo. 

I'm open to suggestions.

Richard, do you have a test case for this? I tried HEAD and my own build with a
bitmap font, both worked (and produced metrics). I'm on 10.4.8. The font I tried
was '04b-31', a bitmap ttf. There is the whole apple disclaimer on bitmap fonts
though:

http://developer.apple.com/documentation/Carbon/Conceptual/ATS_Concepts/atsfonts_concepts/chapter_2_section_3.html

I'm seeing this when using pretty much any GTK+ app (with the quartz backend). I
can try to produce a smaller test case using only cairo, but I don't have much
time so I can't promise to have it done "soon" :/

#1  0x000c7f38 in pango_cairo_atsui_font_get_glyph_extents (font=0x153a4280,
glyph=65535, ink_rect=0x0, logical_rect=0xbfffede0) at pangocairo-atsuifont.c:182

Well, glyph=65535 is really suspicious here.

Ah, thanks for pointing me in the right direction :) As it turns out, that corresonds to 
kATSDeletedGlyphcode:

http://lockecole.rh.rit.edu/dev2/documentation/Carbon/Reference/ATS/atsfontsref_Reference/
chapter_1.4_section_4.html#//apple_ref/c/econst/kATSDeletedGlyphcode

I'm not sure I understand what the docs mean here (a glyph is deleted?) but it seems that mapping that to 
PANGO_GLYPH_EMPTY works. Does that make sense?

Ok, the old bad convention for deleted glyphs!  Mozilla uses that convention too.

What it means that it's deleted is that for example you have the text "fi". 
They are first converted to two glyphs, for "f" and for "i", then during further
processing they form a ligature and the first one is replaced by the glyph for
"fi", while the second one is "deleted".  To avoid having to memmove the entire
array, or the mappings, etc, some systems use a magic code...

You can either ignore it complete, or like you suggested, substitute
PANGO_GLYPH_EMPTY with width 0.  The latter is probably more, I don't know,
clean?  Anyway, go for it :).

Thanks, the deleted glyph makes sense now. I'll go with the EMPTY way since the pangocairo-atsui-font 
code is already setup to handle that, so it will be a clean patch.

We can close this bug report I guess, and I can file a new one in gnome bugzilla for pango. (Or is the 
original bug I reported also a bug? That UNSUPPORTED triggers the assert...)

(In reply to comment #9)
> Thanks, the deleted glyph makes sense now. I'll go with the EMPTY way since
the pangocairo-atsui-font 
> code is already setup to handle that, so it will be a clean patch.

Thanks.

> We can close this bug report I guess, and I can file a new one in gnome
bugzilla for pango. (Or is the 
> original bug I reported also a bug? That UNSUPPORTED triggers the assert...)

I think the assert just did what it was supposed to do.  But on further
thinking, you probably should fix it to return zero extents. 
cairo_get_glyph_extents() should not fail, even if glyph ids are invalid, etc.

In fact, even better would be to write a test for the cairo test suite that
tries to render out-of-range glyphs (negative ones, large ones, 0xffff, etc). 
So we catch similar problems in other backends too.

(In reply to comment #10)
> > We can close this bug report I guess, and I can file a new one in gnome
> bugzilla for pango. (Or is the 
> > original bug I reported also a bug? That UNSUPPORTED triggers the assert...)
> 
> I think the assert just did what it was supposed to do.  But on further
> thinking, you probably should fix it to return zero extents. 
> cairo_get_glyph_extents() should not fail, even if glyph ids are invalid, etc.

Right. There is still a bug in cairo here. But it's not that we should fix the
caller to expect UNSUPPORTED from _get_glyph_extents, (when asking only for
metrics). The bug is that that function should never be returning UNSUPPORTED in
that case anyway.

-Carl

(Tracking the pango part here http://bugzilla.gnome.org/show_bug.cgi?id=395328)

Created attachment 8393 [details] [review]
Test that triggers the bug in atsui (and in freetype)

This patch adds an XFAIL test which measures and displays a number of glyph ids
which are out of range. With atsui, this produces 'unsupported' statuses, with
ft, it produces 'out of memory' errors. (I think these are possibly bogus
statuses but I've not looked into the cause yet). I've also got a patch for the
atsui backend that measures glyphs directly, which makes it pass this test.

Brian, the reason the ft backend reports out-of-memory, is because of
cairo-ft-font.c:_cairo_ft_scaled_glyph_init():

    error = FT_Load_Glyph (scaled_font->unscaled->face, 
                           _cairo_scaled_glyph_index(scaled_glyph),
                           load_flags);
    
    if (error) {
        status = CAIRO_STATUS_NO_MEMORY;
        goto FAIL;
    }

So, all we need to do is to check what the returned error is, and act appropriately.

We need to decide what we're gonna do though.  I suggest:

  - If the glyph id is nonexistent, the backend should report
CAIRO_STATUS_INVALID_INDEX.

  - Scaled font layer acts if nonexistent glyphs had extents 0,0,0,0.  We also
change cairo_show_glyphs, cairo_glyph_extents, and
cairo_scaled_font_glyph_extents to return cairo_status_t, and return
INVALID_INDEX if at least one glyph id was such.  However, that error can be
ignored.

  - Possibly do similar for cairo_show_text, cairo_text_extents, and
cairo_scaled_font_text_extents too: they return something other than SUCCESS if
there was at least one char that didn't map to any glyphs.

(In reply to comment #14)
> Brian, the reason the ft backend reports out-of-memory, is because of
> cairo-ft-font.c:_cairo_ft_scaled_glyph_init():
[snip]

Yup. Should I push the test then?

> We need to decide what we're gonna do though.  I suggest:
> 
>   - If the glyph id is nonexistent, the backend should report
> CAIRO_STATUS_INVALID_INDEX.
>   - Scaled font layer acts if nonexistent glyphs had extents 0,0,0,0.  We also
> change cairo_show_glyphs, cairo_glyph_extents, and
> cairo_scaled_font_glyph_extents to return cairo_status_t, and return
> INVALID_INDEX if at least one glyph id was such.  However, that error can be
> ignored.

Ok. Since you seem to be suggesting that each font backend decides for itself
what nonexistent means, I'd suggest 0x0-0xffff is the valid range for atsui -
while 0x0-0xfffe is the usable range, reporting INVALID_INDEX for 0xffff would
just trip up code ported from freetype (ie this bug all over again, with a
different error code). I'd handle 0xffff as a special case in the font backend.
 
>   - Possibly do similar for cairo_show_text, cairo_text_extents, and
> cairo_scaled_font_text_extents too: they return something other than SUCCESS if
> there was at least one char that didn't map to any glyphs.

I'm not sure. Making these return non-zero status would break callers that just
checked for SUCCESS as soon as the user changes font (or if the user has
different fonts from the developer). Since the char should just map to glyphid 0
and get rendered as a box, do we need to do anything more? 

Are we debating public API changes here? (I'm not entirely sure.)

Either way, it seems to me the discussion would be better on the cairo list at
this point. Would one of you please post a summary of what we've learned and
what still needs to be decided?

Thanks,

-Carl

(In reply to comment #15)
> (In reply to comment #14)
> > Brian, the reason the ft backend reports out-of-memory, is because of
> > cairo-ft-font.c:_cairo_ft_scaled_glyph_init():
> [snip]
> 
> Yup. Should I push the test then?

Yes, please push the test.

> > We need to decide what we're gonna do though.  I suggest:
> > 
> >   - If the glyph id is nonexistent, the backend should report
> > CAIRO_STATUS_INVALID_INDEX.
> >   - Scaled font layer acts if nonexistent glyphs had extents 0,0,0,0.  We also
> > change cairo_show_glyphs, cairo_glyph_extents, and
> > cairo_scaled_font_glyph_extents to return cairo_status_t, and return
> > INVALID_INDEX if at least one glyph id was such.  However, that error can be
> > ignored.
> 
> Ok. Since you seem to be suggesting that each font backend decides for itself
> what nonexistent means, I'd suggest 0x0-0xffff is the valid range for atsui -
> while 0x0-0xfffe is the usable range, reporting INVALID_INDEX for 0xffff would
> just trip up code ported from freetype (ie this bug all over again, with a
> different error code). I'd handle 0xffff as a special case in the font backend.

No, the backend just needs to return UNSUPPORTED if the font doesn't have the
glyph.  That's all.  Are you saying that with ATSUI, there's no way to figure
out if the glyph exists or not? I don't mind you special-casing 0xffff, but a
portable application shouldn't let that reach cairo anyway.

> >   - Possibly do similar for cairo_show_text, cairo_text_extents, and
> > cairo_scaled_font_text_extents too: they return something other than SUCCESS if
> > there was at least one char that didn't map to any glyphs.
> 
> I'm not sure. Making these return non-zero status would break callers that just
> checked for SUCCESS as soon as the user changes font (or if the user has
> different fonts from the developer). Since the char should just map to glyphid 0
> and get rendered as a box, do we need to do anything more? 

That's exactly why I suggested this.  If you ignore the new return value, you
get the old behavior: boxes drawn, no error, nothing.  If you care about the
boxes, you check the return value.

If we wanted to make the box issue a fatal error, we'd made it set error on the
cairo_t, but I don't think that makes sense.  (maybe it does for glyphs, but not
for text).

Ok, I'll write to the list now.

Comment on attachment 8393 [details] [review]
Test that triggers the bug in atsui (and in freetype)

Test committed as 9e332eadad00e115fdcdbc816608a0537f5a9b9c

Created attachment 8466 [details] [review]
patch 1/2: don't show deleted glyphs

This patch ensures that deleted glyphs don't get shown. I've gone down this
route instead of measuring the glyph's inked area, since it turns out that this
is the root of the problem.

Created attachment 8467 [details] [review]
patch 2/2: out-of-range glyphs are deleted.

This one deals with glyphs outside the allowable range for ATSUI. Experimenting
with 'in-range' glyphs that are not in the font showed that they render as
boxes, the same as glyphid 0, and so that kind of error is undetectable. Once
both these patches are applied, the test passes.

pushed as c316b7220dcd59653533a487d81c5e3d71729218

The code now passes the tests, and the original bug went away in 
http://bugzilla.gnome.org/show_bug.cgi?id=395328

... so I'm going to mark this as fixed now.

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