Re: [ft-devel] completed CPAL/COLR support

[Werner LEMBERG]

> 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.


    Werner