[skera] Preserve BASE versions and coordinate formats

skera/src/base.rs

@@ -1,4 +1,4 @@
-//! impl subset() for hmtx
+//! Subset BASE axes, coordinates, and their variation store.
 
 use crate::{
     offset::{SerializeCopy, SerializeSubset},
@@ -13,7 +13,7 @@ use read_fonts::{
         BaseLangSysRecord, BaseScript, BaseScriptList, BaseScriptRecord, BaseValues,
         FeatMinMaxRecord, MinMax,
     },
-    types::{FixedSize, GlyphId, MajorMinor, Offset16, Offset32},
+    types::{Compatible, FixedSize, GlyphId, Offset16, Offset32},
     FontData, FontRef, MinByteRange, TopLevelTable,
 };
 use write_fonts::FontBuilder;
@@ -61,27 +61,26 @@ impl Subset for Base<'_> {
                 .map_err(|_| SubsetError::SubsetTableError(Base::TAG))?;
         }
 
-        //itemVarStore offset
-        if let Some(var_store) = self
-            .item_var_store()
-            .transpose()
-            .map_err(|_| SubsetError::SubsetTableError(Base::TAG))?
-        {
+        // BASE 1.1 retains this field even when the store offset is null.
+        if self.version().compatible((1, 1)) {
             let varstore_offset_pos = s
                 .embed(0_u32)
                 .map_err(|_| SubsetError::SubsetTableError(Base::TAG))?;
-
-            Offset32::serialize_subset(
-                &var_store,
-                s,
-                plan,
-                (&plan.base_varstore_inner_maps, false),
-                varstore_offset_pos,
-            )
-            .is_empty()
-            .map_err(|_| SubsetError::SubsetTableError(Base::TAG))?;
-        } else if self.version().minor > 0 {
-            s.copy_assign(0, MajorMinor::new(1, 0));
+            if let Some(var_store) = self
+                .item_var_store()
+                .transpose()
+                .map_err(|_| SubsetError::SubsetTableError(Base::TAG))?
+            {
+                Offset32::serialize_subset(
+                    &var_store,
+                    s,
+                    plan,
+                    (&plan.base_varstore_inner_maps, false),
+                    varstore_offset_pos,
+                )
+                .is_empty()
+                .map_err(|_| SubsetError::SubsetTableError(Base::TAG))?;
+            }
         }
         Ok(())
     }

skera/src/instance/layout.rs

@@ -3,7 +3,7 @@ use super::{rebase::renormalize, scalars::VariationScalars, AxisPlan, StorePlan}
 use crate::SubsetError;
 use read_fonts::{
     tables::variations::{DeltaSetIndex, ItemVariationStore},
-    types::{F2Dot14, Tag},
+    types::{Compatible, F2Dot14, Tag},
     FontRef, TableProvider,
 };
 use std::cell::{OnceCell, RefCell};
@@ -475,11 +475,6 @@ impl Apply for BaseCoord {
     fn apply(&mut self, c: &Context) -> Result<(), SubsetError> {
         if let Self::Format3(a) = self {
             c.apply(&mut a.coordinate, &mut a.device)?;
-            if a.device.is_none() {
-                *self = Self::Format1(BaseCoordFormat1 {
-                    coordinate: a.coordinate,
-                });
-            }
         }
         Ok(())
     }
@@ -1087,10 +1082,31 @@ pub(super) fn instance(
                 .map(Into::into)
                 .unwrap_or_default()
         };
-        save(tables, b"BASE", &table)?;
+        save(tables, b"BASE", &VersionedBase(&table, base.version()))?;
     }
     Ok(())
 }
+
+// The owned BASE writer chooses a version from the presence of its store.
+// HarfBuzz preserves the source version after dropping an instantiated store,
+// including the null Offset32 field required by a BASE 1.1 header.
+struct VersionedBase<'a>(&'a Base, read_fonts::types::MajorMinor);
+impl write_fonts::FontWrite for VersionedBase<'_> {
+    fn write_into(&self, writer: &mut write_fonts::TableWriter) {
+        self.1.write_into(writer);
+        self.0.horiz_axis.write_into(writer);
+        self.0.vert_axis.write_into(writer);
+        if self.1.compatible((1, 1)) {
+            self.0.item_var_store.write_into(writer);
+        }
+    }
+}
+impl write_fonts::validate::Validate for VersionedBase<'_> {
+    fn validate_impl(&self, ctx: &mut write_fonts::validate::ValidationCtx) {
+        self.0.validate_impl(ctx);
+    }
+}
+
 fn save(
     tables: &mut BTreeMap<Tag, Vec<u8>>,
     tag: &[u8; 4],

skera/tests/base_test.rs

@@ -0,0 +1,188 @@
+//! BASE keeps its original header version, including a null 1.1 store offset.
+//! References use an equivalent synthetic font and HarfBuzz c82300aefb.
+use skera::{instance_font, parse_axis_limits, subset_font, Plan, SubsetFlags};
+use write_fonts::read::tables::base as read;
+use write_fonts::{
+    from_obj::ToOwnedTable,
+    read::{collections::IntSet, FontData, FontRead, FontRef, TableProvider},
+    tables::{base::*, layout::*, variations::*},
+    types::{F2Dot14, MajorMinor, Tag},
+    FontBuilder,
+};
+
+fn base(variable: bool, minor: u16) -> Vec<u8> {
+    let axis = || {
+        Axis::new(
+            Some(BaseTagList::new(vec![Tag::new(b"ideo"), Tag::new(b"romn")])),
+            BaseScriptList::new(vec![BaseScriptRecord::new(
+                Tag::new(b"latn"),
+                BaseScript::new(
+                    Some(BaseValues::new(
+                        1,
+                        vec![
+                            BaseCoord::format_1(100),
+                            BaseCoord::format_3(
+                                200,
+                                Some(if variable {
+                                    VariationIndex::new(0, 0).into()
+                                } else {
+                                    Device::new(10, 12, &[1, 0, -1]).into()
+                                }),
+                            ),
+                        ],
+                    )),
+                    None,
+                    vec![],
+                ),
+            )]),
+        )
+    };
+    let mut table = Base::new(Some(axis()), Some(axis()));
+    if minor > 0 {
+        let pos = RegionAxisCoordinates::new(F2Dot14::ZERO, F2Dot14::ONE, F2Dot14::ONE);
+        let zero = RegionAxisCoordinates::new(F2Dot14::ZERO, F2Dot14::ZERO, F2Dot14::ZERO);
+        table.item_var_store = ItemVariationStore {
+            variation_region_list: VariationRegionList::new(
+                2,
+                vec![VariationRegion::new(vec![pos, zero])],
+            )
+            .into(),
+            item_variation_data: vec![ItemVariationData {
+                item_count: 1,
+                word_delta_count: 1,
+                region_indexes: vec![0],
+                delta_sets: (-1i16).to_be_bytes().to_vec(),
+            }
+            .into()],
+        }
+        .into();
+    }
+    let mut raw = write_fonts::dump_table(&table).unwrap();
+    if minor > 0 && !variable {
+        raw[8..12].fill(0);
+    }
+    raw
+}
+fn font(raw: &[u8]) -> Vec<u8> {
+    let bytes = std::fs::read("test-data/fonts/Roboto-Variable.composite.ttf").unwrap();
+    let font = FontRef::new(&bytes).unwrap();
+    let mut builder = FontBuilder::new();
+    for r in font.table_directory().table_records() {
+        if r.tag() != Tag::new(b"avar") {
+            builder.add_raw(r.tag(), font.data_for_tag(r.tag()).unwrap());
+        }
+    }
+    builder.add_raw(Tag::new(b"BASE"), raw);
+    builder.build()
+}
+fn subset(bytes: &[u8]) -> Vec<u8> {
+    let font = FontRef::new(bytes).unwrap();
+    let plan = Plan::new(
+        &skera::populate_gids("*").unwrap(),
+        &IntSet::empty(),
+        &font,
+        SubsetFlags::SUBSET_FLAGS_NOTDEF_OUTLINE,
+        &IntSet::empty(),
+        &IntSet::all(),
+        &IntSet::all(),
+        &IntSet::all(),
+        &IntSet::all(),
+    );
+    subset_font(&font, &plan).unwrap()
+}
+fn check(bytes: &[u8], minor: u16, store: bool, expected: &Base) {
+    let font = FontRef::new(bytes).unwrap();
+    let base = font.base().unwrap();
+    assert_eq!(base.version(), MajorMinor::new(1, minor));
+    if minor > 0 {
+        assert_eq!(base.item_var_store_offset().unwrap().is_null(), !store);
+    }
+    let actual: Base = base.to_owned_table();
+    assert_eq!(actual, *expected);
+}
+#[test]
+fn null_stores_preserve_versions_axes_coordinates_and_hint_devices() {
+    for minor in [0, 1] {
+        let raw = base(false, minor);
+        let expected: Base = write_fonts::read::tables::base::Base::read(FontData::new(&raw))
+            .unwrap()
+            .to_owned_table();
+        let source = font(&raw);
+        check(&subset(&source), minor, false, &expected);
+        for limits in ["wght=650,wdth=100", "wght=650", "wght=300:700"] {
+            let bytes = instance_font(
+                &FontRef::new(&source).unwrap(),
+                &parse_axis_limits(limits).unwrap(),
+            )
+            .unwrap();
+            check(&bytes, minor, false, &expected);
+            check(&subset(&bytes), minor, false, &expected);
+        }
+    }
+}
+#[test]
+fn instancing_a_base_store_keeps_a_null_version_1_1_store_field() {
+    let source = font(&base(true, 1));
+    for (limits, store) in [("wght=650,wdth=100", false), ("wght=650", true)] {
+        let bytes = instance_font(
+            &FontRef::new(&source).unwrap(),
+            &parse_axis_limits(limits).unwrap(),
+        )
+        .unwrap();
+        let font = FontRef::new(&bytes).unwrap();
+        let base = font.base().unwrap();
+        assert_eq!(base.version(), MajorMinor::VERSION_1_1);
+        assert_eq!(base.item_var_store_offset().unwrap().is_null(), !store);
+        for axis in [base.horiz_axis(), base.vert_axis()] {
+            let axis = axis.unwrap().unwrap();
+            let list = axis.base_script_list().unwrap();
+            let script = list.base_script_records()[0]
+                .base_script(list.offset_data())
+                .unwrap();
+            let values = script.base_values().unwrap().unwrap();
+            let read::BaseCoord::Format3(coord) = values.base_coords().get(1).unwrap() else {
+                panic!()
+            };
+            assert_eq!(coord.coordinate(), 200);
+            assert_eq!(coord.device().is_some(), store);
+        }
+        assert_eq!(
+            FontRef::new(&subset(&bytes))
+                .unwrap()
+                .base()
+                .unwrap()
+                .version(),
+            MajorMinor::VERSION_1_1
+        );
+    }
+}
+
+#[test]
+fn base_tables_match_harfbuzz_with_null_and_instantiated_stores() {
+    for (variable, minor, operation) in [
+        (false, 0, "subset"),
+        (false, 1, "subset"),
+        (true, 1, "full"),
+    ] {
+        let source = font(&base(variable, minor));
+        let bytes = if variable {
+            instance_font(
+                &FontRef::new(&source).unwrap(),
+                &parse_axis_limits("wght=650,wdth=100").unwrap(),
+            )
+            .unwrap()
+        } else {
+            source
+        };
+        let bytes = subset(&bytes);
+        let reference = std::fs::read(format!(
+            "test-data/expected/base/{minor}-{}-{operation}.bin",
+            u8::from(variable)
+        ))
+        .unwrap();
+        let expected = read::Base::read(FontData::new(&reference)).unwrap();
+        assert_eq!(expected.version(), MajorMinor::new(1, minor));
+        let expected: Base = expected.to_owned_table();
+        check(&bytes, minor, false, &expected);
+    }
+}