Merge "Revert "Cache the layout result if the font feature is specified"" into main
diff --git a/include/minikin/Debug.h b/include/minikin/Debug.h
index 20d8d8d..4308c90 100644
--- a/include/minikin/Debug.h
+++ b/include/minikin/Debug.h
@@ -25,7 +25,6 @@
struct MinikinRect;
struct MinikinExtent;
struct MinikinPaint;
-struct FontFeature;
class Range;
class U16StringPiece;
class LayoutPiece;
@@ -41,8 +40,6 @@
std::string toString(const MinikinExtent& extent);
std::string toString(const LayoutPiece& layout);
std::string toString(const MinikinPaint& paint);
-std::string toString(const FontFeature& feature);
-std::string toString(const std::vector<FontFeature>& features);
} // namespace debug
diff --git a/include/minikin/FontFeature.h b/include/minikin/FontFeature.h
deleted file mode 100644
index c47594b..0000000
--- a/include/minikin/FontFeature.h
+++ /dev/null
@@ -1,77 +0,0 @@
-/*
- * Copyright (C) 2021 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.
- */
-
-#ifndef MINIKIN_FONT_FEATURE_H
-#define MINIKIN_FONT_FEATURE_H
-
-#include <hb.h>
-
-#include <ostream>
-#include <string_view>
-#include <vector>
-
-namespace minikin {
-
-struct MinikinPaint;
-
-// Subset of the hb_feature_t since we don't allow setting features on ranges.
-struct FontFeature {
- hb_tag_t tag;
- uint32_t value;
-
- static std::vector<FontFeature> parse(std::string_view fontFeatureString);
-};
-
-/**
- * Returns the final set of font features based on the features requested by this paint object and
- * extra defaults or implied font features.
- *
- * Features are included from the paint object if they are:
- * 1) in a supported range
- *
- * Default features are added based if they are:
- * 1) implied due to Paint settings such as letterSpacing
- * 2) default features that do not conflict with requested features
- */
-std::vector<hb_feature_t> cleanAndAddDefaultFontFeatures(const MinikinPaint& features);
-
-// For gtest output
-inline std::ostream& operator<<(std::ostream& os, const FontFeature& feature) {
- return os << static_cast<char>(feature.tag >> 24) << static_cast<char>(feature.tag >> 16)
- << static_cast<char>(feature.tag >> 8) << static_cast<char>(feature.tag) << " "
- << feature.value;
-}
-
-inline std::ostream& operator<<(std::ostream& os, const std::vector<FontFeature>& features) {
- for (size_t i = 0; i < features.size(); ++i) {
- if (i != 0) {
- os << ", ";
- }
- os << features;
- }
- return os;
-}
-
-constexpr bool operator==(const FontFeature& l, const FontFeature& r) {
- return l.tag == r.tag && l.value == r.value;
-}
-
-constexpr bool operator!=(const FontFeature& l, const FontFeature& r) {
- return !(l == r);
-}
-
-} // namespace minikin
-#endif // MINIKIN_LAYOUT_H
diff --git a/include/minikin/Hasher.h b/include/minikin/Hasher.h
index 4e20195..4a76b29 100644
--- a/include/minikin/Hasher.h
+++ b/include/minikin/Hasher.h
@@ -18,9 +18,9 @@
#define MINIKIN_HASHER_H
#include <cstdint>
+
#include <string>
-#include "minikin/FontFeature.h"
#include "minikin/Macros.h"
namespace minikin {
@@ -57,20 +57,6 @@
return update(bits.i);
}
- inline Hasher& update(const std::vector<FontFeature>& features) {
- update(features.size());
- for (const FontFeature& feature : features) {
- update(feature);
- }
- return *this;
- }
-
- inline Hasher& update(const FontFeature& feature) {
- update(feature.tag);
- update(feature.value);
- return *this;
- }
-
inline Hasher& updateShorts(const uint16_t* data, uint32_t length) {
update(length);
uint32_t i;
diff --git a/include/minikin/LayoutCache.h b/include/minikin/LayoutCache.h
index fa572fb..74300a2 100644
--- a/include/minikin/LayoutCache.h
+++ b/include/minikin/LayoutCache.h
@@ -55,7 +55,6 @@
mStartHyphen(startHyphen),
mEndHyphen(endHyphen),
mIsRtl(dir),
- mFontFeatureSettings(paint.fontFeatureSettings),
mHash(computeHash()) {}
bool operator==(const LayoutCacheKey& o) const {
@@ -65,7 +64,6 @@
mFontFlags == o.mFontFlags && mLocaleListId == o.mLocaleListId &&
mFamilyVariant == o.mFamilyVariant && mStartHyphen == o.mStartHyphen &&
mEndHyphen == o.mEndHyphen && mIsRtl == o.mIsRtl && mNchars == o.mNchars &&
- mFontFeatureSettings == o.mFontFeatureSettings &&
!memcmp(mChars, o.mChars, mNchars * sizeof(uint16_t));
}
@@ -79,7 +77,6 @@
void freeText() {
delete[] mChars;
mChars = NULL;
- mFontFeatureSettings.clear();
}
uint32_t getMemoryUsage() const { return sizeof(LayoutCacheKey) + sizeof(uint16_t) * mNchars; }
@@ -102,7 +99,6 @@
StartHyphenEdit mStartHyphen;
EndHyphenEdit mEndHyphen;
bool mIsRtl;
- std::vector<FontFeature> mFontFeatureSettings;
// Note: any fields added to MinikinPaint must also be reflected here.
// TODO: language matching (possibly integrate into style)
android::hash_t mHash;
@@ -124,7 +120,6 @@
.update(packHyphenEdit(mStartHyphen, mEndHyphen))
.update(mIsRtl)
.updateShorts(mChars, mNchars)
- .update(mFontFeatureSettings)
.hash();
}
};
diff --git a/include/minikin/MinikinPaint.h b/include/minikin/MinikinPaint.h
index 04cab1d..54b476f 100644
--- a/include/minikin/MinikinPaint.h
+++ b/include/minikin/MinikinPaint.h
@@ -23,7 +23,6 @@
#include "minikin/FamilyVariant.h"
#include "minikin/FontCollection.h"
#include "minikin/FontFamily.h"
-#include "minikin/FontFeature.h"
#include "minikin/Hasher.h"
namespace minikin {
@@ -46,7 +45,7 @@
};
// Possibly move into own .h file?
-// Note: if you add a field here, either add it to LayoutCacheKey
+// Note: if you add a field here, either add it to LayoutCacheKey or to skipCache()
struct MinikinPaint {
MinikinPaint(const std::shared_ptr<FontCollection>& font)
: size(0),
@@ -60,7 +59,7 @@
fontFeatureSettings(),
font(font) {}
- bool skipCache() const;
+ bool skipCache() const { return !fontFeatureSettings.empty(); }
float size;
float scaleX;
@@ -71,7 +70,7 @@
uint32_t localeListId;
FontStyle fontStyle;
FamilyVariant familyVariant;
- std::vector<FontFeature> fontFeatureSettings;
+ std::string fontFeatureSettings;
std::shared_ptr<FontCollection> font;
void copyFrom(const MinikinPaint& paint) { *this = paint; }
@@ -101,7 +100,7 @@
.update(localeListId)
.update(fontStyle.identifier())
.update(static_cast<uint8_t>(familyVariant))
- .update(fontFeatureSettings)
+ .updateString(fontFeatureSettings)
.update(font->getId())
.hash();
}
diff --git a/libs/minikin/Debug.cpp b/libs/minikin/Debug.cpp
index 8168a77..42d6d8e 100644
--- a/libs/minikin/Debug.cpp
+++ b/libs/minikin/Debug.cpp
@@ -21,7 +21,6 @@
#include <sstream>
-#include "minikin/FontFeature.h"
#include "minikin/FontFileParser.h"
#include "minikin/LayoutCore.h"
#include "minikin/LocaleList.h"
@@ -80,18 +79,6 @@
return ss.str();
}
-std::string toString(const FontFeature& feature) {
- std::stringstream ss;
- ss << feature;
- return ss.str();
-}
-
-std::string toString(const std::vector<FontFeature>& features) {
- std::stringstream ss;
- ss << features;
- return ss.str();
-}
-
std::string toString(const LayoutPiece& layout) {
std::stringstream ss;
ss << "{advance=" << layout.advance() << ", extent=" << toString(layout.extent())
diff --git a/libs/minikin/FeatureFlags.h b/libs/minikin/FeatureFlags.h
index f1da23d..3c2e455 100644
--- a/libs/minikin/FeatureFlags.h
+++ b/libs/minikin/FeatureFlags.h
@@ -39,14 +39,6 @@
#endif // __ANDROID__
}
-inline bool inter_character_justification() {
-#ifdef __ANDROID__
- return com_android_text_flags_inter_character_justification();
-#else
- return true;
-#endif // __ANDROID__
-}
-
} // namespace features
#endif // FEATURE_FLAGS
diff --git a/libs/minikin/FontFeatureUtils.cpp b/libs/minikin/FontFeatureUtils.cpp
index cc0749a..e1ec065 100644
--- a/libs/minikin/FontFeatureUtils.cpp
+++ b/libs/minikin/FontFeatureUtils.cpp
@@ -14,29 +14,12 @@
* limitations under the License.
*/
+#include "FontFeatureUtils.h"
+
#include "StringPiece.h"
-#include "minikin/FontFeature.h"
-#include "minikin/MinikinPaint.h"
namespace minikin {
-std::vector<FontFeature> FontFeature::parse(std::string_view fontFeatureSettings) {
- std::vector<FontFeature> features;
-
- SplitIterator it(StringPiece(fontFeatureSettings), ',');
- while (it.hasNext()) {
- StringPiece featureStr = it.next();
- static hb_feature_t feature;
- // We do not allow setting features on ranges. As such, reject any setting that has
- // non-universal range.
- if (hb_feature_from_string(featureStr.data(), featureStr.size(), &feature) &&
- feature.start == 0 && feature.end == (unsigned int)-1) {
- features.push_back({feature.tag, feature.value});
- }
- }
- return features;
-}
-
std::vector<hb_feature_t> cleanAndAddDefaultFontFeatures(const MinikinPaint& paint) {
std::vector<hb_feature_t> features;
// Disable default-on non-required ligature features if letter-spacing
@@ -57,17 +40,27 @@
static constexpr hb_tag_t halt_tag = HB_TAG('h', 'a', 'l', 't');
static constexpr hb_tag_t palt_tag = HB_TAG('p', 'a', 'l', 't');
- for (const FontFeature& feature : paint.fontFeatureSettings) {
- // OpenType requires disabling default `chws` feature if glyph-width features.
- // https://docs.microsoft.com/en-us/typography/opentype/spec/features_ae#tag-chws
- // Here, we follow Chrome's impl: not enabling default `chws` feature if `palt` or
- // `halt` is enabled.
- // https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/platform/fonts/shaping/font_features.cc;drc=77a9a09de0688ca449f5333a305ceaf3f36b6daf;l=215
- if (feature.tag == chws_tag ||
- (feature.value && (feature.tag == halt_tag || feature.tag == palt_tag))) {
- default_enable_chws = false;
+ SplitIterator it(paint.fontFeatureSettings, ',');
+ while (it.hasNext()) {
+ StringPiece featureStr = it.next();
+ static hb_feature_t feature;
+ // We do not allow setting features on ranges. As such, reject any setting that has
+ // non-universal range.
+ if (hb_feature_from_string(featureStr.data(), featureStr.size(), &feature) &&
+ feature.start == 0 && feature.end == (unsigned int)-1) {
+ // OpenType requires disabling default `chws` feature if glyph-width features.
+ // https://docs.microsoft.com/en-us/typography/opentype/spec/features_ae#tag-chws
+ // Here, we follow Chrome's impl: not enabling default `chws` feature if `palt` or
+ // `halt` is enabled.
+ // https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/platform/fonts/shaping/font_features.cc;drc=77a9a09de0688ca449f5333a305ceaf3f36b6daf;l=215
+ if (default_enable_chws &&
+ (feature.tag == chws_tag ||
+ (feature.value && (feature.tag == halt_tag || feature.tag == palt_tag)))) {
+ default_enable_chws = false;
+ }
+
+ features.push_back(feature);
}
- features.push_back({feature.tag, feature.value, 0, ~0u});
}
if (default_enable_chws) {
diff --git a/libs/minikin/FontFeatureUtils.h b/libs/minikin/FontFeatureUtils.h
new file mode 100644
index 0000000..1a02173
--- /dev/null
+++ b/libs/minikin/FontFeatureUtils.h
@@ -0,0 +1,40 @@
+/*
+ * Copyright (C) 2021 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.
+ */
+
+#ifndef MINIKIN_FONT_FEATURE_UTILS_H
+#define MINIKIN_FONT_FEATURE_UTILS_H
+
+#include <hb.h>
+
+#include "minikin/MinikinPaint.h"
+
+namespace minikin {
+
+/**
+ * Returns the final set of font features based on the features requested by this paint object and
+ * extra defaults or implied font features.
+ *
+ * Features are included from the paint object if they are:
+ * 1) in a supported range
+ *
+ * Default features are added based if they are:
+ * 1) implied due to Paint settings such as letterSpacing
+ * 2) default features that do not conflict with requested features
+ */
+std::vector<hb_feature_t> cleanAndAddDefaultFontFeatures(const MinikinPaint& paint);
+
+} // namespace minikin
+#endif // MINIKIN_LAYOUT_UTILS_H
diff --git a/libs/minikin/LayoutCore.cpp b/libs/minikin/LayoutCore.cpp
index 10c932a..b89958e 100644
--- a/libs/minikin/LayoutCore.cpp
+++ b/libs/minikin/LayoutCore.cpp
@@ -34,11 +34,11 @@
#include <vector>
#include "BidiUtils.h"
+#include "FontFeatureUtils.h"
#include "LayoutUtils.h"
#include "LocaleListCache.h"
#include "MinikinInternal.h"
#include "minikin/Emoji.h"
-#include "minikin/FontFeature.h"
#include "minikin/HbUtils.h"
#include "minikin/LayoutCache.h"
#include "minikin/LayoutPieces.h"
diff --git a/libs/minikin/MinikinInternal.cpp b/libs/minikin/MinikinInternal.cpp
index 5b81406..d02f71f 100644
--- a/libs/minikin/MinikinInternal.cpp
+++ b/libs/minikin/MinikinInternal.cpp
@@ -17,12 +17,9 @@
#define LOG_TAG "Minikin"
-#include "MinikinInternal.h"
-
#include <log/log.h>
-#include "FeatureFlags.h"
-#include "minikin/MinikinPaint.h"
+#include "MinikinInternal.h"
namespace minikin {
@@ -48,12 +45,4 @@
return isBMPVariationSelector(codePoint) || isVariationSelectorSupplement(codePoint);
}
-bool MinikinPaint::skipCache() const {
- if (features::inter_character_justification()) {
- return false; // if the flag is on, do not skip the cache.
- } else {
- return !fontFeatureSettings.empty();
- }
-}
-
} // namespace minikin
diff --git a/libs/minikin/StringPiece.h b/libs/minikin/StringPiece.h
index 84c7d17..befb312 100644
--- a/libs/minikin/StringPiece.h
+++ b/libs/minikin/StringPiece.h
@@ -29,7 +29,6 @@
StringPiece(const char* data) : mData(data), mLength(data == nullptr ? 0 : strlen(data)) {}
StringPiece(const char* data, size_t length) : mData(data), mLength(length) {}
StringPiece(const std::string& str) : mData(str.data()), mLength(str.size()) {}
- StringPiece(std::string_view str) : mData(str.data()), mLength(str.size()) {}
inline const char* data() const { return mData; }
inline size_t length() const { return mLength; }
diff --git a/tests/perftests/Android.bp b/tests/perftests/Android.bp
index c4260b3..362ba10 100644
--- a/tests/perftests/Android.bp
+++ b/tests/perftests/Android.bp
@@ -46,7 +46,5 @@
"libicu",
"liblog",
"libcutils",
- "aconfig_text_flags_c_lib",
- "server_configurable_flags",
],
}
diff --git a/tests/stresstest/Android.bp b/tests/stresstest/Android.bp
index 420a4b1..7e0379c 100644
--- a/tests/stresstest/Android.bp
+++ b/tests/stresstest/Android.bp
@@ -42,8 +42,6 @@
"libutils",
"libz",
"libcutils",
- "aconfig_text_flags_c_lib",
- "server_configurable_flags",
],
srcs: [
diff --git a/tests/unittest/FontFeatureTest.cpp b/tests/unittest/FontFeatureTest.cpp
index 6f01842..7f9cdf4 100644
--- a/tests/unittest/FontFeatureTest.cpp
+++ b/tests/unittest/FontFeatureTest.cpp
@@ -14,12 +14,10 @@
* limitations under the License.
*/
-#include <com_android_text_flags.h>
-#include <flag_macros.h>
#include <gtest/gtest.h>
+#include "FontFeatureUtils.h"
#include "FontTestUtils.h"
-#include "minikin/FontFeature.h"
#include "minikin/MinikinPaint.h"
namespace minikin {
@@ -55,7 +53,7 @@
TEST_F(DefaultFontFeatureTest, disable) {
auto paint = MinikinPaint(font);
- paint.fontFeatureSettings = FontFeature::parse("\"chws\" off");
+ paint.fontFeatureSettings = "\"chws\" off";
auto f = cleanAndAddDefaultFontFeatures(paint);
std::sort(f.begin(), f.end(), compareFeatureTag);
@@ -67,7 +65,7 @@
TEST_F(DefaultFontFeatureTest, preserve) {
auto paint = MinikinPaint(font);
- paint.fontFeatureSettings = FontFeature::parse("\"ruby\" on");
+ paint.fontFeatureSettings = "\"ruby\" on";
auto f = cleanAndAddDefaultFontFeatures(paint);
std::sort(f.begin(), f.end(), compareFeatureTag);
@@ -97,7 +95,7 @@
TEST_F(DefaultFontFeatureTest, halt_disable_chws) {
auto paint = MinikinPaint(font);
- paint.fontFeatureSettings = FontFeature::parse("\"halt\" on");
+ paint.fontFeatureSettings = "\"halt\" on";
auto f = cleanAndAddDefaultFontFeatures(paint);
EXPECT_EQ(1u, f.size());
@@ -107,7 +105,7 @@
TEST_F(DefaultFontFeatureTest, palt_disable_chws) {
auto paint = MinikinPaint(font);
- paint.fontFeatureSettings = FontFeature::parse("\"palt\" on");
+ paint.fontFeatureSettings = "\"palt\" on";
auto f = cleanAndAddDefaultFontFeatures(paint);
EXPECT_EQ(1u, f.size());
@@ -118,7 +116,7 @@
TEST_F(DefaultFontFeatureTest, halt_disable_chws_large_letter_spacing) {
auto paint = MinikinPaint(font);
paint.letterSpacing = 1.0; // em
- paint.fontFeatureSettings = FontFeature::parse("\"halt\" on");
+ paint.fontFeatureSettings = "\"halt\" on";
auto f = cleanAndAddDefaultFontFeatures(paint);
std::sort(f.begin(), f.end(), compareFeatureTag);
@@ -135,7 +133,7 @@
TEST_F(DefaultFontFeatureTest, palt_disable_chws_large_letter_spacing) {
auto paint = MinikinPaint(font);
paint.letterSpacing = 1.0; // em
- paint.fontFeatureSettings = FontFeature::parse("\"palt\" on");
+ paint.fontFeatureSettings = "\"palt\" on";
auto f = cleanAndAddDefaultFontFeatures(paint);
std::sort(f.begin(), f.end(), compareFeatureTag);
@@ -149,27 +147,4 @@
EXPECT_TRUE(f[2].value);
}
-class FontFeatureTest : public testing::Test {
-protected:
- std::shared_ptr<FontCollection> font;
-
- virtual void SetUp() override { font = buildFontCollection("Ascii.ttf"); }
-};
-
-TEST_F_WITH_FLAGS(FontFeatureTest, do_not_skip_cache_if_flagEnabled,
- REQUIRES_FLAGS_ENABLED(ACONFIG_FLAG(com::android::text::flags,
- inter_character_justification))) {
- auto paint = MinikinPaint(font);
- paint.fontFeatureSettings = FontFeature::parse("\"palt\" on");
- EXPECT_FALSE(paint.skipCache());
-}
-
-TEST_F_WITH_FLAGS(FontFeatureTest, do_not_skip_cache_if_flagDisabled,
- REQUIRES_FLAGS_DISABLED(ACONFIG_FLAG(com::android::text::flags,
- inter_character_justification))) {
- auto paint = MinikinPaint(font);
- paint.fontFeatureSettings = FontFeature::parse("\"palt\" on");
- EXPECT_TRUE(paint.skipCache());
-}
-
} // namespace minikin
diff --git a/tests/unittest/LayoutCacheTest.cpp b/tests/unittest/LayoutCacheTest.cpp
index 5d20456..b1d60ba 100644
--- a/tests/unittest/LayoutCacheTest.cpp
+++ b/tests/unittest/LayoutCacheTest.cpp
@@ -246,11 +246,11 @@
SCOPED_TRACE("Different font feature settings");
auto collection = buildFontCollection("Ascii.ttf");
MinikinPaint paint1(collection);
- paint1.fontFeatureSettings = FontFeature::parse("");
+ paint1.fontFeatureSettings = "";
layoutCache.getOrCreate(text1, Range(0, text1.size()), paint1, false /* LTR */,
StartHyphenEdit::NO_EDIT, EndHyphenEdit::NO_EDIT, false, layout1);
MinikinPaint paint2(collection);
- paint2.fontFeatureSettings = FontFeature::parse("'liga' on");
+ paint2.fontFeatureSettings = "'liga' on";
layoutCache.getOrCreate(text1, Range(0, text1.size()), paint2, false /* LTR */,
StartHyphenEdit::NO_EDIT, EndHyphenEdit::NO_EDIT, false, layout2);
EXPECT_NE(layout1.get(), layout2.get());
diff --git a/tests/unittest/LayoutCoreTest.cpp b/tests/unittest/LayoutCoreTest.cpp
index 264454a..139a7ea 100644
--- a/tests/unittest/LayoutCoreTest.cpp
+++ b/tests/unittest/LayoutCoreTest.cpp
@@ -54,7 +54,7 @@
const std::string fontFeaturesSettings) {
MinikinPaint paint(fc);
paint.size = 10.0f; // make 1em = 10px
- paint.fontFeatureSettings = FontFeature::parse(fontFeaturesSettings);
+ paint.fontFeatureSettings = fontFeaturesSettings;
return buildLayout(text, paint);
}
