Re: [PATCH] GSoC #10 new font engine (UTF-8 support)

[Colin D Bennett]

Hi Vesa,

I have implemented UTF-8 support in the new font engine.

I have also added smart glyph fallback behavior, which is used when the
requested font is missing a glyph from the text being rendered.  In
that case, all other loaded fonts are checked to see if any of them
have the requested character.  The font most similar (mainly in size)
to the current font is selected.  My example in the 'videotest' command
uses the '10x20' fixed font (the name is "Fixed 20" when requesting the
font) which provides all the Unicode characters that the test uses.
However, when the test uses Helvetica, which is missing many of the
non-ASCII characters, the smart glyph substitution uses glyphs from
other fonts (including Fixed 20) to render those characters.

To see what this looks like, look at
<http://grub.gibibit.com/Journal_2008-10-30_utf8_test.png>.  This is a
screen shot of the enhanced videotest (GSoC patch #12) rendering UTF-8
strings.  I don't have many good fonts here, but it is nice to see that
Helvetica 8 (the bottom font) uses smaller replacement glyphs for its
missing characters than the others do.  Without the glyph substitution,
you would see the 'unknown glyph' question-mark symbol instead.

Attached is my new patch which hopefully addresses your points below.

Summary of changes since the previous patch:
- UTF-8 text support (including 'videotest' changes to test it)
- Smart substitution for missing glyphs
- Text rendering code is in the font module instead of vbe
- Comment formatting fixed
- Single font.c file instead of multiple source files

Responses to your comments follow.

On Sun, 05 Oct 2008 11:50:01 +0300
Vesa Jääskeläinen <address@hidden> wrote:

> Colin D Bennett wrote:
> > Clean patch against SVN revision 1885.
> > 
> > Regards,
> > Colin
> 
> Thanks for the re-base.
> 
> Here are some comments about the patch.
> 
> Check the commenting of multi line comments.

Ok, fixed.

> 
> > +int
> > +grub_font_get_string_width (grub_font_t font, const char *str)
> > +{
> > +  int i;
> > +  int width;
> > +  struct grub_font_glyph *glyph;
> > +  grub_size_t len;
> > +
> > +  len = grub_strlen (str);
> > +
> > +  for (i = 0, width = 0; i < len; i++)
> > +    {
> > +      glyph = grub_font_get_glyph (font, str[i]);
> > +      width += glyph->device_width;
> > +    }
> > +
> > +  return width;
> > +}
> 
> I do not see this being utf-8 compatible eg. it breaks our Unicode
> displaying support. See how grub_putchar() handles this.
> 
> There is also the same problem in grub_video_vbe_draw_string().

Fixed.

> > === modified file 'conf/sparc64-ieee1275.rmk'
> > --- conf/sparc64-ieee1275.rmk       2008-09-08 12:52:30 +0000
> > +++ conf/sparc64-ieee1275.rmk       2008-10-05 04:30:04 +0000
> 
> As font code is common code. There is no need to modify sparc64
> specific makefile. You have correctly modified common.rmk so that is
> enough. If sparc64 support should be moved to use common.rmk like any
> other arch. Let it rot.

Ok, I reverted my changes to the sparc64 file.

> > === added file 'font/loader.c'
> > --- font/loader.c   1970-01-01 00:00:00 +0000
> > +++ font/loader.c   2008-10-05 04:30:04 +0000
> 
> > +/*
> > +   Read a 16-bit big-endian integer from FILE, convert it to
> > native byte
> > +   order, and store it in *VALUE.
> > +   Returns 0 on success, 1 on failure.
> > + */
> > +static int
> > +read_be_uint16 (grub_file_t file, grub_uint16_t * value)
> 
> Are you fan of bigendian or why did you choose that :) ?

Well, I had to choose a byte order, and I arbitrarily chose
big-endian (isn't that the "network byte order" used for TCP/IP, etc.?)

> > +struct grub_font_glyph *
> > +grub_font_get_glyph (grub_font_t font, grub_uint32_t code)
> 
> I would propose to but all public API's to one file.

Ok, it's unified into a single font.c -- this is good for
encapsulation, as well.  The 'font_internal.h' is gone.

> > === modified file 'include/grub/video.h'
> > --- include/grub/video.h    2008-10-05 04:28:39 +0000
> > +++ include/grub/video.h    2008-10-05 04:30:04 +0000
> 
> Now that you have moved glyph rendering out from specialized glyph
> rendering to bitmap rendering I would propose that no font rendering
> code resides in Video API. I think this is a good move.
> 
> I would move that to font.mod (or perhaps to graphical terminal?). We
> can adapt other graphical terminals to use video API to render stuff.
> It just takes a bit time. But I think this time is as good as any to
> start that process.

All font rendering functions are in the font module now.  Yes, this is
a much cleaner way to do it.

> Lets have another look at it after those modifications :)
> 
> Thanks,
> Vesa Jääskeläinen

Let me know how this looks.

Regards,
Colin

10_font-engine-utf8_2008-10-30.patch
Description: Text Data