Tests

[kirawi]

cc @dfrg

I'm having some trouble with a few tests, so could you possibly take a look at what I need to do? I'll also take a look myself tomorrow. The results should be in the CI. I would guess around 200-300 of the fails are because of this: [uni1A45|uni1A32@592,0|uni1A5B.ratha_nohost@1523,0|uni1A69@1523,0] versus [uni1A45|uni1A321A5B@592,0|uni1A69@1184,-734] but I'm not sure how to do this.

(The code is very terrible right now, it'll be much better by merging time).

Generator:

```rs use std::{ fmt::{self, Display}, fs::File, io::{BufWriter, Write}, mem, path::PathBuf, }; #[derive(Clone, Copy, Debug, PartialEq, Eq)] pub struct ParseError; impl fmt::Display for ParseError { fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { write!(f, "missing closing quote") } } impl std::error::Error for ParseError {} enum State { /// Within a delimiter. Delimiter, /// After backslash, but before starting word. Backslash, /// Within an unquoted word. Unquoted, /// After backslash in an unquoted word. UnquotedBackslash, /// Within a single quoted word. SingleQuoted, /// Within a double quoted word. DoubleQuoted, /// After backslash inside a double quoted word. DoubleQuotedBackslash, /// Inside a comment. Comment, InPath, } pub fn split(s: &str) -> Result, ParseError> { use State::*; let mut words = Vec::new(); let mut word = String::new(); let mut chars = s.chars(); let mut state = Delimiter; loop { let c = chars.next(); state = match state { Delimiter => match c { None => break, Some('\'') => SingleQuoted, Some(c @ '[') => { word.push(c); SingleQuoted } Some('\"') => DoubleQuoted, Some('\\') => Backslash, Some('\t') | Some(' ') | Some('\n') | Some(':') | Some(';') => Delimiter, Some('#') => Comment, Some(c) => { word.push(c); Unquoted } }, Backslash => match c { None => { word.push('\\'); words.push(mem::replace(&mut word, String::new())); break; } Some('\n') => Delimiter, Some(c) => { word.push(c); Unquoted } }, Unquoted => match c { None => { words.push(mem::replace(&mut word, String::new())); break; } Some('\'') => SingleQuoted, Some(c @ '[') => { word.push(c); SingleQuoted } Some('\"') => DoubleQuoted, Some('\\') => UnquotedBackslash, Some('\t') | Some(' ') | Some('\n') | Some(':') | Some(';') => { words.push(mem::replace(&mut word, String::new())); Delimiter } Some(c @ '/') => { word.push(c); InPath } Some(c) => { word.push(c); Unquoted } }, UnquotedBackslash => match c { None => { word.push('\\'); words.push(mem::replace(&mut word, String::new())); break; } Some('\n') => Unquoted, Some(c) => { word.push(c); Unquoted } }, SingleQuoted => match c { None => return Err(ParseError), Some(c @ ']') => { word.push(c); Unquoted } Some('\'') => Unquoted, Some(c) => { word.push(c); SingleQuoted } }, DoubleQuoted => match c { None => return Err(ParseError), Some('\"') => Unquoted, Some('\\') => DoubleQuotedBackslash, Some(c) => { word.push(c); DoubleQuoted } }, DoubleQuotedBackslash => match c { None => return Err(ParseError), Some('\n') => DoubleQuoted, Some(c @ '$') | Some(c @ '`') | Some(c @ '"') | Some(c @ '\\') => { word.push(c); DoubleQuoted } Some(c) => { word.push('\\'); word.push(c); DoubleQuoted } }, Comment => match c { None => break, Some('\n') => Delimiter, Some(_) => Comment, }, InPath => match c { None => break, Some(';') => { words.push(mem::replace(&mut word, String::new())); Delimiter } Some(c) => { word.push(c); InPath } }, } } Ok(words) } #[derive(Default, Debug)] struct TestCase { name: String, path: PathBuf, font_size: usize, features: Vec<(String, u16)>, variations: Vec<(String, f32)>, input: Vec, output: Vec, } impl Display for TestCase { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { if self.output.contains(&"*".to_string()) { write!( f, "create_test!({}, {:?}, {}, &{:?}, &{:?}, &{:?});", self.name, self.path.to_str().unwrap(), self.font_size, self.features, self.variations, self.input, ) } else { write!( f, "create_test!({}, {:?}, {}, &{:?}, &{:?}, &{:?}, &{:?});", self.name, self.path.to_str().unwrap(), self.font_size, self.features, self.variations, self.input, self.output, ) } } } impl TestCase { pub fn parse(arguments: Vec) -> Self { let mut case = Self::default(); case.font_size = 75; for arg in arguments.iter() { if arg.starts_with("../") { // Path case.path = arg.into(); } else if arg.starts_with("--") { // Features/Variations let arg = arg.strip_prefix("--").unwrap(); if arg.contains("=") { // Variation let split: Vec<_> = arg.split("=").collect(); let variation: (String, f32) = ( split.first().unwrap().to_string(), split.last().unwrap().parse().unwrap_or(0.0), ); case.variations.push(variation); } else { // Feature case.features.push((arg.to_string(), 1)); } } else if arg.starts_with("[") || arg == "*" { // Output let output = arg.trim_matches(|chr| chr == '[' || chr == ']'); for chr in output.split('|') { // if chr.contains('=') { // Failed attempt at parsing runs/positions. // let mut split = chr.split('='); // let unicode = split.next().unwrap().to_string(); // let mut split_for_offsets = split.next().unwrap().split('+'); // let x = split_for_offsets.next().unwrap().parse().expect(chr); // let y = split_for_offsets.next().unwrap().parse().unwrap(); // case.output.push((unicode, x, y)); // } else { case.output.push(chr.to_string()); // } }; } } let input = arguments .get( arguments .len() .checked_sub(2) .unwrap_or_else(|| panic!("{:#?}", arguments)), ) .unwrap(); for chr in input.split(',') { let chr = chr.trim_start_matches("U+"); let parsed_chr: char = char::from_u32(u32::from_str_radix(chr, 16).expect(chr)).expect(chr); case.input.push(parsed_chr); } case } } fn main() -> std::io::Result<()> { let input = PathBuf::from("data"); let output = PathBuf::from("output"); let rendering_tests = input.join("text-rendering-tests"); let aot_tests = input.join("aots"); let inhouse_tests = input.join("in-house"); std::fs::create_dir_all(output.clone())?; if inhouse_tests.exists() { let directory = std::fs::read_dir(inhouse_tests.join("tests"))? .flatten() .filter(|file| file.file_type().unwrap().is_file()); let mut file = BufWriter::new(File::create(output.join("text-rendering-tests.txt"))?); // Iterate over each file in the directory and generate tests. for test in directory { let content = std::fs::read_to_string(test.path())?; for line in content.lines() { // file.write(format!("{:#?}\n", split(line).unwrap()).as_bytes())?; let val = split(line).unwrap(); if val.is_empty() { continue; } let formatted_string = format!("{}\n", TestCase::parse(val)); file.write(formatted_string.as_bytes())?; } } } else { panic!() } Ok(()) } ```

[dfrg]

I thought I had kept the code consistently formatted, but that appears to have not been the case. I'm going to go ahead and push a mass formatting commit to remove all the noise from the PR.

It seems that a lot of the failures may be due to missing script information. I'll address that in a review in a moment.

[kirawi]

Yep, I've applied those. The remaining fails appear to be legitimate bugs, some bug on my part with runs, another bug on my part with opening the font, and the aforementioned issue where it doesn't seem to "merge" certain glyphs (referenced up above). I'm not sure if the last problem is just because I'm missing something in the test harness implementation.

[dfrg]

Some of these are tests of ancient unused cmap formats that I'm unlikely to support. There also appear to be tests that should probably pass but are missing glyph names for some reason. A couple are showing a y-offset that doesn't match.

[kirawi]

I (painfully) manually went through the failing tests, and there appear to be only 134 legitimate ones. Some may still be errors on my part, such as off by one errors (possibly rounding), but I included them just in case.

Notably, gpos, hvar, shknda, shlana (though Harfbuzz themselves has all that disabled because of a problem they had as well), and morx.

[kirawi]

Do you want to disable shlana as well? I'm not sure what it is, but 125/209 fail.

[kirawi]

With the tests I've disabled, the ratio is now 361:148. 108 are the errors I noted as being legitimate. Pretty good! By the way, I included glyph name issues as legitimate errors as well, so the actual number of "legitimate" errors may be significantly lower, possibly ~30 less.

[dfrg]

If HarfBuzz isn't testing against it, then probably best to remove it for now.

[dfrg]

This looks great so far! Thanks so much for taking the time.

[kirawi]

Of note:

#[test]
fn morx_24_0() {
    assert_eq!(
        shape("fonts/TestMORXTwentyfour.ttf", 0, &[], "ABCDE"),
        "*"
    )
}

seems to cause an endless loop. I've disabled it for now.

[dfrg]

So the gvar tests are odd. Looks like it's failing to load the fonts outright, so it might be using the old Apple style true header word or something. I'm not sure what's going on in those tests, but if they're expecting to extract mark offsets from contour points, then those will definitely be ignored as I don't have any intention of loading/scaling/hinting outlines during layout.

The hvar tests are good from what I can see. The interpolation is likely to be slightly different depending on implementation of the item variation store code (floating point vs fixed point), so off-by-one errors are to be expected.

The morx tests that panic with OOB in the glyph buffer are very troubling, so I'll likely prioritize those.

[kirawi]

So I think for the rounding errors, what I think we could do is use a tuple struct over the vec used in shape(), and then implement the compare trait and allow it to be equal within <1 of error. We might want to identify what is actually rounding error or a logic bug first before that though.

[dfrg]

Agreed with rounding errors. I'd rather leave them in for the moment to make sure they're not errors in the lookups.

[dfrg]

So for RTL runs, it looks like it has the proper glyph order but incorrect offsets. This is going to be tricky because HarfBuzz simply handles this differently. It does a full logical reverse of the glyph buffer and provides appropriate offsets (so marks precede bases). The swash shaper maintains logical order and provides per-cluster mark offsets with the intention that clusters will be reversed but not glyphs. There is a mapping between the two but I can't think of a trivial change to the shape function right now.

[dfrg]

Some fixes pushed to master should correct the failed font loading in the gvar tests. I believe adding .insert_dotted_circles(true) while building the shaper should also fix most of the Balinese tests. The RTL tests will require more work and may need some changes to the shaper. I'm going to have to give some more thought to this.

[kirawi]

insert_dotted_circles(true) had less passes compared to insert_dotted_circles(false). I'm not sure what's going on there.

[kirawi]

Of note:

#[test]
fn morx_24_0() {
    assert_eq!(
        shape("fonts/TestMORXTwentyfour.ttf", 0, &[], "ABCDE"),
        "*"
    )
}

seems to cause an endless loop. I've disabled it for now.

This might be on me here, I notice in the original test https://github.com/harfbuzz/harfbuzz/blob/main/test/shaping/data/text-rendering-tests/tests/MORX-24.tests it doesn't specify the font size. It might be a good idea to have something break the infinite loop when the font size is 0, though.

[kirawi]

Are AOTs supported? I'm finding that the shaper doesn't really work at all with the AOT tests.

[dfrg]

The tests with the "*" as a result seem to indicate some sort of issue with the shaping tables in the font. It looks like those might exercise limits in recursion which swash implements for GSUB but not (yet) for morx.

I'll look into the AOTs but I don't see any reason why they shouldn't work. It seems like they all use direct glyph ids rather than names and also seem to enable some OpenType features.

[dfrg]

So the AOTs seem to be using control characters which the shaper simply removes. I've added an option to keep these which should be enabled for these tests: ShaperBuilder::retain_ignorables(bool).

It looks like you'll also need to parse the --features flag and pass those like variations to ShaperBuilder::features as a sequence of (&str, u16) where the value is always set to 1.

As mentioned, the AOTs use raw glyph ids rather than glyph names, and some of the tests disable positions.

It really would be nice if these tests were somehow standardized rather than using arbitrary hb-shape command line flags :(

[kirawi]

The latest commits caused some regressions.

[dfrg]

Thanks. Removed a speculative change that didn't pan out. Added a fix that might cover some of those tests with correct glyphs but incorrect offsets.

[kirawi]

57 more tests to go :)

I'll try to get the remaining AOT and In-House tests pushed today. They're fairly compatible with the existing stuff written for the rendering tests, but I'll go ahead and rewrite the generator tool since I'm finding it unsatisfactory.

[dfrg]

We should probably disable GPOS-2, GSUB-1, KERN-1 and KERN-2. The results are correct, but use glyph names pulled from the CFF table and I'd really rather not add all that code to support SIDs and Adobe charsets.

[kirawi]

I found out allsorts has their tests available as XML. I'm not sure if it would be worth switching at this point though.

[kirawi]

With the latest commit, morx_34_0 and morx_36_0 suffer from an infinite loop as well. This time I checked and I don't believe the infinite loops have anything to do with font size. Otherwise, 3 more tests passed.

[dfrg]

It looks like the Allsorts tests have some overlap (the AOTs) but also a set of tests designed by them. They're definitely worth looking at in the future, but probably best to limit the scope to HarfBuzz for now.

Those fonts are designed to cause infinite loops :) The test is that the shaper does something reasonable about it. That looks to be the meaning of the expected "*" output of some tests. In any case, I just pushed some code that should catch those and probably fixes a few more of the morx tests.

I have fixes for most of the Balinese and Kannada tests, but they're really targeted hacks to help me understand the problem. I have a bit of extra time this week, so I'm going to allocate some time over the next two days to see if I can come up with a proper model rather than pushing some ad hoc solutions.

[kirawi]

Take your time :) The infinite loops are gone now, but it doesn't return the expected *.

[kirawi]

Sorry about the delay, I've had a busy week but I'll try to get back to this as soon as possible.

[dfrg]

Take your time :) I've decided to do a bit of redesign on the cluster model to fix the Indic edge cases, but I won't be able to get to that for a while. So there won't be any fixes coming for those failures. When you get around to the other sets of tests, I'll see if there is any low hanging fruit I can address from those without major updates.

Edit: Oh, as for the tests with "*", I imagine the best course of action is to not emit the assert_eq for those and just run the shaper. If the test suite doesn't hang, then it's a pass.

[kirawi]

Alright, I'm back to working on this.

[kirawi]

Got parsing to work by reliably now. I'm using a modified version of shell-words' split() function. Example output:

``` [ "../fonts/MORXTwentyeight.ttf", "U+0041,U+0078,U+0045,U+0079,U+0044,U+0079,U+0079", "[A_E_D=0+1394|x=0+529|y=0+510|y=5+510|y=6+510]", ] [ "../fonts/TRAK.ttf", "U+0041,U+0042,U+0043", "[A.alt=0+1000|B=1+1000|C.alt=2+1000]", ] [ "../fonts/TRAK.ttf", "--font-ptem=.5", "U+0041,U+0042,U+0043", "[A.alt=0@100,0+1200|B=1@100,0+1200|C.alt=2@100,0+1200]", ] [ "../fonts/TRAK.ttf", "--font-ptem=1", "U+0041,U+0042,U+0043", "[A.alt=0@100,0+1200|B=1@100,0+1200|C.alt=2@100,0+1200]", ] [ "../fonts/TRAK.ttf", "--font-ptem=2", "U+0041,U+0042,U+0043", "[A.alt=0@100,0+1200|B=1@100,0+1200|C.alt=2@100,0+1200]", ] ``` Here's the code: ```rs #[derive(Clone, Copy, Debug, PartialEq, Eq)] pub struct ParseError; impl fmt::Display for ParseError { fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { write!(f, "missing closing quote") } } impl std::error::Error for ParseError {} enum State { /// Within a delimiter. Delimiter, /// After backslash, but before starting word. Backslash, /// Within an unquoted word. Unquoted, /// After backslash in an unquoted word. UnquotedBackslash, /// Within a single quoted word. SingleQuoted, /// Within a double quoted word. DoubleQuoted, /// After backslash inside a double quoted word. DoubleQuotedBackslash, /// Inside a comment. Comment, } pub fn split(s: &str) -> Result, ParseError> { use State::*; let mut words = Vec::new(); let mut word = String::new(); let mut chars = s.chars(); let mut state = Delimiter; loop { let c = chars.next(); state = match state { Delimiter => match c { None => break, Some('\'') => SingleQuoted, Some('\"') => DoubleQuoted, Some('\\') => Backslash, Some('\t') | Some(' ') | Some('\n') | Some(':') => Delimiter, Some('#') => Comment, Some(c) => { word.push(c); Unquoted } }, Backslash => match c { None => { word.push('\\'); words.push(mem::replace(&mut word, String::new())); break; } Some('\n') => Delimiter, Some(c) => { word.push(c); Unquoted } }, Unquoted => match c { None => { words.push(mem::replace(&mut word, String::new())); break; } Some('\'') => SingleQuoted, Some('\"') => DoubleQuoted, Some('\\') => UnquotedBackslash, Some('\t') | Some(' ') | Some('\n') | Some(':') => { words.push(mem::replace(&mut word, String::new())); Delimiter } Some(c) => { word.push(c); Unquoted } }, UnquotedBackslash => match c { None => { word.push('\\'); words.push(mem::replace(&mut word, String::new())); break; } Some('\n') => Unquoted, Some(c) => { word.push(c); Unquoted } }, SingleQuoted => match c { None => return Err(ParseError), Some('\'') => Unquoted, Some(c) => { word.push(c); SingleQuoted } }, DoubleQuoted => match c { None => return Err(ParseError), Some('\"') => Unquoted, Some('\\') => DoubleQuotedBackslash, Some(c) => { word.push(c); DoubleQuoted } }, DoubleQuotedBackslash => match c { None => return Err(ParseError), Some('\n') => DoubleQuoted, Some(c @ '$') | Some(c @ '`') | Some(c @ '"') | Some(c @ '\\') => { word.push(c); DoubleQuoted } Some(c) => { word.push('\\'); word.push(c); DoubleQuoted } }, Comment => match c { None => break, Some('\n') => Delimiter, Some(_) => Comment, }, } } Ok(words) } ```

[dfrg]

That looks fantastic.

I've been buried in work on text layout so I haven't had much time to work on this. I do have an alternative implementation of your shaping function that handles RTL runs properly so I'll post that when I get the time. We'll definitely need to fully parse those offsets/advances to handle the off-by-one cases.

On a side note, some of those cmap tests will need to be included again as they actually test subtable 14 (variation selectors). I have some code written to handle those but it hasn't been incorporated into swash yet as it will require some API changes that might affect users. One of those cmap tests is for an old Apple Turkish encoding which won't ever be supported, so it'll still have to be omitted.

Thanks for all of your help!

[kirawi]

Yeah, no problem! I'm trying to get this done, but I'm really having some difficulty finding the time 😅

I think all that's left is to extract features and the font size, as well as address the previous problems.

[kirawi]

The generator tool is semi-ready (I edited the first comment with the new generator code). Just need to prepare it for the actual tests, and add a macro for generating tests. I am 100% certain I am probably misparsing features & variations.

Example TestCase

```rs TestCase { name: "", path: "../fonts/MORXTwentyeight.ttf", font_size: 75, features: [], variations: [], input: [ 'A', 'x', 'E', 'y', 'D', 'y', 'y', ], output: [ "A_E_D=0+1394", "x=0+529", "y=0+510", "y=5+510", "y=6+510", ], } ``` which is turned into ```rs create_test!(, "../fonts/MORXTwentyeight.ttf", 75, &[], &[], &['A', 'x', 'E', 'y', 'D', 'y', 'y'], &["A_E_D=0+1394", "x=0+529", "y=0+510", "y=5+510", "y=6+510"]); ```

[kirawi]

Seems like I broke something when trying to rebase.

[kirawi]

... I'm going to create a new PR, I have no idea what happened.