[2nd attempt] Cache the layout result if the font feature is specified

The previous performance regression was likely feature flag read
in performance critical path. Now it is cached and should not have a
big impact to the performance.

Before this patch, the layout is not cahced if the font feature settings
are specified. This CL caches the layout even if the font feature is
specifid.

This is necessary because the Java layer will use font feature settings
for disabling ligature if the inter word justification is specified.

Bug: 283193133
Test: minikin_tests
Change-Id: I3391296d081275e016fa5208cea010b2f0e6f1f2

6 files changed, 56 insertions, 64 deletions

diff --git a/libs/minikin/Debug.cpp b/libs/minikin/Debug.cpp
index 42d6d8e..8168a77 100644
--- a/libs/minikin/Debug.cpp
+++ b/libs/minikin/Debug.cpp
@@ -21,6 +21,7 @@
 
 #include <sstream>
 
+#include "minikin/FontFeature.h"
 #include "minikin/FontFileParser.h"
 #include "minikin/LayoutCore.h"
 #include "minikin/LocaleList.h"
@@ -79,6 +80,18 @@ std::string toString(const MinikinExtent& extent) {
     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/FontFeatureUtils.cpp b/libs/minikin/FontFeatureUtils.cpp
index e1ec065..cc0749a 100644
--- a/libs/minikin/FontFeatureUtils.cpp
+++ b/libs/minikin/FontFeatureUtils.cpp
@@ -14,12 +14,29 @@
  * 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
@@ -40,27 +57,17 @@ std::vector<hb_feature_t> cleanAndAddDefaultFontFeatures(const MinikinPaint& pai
     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');
 
-    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);
+    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;
         }
+        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
deleted file mode 100644
index 1a02173..0000000
--- a/libs/minikin/FontFeatureUtils.h
+++ /dev/null
@@ -1,40 +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_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 b89958e..10c932a 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 d02f71f..5b81406 100644
--- a/libs/minikin/MinikinInternal.cpp
+++ b/libs/minikin/MinikinInternal.cpp
@@ -17,9 +17,12 @@
 
 #define LOG_TAG "Minikin"
 
+#include "MinikinInternal.h"
+
 #include <log/log.h>
 
-#include "MinikinInternal.h"
+#include "FeatureFlags.h"
+#include "minikin/MinikinPaint.h"
 
 namespace minikin {
 
@@ -45,4 +48,12 @@ bool isVariationSelector(uint32_t codePoint) {
     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 befb312..84c7d17 100644
--- a/libs/minikin/StringPiece.h
+++ b/libs/minikin/StringPiece.h
@@ -29,6 +29,7 @@ public:
     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; }