[HarfBuzz] collect_glyphs() API

[Jonathan Kew]

On 4/12/12 22:21, Behdad Esfahbod wrote:
> Hi Werner and Jonathan,
>
> Just wanted to follow up and bring the new
> hb_ot_layout_lookup_collect_glyphs() to your attention.
>
> The implementation of this API is complete as far as I'm concerned.  HOWEVER,
> I've done absolutely zero testing.

I've finally gotten around to experimenting with this in Firefox, and 
run into a couple of issues that mean it doesn't quite work as-is:

(1) hb_ot_layout_collect_lookups(), called without a specific language 
tag, fails to take account of the DefaultLangSys in the script 
record(s), which means it misses some lookups -- in the (common) case 
where the script has ONLY a DefaultLangSys, it returns no lookups at all.

A possible fix is to explicitly collect lookups for the DefaultLangSys 
in addition to any specified languages.

(In _hb_ot_layout_collect_lookups_languages:)

     /* All languages... */
     unsigned int count = hb_ot_layout_script_get_language_tags (face, 
table_tag, script_index, 0, NULL, NULL);
     for (unsigned int language_index = 0; language_index < count; 
language_index++)
       _hb_ot_layout_collect_lookups_features (face, table_tag, 
script_index, language_index, features, lookup_indexes);
     /* ...including the DefaultLangSys, which is not in the array of 
LangSysRecords */
     _hb_ot_layout_collect_lookups_features (face, table_tag, 
script_index, OT::Index::NOT_FOUND_INDEX, features, lookup_indexes);

(2) The API claims that you can pass NULL for any of the glyph sets. 
Internally, it handles this by using hb_set_get_empty() in place of any 
NULL parameters. However, this breaks because the low-level 
collect_glyphs() functions will then be trying to add glyphs to the 
(static, const) empty set. Ouch. For me, it dies with a protection error 
because that object is read-only, and const_casting it to a non-const 
hb_set* doesn't magically make it writable. But even if it didn't die, 
it would still be bad because it would make the "empty" object no longer 
empty!

This applies not only to any NULL glyph-set parameters that the user has 
passed, but also the use of hb_set_get_empty() in the 
hb_collect_glyphs_context_t::recurse() function to replace the user's 
before/input/after sets while recursing.

This could be fixed at various levels: the hb_set methods that modify a 
set could be made no-ops if the set is hb_set_get_empty(), though this 
adds an extra test to all those low-level operations; or -- better, I 
think -- the collect_glyphs() methods could check for (and skip) NULL 
sets, so that we don't need to use the const "empty" set for this purpose.

(To avoid the risk of errors like this, maybe hb_set_get_empty should be 
returning a const set?)

> That has to wait until I have time to do a
> cmdline tool for all the introspection APIs.  As such, thought I give you a
> heads up so you can go ahead and test, err, use it.
>
> Jonathan, I believe you know how to use the API.
>
> Werner, here's how you use the API:
>
>    1. Create a new set, call lookups (hb_set_create ()).
>
>    2. Collect the index of the lookups you want to process into the set.
>
>    3. Create sets for "backtrack", "input", "lookahead", and "output" glyph
> sets.  They can point to the same object, or some can be NULL.
>
>    4. Iterate over the lookup indices, and call collect_glyphs() for each.
>
> Note that if you reuse a set, you are responsible for emptying it first.
>
> Here is how one iterates over the set:
>
>    hb_codepoint_t lookup_index;
>    for (lookup_index = -1; hb_set_next (lookups, &lookup_index);)
>        hb_ot_layout_lookup_collect_glyphs (face, HB_OT_TAG_GSUB, lookup_index,
> ...);
>
> I probably should change the set API to use unsigned int instead of
> hb_codepoint_t, but I'm not going to worry about it that much right now.
>
> As for step 2, collecting lookup indices, there are different things you can
> do.  Based on what you described to me before, I think using
> hb_ot_layout_collect_lookups() makes most sense for you.  Say, if you want the
> 'scmp' feature for all scripts and languages, you call:
>
>    hb_tag_t features[] = {HB_TAG ('s','c','m','p'), HB_TAG_NONE};
>    hb_ot_layout_collect_lookups (face, HB_OT_TAG_GSUB, NULL, NULL, features,
> lookups);
>
> This will collect all the lookup indices for all scmp features in all scripts
> / language systems into the lookups set.  You can of course limit this to
> specific scripts, etc.
>
> Hope that helps,
>