From 4a4e6574c02774c758974c1fadf161f829baaaa3 Mon Sep 17 00:00:00 2001 From: Seigo Nonaka Date: Thu, 23 Jun 2016 13:22:16 +0900 Subject: [PATCH] Fix lookup order for VS in itemization. This is partial revert of Iced1349e3ca750821d8882c551551f65bb569794. Due to sorting of target family vectors, the font family order from XML settings file is broken. Making unique operation stable doesn't fix the issue completely since some font families are appended for the fallback which also breaks the original order. By this change, itemization becomes 3x slower than before if variation selector is appended. Bug: 29585939 Change-Id: I7c1a8a57f04111a30cd41a5cd5bec25fcfb3972e --- libs/minikin/FontCollection.cpp | 17 +- tests/data/NoCmapFormat14.ttf | Bin 0 -> 844 bytes tests/data/NoCmapFormat14.ttx | 207 ++++++++++++++++++ tests/data/VarioationSelectorTest-Regular.ttf | Bin 1204 -> 1008 bytes tests/data/VarioationSelectorTest-Regular.ttx | 87 ++------ tests/unittest/Android.mk | 1 + tests/unittest/FontCollectionItemizeTest.cpp | 73 ++++++ 7 files changed, 301 insertions(+), 84 deletions(-) create mode 100644 tests/data/NoCmapFormat14.ttf create mode 100644 tests/data/NoCmapFormat14.ttx diff --git a/libs/minikin/FontCollection.cpp b/libs/minikin/FontCollection.cpp index 97c206881f1..19ad7523f7d 100644 --- a/libs/minikin/FontCollection.cpp +++ b/libs/minikin/FontCollection.cpp @@ -281,22 +281,11 @@ FontFamily* FontCollection::getFamilyForChar(uint32_t ch, uint32_t vs, return mFamilies[0]; } - const std::vector* familyVec = &mFamilyVec; + const std::vector& familyVec = (vs == 0) ? mFamilyVec : mFamilies; Range range = mRanges[ch >> kLogCharsPerPage]; - std::vector familyVecForVS; if (vs != 0) { - // If variation selector is specified, need to search for both the variation sequence and - // its base codepoint. Compute the union vector of them. - familyVecForVS = mVSFamilyVec; - familyVecForVS.insert(familyVecForVS.end(), - mFamilyVec.begin() + range.start, mFamilyVec.begin() + range.end); - std::sort(familyVecForVS.begin(), familyVecForVS.end()); - auto last = std::unique(familyVecForVS.begin(), familyVecForVS.end()); - familyVecForVS.erase(last, familyVecForVS.end()); - - familyVec = &familyVecForVS; - range = { 0, familyVecForVS.size() }; + range = { 0, mFamilies.size() }; } #ifdef VERBOSE_DEBUG @@ -305,7 +294,7 @@ FontFamily* FontCollection::getFamilyForChar(uint32_t ch, uint32_t vs, FontFamily* bestFamily = nullptr; uint32_t bestScore = kUnsupportedFontScore; for (size_t i = range.start; i < range.end; i++) { - FontFamily* family = (*familyVec)[i]; + FontFamily* family = familyVec[i]; const uint32_t score = calcFamilyScore(ch, vs, variant, langListId, family); if (score == kFirstFontScore) { // If the first font family supports the given character or variation sequence, always diff --git a/tests/data/NoCmapFormat14.ttf b/tests/data/NoCmapFormat14.ttf new file mode 100644 index 0000000000000000000000000000000000000000..2a0c46c7aca19c80833cf8f1a3f0e1e39be7b232 GIT binary patch literal 844 zcmbVKF;5gh6#iy*y*uCnmE9#wL5u~&a3O{QjgbTrAV;{xV1qh`%U!YSUheh+6eep2 zR1%Bh2UyaQ5U|9A#zy~u1qGcAmS+8CckhB48)h@_z3+YBym>n_5C8@d!Gc0C~u9(NyyWKCwe>)gkHa;79$SmR->nfgR&1#mNyg|?Mxuxk zwum1MbrYQ>t2~7}D9C;JoSDVcix(&h(;cb)_@nx>#kC5aIFS=sialJg=L;$mjkuQP zBvjg&IVldoyBMS|!Zr-8ZPZQ_$EiJfm#!w4=Xv&eW?&S3E=52A*{zWo?emgrM>DN` zBcH=Peinp1K=v_~vRHDb{VcX` + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + No Cmap Format 14 Subtable Test + + + Regular + + + No Cmap Format 14 Subtable Test + + + No Cmap Format 14 SubtableTest-Regular + + + No Cmap Format 14 Subtable Test + + + Regular + + + No Cmap Format 14 Subtable Test + + + No Cmap Format 14 SubtableTest-Regular + + + + + + + + + + + + + + + + diff --git a/tests/data/VarioationSelectorTest-Regular.ttf b/tests/data/VarioationSelectorTest-Regular.ttf index dfb0b2d89eab90a68cc9db8f0f4889f4060e8fe6..0504c67fb7a6403167ac8b53ae177c7b53d7912c 100644 GIT binary patch delta 476 zcmZWlF-t;G6#nje&$NPDfo%;2tqn%BR6inw1|Lzc(jK8{d=-VQRLDfPHq)M z)DYAdSp9{F#+Fv?J8B?wF5fxd{mwb}+(ewY|yeC2%Lf$*V4k9Bcq$-&!QDlKArT0~^TX(hbSrBZN0RJAf0YD7aTi-9lc zm_oh|nXgc^MN!zz_Abv1&5(*-vY+=A{_ErsurSMR9~W;CI?UILcDqfM`(s%{z~SF^M5-Ol~aJ@=gMefNFYmlZn=>4oFlAfWPu9hq60f?X^E z#v_31XgD-!Myj{TPl?Vb1uJhnkbe-((O4pBH5yyYZgB0NnVSfy7B$Sg`2Dd^GLGk} zEAD%^ZkP?l!Xw>72Y|Uk%^ROvXipSx&;eL`Vu0dIEY`WXvD%(*DD#sFUVZydy7va& zcVBu{P1aGX=~4VOcsQ+Pgcl|%+27!Bz9&2=aMm@Bf*d9_je81(%26f0?kF+H z6?2X<*-nV`xuY!hTS7`mwAs$Qi?NUOk^i_!}&!pqJQ>`u$ zrrO9o9K6dwdhMt?W<&IqU<1qW=N diff --git a/tests/data/VarioationSelectorTest-Regular.ttx b/tests/data/VarioationSelectorTest-Regular.ttx index a063a5ee934..f86f008b081 100644 --- a/tests/data/VarioationSelectorTest-Regular.ttx +++ b/tests/data/VarioationSelectorTest-Regular.ttx @@ -18,18 +18,7 @@ - - - - - - - - - - - - + @@ -147,45 +136,36 @@ - - - - - - - - - - - - + - - - + + + + - - - + + + - - - + + + - - - + + + + @@ -202,40 +182,7 @@ - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - + diff --git a/tests/unittest/Android.mk b/tests/unittest/Android.mk index b43e3c85703..a81d17cca7e 100644 --- a/tests/unittest/Android.mk +++ b/tests/unittest/Android.mk @@ -32,6 +32,7 @@ font_src_files := \ data/Italic.ttf \ data/Ja.ttf \ data/Ko.ttf \ + data/NoCmapFormat14.ttf \ data/NoGlyphFont.ttf \ data/Regular.ttf \ data/TextEmojiFont.ttf \ diff --git a/tests/unittest/FontCollectionItemizeTest.cpp b/tests/unittest/FontCollectionItemizeTest.cpp index 367739683d1..978ba9f499f 100644 --- a/tests/unittest/FontCollectionItemizeTest.cpp +++ b/tests/unittest/FontCollectionItemizeTest.cpp @@ -44,6 +44,9 @@ const char kColorEmojiFont[] = kTestFontDir "ColorEmojiFont.ttf"; const char kTextEmojiFont[] = kTestFontDir "TextEmojiFont.ttf"; const char kMixedEmojiFont[] = kTestFontDir "ColorTextMixedEmojiFont.ttf"; +const char kHasCmapFormat14Font[] = kTestFontDir "NoCmapFormat14.ttf"; +const char kNoCmapFormat14Font[] = kTestFontDir "VarioationSelectorTest-Regular.ttf"; + typedef ICUTestBase FontCollectionItemizeTest; // Utility function for calling itemize function. @@ -1392,4 +1395,74 @@ TEST_F(FontCollectionItemizeTest, itemize_genderBalancedEmoji) { EXPECT_EQ(kColorEmojiFont, getFontPath(runs[0])); } +// For b/29585939 +TEST_F(FontCollectionItemizeTest, itemizeShouldKeepOrderForVS) { + const FontStyle kDefaultFontStyle; + + MinikinAutoUnref dummyFont(MinikinFontForTest::createFromFile(kNoGlyphFont)); + MinikinAutoUnref fontA(MinikinFontForTest::createFromFile(kZH_HansFont)); + MinikinAutoUnref fontB(MinikinFontForTest::createFromFile(kZH_HansFont)); + + MinikinAutoUnref dummyFamily(new FontFamily()); + MinikinAutoUnref familyA(new FontFamily()); + MinikinAutoUnref familyB(new FontFamily()); + + dummyFamily->addFont(dummyFont.get()); + familyA->addFont(fontA.get()); + familyB->addFont(fontB.get()); + + std::vector families = + { dummyFamily.get(), familyA.get(), familyB.get() }; + std::vector reversedFamilies = + { dummyFamily.get(), familyB.get(), familyA.get() }; + + MinikinAutoUnref collection(new FontCollection(families)); + MinikinAutoUnref reversedCollection(new FontCollection(reversedFamilies)); + + // Both fontA/fontB support U+35A8 but don't support U+35A8 U+E0100. The first font should be + // selected. + std::vector runs; + itemize(collection.get(), "U+35A8 U+E0100", kDefaultFontStyle, &runs); + EXPECT_EQ(fontA.get(), runs[0].fakedFont.font); + + itemize(reversedCollection.get(), "U+35A8 U+E0100", kDefaultFontStyle, &runs); + EXPECT_EQ(fontB.get(), runs[0].fakedFont.font); +} + +// For b/29585939 +TEST_F(FontCollectionItemizeTest, itemizeShouldKeepOrderForVS2) { + const FontStyle kDefaultFontStyle; + + MinikinAutoUnref dummyFont(MinikinFontForTest::createFromFile(kNoGlyphFont)); + MinikinAutoUnref hasCmapFormat14Font( + MinikinFontForTest::createFromFile(kHasCmapFormat14Font)); + MinikinAutoUnref noCmapFormat14Font( + MinikinFontForTest::createFromFile(kNoCmapFormat14Font)); + + MinikinAutoUnref dummyFamily(new FontFamily()); + MinikinAutoUnref hasCmapFormat14Family(new FontFamily()); + MinikinAutoUnref noCmapFormat14Family(new FontFamily()); + + dummyFamily->addFont(dummyFont.get()); + hasCmapFormat14Family->addFont(hasCmapFormat14Font.get()); + noCmapFormat14Family->addFont(noCmapFormat14Font.get()); + + std::vector families = + { dummyFamily.get(), hasCmapFormat14Family.get(), noCmapFormat14Family.get() }; + std::vector reversedFamilies = + { dummyFamily.get(), noCmapFormat14Family.get(), hasCmapFormat14Family.get() }; + + MinikinAutoUnref collection(new FontCollection(families)); + MinikinAutoUnref reversedCollection(new FontCollection(reversedFamilies)); + + // Both hasCmapFormat14Font/noCmapFormat14Font support U+5380 but don't support U+5380 U+E0100. + // The first font should be selected. + std::vector runs; + itemize(collection.get(), "U+5380 U+E0100", kDefaultFontStyle, &runs); + EXPECT_EQ(hasCmapFormat14Font.get(), runs[0].fakedFont.font); + + itemize(reversedCollection.get(), "U+5380 U+E0100", kDefaultFontStyle, &runs); + EXPECT_EQ(noCmapFormat14Font.get(), runs[0].fakedFont.font); +} + } // namespace minikin