subsetting variable COLRv1 produces invalid font without ItemVarStore

[anthrotype]

Take FoldIt[wght].ttf font at https://github.com/google/fonts/tree/main/ofl/foldit, and run it through hb-subset, e.g.

$ hb-subset --output-file=Foldit.subset.hb.ttf 'FoldIt[wght].ttf' ABC

The subset font's COLR table still contains variable paints with VarIndexes and a DeltaSetIndexMap, however the ItemVariationStore has been deleted, so these indices point nowhere (which sometimes leads the browser tab to crash..).

I think the problem is here, where COLR is subsetted, you'll notice the DeltaSetIndexMap is copied as is, but the VarStore is not:

harfbuzz/src/OT/Color/COLR/COLR.hh

Lines 1970 to 1972 in 6c46da7

	colr_prime->varIdxMap.serialize_copy (c->serializer, varIdxMap, this);
	//TODO: subset varStore once it's implemented in fonttools
	return_trace (true);

The TODO in the comment refers to the fact that the python fonttools subsetter doesn't yet support subsetting COLR's VarStore and VarIndexMap: fonttools/fonttools#2698

However, even if it can't subset it, the fonttools subsetter keeps the entire VarStore. E.g.

$ fonttools subset --output-file=Foldit.subset.ft.ttf --text=ABC 'FoldIt[wght].ttf'

Compare the two font, the only difference is the fonttools-subsetted one contains a VarStore, the hb-subsetted one does not.

[anthrotype]

I tried adding the following, replicating the line where the DeltaSetIndexMap is copied over for the ItemVariationStore, however I get these compile errors, which I don't understand. But I'm sure you can figure it out.

diff --git a/src/OT/Color/COLR/COLR.hh b/src/OT/Color/COLR/COLR.hh
index d45dc4f03..c82f8c220 100644
--- a/src/OT/Color/COLR/COLR.hh
+++ b/src/OT/Color/COLR/COLR.hh
@@ -1969,6 +1969,7 @@ struct COLR
     colr_prime->clipList.serialize_subset (c, clipList, this);
     colr_prime->varIdxMap.serialize_copy (c->serializer, varIdxMap, this);
     //TODO: subset varStore once it's implemented in fonttools
+    colr_prime->varStore.serialize_copy (c->serializer, varStore, this);
     return_trace (true);
   }

[16/232] Compiling C++ object src/libharfbuzz-subset.0.dylib.p/hb-subset.cc.o
FAILED: src/libharfbuzz-subset.0.dylib.p/hb-subset.cc.o
c++ -Isrc/libharfbuzz-subset.0.dylib.p -Isrc -I../src -I. -I.. -fcolor-diagnostics -Wall -Winvalid-pch -Wnon-virtual-dtor -std=c++11 -fno-exceptions -fno-rtti -O0 -g -fno-exceptions -fno-rtti -fno-threadsafe-statics -fvisibility-inlines-hidden -DHAVE_CONFIG_H -Wno-non-virtual-dtor -MD -MQ src/libharfbuzz-subset.0.dylib.p/hb-subset.cc.o -MF src/libharfbuzz-subset.0.dylib.p/hb-subset.cc.o.d -o src/libharfbuzz-subset.0.dylib.p/hb-subset.cc.o -c ../src/hb-subset.cc
In file included from ../src/hb-subset.cc:28:
In file included from ../src/OT/Color/sbix/../../../hb-open-type.hh:37:
In file included from ../src/hb-subset.hh:36:
../src/graph/../hb-serialize.hh:650:12: error: no matching member function for call to '_copy'
  { return _copy (src, hb_prioritize, std::forward<Ts> (ds)...); }
           ^~~~~
../src/OT/Color/sbix/../../../hb-open-type.hh:404:19: note: in instantiation of function template specialization 'hb_serialize_context_t::copy<OT::VariationStore>' requested here
    bool ret = c->copy (src_base+src, std::forward<Ts> (ds)...);
                  ^
../src/OT/Color/sbix/../../../hb-open-type.hh:413:12: note: in instantiation of function template specialization 'OT::OffsetTo<OT::VariationStore, OT::IntType<unsigned int, 4>, true>::serialize_copy<>' requested here
  { return serialize_copy (c, src, src_base, dst_bias, hb_serialize_context_t::Head); }
           ^
../src/OT/Color/COLR/COLR.hh:1972:26: note: in instantiation of member function 'OT::OffsetTo<OT::VariationStore, OT::IntType<unsigned int, 4>, true>::serialize_copy' requested here
    colr_prime->varStore.serialize_copy (c->serializer, varStore, this);
                         ^
../src/graph/../hb-serialize.hh:634:3: note: candidate template ignored: substitution failure [with Type = OT::VariationStore, Ts = <>]: no member named 'copy' in 'OT::VariationStore'
  _copy (const Type &src, hb_priority<1>, Ts&&... ds) HB_RETURN
  ^
../src/graph/../hb-serialize.hh:638:3: note: candidate template ignored: substitution failure [with Type = OT::VariationStore]: object of type 'OT::VariationStore' cannot be assigned because its copy assignment operator is implicitly deleted
  _copy (const Type &src, hb_priority<0>) -> decltype (&(hb_declval<Type> () = src))
  ^                                                                          ~
1 error generated.
ninja: build stopped: subcommand failed.

[jfkthame]

The subset font's COLR table still contains variable paints with VarIndexes and a DeltaSetIndexMap, however the ItemVariationStore has been deleted, so these indices point nowhere (which sometimes leads the browser tab to crash..).

Which browser, and was the subsetted font installed locally or loaded as a @font-face resource? (Sounds like this may be something that COLR validation in OTS needs to check...)

[anthrotype]

Chrome 109, but it's already fixed in beta/canary (I believe since 110).
The bug was fixed by @behdad in freetype a couple of months ago:
https://gitlab.freedesktop.org/freetype/freetype/-/commit/e97cb9e8da39673caeadf4b99a3aa1fb9e4c7301

Firefox did not crash, simply does not variate the COLR table. OTS doesn't complain either about the missing VarStore, since it is technically an optional field. Generally the problem is not its presence/absence, but the fact that some variable paints may contain VarIndexBase that ultimately point out-of-range. In this case, since the VarStore is absent all remaining VarIndex are out of range.

[anthrotype]

loaded as a https://github.com/font-face resource

yes, loaded as a font-face

[behdad]

I tried adding the following, replicating the line where the DeltaSetIndexMap is copied over for the ItemVariationStore, however I get these compile errors, which I don't understand. But I'm sure you can figure it out.

That's because the VariationStore, unlike DeltasSetIndexMap, does not implement a copy function. It's more work to implement that.

[behdad]

It has a serialize method though. Let me see if I can use that quickly to implement a copy.

[behdad]

cc @qxliu76

[behdad]

Actually, @qxliu76 can you work on this?

[qxliu76]

yes I'll work on this.

[behdad]

@qxliu76 I fixed it. Would be good if you can add a test. And ideally implement actual subsetting.

[qxliu76]

Oh, thanks! That's impressively fast!