Re: [PATCH] Font antialiasing v2

[Evgeny Kolesnikov]

On Fri, 2010-04-02 at 22:23 +0200, Vladimir 'φ-coder/phcoder' Serbinenko
wrote:

> -     for (j = font_info->ranges[i * 2]; j <= font_info->ranges[i * 2 + 1];
> -          j++)
> -       add_char (font_info, face, j);
> +           for (j = font_info->ranges[i * 2]; j <= font_info->ranges[i * 2 + 
> 1]; j++)
> +             add_char (font_info, face, j);
> 
> Can you fix the style of your patch to avoid hunks like these? (Hint:
> GNU indent)

Most of sources related to font functionality contains awful mix of
spaces and tabs. Should I fix this? Strictly use spaces everywhere? 

> -  glyph_bitmap.mode_info.blit_format = GRUB_VIDEO_BLIT_FORMAT_1BIT_PACKED;
> -  glyph_bitmap.mode_info.bpp = 1;
> -
> -  /* Really 1 bit per pixel.  */
> -  glyph_bitmap.mode_info.bytes_per_pixel = 0;
> -
> These bits need to be filled even if blit_format is set

OK. 

> +#define FONT_FORMAT_STORAGE_PACK_MASK 0x0F
> +#define FONT_FORMAT_STORAGE_PACKED 1
> +#define FONT_FORMAT_STORAGE_DEPTH_MASK 0xF0
> +#define FONT_FORMAT_STORAGE_1BIT 0
> +#define FONT_FORMAT_STORAGE_8BIT_GRAY 32
> Using entire byte for this is quite a waste. It's better to use 2 or 3 last 
> bit as STORAGE_FORMAT

This byte is already wasted with packed/unpacked bit. 
3 bits actually, others as reserved. And I used reserved ones.
See http://grub.gibibit.com/New_font_format (PFF2 spec, CHIX, item 2).

> Also I doubt usefulness of a font in which only some glyphs are anti-aliased. 
> Perhaps we could move antialiasing flags to file header and shave off few 
> bytes?

Yes this makes sense. I.e. substitution glyph for unknown symbol 
have no AA. Also frames and other geometry can benefit from this.
Anyway this byte is already wasted in original spec.

Moreover I thinking about transparent gz or xz reader for font
file. This will drastically reduce the size.

> Another point is that although storage_flags are present since some time 
> trying to use antialised fonts in older grub will result in garbage. I prefer 
> to put PFF3 signature if any flag is used. It will also allow more freedom it 
> fields specification,

Well, generally I agree. This will be more evident in case of troubles.

> +  fgcolor = grub_video_fb_map_rgba (src->mode_info->fg_red,
> +                                 src->mode_info->fg_green,
> +                                 src->mode_info->fg_blue,
> +                                 src->mode_info->fg_alpha);
> background color isn't handled correctly (it's not always transparent)
> +  grub_uint8_t fa, a;

Actually it always 0x00000000. See grub_font_draw_glyph: 
there is only FG color in declaration.

Moreover I don't think that bg color is useful because
we handle alpha channel correctly for fg and mask itself,
so anyone can blit glyph onto everything he want.

Also filling rectangle with color below text and then
rendering only fg mask will be way more fast.

But if you insist we should fix grub_font_draw_glyph and all
it's callers at first place. How exactly?

> Please avoid mixing declarations and code

OK.