GNOME Bugzilla – Bug 313551
Hex box drawing for Cairo
Last modified: 2006-05-19 17:49:28 UTC
Right now unknown characters just vanish from Cairo output; this is bad, since you can easily create a corrupt document and not realize it. The hex-box-drawing code from Xft needs to be added to pangocairo-render.c (Some challenges to coordinate the rendering code, which is backend-independent, with the font selection and sizing code, which is in the backends ... PangoCairoFontIface may need extending ... this is fine, since it's private.)
As an end user, I would have to say having the hex-box rendered was fairly un-intuitive. It took me a little over two hours to find an explanation for why those little hex-boxes were there... So beyond an api, and programmatic angle, and looking from a usability angle. Quite confusing. The character that wasn't rendering was below, however I think I would kill at the moment for a configuration flag to turn that off, or use a different representative character for an unknown glyph. [not a printable character] U+0097 <control> General Character Properties Unicode category: Other, Control Various Useful Representations UTF-8: 0xC2 0x97 Octal escaped UTF-8: \302\227 Decimal entity reference: — Annotations and Cross References Alias names: • END OF GUARDED AREA
I'd consider this a blocker (really), as it can both make you create corrupt documents and loose arbitrarily large chunks of it while giving _absolutely no_ indication of the fact. As such, it's a data loss scenario (never thought it'd be possible in Pango...) and should be fixed before anything else. Owen, is it OK to set severity to blocker here? I only run pangocairo since two weeks and have already been bitten by that more times than I can count (and I don't work with obscure scripts). It's a pity I'm not able to contribute anyhing more constructive, but I really think it's a top priority issue. The target version has been already missed twice.
Why cannot you contribute anything yourself?
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=169639 Red Hat Bugzilla equivalent report with some suggestions of implementation details. Please chime in if you have any better ideas of how this should be done.
Behdad: lack of knowledge of pango enough to do it quickly, and lack of time to do it slowly.
Created attachment 55661 [details] [review] dashed-box patch I applied the attached patch to HEAD, that simply draws an empty dashed box on missing glyphs. Better than nothing. Should be ported to pango-1-10 branch.
Created attachment 55970 [details] [review] draw the box and hex code on missing glyphs I have already fixed the bug, it can draw the box and hex code on missing glyphs. Please review the patch.
Thanks. While the patch *works* on Linux, it breaks everything on other systems. pangocairo_render.c is and should remain independent of the font technology used, so, you cannot use/include pangocairo-fcfont.h in it. As Owen explained, this needs some changes to PangoCairoFontIface. And while we're at it, I like to see the code for hexbox abstracted and reused for all backends, instead of duplicating. And please use diff -u next time :).
I think it's possible to write the code in _pango_xft_font_get_mini_font to be generic in the Cairo case: _pango_xft_font_map_get_info (fcfont->fontmap, &display, &screen); context = pango_xft_get_context (display, screen); Can be written as: PangoFontMap *fontmap = pango_font_get_font_map(font); context = pango_cairo_font_map_create_context(fontmap); pango_cairo_update_context(crenderer->cr, context); The computation of mini_width, mini_height can be done by doing using cairo_save(crenderer->cr); _pango_cairo_font_install (PANGO_CAIRO_FONT (mini_font), crenderer->cr); [...] cairo_text_extents(crenderer->cr, digit, &extents); [...] cairo_restore(crenderer->cr); So, then the only question is how to cache this stuff so it doesn't have to be computed every character, You can use g_object_set_data_full() on the PangoFont for that. (Create a structure and store things in the structure.) [ Would it be OK to compute it every character? Someone might load a web page in firefox with 100,000 unknown characters in it, so probably not ]
Created attachment 56192 [details] [review] draw hexbox on missing glyph without font technology I write a patch about drawing hexbox on missing glyph without font technology. Please review the patch. :)
I would probably just use an array for the hexdigits: const char hexdigits[] = "0123456789ABCDEF"; and then hexdigits[i]; I don't see the reason to call memset (hexbox_string, 0, sizeof (hexbox_string)) in two places. You could either just do it once before the call to g_unichar_to_utf8, or just do len = g_unichar_to_utf8 (...); hexbox_string[len] = 0; But why do you call g_unichar_to_utf8() in the first place, considering you will only see hex digits ? Is this for non-latin digits ? I think I'd rather see us use standard hex digits all the time. If we can adjust the code the emit the digits in reverse, we could just do something like: while (foo > 0) { bar = foo & 0xf; foo = foo >> 4; emit (hexdigit[bar]); }
Created attachment 56249 [details] [review] draw hexbox on missing glyph (raise the efficiency) delete memset() and g_unicode_to_utf8() during drawing hexbox to raise the efficiency. add "const char hexdigits[] = "0123456789ABCDEF";" and modify "c[0] = i < 10 ? '0' + i : 'A' + i - 10;" to "c[0] = hexdigits[i]" in _pango_cairo_get_mini_font() to raise the efficiency.
The code looks generally good, just that the caching doesn't make any sense. Like Owen said, we should use g_object_set_data_full to cache the minifont info. Other than that, I'm not sure that *mini_pad = MAX (height / 10, 1); makes any sense in the cairo backend.
At first I used g_object_set_data() and g_ogject_get_data() to cache the minifont info, and when I talked with Owen, he said that I should use a structure to cache the minifont info. So I modified it. I debuged "*mini_pad = MAX (height / 10, 1);", and It worked well.
Created attachment 56396 [details] [review] new pango_cairo_draw_hexbox.patch Thanks for all the suggestions, I updated the patch, now minifont is cached by g_object_set_data_full() and the minipad is calculated by space left after the calculation of hex character's width: mini_pad = MAX ((((gi->geometry.width / PANGO_SCALE) - cols * mini_width) / (cols + 3)), 1);
Created attachment 56435 [details] [review] draft patch I'm attaching a draft of the patch the way I like to see. Remains to do: * Implement glyph_extents_missing for pangocairo like the one in pangoxft, to set the width of the unknown glyphs according to the mini font extents and the number of columns. * Take care of ascent and decent. * Fix the pad = MAX(., 1). My problem with this is that in cairo, "1" is in no way special. It doesn't necessarily mean 1 pixel, it may be 1 PS point for example, or 1 inch, depending on the transformations, backends, etc. Or am I wrong? Anyway, this really needs work.
Created attachment 56538 [details] [review] draw hexbox and add the code of getting the width of missing glyph I have already added the code about getting the width of missing glyph. So you can get the ink_rect and logical_rect of the missing glyph. I did not modify the pad = Max (..., 1); I call the cairo_scaled_font_glyph_extents() and cairo_text_extents() to get the width and height in cairo_text_extents_t in user-space coordinates. So in the "pad = Max (height / 10, 1);", "1" means that it is in user-space coordinates, the same as height and width. Who can help me to fix the "1"? :)
Patch looks good. I will test/clean-up a bit and commit. I like to take care of decent for example.
Thank you very much! :) I will learn more and try to fix the other bugs. :)
Created attachment 56727 [details] [review] almost there Humm, seems like your patch needed more work than I expected. Anyway, attached patch is *almost* done, except for: * When I run gucharmap with it, I see hexboxes dancing up and down. There's something wrong with vertical extents too. * There is a TODO item in the patch. I need a way to convert from gunichar to PangoGlyph in pangocairo-render.c, but donno how to do. One way is using cairo toy text api, but I'm avoiding that since I don't have a cairo context around. Another way is to use the approximate digit width and font extents instead, but that leaves a lot of whitespace around the digits. Any idea? Owen? * Needs some finetuning such that looks better on the screen. Since it's cairo backend, I don't align anything or use integers at all. That makes a fuzzy look.
And also, with this changes, both missing_get_extents and get_hex_box_info should go into pangocairo-font.c, and we can almost easily get cairo-win32 and cairo-atsui hexboxes working too.
Basically what we need is a function cairo_scaled_font_text_extents, such that we don't have to convert chars to glyphs, but there's no such function in cairo. There is a cairo_text_extents, but as I said, that takes a cairo context, so we need to create a cairo context and install the font, then use cairo_text_extents. Not the best approach, but possible.
Created attachment 56749 [details] [review] final patch Ok, this patch solves the issues above, except for the tuning part, but the output is pretty good already. It can use a review, otherwise I will commit it in a week or so.
For the cairo part, what I did is creating a 0x0 image surface, create a cairo context for it, install the font on the cairo context and use cairo_text_extents. Should work good enough.
I think you need to at least set the transform on the cairo_t you create to match that saved in Pango context. Would suggest you file a bug against Cairo asking for scaled_font_text_extents() and or a chars-to-glyphs() function. (I've opposed a chars-to-glyphs function in the past, since I think it is liable to misuse, but it might be a little simpler.)
Ah right. The problem is that I don't have ctm from the Pango context around. But that data is already saved somewhere in cairo_scaled_font_t, with no real use. Having getters for cairo_scaled_font_t, or a way to install an cairo_scaled_font_t that sets cairo ctm too will solve that. I file bugs agains cairo for both issues, whichever comes first we will use...
Ok, filed: Need cairo_scaled_font_text_extents https://bugs.freedesktop.org/show_bug.cgi?id=5495 Add getters for cairo_scaled_font_t https://bugs.freedesktop.org/show_bug.cgi?id=5496
Created attachment 57343 [details] [review] final final patch Ok, finally finished and committed. I noticed that my previous patch was working correctly already and we don't really need to set ctm, since the font size that we get from current font already has the effect of the ctm... Anyway, it seems to work correctly. The only problem I observe is that in small sizes, hintint and antialiasing make the output rather ugly at certain sizes... Not easy to fix... Turning cairo antialiasing on/off or playing with hinting options may help, but I prefer not to mess with them. Please test and see if you see any unacceptable output. 2006-01-14 Behdad Esfahbod <behdad@gnome.org> Draw hexbox for cairo backend. Bug #313551. Based on patch by LingNing Zhang. * pango/pangocairo-private.h (_PangoCairoFontIface): Add new methods: get_font_face and get_scaled_font, and getters: _pango_cairo_font_get_font_face, _pango_cairo_font_get_scaled_font. * pango/pangocairo-private.h: Add _PangoCairoHexBoxInfo, and getter _pango_cairo_get_hex_box_info, and _pango_cairo_get_glyph_extents_missing. * pango/pangocairo-fcfont.c, pango/pangocairo-atsuifont.c, * pango/pangocairo-win32font.c: Export get_font_face and get_scaled_font methods. * pango/pangocairo-fcfont.c: Use _pango_cairo_get_glyph_extents_missing on missing glyphs. * pango/pangocairo-font.c: Implement _pango_cairo_font_get_font_face, _pango_cairo_font_get_scaled_font, _pango_cairo_get_hex_box_info, and _pango_cairo_get_glyph_extents_missing. * pango/pangocairo-render.c (_pango_cairo_renderer_draw_unknown_glyph): Added. * pango/pangocairo-render.c (pango_cairo_renderer_draw_glyphs): Cleaned up to use the added function above.
I'm closing this, but people with win32 and ATSUI knowledge should be able to add hexbox support to win32 and atsui cairo backends quite easily. I opened bug #326960 for those. Feel free to submit patches.
The way to handle the ugliness issue would be to hint the positions of the hex box and characters inside it. You can figure out if hinting is appropriate by looking at the matrix for the font and at that cairo_font_options_t. (It makes no sense to hint when the matrix has a non-scale component.)
*** Bug 324146 has been marked as a duplicate of this bug. ***
Created attachment 58492 [details] [review] hinting patch The hexbox can't look any better now! 2006-01-31 Behdad Esfahbod <behdad@gnome.org> * pango/pangocairo-private.h, pango/pangocairo-font.c, * pango/pangocairo-render.c: Hint hexbox. Also draw a singl-row hexbox for very small sizes.