Re: Contributing COLRv1

[Dominik Röttsches]

Hello Werner,

I really appreciate that you took the time and put all the effort into
working through this larger set of patches with reviews and fixups.
Thank you very much!

On Sat, Jan 2, 2021 at 11:38 AM Werner LEMBERG <wl@gnu.org> wrote:
> > With your agreement — and I am happy to hear feedback and answer
> > questions — I would like to start upstreaming our implementation of
> > COLRv1 support into FreeType.  [...]
>
> I've just pushed a branch 'colr' to our 'freetype2' git repository
> that holds a revised (and reformatted) version of your commits.
> Please check!

I took some notes on what formatting details I need to pay attention
to in the future (in particular with regards to argument lists, double
spaces after types and between doc sentences, use of tilde in docs,
and I took some hints on how to phrase commit messages with regards to
distinction between header and impl files.).

In short, all changes look good to me - from my point of view, they're
good to go for merging!

One really minor nit, and an entirely optional change: In the 4th
commit in the series, there are reformatting fixes changing code  that
was introduced in code in the third commit of the series. In other
words commit 223edbfc0 contains

@@ -389,7 +684,7 @@
     p = (FT_Byte*)( colr->base_glyphs_v1 +
                     base_glyph_v1_record.paint_offset );
-    if ( p >= (FT_Byte*)( colr->table + colr->table_size ) )
+    if ( p >= ( (FT_Byte*)colr->table + colr->table_size ) )
       return 0;

which may be better placed in cd3c13fa51d5, the previous commit, which
introduces this code. But as mentioned, totally optional and probably
not worth the hassle of rebaselining etc.


> > Among the things I would like to hear your feedback on: Would you
> > prefer a separate feature flag to be used to distinguish between
> > COLRv0 and COLRv1, or would we keep this new implementation under
> > the same feature flag TT_CONFIG_OPTION_COLOR_LAYERS?
>
> Not sure whether I've answered that already, but I think one config
> option for both COLR v0 and v1 is sufficient.

Sounds good to me, thanks. For reference and for others reading the
thread: That's the way it's done at the moment (compare
https://git.savannah.gnu.org/cgit/freetype/freetype2.git/commit/?h=colr&id=b546d8c883039e57584b2987b53f886fa3bda322
and PUT_COLOR_LAYERS_V1 mapping to PUT_COLOR_LAYERS.

What are the next steps - would you be ready to merge this branch? Is
there something else that I could help with?

Dominik