[NOT READY] Update visual images with harfbuzz 1.0.5

[springmeyer]

When we decide to upgrade harfbuzz to 1.x in bootstrap.sh then we'll need to merge this to keep visual tests passing.

This is because somewhere between harfbuzz 0.9.x and 1.x subtle differences appear in font positioning.

/cc @artemp @talaj @flippmoke @BergWerkGIS

[springmeyer]

@behdad - no action needed here on your part. But cc'ing you just in case you are interested these differences. To recap: the Mapnik visual test suite captures pixel differences. One goal of this is so that we can understand when dependencies of Mapnik might change the look of rendered maps. I've never seen changes before due to harfbuzz - this is the first time an upgrade has triggered visual differences. They mostly look minor and okay, but I'll be investigating more when I have time.

[artemp]

@springmeyer - I've been using harfbuzz-1.0.4 for a while and I didn't see any visual test related issues - is this for 1.0.5 only ?
<rant> visual tests are so fragile</rant>

[springmeyer]

@springmeyer - I've been using harfbuzz-1.0.4 for a while and I didn't see any visual test related issues - is this for 1.0.5 only ?

Oh, interesting! I only tested with v1.0.5 so perhaps it is some change in harfbuzz within the 1.x series and not between that and v0.9.x. I'll take a closer look.

visual tests are so fragile

By design! Just to be clear: I like the fact that I noticed this and that I had to create this ticket to prepare for migrating the tests to a new harfbuzz version. If you don't like the design then revisit mapnik/mapnik#3049 (comment) and let's discuss there.

[springmeyer]

I've been using harfbuzz-1.0.4 for a while and I didn't see any visual test related issues - is this for 1.0.5 only ?

Confirmed: the change impacting visual results happened between harfbuzz v1.0.4 and v1.0.5.

I suspected harfbuzz/harfbuzz@88da7bb after looking through recent commits, but that does not appear to be the change. Something else...

[springmeyer]

Narrowed down to harfbuzz/harfbuzz@2a9627c.

This patch fixes all visual tests:

diff --git a/include/mapnik/text/harfbuzz_shaper.hpp b/include/mapnik/text/harfbuzz_shaper.hpp
index ac436a1..f1f6dbf 100644
--- a/include/mapnik/text/harfbuzz_shaper.hpp
+++ b/include/mapnik/text/harfbuzz_shaper.hpp
@@ -101,6 +101,7 @@ static void shape_text(text_line & line,
             hb_buffer_set_direction(buffer.get(), (text_item.dir == UBIDI_RTL)?HB_DIRECTION_RTL:HB_DIRECTION_LTR);
             hb_buffer_set_script(buffer.get(), _icu_script_to_script(text_item.script));
             hb_font_t *font(hb_ft_font_create(face->get_face(), nullptr));
+            hb_ft_font_set_load_flags(font,FT_LOAD_DEFAULT | FT_LOAD_NO_HINTING);
             hb_shape(font, buffer.get(), ff_settings.get_features(), ff_count);
             hb_font_destroy(font);

[springmeyer]

solved via mapnik core change. closing.

[behdad]

Yes, sorry about that. I was undecided whether to make this change of behavior or not. Still not sure how many peoples' code it's breaking. Humm.. The OCD side of me wanted to keep FT_LOAD_DEFAULT as default...

[behdad]

WDYT, should I change it back?

[behdad]

I'm reverting the behavior change in HB. See: harfbuzz/harfbuzz#143

[springmeyer]

@behdad - hi, just seeing your comments now - Thank you for letting me know about the revert. I've adapted upstream to use the new API in mapnik/mapnik@5da6563, which is great. But yes, I agree with the revert still as those defaults seem sensible.