Re: [PATCH] Font antialiasing v2

[Vladimir 'φ-coder/phcoder' Serbinenko]

Evgeny Kolesnikov wrote:
> 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? 
>
>   
I've run indent on the file in the tree. If you do the same on your tree
it should help.
You can also try generating patches with -bB
>> +#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).
>
>   
Well "current" doesn't mean "best possible". I want to know motivation
behind this packed/unpacked bit.
Mixing compression and font engine will make the code more complex and
bug prone. It's better to put compression layer below the font and make
font subsystem unaware of it. The only exception is if compression takes
advantage of knowing font structures.
>> 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.
>   
We can change the spec and make pff3. Also notice I didn't oppose to
glyph in memory having this info, only on disk
> Moreover I thinking about transparent gz or xz reader for font
> file. This will drastically reduce the size.
>
>   
You can already gz it.
>   
>> +  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.
>   
Ok then

-- 
Regards
Vladimir 'φ-coder/phcoder' Serbinenko

signature.asc
Description: OpenPGP digital signature