[HarfBuzz] collect_glyphs() API

[Behdad Esfahbod]

On 12-12-31 11:33 AM, Jonathan Kew wrote:
> 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.

I see.  I'll think about it.  Not sure how much control is desired on that.


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

Ah my bad.  Indeed the idea was that trying to modify the empty set would fail
silently.  That's the pattern in all other HarfBuzz API, and unless we see the
performance difference really matters, I'd rather fix it that way.


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

That would limit how it can be used.  I do have to check API for const
correction sometime, will get back to this one then.


> (3) The collect_class callback expects to find the ClassDef in the data
> pointer of the lookup_context, but in a couple of places (ContextFormat2,
> ChainContextFormat2) the code just sets this field to NULL. This will crash
> when the callback is used. Patch:

Thanks.  That part of code really can use some templatizing and code sharing...

I'll try to fix these today.

behdad