Skip to content

Commit d686d76

Browse files
author
Fabrice Di Meglio
committed
Fix bug #5332081 TextLayoutCache needs to be able to have more cache hits
- makes TextLayoutCache not carring about start/count. Basically he will cache the result for the full string and gives back the "chunk" corresponding to start/count - changed the TextLayoutCacheValue API to take start/count parameters - added the Harfbuzz LogClusters in TextLayoutCacheValue as it is needed for extracting the start/count "chunk" - fix potential issue of cache key leaking Change-Id: I9276f9bec744e8de36349acfba8429f7c6f83394
1 parent fd4d90b commit d686d76

File tree

7 files changed

+306
-240
lines changed

7 files changed

+306
-240
lines changed

core/jni/android/graphics/Canvas.cpp

Lines changed: 10 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -758,21 +758,7 @@ class SkCanvasGlue {
758758
jfloat x, jfloat y, int flags, SkPaint* paint) {
759759

760760
jint count = end - start;
761-
sp<TextLayoutCacheValue> value;
762-
#if USE_TEXT_LAYOUT_CACHE
763-
value = TextLayoutCache::getInstance().getValue(paint, textArray, start, count,
764-
end, flags);
765-
if (value == NULL) {
766-
LOGE("Cannot get TextLayoutCache value");
767-
return ;
768-
}
769-
#else
770-
value = new TextLayoutCacheValue();
771-
value->computeValues(paint, textArray, start, count, end, flags);
772-
#endif
773-
774-
doDrawGlyphs(canvas, value->getGlyphs(), 0, value->getGlyphsCount(),
775-
x, y, flags, paint);
761+
drawTextWithGlyphs(canvas, textArray + start, 0, count, count, x, y, flags, paint);
776762
}
777763

778764
static void drawTextWithGlyphs(SkCanvas* canvas, const jchar* textArray,
@@ -781,19 +767,23 @@ class SkCanvasGlue {
781767

782768
sp<TextLayoutCacheValue> value;
783769
#if USE_TEXT_LAYOUT_CACHE
784-
value = TextLayoutCache::getInstance().getValue(paint, textArray, start, count,
785-
contextCount, flags);
770+
value = TextLayoutCache::getInstance().getValue(paint, textArray, contextCount, flags);
786771
if (value == NULL) {
787772
LOGE("Cannot get TextLayoutCache value");
788773
return ;
789774
}
790775
#else
791776
value = new TextLayoutCacheValue();
792-
value->computeValues(paint, textArray, start, count, contextCount, flags);
777+
value->computeValues(paint, textArray, contextCount, flags);
793778
#endif
794779

795-
doDrawGlyphs(canvas, value->getGlyphs(), 0, value->getGlyphsCount(),
796-
x, y, flags, paint);
780+
size_t startIndex = 0;
781+
size_t glyphsCount = 0;
782+
value->getGlyphsIndexAndCount(start, count, &startIndex, &glyphsCount);
783+
jchar* glyphs = new jchar[glyphsCount];
784+
value->getGlyphs(startIndex, glyphsCount, glyphs);
785+
doDrawGlyphs(canvas, glyphs, 0, glyphsCount, x, y, flags, paint);
786+
delete[] glyphs;
797787
}
798788

799789
static void doDrawGlyphs(SkCanvas* canvas, const jchar* glyphArray, int index, int count,

core/jni/android/graphics/Paint.cpp

Lines changed: 7 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -352,7 +352,7 @@ class SkPaintGlue {
352352
jfloat result = 0;
353353
#if RTL_USE_HARFBUZZ
354354
TextLayout::getTextRunAdvances(paint, textArray, index, count, textLength,
355-
paint->getFlags(), NULL /* dont need all advances */, result);
355+
paint->getFlags(), NULL /* dont need all advances */, &result);
356356
#else
357357
// we double count, since measureText wants a byteLength
358358
SkScalar width = paint->measureText(textArray + index, count << 1);
@@ -382,7 +382,7 @@ class SkPaintGlue {
382382

383383
#if RTL_USE_HARFBUZZ
384384
TextLayout::getTextRunAdvances(paint, textArray, start, count, textLength,
385-
paint->getFlags(), NULL /* dont need all advances */, width);
385+
paint->getFlags(), NULL /* dont need all advances */, &width);
386386
#else
387387

388388
width = SkScalarToFloat(paint->measureText(textArray + start, count << 1));
@@ -406,7 +406,7 @@ class SkPaintGlue {
406406

407407
#if RTL_USE_HARFBUZZ
408408
TextLayout::getTextRunAdvances(paint, textArray, 0, textLength, textLength,
409-
paint->getFlags(), NULL /* dont need all advances */, width);
409+
paint->getFlags(), NULL /* dont need all advances */, &width);
410410
#else
411411
width = SkScalarToFloat(paint->measureText(textArray, textLength << 1));
412412
#endif
@@ -435,10 +435,8 @@ class SkPaintGlue {
435435
jfloat* widthsArray = autoWidths.ptr();
436436

437437
#if RTL_USE_HARFBUZZ
438-
jfloat totalAdvance;
439-
440438
TextLayout::getTextRunAdvances(paint, text, 0, count, count,
441-
paint->getFlags(), widthsArray, totalAdvance);
439+
paint->getFlags(), widthsArray, NULL);
442440
#else
443441
SkScalar* scalarArray = (SkScalar*)widthsArray;
444442

@@ -489,7 +487,7 @@ class SkPaintGlue {
489487
HB_FontRec font;
490488
FontData fontData;
491489
TextLayoutCacheValue::shapeWithHarfbuzz(&shaperItem, &font, &fontData, paint, text,
492-
start, count, contextCount, flags);
490+
contextCount, flags);
493491

494492
int glyphCount = shaperItem.num_glyphs;
495493
for (int i = 0; i < glyphCount; i++) {
@@ -533,7 +531,7 @@ class SkPaintGlue {
533531
jfloat totalAdvance = 0;
534532

535533
TextLayout::getTextRunAdvances(paint, text, start, count, contextCount, flags,
536-
advancesArray, totalAdvance);
534+
advancesArray, &totalAdvance);
537535

538536
if (advances != NULL) {
539537
env->SetFloatArrayRegion(advances, advancesIndex, count, advancesArray);
@@ -604,10 +602,8 @@ class SkPaintGlue {
604602
jint count, jint flags, jint offset, jint opt) {
605603
#if RTL_USE_HARFBUZZ
606604
jfloat scalarArray[count];
607-
jfloat totalAdvance = 0;
608-
609605
TextLayout::getTextRunAdvances(paint, text, start, count, count, flags,
610-
scalarArray, totalAdvance);
606+
scalarArray, NULL);
611607
#else
612608
SkScalar scalarArray[count];
613609
jchar buffer[count];

core/jni/android/graphics/TextLayout.cpp

Lines changed: 80 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -253,40 +253,110 @@ void TextLayout::drawTextRun(SkPaint* paint, const jchar* chars,
253253

254254
void TextLayout::getTextRunAdvances(SkPaint* paint, const jchar* chars, jint start,
255255
jint count, jint contextCount, jint dirFlags,
256-
jfloat* resultAdvances, jfloat& resultTotalAdvance) {
256+
jfloat* resultAdvances, jfloat* resultTotalAdvance) {
257257
sp<TextLayoutCacheValue> value;
258258
#if USE_TEXT_LAYOUT_CACHE
259259
// Return advances from the cache. Compute them if needed
260-
value = TextLayoutCache::getInstance().getValue(
261-
paint, chars, start, count, contextCount, dirFlags);
260+
value = TextLayoutCache::getInstance().getValue(paint, chars, contextCount, dirFlags);
262261
#else
263262
value = new TextLayoutCacheValue();
264-
value->computeValues(paint, chars, start, count, contextCount, dirFlags);
263+
value->computeValues(paint, chars, contextCount, dirFlags);
265264
#endif
266265
if (value != NULL) {
267-
if (resultAdvances != NULL) {
268-
memcpy(resultAdvances, value->getAdvances(), value->getAdvancesCount() * sizeof(jfloat));
266+
if (resultAdvances) {
267+
value->getAdvances(start, count, resultAdvances);
268+
}
269+
if (resultTotalAdvance) {
270+
*resultTotalAdvance = value->getTotalAdvance(start, count);
269271
}
270-
resultTotalAdvance = value->getTotalAdvance();
271272
}
272273
}
273274

274275
void TextLayout::getTextRunAdvancesHB(SkPaint* paint, const jchar* chars, jint start,
275276
jint count, jint contextCount, jint dirFlags,
276277
jfloat* resultAdvances, jfloat& resultTotalAdvance) {
277278
// Compute advances and return them
278-
TextLayoutCacheValue::computeValuesWithHarfbuzz(paint, chars, start, count, contextCount,
279-
dirFlags, resultAdvances, &resultTotalAdvance, NULL, NULL);
279+
TextLayoutCacheValue::computeValuesWithHarfbuzz(paint, chars, contextCount,
280+
dirFlags, resultAdvances, &resultTotalAdvance, NULL, NULL, NULL);
280281
}
281282

282283
void TextLayout::getTextRunAdvancesICU(SkPaint* paint, const jchar* chars, jint start,
283284
jint count, jint contextCount, jint dirFlags,
284285
jfloat* resultAdvances, jfloat& resultTotalAdvance) {
285286
// Compute advances and return them
286-
TextLayoutCacheValue::computeAdvancesWithICU(paint, chars, start, count, contextCount, dirFlags,
287+
computeAdvancesWithICU(paint, chars, start, count, contextCount, dirFlags,
287288
resultAdvances, &resultTotalAdvance);
288289
}
289290

291+
void TextLayout::computeAdvancesWithICU(SkPaint* paint, const UChar* chars,
292+
size_t start, size_t count, size_t contextCount, int dirFlags,
293+
jfloat* outAdvances, jfloat* outTotalAdvance) {
294+
SkAutoSTMalloc<CHAR_BUFFER_SIZE, jchar> tempBuffer(contextCount);
295+
jchar* buffer = tempBuffer.get();
296+
SkScalar* scalarArray = (SkScalar*)outAdvances;
297+
298+
// this is where we'd call harfbuzz
299+
// for now we just use ushape.c
300+
size_t widths;
301+
const jchar* text;
302+
if (dirFlags & 0x1) { // rtl, call arabic shaping in case
303+
UErrorCode status = U_ZERO_ERROR;
304+
// Use fixed length since we need to keep start and count valid
305+
u_shapeArabic(chars, contextCount, buffer, contextCount,
306+
U_SHAPE_LENGTH_FIXED_SPACES_NEAR |
307+
U_SHAPE_TEXT_DIRECTION_LOGICAL | U_SHAPE_LETTERS_SHAPE |
308+
U_SHAPE_X_LAMALEF_SUB_ALTERNATE, &status);
309+
// we shouldn't fail unless there's an out of memory condition,
310+
// in which case we're hosed anyway
311+
for (int i = start, e = i + count; i < e; ++i) {
312+
if (buffer[i] == UNICODE_NOT_A_CHAR) {
313+
buffer[i] = UNICODE_ZWSP; // zero-width-space for skia
314+
}
315+
}
316+
text = buffer + start;
317+
widths = paint->getTextWidths(text, count << 1, scalarArray);
318+
} else {
319+
text = chars + start;
320+
widths = paint->getTextWidths(text, count << 1, scalarArray);
321+
}
322+
323+
jfloat totalAdvance = 0;
324+
if (widths < count) {
325+
#if DEBUG_ADVANCES
326+
LOGD("ICU -- count=%d", widths);
327+
#endif
328+
// Skia operates on code points, not code units, so surrogate pairs return only
329+
// one value. Expand the result so we have one value per UTF-16 code unit.
330+
331+
// Note, skia's getTextWidth gets confused if it encounters a surrogate pair,
332+
// leaving the remaining widths zero. Not nice.
333+
for (size_t i = 0, p = 0; i < widths; ++i) {
334+
totalAdvance += outAdvances[p++] = SkScalarToFloat(scalarArray[i]);
335+
if (p < count &&
336+
text[p] >= UNICODE_FIRST_LOW_SURROGATE &&
337+
text[p] < UNICODE_FIRST_PRIVATE_USE &&
338+
text[p-1] >= UNICODE_FIRST_HIGH_SURROGATE &&
339+
text[p-1] < UNICODE_FIRST_LOW_SURROGATE) {
340+
outAdvances[p++] = 0;
341+
}
342+
#if DEBUG_ADVANCES
343+
LOGD("icu-adv = %f - total = %f", outAdvances[i], totalAdvance);
344+
#endif
345+
}
346+
} else {
347+
#if DEBUG_ADVANCES
348+
LOGD("ICU -- count=%d", count);
349+
#endif
350+
for (size_t i = 0; i < count; i++) {
351+
totalAdvance += outAdvances[i] = SkScalarToFloat(scalarArray[i]);
352+
#if DEBUG_ADVANCES
353+
LOGD("icu-adv = %f - total = %f", outAdvances[i], totalAdvance);
354+
#endif
355+
}
356+
}
357+
*outTotalAdvance = totalAdvance;
358+
}
359+
290360
// Draws a paragraph of text on a single line, running bidi and shaping
291361
void TextLayout::drawText(SkPaint* paint, const jchar* text, jsize len,
292362
int bidiFlags, jfloat x, jfloat y, SkCanvas* canvas) {

core/jni/android/graphics/TextLayout.h

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -71,7 +71,7 @@ class TextLayout {
7171

7272
static void getTextRunAdvances(SkPaint* paint, const jchar* chars, jint start,
7373
jint count, jint contextCount, jint dirFlags,
74-
jfloat* resultAdvances, jfloat& resultTotalAdvance);
74+
jfloat* resultAdvances, jfloat* resultTotalAdvance);
7575

7676
static void getTextRunAdvancesICU(SkPaint* paint, const jchar* chars, jint start,
7777
jint count, jint contextCount, jint dirFlags,
@@ -106,5 +106,9 @@ class TextLayout {
106106
UErrorCode &status);
107107
static void handleText(SkPaint* paint, const jchar* text, jsize len,
108108
int bidiFlags, jfloat x, jfloat y, SkCanvas* canvas, SkPath* path);
109+
110+
static void computeAdvancesWithICU(SkPaint* paint, const UChar* chars,
111+
size_t start, size_t count, size_t contextCount, int dirFlags,
112+
jfloat* outAdvances, jfloat* outTotalAdvance);
109113
};
110114
} // namespace android

0 commit comments

Comments
 (0)