Mixed languages text crashes

Extending text to grapheme edges should correct glyph range
Bug: skia:10087
Change-Id: I254901aaaa40c2782d1afbd5d5390599bdd7c922

Change-Id: I1d51076656d09e4d2e35e3ddad28bfd60fc87081
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/281756
Reviewed-by: Ben Wagner <bungeman@google.com>
Commit-Queue: Julia Lavrova <jlavrova@google.com>

modules/skparagraph/include/ParagraphCache.h

@@ -1,4 +1,4 @@
-           // Copyright 2019 Google LLC.
+// Copyright 2019 Google LLC.
 #ifndef ParagraphCache_DEFINED
 #define ParagraphCache_DEFINED
 

modules/skparagraph/include/TextStyle.h

@@ -277,7 +277,6 @@ typedef size_t TextIndex;
 typedef SkRange<size_t> TextRange;
 const SkRange<size_t> EMPTY_TEXT = EMPTY_RANGE;
 
-
 struct Block {
     Block() : fRange(EMPTY_RANGE), fStyle() { }
     Block(size_t start, size_t end, const TextStyle& style) : fRange(start, end), fStyle(style) {}

modules/skparagraph/include/TypefaceFontProvider.h

@@ -10,7 +10,6 @@
 #include "include/core/SkFontMgr.h"
 #include "include/core/SkStream.h"
 #include "include/core/SkString.h"
-#include "src/core/SkFontDescriptor.h"
 
 namespace skia {
 namespace textlayout {

modules/skparagraph/src/OneLineShaper.cpp

@@ -101,14 +101,15 @@ void OneLineShaper::fillGaps(size_t startingCount) {
 
     auto count = fUnresolvedBlocks.size();
     for (size_t i = 0; i < count; ++i) {
-        auto unresolved = fUnresolvedBlocks.front();
+        auto front = fUnresolvedBlocks.front();
         fUnresolvedBlocks.pop();
-        fUnresolvedBlocks.push(unresolved);
+        fUnresolvedBlocks.push(front);
         if (i < startingCount) {
             // Skip the first ones
             continue;
         }
 
+        auto& unresolved = fUnresolvedBlocks.back();
         TextRange resolvedText(resolvedTextStart, fCurrentRun->leftToRight() ? unresolved.fText.start : unresolved.fText.end);
         if (resolvedText.width() > 0) {
             if (!fCurrentRun->leftToRight()) {
@@ -118,15 +119,25 @@ void OneLineShaper::fillGaps(size_t startingCount) {
             GlyphRange resolvedGlyphs(resolvedGlyphsStart, unresolved.fGlyphs.start);
             RunBlock resolved(fCurrentRun, resolvedText, resolvedGlyphs, resolvedGlyphs.width());
 
+            if (resolvedGlyphs.width() == 0) {
+                // Extend the unresolved block with an empty resolved
+                if (unresolved.fText.end <= resolved.fText.start) {
+                    unresolved.fText.end = resolved.fText.end;
+                }
+                if (unresolved.fText.start >= resolved.fText.end) {
+                    unresolved.fText.start = resolved.fText.start;
+                }
+            } else {
                 fResolvedBlocks.emplace_back(resolved);
             }
+        }
         resolvedGlyphsStart = unresolved.fGlyphs.end;
         resolvedTextStart =  fCurrentRun->leftToRight()
                                 ? unresolved.fText.end
                                 : unresolved.fText.start;
     }
 
-    TextRange resolvedText(resolvedTextStart, resolvedTextLimits.end);
+    TextRange resolvedText(resolvedTextStart,resolvedTextLimits.end);
     if (resolvedText.width() > 0) {
         if (!fCurrentRun->leftToRight()) {
             std::swap(resolvedText.start, resolvedText.end);
@@ -134,7 +145,6 @@ void OneLineShaper::fillGaps(size_t startingCount) {
 
         GlyphRange resolvedGlyphs(resolvedGlyphsStart, fCurrentRun->size());
         RunBlock resolved(fCurrentRun, resolvedText, resolvedGlyphs, resolvedGlyphs.width());
-
         fResolvedBlocks.emplace_back(resolved);
     }
 }
@@ -191,8 +201,12 @@ void OneLineShaper::finish(TextRange blockText, SkScalar height, SkScalar& advan
 
         auto runAdvance = SkVector::Make(run->posX(glyphs.end) - run->posX(glyphs.start), run->fAdvance.fY);
         const SkShaper::RunHandler::RunInfo info = {
-                run->fFont, run->fBidiLevel, runAdvance, glyphs.width(),
+                run->fFont,
-                SkShaper::RunHandler::Range(text.start - run->fClusterStart, text.width())};
+                run->fBidiLevel,
+                runAdvance,
+                glyphs.width(),
+                SkShaper::RunHandler::Range(text.start - run->fClusterStart, text.width())
+        };
         this->fParagraph->fRuns.emplace_back(
                     this->fParagraph,
                     info,
@@ -249,7 +263,7 @@ TextRange OneLineShaper::normalizeTextRange(GlyphRange glyphRange) {
         return TextRange(clusterIndex(glyphRange.end - 1),
                 glyphRange.start > 0
                 ? clusterIndex(glyphRange.start - 1)
-                : fCurrentRun->fClusterStart + fCurrentRun->fTextRange.width());
+                : fCurrentRun->fTextRange.end);
     }
 }
 
@@ -268,7 +282,7 @@ void OneLineShaper::addUnresolvedWithRun(GlyphRange glyphRange) {
     RunBlock unresolved(fCurrentRun, clusteredText(glyphRange), glyphRange, 0);
     if (unresolved.fGlyphs.width() == fCurrentRun->size()) {
         SkASSERT(unresolved.fText.width() == fCurrentRun->fTextRange.width());
-    } else if (fUnresolvedBlocks.size() > 1) {
+    } else if (fUnresolvedBlocks.size() > 0) {
         auto& lastUnresolved = fUnresolvedBlocks.back();
         if (lastUnresolved.fRun != nullptr &&
             lastUnresolved.fRun->fIndex == fCurrentRun->fIndex) {
@@ -278,10 +292,15 @@ void OneLineShaper::addUnresolvedWithRun(GlyphRange glyphRange) {
               lastUnresolved.fText.end = unresolved.fText.end;
               lastUnresolved.fGlyphs.end = glyphRange.end;
               return;
+            } else if(lastUnresolved.fText == unresolved.fText) {
+                // Nothing was resolved; ignore it
+                return;
+            } else if (lastUnresolved.fText.contains(unresolved.fText)) {
+                // We get here for the very first unresolved piece
+                return;
             } else if (lastUnresolved.fText.intersects(unresolved.fText)) {
                 // Few pieces of the same unresolved text block can ignore the second one
-                lastUnresolved.fGlyphs.start =
+                lastUnresolved.fGlyphs.start = std::min(lastUnresolved.fGlyphs.start, glyphRange.start);
-                        std::min(lastUnresolved.fGlyphs.start, glyphRange.start);
                 lastUnresolved.fGlyphs.end = std::max(lastUnresolved.fGlyphs.end, glyphRange.end);
                 lastUnresolved.fText = clusteredText(lastUnresolved.fGlyphs);
                 return;
@@ -630,7 +649,8 @@ bool OneLineShaper::shape() {
     return result;
 }
 
-TextRange OneLineShaper::clusteredText(GlyphRange glyphs) {
+// When we extend TextRange to the grapheme edges, we also extend glyphs range
+TextRange OneLineShaper::clusteredText(GlyphRange& glyphs) {
 
     enum class Dir { left, right };
     enum class Pos { inclusive, exclusive };
@@ -661,6 +681,24 @@ TextRange OneLineShaper::clusteredText(GlyphRange glyphs) {
     textRange.start = findBaseChar(textRange.start, Dir::left);
     textRange.end = findBaseChar(textRange.end, Dir::right);
 
+    // Correct the glyphRange in case we extended the text to the grapheme edges
+    // TODO: code it without if (as a part of LTR/RTL refactoring)
+    if (fCurrentRun->leftToRight()) {
+        while (glyphs.start > 0 && clusterIndex(glyphs.start) > textRange.start) {
+          glyphs.start--;
+        }
+        while (glyphs.end < fCurrentRun->size() && clusterIndex(glyphs.end) < textRange.end) {
+          glyphs.end++;
+        }
+    } else {
+        while (glyphs.start > 0 && clusterIndex(glyphs.start - 1) < textRange.end) {
+          glyphs.start--;
+        }
+        while (glyphs.end < fCurrentRun->size() && clusterIndex(glyphs.end) > textRange.start) {
+          glyphs.end++;
+        }
+    }
+
     return { textRange.start, textRange.end };
 }
 

modules/skparagraph/src/OneLineShaper.h

@@ -12,9 +12,6 @@
 namespace skia {
 namespace textlayout {
 
-typedef size_t GlyphIndex;
-typedef SkRange<GlyphIndex> GlyphRange;
-
 class ParagraphImpl;
 class OneLineShaper : public SkShaper::RunHandler {
 public:
@@ -22,7 +19,8 @@ public:
         : fParagraph(paragraph)
         , fHeight(0.0f)
         , fAdvance(SkPoint::Make(0.0f, 0.0f))
-        , fUnresolvedGlyphs(0) { }
+        , fUnresolvedGlyphs(0)
+        , fUniqueRunId(paragraph->fRuns.size()){ }
 
     bool shape();
 
@@ -80,19 +78,18 @@ private:
     void commitLine() override {}
 
     Buffer runBuffer(const RunInfo& info) override {
-        auto index = fUnresolvedBlocks.size() + fResolvedBlocks.size();
         fCurrentRun = std::make_shared<Run>(fParagraph,
                                            info,
                                            fCurrentText.start,
                                            fHeight,
-                                           index,
+                                           ++fUniqueRunId,
                                            fAdvance.fX);
         return fCurrentRun->newRunBuffer();
     }
 
     void commitRunBuffer(const RunInfo&) override;
 
-    TextRange clusteredText(GlyphRange glyphs);
+    TextRange clusteredText(GlyphRange& glyphs);
     ClusterIndex clusterIndex(GlyphIndex glyph) {
         return fCurrentText.start + fCurrentRun->fClusterIndexes[glyph];
     }
@@ -108,6 +105,7 @@ private:
     SkScalar fHeight;
     SkVector fAdvance;
     size_t fUnresolvedGlyphs;
+    size_t fUniqueRunId;
 
     // TODO: Something that is not thead-safe since we don't need it
     std::shared_ptr<Run> fCurrentRun;

modules/skparagraph/src/ParagraphImpl.cpp

@@ -724,8 +724,13 @@ std::vector<TextBox> ParagraphImpl::getRectsForRange(unsigned start,
                     {
                         // TODO: this is just a patch. Make it right later (when it's clear what and how)
                         trailingSpaces = clip;
+                        if(run->leftToRight()) {
                             trailingSpaces.fLeft = line.width();
                             clip.fRight = line.width();
+                        } else {
+                            trailingSpaces.fRight = 0;
+                            clip.fLeft = 0;
+                        }
                     } else if (this->fParagraphStyle.getTextDirection() == TextDirection::kRtl &&
                         !run->leftToRight())
                     {

modules/skparagraph/src/Run.cpp

@@ -124,7 +124,7 @@ std::tuple<bool, ClusterIndex, ClusterIndex> Run::findLimitingClusters(TextRange
             }
         }
     } else {
-        // TODO: Do we need to use this branch?..
+        // We need this branch when we draw styles for the part of a cluster
         for (auto i = fClusterRange.start; i != fClusterRange.end; ++i) {
             auto& cluster = fMaster->cluster(i);
             if (cluster.textRange().end > text.start && start == nullptr) {

modules/skparagraph/src/Run.h

@@ -9,7 +9,6 @@
 #include "modules/skparagraph/include/TextStyle.h"
 #include "modules/skshaper/include/SkShaper.h"
 #include "src/core/SkSpan.h"
-#include "src/core/SkTraceEvent.h"
 #include <functional>  // std::function
 
 namespace skia {
@@ -33,6 +32,27 @@ typedef SkRange<GraphemeIndex> GraphemeRange;
 typedef size_t CodepointIndex;
 typedef SkRange<CodepointIndex> CodepointRange;
 
+typedef size_t GlyphIndex;
+typedef SkRange<GlyphIndex> GlyphRange;
+
+/* This is a part of future LTR/RTL refactoring
+// This is a part of a shaped text
+// Text range (a, b) can be:
+// LTR: [a:b) where a < b
+// RTL: (b:a] where a > b
+class ShapedSpan {
+  public:
+      ShapedSpan(Run* run, TextRange textRange, GlyphRange glyphRange)
+        : fRun(run)
+        , fText(textRange)
+        , fGlyphs(glyphRange) { }
+
+  private:
+      Run* fRun;
+      TextRange fText;
+      GlyphRange fGlyphs;
+};
+*/
 struct RunShifts {
     RunShifts() { }
     RunShifts(size_t count) { fShifts.push_back_n(count, 0.0); }
@@ -227,13 +247,9 @@ class Cluster {
 public:
     enum BreakType {
         None,
-        CharacterBoundary,       // not yet in use (UBRK_CHARACTER)
+        GraphemeBreak,  // calculated for all clusters (UBRK_CHARACTER)
-        WordBoundary,            // calculated for all clusters (UBRK_WORD)
+        SoftLineBreak,  // calculated for all clusters (UBRK_LINE & UBRK_CHARACTER)
-        WordBreakWithoutHyphen,  // calculated only for hyphenated words
-        WordBreakWithHyphen,
-        SoftLineBreak,  // calculated for all clusters (UBRK_LINE)
         HardLineBreak,  // calculated for all clusters (UBRK_LINE)
-        GraphemeBreak,
     };
 
     Cluster()

modules/skparagraph/src/TypefaceFontProvider.cpp

@@ -1,8 +1,10 @@
 // Copyright 2019 Google LLC.
 #include "modules/skparagraph/include/TypefaceFontProvider.h"
 #include <algorithm>
+#include "include/core/SkFontMgr.h"
 #include "include/core/SkString.h"
 #include "include/core/SkTypeface.h"
+#include "src/core/SkFontDescriptor.h"
 
 namespace skia {
 namespace textlayout {

modules/skparagraph/utils/TestFontCollection.h

@@ -1,6 +1,7 @@
 // Copyright 2019 Google LLC.
 #include "modules/skparagraph/include/FontCollection.h"
 #include "modules/skparagraph/include/TypefaceFontProvider.h"
+#include "src/core/SkFontDescriptor.h"
 
 namespace skia {
 namespace textlayout {

tests/SkParagraphTest.cpp

@@ -1,6 +1,7 @@
 // Copyright 2019 Google LLC.
 #include <sstream>
 #include <thread>
+#include "include/core/SkFontMgr.h"
 #include "modules/skparagraph/include/TypefaceFontProvider.h"
 #include "modules/skparagraph/src/ParagraphBuilderImpl.h"
 #include "modules/skparagraph/src/ParagraphImpl.h"