From b01028d1d7bc3906ef71c72ad985919f79304b5e Mon Sep 17 00:00:00 2001 From: Roozbeh Pournader Date: Mon, 13 Mar 2017 21:52:52 -0700 Subject: [PATCH] Break grapheme clusters after viramas if they end a cluster Previously, we stayed on the conservative side and disallowed any grapheme breaks (and thus cursoring) where a virama was followed by a letter, since we did not know if the virama would be forming a cluster with the letter or not. This created problems with Indic languages with infrequent conjuncts, such as Tamil. Now we use the information in calculated advances to find if a cluster is formed. If there is no cluster, we break the grapheme and allow cursoring after the virama. Test: Unit tests added to GraphemeBreakTests and MeasurementTests. Test: Also manually tested Tamil sequences. Bug: 35721792 Change-Id: Ib159edb94b3ad6f693f0d3dad016b332b2cef447 --- include/minikin/GraphemeBreak.h | 12 +++--- libs/minikin/GraphemeBreak.cpp | 37 ++++++++--------- libs/minikin/Measurement.cpp | 7 ++-- tests/perftests/GraphemeBreak.cpp | 6 +-- tests/unittest/Android.mk | 1 + tests/unittest/GraphemeBreakTests.cpp | 44 +++++++++++++++++--- tests/unittest/MeasurementTests.cpp | 60 +++++++++++++++++++++++++++ 7 files changed, 131 insertions(+), 36 deletions(-) create mode 100644 tests/unittest/MeasurementTests.cpp diff --git a/include/minikin/GraphemeBreak.h b/include/minikin/GraphemeBreak.h index b501f6784fa..f1b5102a0c7 100644 --- a/include/minikin/GraphemeBreak.h +++ b/include/minikin/GraphemeBreak.h @@ -31,15 +31,15 @@ public: }; // Determine whether the given offset is a grapheme break. - // This implementation generally follows Unicode TR29 extended - // grapheme break, but with some tweaks to more closely match - // existing implementations. - static bool isGraphemeBreak(const uint16_t* buf, size_t start, size_t count, size_t offset); + // This implementation generally follows Unicode's UTR #29 extended + // grapheme break, with various tweaks. + static bool isGraphemeBreak(const float* advances, const uint16_t* buf, size_t start, + size_t count, size_t offset); // Matches Android's Java API. Note, return (size_t)-1 for AT to // signal non-break because unsigned return type. - static size_t getTextRunCursor(const uint16_t* buf, size_t start, size_t count, - size_t offset, MoveOpt opt); + static size_t getTextRunCursor(const float* advances, const uint16_t* buf, size_t start, + size_t count, size_t offset, MoveOpt opt); }; } // namespace minikin diff --git a/libs/minikin/GraphemeBreak.cpp b/libs/minikin/GraphemeBreak.cpp index 1b3d1ab33ba..56f3a52a5df 100644 --- a/libs/minikin/GraphemeBreak.cpp +++ b/libs/minikin/GraphemeBreak.cpp @@ -55,8 +55,8 @@ bool isPureKiller(uint32_t c) { || c == 0xA953 || c == 0xABED || c == 0x11134 || c == 0x112EA || c == 0x1172B); } -bool GraphemeBreak::isGraphemeBreak(const uint16_t* buf, size_t start, size_t count, - size_t offset) { +bool GraphemeBreak::isGraphemeBreak(const float* advances, const uint16_t* buf, size_t start, + size_t count, const size_t offset) { // This implementation closely follows Unicode Standard Annex #29 on // Unicode Text Segmentation (http://www.unicode.org/reports/tr29/), // implementing a tailored version of extended grapheme clusters. @@ -73,8 +73,9 @@ bool GraphemeBreak::isGraphemeBreak(const uint16_t* buf, size_t start, size_t co uint32_t c1 = 0; uint32_t c2 = 0; size_t offset_back = offset; + size_t offset_forward = offset; U16_PREV(buf, start, offset_back, c1); - U16_NEXT(buf, offset, start + count, c2); + U16_NEXT(buf, offset_forward, start + count, c2); int32_t p1 = tailoredGraphemeClusterBreak(c1); int32_t p2 = tailoredGraphemeClusterBreak(c2); // Rule GB3, CR x LF @@ -117,25 +118,23 @@ bool GraphemeBreak::isGraphemeBreak(const uint16_t* buf, size_t start, size_t co } } - // Note that the offset has moved forwared 2 code units by U16_NEXT. // The number 4 comes from the number of code units in a whole flag. - return (offset - 2 - offset_back) % 4 == 0; + return (offset - offset_back) % 4 == 0; } // Rule GB9, x (Extend | ZWJ); Rule GB9a, x SpacingMark; Rule GB9b, Prepend x if (p2 == U_GCB_EXTEND || p2 == U_GCB_ZWJ || p2 == U_GCB_SPACING_MARK || p1 == U_GCB_PREPEND) { return false; } - // Cluster indic syllables together (tailoring of UAX #29) - // Known limitation: this is overly conservative, and assumes that the virama may form a - // conjunct with the following letter, which doesn't always happen. - // - // There is no easy solution to do this correctly. Even querying the font does not help (with - // the current font technoloies), since the font may be creating the conjunct using multiple - // glyphs, while the user may be perceiving that sequence of glyphs as one conjunct or one - // letter. + // Cluster Indic syllables together (tailoring of UAX #29). + // Immediately after each virama (that is not just a pure killer) followed by a letter, we + // check to see if the next character has a non-zero width assigned to it in the advances + // array. A zero width means a cluster is formed with the virama (so there is no grapheme + // break), while a non-zero width means a new cluster is started (so there may be a grapheme + // break). if (u_getIntPropertyValue(c1, UCHAR_CANONICAL_COMBINING_CLASS) == 9 // virama && !isPureKiller(c1) - && u_getIntPropertyValue(c2, UCHAR_GENERAL_CATEGORY) == U_OTHER_LETTER) { + && u_getIntPropertyValue(c2, UCHAR_GENERAL_CATEGORY) == U_OTHER_LETTER + && (advances == nullptr || advances[offset - start] == 0)) { return false; } // Tailoring: make emoji sequences with ZWJ a single grapheme cluster @@ -170,8 +169,8 @@ bool GraphemeBreak::isGraphemeBreak(const uint16_t* buf, size_t start, size_t co return true; } -size_t GraphemeBreak::getTextRunCursor(const uint16_t* buf, size_t start, size_t count, - size_t offset, MoveOpt opt) { +size_t GraphemeBreak::getTextRunCursor(const float* advances, const uint16_t* buf, size_t start, + size_t count, size_t offset, MoveOpt opt) { switch (opt) { case AFTER: if (offset < start + count) { @@ -179,7 +178,7 @@ size_t GraphemeBreak::getTextRunCursor(const uint16_t* buf, size_t start, size_t } // fall through case AT_OR_AFTER: - while (!isGraphemeBreak(buf, start, count, offset)) { + while (!isGraphemeBreak(advances, buf, start, count, offset)) { offset++; } break; @@ -189,12 +188,12 @@ size_t GraphemeBreak::getTextRunCursor(const uint16_t* buf, size_t start, size_t } // fall through case AT_OR_BEFORE: - while (!isGraphemeBreak(buf, start, count, offset)) { + while (!isGraphemeBreak(advances, buf, start, count, offset)) { offset--; } break; case AT: - if (!isGraphemeBreak(buf, start, count, offset)) { + if (!isGraphemeBreak(advances, buf, start, count, offset)) { offset = (size_t)-1; } break; diff --git a/libs/minikin/Measurement.cpp b/libs/minikin/Measurement.cpp index 0ed7a670bfd..f0d15f24e80 100644 --- a/libs/minikin/Measurement.cpp +++ b/libs/minikin/Measurement.cpp @@ -54,7 +54,8 @@ static float getRunAdvance(const float* advances, const uint16_t* buf, size_t la int numGraphemeClustersAfter = 0; for (size_t i = lastCluster; i < nextCluster; i++) { bool isAfter = i >= offset; - if (GraphemeBreak::isGraphemeBreak(buf, start, count, i)) { + if (GraphemeBreak::isGraphemeBreak( + advances + (start - layoutStart), buf, start, count, i)) { numGraphemeClusters++; if (isAfter) { numGraphemeClustersAfter++; @@ -86,7 +87,7 @@ size_t getOffsetForAdvance(const float* advances, const uint16_t* buf, size_t st float x = 0.0f, xLastClusterStart = 0.0f, xSearchStart = 0.0f; size_t lastClusterStart = start, searchStart = start; for (size_t i = start; i < start + count; i++) { - if (GraphemeBreak::isGraphemeBreak(buf, start, count, i)) { + if (GraphemeBreak::isGraphemeBreak(advances, buf, start, count, i)) { searchStart = lastClusterStart; xSearchStart = xLastClusterStart; } @@ -103,7 +104,7 @@ size_t getOffsetForAdvance(const float* advances, const uint16_t* buf, size_t st size_t best = searchStart; float bestDist = FLT_MAX; for (size_t i = searchStart; i <= start + count; i++) { - if (GraphemeBreak::isGraphemeBreak(buf, start, count, i)) { + if (GraphemeBreak::isGraphemeBreak(advances, buf, start, count, i)) { // "getRunAdvance(layout, buf, start, count, i) - advance" but more efficient float delta = getRunAdvance(advances, buf, start, searchStart, count - searchStart, i) diff --git a/tests/perftests/GraphemeBreak.cpp b/tests/perftests/GraphemeBreak.cpp index cfee7c642c1..6d6cf5b1783 100644 --- a/tests/perftests/GraphemeBreak.cpp +++ b/tests/perftests/GraphemeBreak.cpp @@ -38,7 +38,7 @@ static void BM_GraphemeBreak_Ascii(benchmark::State& state) { LOG_ALWAYS_FATAL_IF(result_size != 12); const size_t testIndex = state.range(0); while (state.KeepRunning()) { - GraphemeBreak::isGraphemeBreak(buffer, 0, result_size, testIndex); + GraphemeBreak::isGraphemeBreak(nullptr, buffer, 0, result_size, testIndex); } } BENCHMARK(BM_GraphemeBreak_Ascii) @@ -53,7 +53,7 @@ static void BM_GraphemeBreak_Emoji(benchmark::State& state) { LOG_ALWAYS_FATAL_IF(result_size != 12); const size_t testIndex = state.range(0); while (state.KeepRunning()) { - GraphemeBreak::isGraphemeBreak(buffer, 0, result_size, testIndex); + GraphemeBreak::isGraphemeBreak(nullptr, buffer, 0, result_size, testIndex); } } BENCHMARK(BM_GraphemeBreak_Emoji) @@ -68,7 +68,7 @@ static void BM_GraphemeBreak_Emoji_Flags(benchmark::State& state) { LOG_ALWAYS_FATAL_IF(result_size != 12); const size_t testIndex = state.range(0); while (state.KeepRunning()) { - GraphemeBreak::isGraphemeBreak(buffer, 0, result_size, testIndex); + GraphemeBreak::isGraphemeBreak(nullptr, buffer, 0, result_size, testIndex); } } BENCHMARK(BM_GraphemeBreak_Emoji_Flags) diff --git a/tests/unittest/Android.mk b/tests/unittest/Android.mk index fdcc0c4e472..716fcc378d8 100644 --- a/tests/unittest/Android.mk +++ b/tests/unittest/Android.mk @@ -76,6 +76,7 @@ LOCAL_SRC_FILES += \ GraphemeBreakTests.cpp \ LayoutTest.cpp \ LayoutUtilsTest.cpp \ + MeasurementTests.cpp \ SparseBitSetTest.cpp \ UnicodeUtilsTest.cpp \ WordBreakerTests.cpp diff --git a/tests/unittest/GraphemeBreakTests.cpp b/tests/unittest/GraphemeBreakTests.cpp index 29b1669bd41..96bd8a8e79f 100644 --- a/tests/unittest/GraphemeBreakTests.cpp +++ b/tests/unittest/GraphemeBreakTests.cpp @@ -26,7 +26,16 @@ bool IsBreak(const char* src) { size_t offset; size_t size; ParseUnicode(buf, BUF_SIZE, src, &size, &offset); - return GraphemeBreak::isGraphemeBreak(buf, 0, size, offset); + return GraphemeBreak::isGraphemeBreak(nullptr, buf, 0, size, offset); +} + +bool IsBreakWithAdvances(const float* advances, const char* src) { + const size_t BUF_SIZE = 256; + uint16_t buf[BUF_SIZE]; + size_t offset; + size_t size; + ParseUnicode(buf, BUF_SIZE, src, &size, &offset); + return GraphemeBreak::isGraphemeBreak(advances, buf, 0, size, offset); } TEST(GraphemeBreak, utf16) { @@ -164,6 +173,31 @@ TEST(GraphemeBreak, tailoring) { EXPECT_FALSE(IsBreak("U+0E01 | U+0E3A U+0E01")); // thai phinthu = pure killer EXPECT_TRUE(IsBreak("U+0E01 U+0E3A | U+0E01")); // thai phinthu = pure killer + // Repetition of above tests, but with a given advances array that implies everything + // became just one cluster. + const float conjoined[] = {1.0, 0.0, 0.0}; + EXPECT_FALSE(IsBreakWithAdvances(conjoined, + "U+0915 | U+094D U+0915")); // Devanagari ka+virama+ka + EXPECT_FALSE(IsBreakWithAdvances(conjoined, + "U+0915 U+094D | U+0915")); // Devanagari ka+virama+ka + EXPECT_FALSE(IsBreakWithAdvances(conjoined, + "U+0E01 | U+0E3A U+0E01")); // thai phinthu = pure killer + EXPECT_TRUE(IsBreakWithAdvances(conjoined, + "U+0E01 U+0E3A | U+0E01")); // thai phinthu = pure killer + + // Repetition of above tests, but with a given advances array that the virama did not + // form a cluster with the following consonant. The difference is that there is now + // a grapheme break after the virama in ka+virama+ka. + const float separate[] = {1.0, 0.0, 1.0}; + EXPECT_FALSE(IsBreakWithAdvances(separate, + "U+0915 | U+094D U+0915")); // Devanagari ka+virama+ka + EXPECT_TRUE(IsBreakWithAdvances(separate, + "U+0915 U+094D | U+0915")); // Devanagari ka+virama+ka + EXPECT_FALSE(IsBreakWithAdvances(separate, + "U+0E01 | U+0E3A U+0E01")); // thai phinthu = pure killer + EXPECT_TRUE(IsBreakWithAdvances(separate, + "U+0E01 U+0E3A | U+0E01")); // thai phinthu = pure killer + // suppress grapheme breaks in zwj emoji sequences, see // http://www.unicode.org/emoji/charts/emoji-zwj-sequences.html EXPECT_FALSE(IsBreak("U+1F469 U+200D | U+2764 U+FE0F U+200D U+1F48B U+200D U+1F468")); @@ -222,10 +256,10 @@ TEST(GraphemeBreak, genderBalancedEmoji) { TEST(GraphemeBreak, offsets) { uint16_t string[] = { 0x0041, 0x06DD, 0x0045, 0x0301, 0x0049, 0x0301 }; - EXPECT_TRUE(GraphemeBreak::isGraphemeBreak(string, 2, 3, 2)); - EXPECT_FALSE(GraphemeBreak::isGraphemeBreak(string, 2, 3, 3)); - EXPECT_TRUE(GraphemeBreak::isGraphemeBreak(string, 2, 3, 4)); - EXPECT_TRUE(GraphemeBreak::isGraphemeBreak(string, 2, 3, 5)); + EXPECT_TRUE(GraphemeBreak::isGraphemeBreak(nullptr, string, 2, 3, 2)); + EXPECT_FALSE(GraphemeBreak::isGraphemeBreak(nullptr, string, 2, 3, 3)); + EXPECT_TRUE(GraphemeBreak::isGraphemeBreak(nullptr, string, 2, 3, 4)); + EXPECT_TRUE(GraphemeBreak::isGraphemeBreak(nullptr, string, 2, 3, 5)); } } // namespace minikin diff --git a/tests/unittest/MeasurementTests.cpp b/tests/unittest/MeasurementTests.cpp new file mode 100644 index 00000000000..7fedecb5ea2 --- /dev/null +++ b/tests/unittest/MeasurementTests.cpp @@ -0,0 +1,60 @@ +/* + * Copyright (C) 2017 The Android Open Source Project + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +#include +#include +#include + +namespace minikin { + +float getAdvance(const float* advances, const char* src) { + const size_t BUF_SIZE = 256; + uint16_t buf[BUF_SIZE]; + size_t offset; + size_t size; + ParseUnicode(buf, BUF_SIZE, src, &size, &offset); + return getRunAdvance(advances, buf, 0, size, offset); +} + +// Latin fi +TEST(Measurement, getRunAdvance_fi) { + const float unligated[] = {30.0, 20.0}; + EXPECT_EQ(0.0, getAdvance(unligated, "| 'f' 'i'")); + EXPECT_EQ(30.0, getAdvance(unligated, "'f' | 'i'")); + EXPECT_EQ(50.0, getAdvance(unligated, "'f' 'i' |")); + + const float ligated[] = {40.0, 0.0}; + EXPECT_EQ(0.0, getAdvance(ligated, "| 'f' 'i'")); + EXPECT_EQ(20.0, getAdvance(ligated, "'f' | 'i'")); + EXPECT_EQ(40.0, getAdvance(ligated, "'f' 'i' |")); +} + +// Devanagari ka+virama+ka +TEST(Measurement, getRunAdvance_kka) { + const float unligated[] = {30.0, 0.0, 30.0}; + EXPECT_EQ(0.0, getAdvance(unligated, "| U+0915 U+094D U+0915")); + EXPECT_EQ(30.0, getAdvance(unligated, "U+0915 | U+094D U+0915")); + EXPECT_EQ(30.0, getAdvance(unligated, "U+0915 U+094D | U+0915")); + EXPECT_EQ(60.0, getAdvance(unligated, "U+0915 U+094D U+0915 |")); + + const float ligated[] = {30.0, 0.0, 0.0}; + EXPECT_EQ(0.0, getAdvance(ligated, "| U+0915 U+094D U+0915")); + EXPECT_EQ(30.0, getAdvance(ligated, "U+0915 | U+094D U+0915")); + EXPECT_EQ(30.0, getAdvance(ligated, "U+0915 U+094D | U+0915")); + EXPECT_EQ(30.0, getAdvance(ligated, "U+0915 U+094D U+0915 |")); +} + +} // namespace minikin