|
| From: | Behdad Esfahbod |
| Subject: | Re: [ft-devel] completed CPAL/COLR support |
| Date: | Sat, 30 Jun 2018 02:21:23 -0400 |
> I haven't got time to fully review the API. But just taking a look
> now, in the example in freetype.h:
>
> if ( palette && layer_glyph_index )
> {
> do
> {
> FT_Color layer_color = palette[layer_color_index];
>
>
> // Load and render glyph `layer_glyph_index', then
> // blend resulting pixmap with previously created pixmaps.
>
> } while ( ( layer_glyph_index =
> FT_Get_Color_Glyph_Layer( face,
> glyph_index,
> &layer_color_index,
> &iterator ) ) != 0 );
> }
>
> First, I think the use of a FT_LayerIterator type is unnecessary. A
> single index could have done.
Using a single index would be slower, since I had to derive the number
of glyph layers from `glyph_index' each time I call
`FT_Get_Color_Glyph_Layer'. Theoretically, I could implement a cache
just for this purpose, but somehow it doesn't feel right to extend
`FT_Face_Internal' for this.
> Second, the use of 0 return value to signify end of iteration is
> problematic given that 0 is a valid glyph index that can appear in
> the layers itself.
OK, will change.
> Third, "palette[layer_color_index]" is recipe for invalid memory
> access.
Is it? The code checks that `layer_color_index' is not out of bound.
Do you have a better idea?
> Let alone that it does not handle the foreground color magic number
> 0xFFFF.
What I could do is to make the magic number 0xFFFF completely
disappear by giving it index `num_palette_entries' while increasing
the size of `palette' (and `num_palette_entries') by one element...
> 1. I'm not sure the modifiable palettes are a good idea.
Yeah. Maybe my memory tricks me, but I somehow remember that this was
asked for...
> 2. Default foreground color of black makes more sense to me. Who
> reads white?
Please read again. It is white only if the palette is intended for a
dark background.
> 3. "palette_types" would have been better called "palette_flags",
> given that that's what is in the spec.
Makes sense. I'll change that.
| [Prev in Thread] | Current Thread | [Next in Thread] |