From 42a5407f2c6403ea7aa7a64eaf19948dc4050df5 Mon Sep 17 00:00:00 2001 From: Chris Craik Date: Tue, 24 Nov 2015 11:41:54 -0800 Subject: [PATCH] Fix RecordingCanvas::refPaint, add tests Also add text align support to TestUtils::drawTextToCanvas Change-Id: I105adb0d15e697c03adfd00a56e8ec9265953ff1 --- libs/hwui/RecordingCanvas.h | 20 +---- libs/hwui/unit_tests/OpReordererTests.cpp | 1 + libs/hwui/unit_tests/RecordingCanvasTests.cpp | 46 ++++++++++- libs/hwui/utils/TestUtils.cpp | 78 +++++++++++-------- libs/hwui/utils/TestUtils.h | 2 +- 5 files changed, 95 insertions(+), 52 deletions(-) diff --git a/libs/hwui/RecordingCanvas.h b/libs/hwui/RecordingCanvas.h index 736cc9ecaf2af..6d0e9e0e0f7a5 100644 --- a/libs/hwui/RecordingCanvas.h +++ b/libs/hwui/RecordingCanvas.h @@ -205,10 +205,6 @@ private: return dstBuffer; } - inline char* refText(const char* text, size_t byteLength) { - return (char*) refBuffer((uint8_t*)text, byteLength); - } - inline const SkPath* refPath(const SkPath* path) { if (!path) return nullptr; @@ -220,13 +216,8 @@ private: } /** - * Returns a RenderThread-safe, const copy of the SkPaint parameter passed in (with deduping - * based on paint generation ID) - * - * Note that this forces Left_Align, since drawText glyph rendering expects left alignment, - * since alignment offsetting has been done at a higher level. This is done to essentially all - * copied paints, since the deduping can mean a paint is shared by drawText commands and other - * types (which wouldn't care about alignment). + * Returns a RenderThread-safe, const copy of the SkPaint parameter passed in + * (with deduping based on paint hash / equality check) */ inline const SkPaint* refPaint(const SkPaint* paint) { if (!paint) return nullptr; @@ -246,11 +237,8 @@ private: // In the unlikely event that 2 unique paints have the same hash we do a // object equality check to ensure we don't erroneously dedup them. if (cachedPaint == nullptr || *cachedPaint != *paint) { - SkPaint* copy = new SkPaint(*paint); - copy->setTextAlign(SkPaint::kLeft_Align); - - cachedPaint = copy; - mDisplayList->paints.emplace_back(copy); + cachedPaint = new SkPaint(*paint); + mDisplayList->paints.emplace_back(cachedPaint); // replaceValueFor() performs an add if the entry doesn't exist mPaintMap.replaceValueFor(key, cachedPaint); refBitmapsInShader(cachedPaint->getShader()); diff --git a/libs/hwui/unit_tests/OpReordererTests.cpp b/libs/hwui/unit_tests/OpReordererTests.cpp index d76086c9cfcd4..8692e5dd9ed1d 100644 --- a/libs/hwui/unit_tests/OpReordererTests.cpp +++ b/libs/hwui/unit_tests/OpReordererTests.cpp @@ -184,6 +184,7 @@ TEST(OpReorderer, textStrikethroughBatching) { textPaint.setAntiAlias(true); textPaint.setTextSize(20); textPaint.setStrikeThruText(true); + textPaint.setTextEncoding(SkPaint::kGlyphID_TextEncoding); for (int i = 0; i < LOOPS; i++) { TestUtils::drawTextToCanvas(&canvas, "test text", textPaint, 10, 100 * (i + 1)); } diff --git a/libs/hwui/unit_tests/RecordingCanvasTests.cpp b/libs/hwui/unit_tests/RecordingCanvasTests.cpp index c23d47e3fab48..959bd44845605 100644 --- a/libs/hwui/unit_tests/RecordingCanvasTests.cpp +++ b/libs/hwui/unit_tests/RecordingCanvasTests.cpp @@ -75,6 +75,7 @@ TEST(RecordingCanvas, drawText) { SkPaint paint; paint.setAntiAlias(true); paint.setTextSize(20); + paint.setTextEncoding(SkPaint::kGlyphID_TextEncoding); TestUtils::drawTextToCanvas(&canvas, "test text", paint, 25, 25); }); @@ -95,6 +96,7 @@ TEST(RecordingCanvas, drawText_strikeThruAndUnderline) { SkPaint paint; paint.setAntiAlias(true); paint.setTextSize(20); + paint.setTextEncoding(SkPaint::kGlyphID_TextEncoding); for (int i = 0; i < 2; i++) { for (int j = 0; j < 2; j++) { paint.setUnderlineText(i != 0); @@ -126,6 +128,7 @@ TEST(RecordingCanvas, drawText_forceAlignLeft) { SkPaint paint; paint.setAntiAlias(true); paint.setTextSize(20); + paint.setTextEncoding(SkPaint::kGlyphID_TextEncoding); paint.setTextAlign(SkPaint::kLeft_Align); TestUtils::drawTextToCanvas(&canvas, "test text", paint, 25, 25); paint.setTextAlign(SkPaint::kCenter_Align); @@ -135,12 +138,17 @@ TEST(RecordingCanvas, drawText_forceAlignLeft) { }); int count = 0; - playbackOps(*dl, [&count](const RecordedOp& op) { + float lastX = FLT_MAX; + playbackOps(*dl, [&count, &lastX](const RecordedOp& op) { count++; ASSERT_EQ(RecordedOpId::TextOp, op.opId); EXPECT_EQ(SkPaint::kLeft_Align, op.paint->getTextAlign()) << "recorded drawText commands must force kLeft_Align on their paint"; - EXPECT_EQ(SkPaint::kGlyphID_TextEncoding, op.paint->getTextEncoding()); // verify TestUtils + + // verify TestUtils alignment offsetting (TODO: move asserts to Canvas base class) + EXPECT_GT(lastX, ((const TextOp&)op).x) + << "x coordinate should reduce across each of the draw commands, from alignment"; + lastX = ((const TextOp&)op).x; }); ASSERT_EQ(3, count); } @@ -334,5 +342,39 @@ TEST(RecordingCanvas, insertReorderBarrier) { EXPECT_TRUE(chunks[1].reorderChildren); } +TEST(RecordingCanvas, refPaint) { + SkPaint paint; + paint.setAntiAlias(true); + paint.setTextSize(20); + paint.setTextAlign(SkPaint::kLeft_Align); + paint.setTextEncoding(SkPaint::kGlyphID_TextEncoding); + + auto dl = TestUtils::createDisplayList(200, 200, [&paint](RecordingCanvas& canvas) { + paint.setColor(SK_ColorBLUE); + // first three should use same paint + canvas.drawRect(0, 0, 200, 10, paint); + SkPaint paintCopy(paint); + canvas.drawRect(0, 10, 200, 20, paintCopy); + TestUtils::drawTextToCanvas(&canvas, "helloworld", paint, 50, 25); + + // only here do we use different paint ptr + paint.setColor(SK_ColorRED); + canvas.drawRect(0, 20, 200, 30, paint); + }); + auto ops = dl->getOps(); + ASSERT_EQ(4u, ops.size()); + + // first three are the same + EXPECT_NE(nullptr, ops[0]->paint); + EXPECT_NE(&paint, ops[0]->paint); + EXPECT_EQ(ops[0]->paint, ops[1]->paint); + EXPECT_EQ(ops[0]->paint, ops[2]->paint); + + // last is different, but still copied / non-null + EXPECT_NE(nullptr, ops[3]->paint); + EXPECT_NE(ops[0]->paint, ops[3]->paint); + EXPECT_NE(&paint, ops[3]->paint); +} + } // namespace uirenderer } // namespace android diff --git a/libs/hwui/utils/TestUtils.cpp b/libs/hwui/utils/TestUtils.cpp index dd6fc36b8042f..6cef85203352b 100644 --- a/libs/hwui/utils/TestUtils.cpp +++ b/libs/hwui/utils/TestUtils.cpp @@ -37,44 +37,56 @@ SkColor TestUtils::interpolateColor(float fraction, SkColor start, SkColor end) } void TestUtils::drawTextToCanvas(TestCanvas* canvas, const char* text, - const SkPaint& inPaint, float x, float y) { - // copy to force TextEncoding (which JNI layer would have done) - SkPaint paint(inPaint); - paint.setTextEncoding(SkPaint::kGlyphID_TextEncoding); + const SkPaint& paint, float x, float y) { + // drawing text requires GlyphID TextEncoding (which JNI layer would have done) + LOG_ALWAYS_FATAL_IF(paint.getTextEncoding() != SkPaint::kGlyphID_TextEncoding, + "must use glyph encoding"); - SkMatrix identity; - identity.setIdentity(); - SkSurfaceProps surfaceProps(0, kUnknown_SkPixelGeometry); - SkAutoGlyphCacheNoGamma autoCache(paint, &surfaceProps, &identity); + SkMatrix identity; + identity.setIdentity(); + SkSurfaceProps surfaceProps(0, kUnknown_SkPixelGeometry); + SkAutoGlyphCacheNoGamma autoCache(paint, &surfaceProps, &identity); - float totalAdvance = 0; - std::vector glyphs; - std::vector positions; - Rect bounds; - while (*text != '\0') { - SkUnichar unichar = SkUTF8_NextUnichar(&text); - glyph_t glyph = autoCache.getCache()->unicharToGlyph(unichar); - autoCache.getCache()->unicharToGlyph(unichar); + float totalAdvance = 0; + std::vector glyphs; + std::vector positions; + Rect bounds; + while (*text != '\0') { + SkUnichar unichar = SkUTF8_NextUnichar(&text); + glyph_t glyph = autoCache.getCache()->unicharToGlyph(unichar); + autoCache.getCache()->unicharToGlyph(unichar); - // push glyph and its relative position - glyphs.push_back(glyph); - positions.push_back(totalAdvance); - positions.push_back(0); + // push glyph and its relative position + glyphs.push_back(glyph); + positions.push_back(totalAdvance); + positions.push_back(0); - // compute bounds - SkGlyph skGlyph = autoCache.getCache()->getUnicharMetrics(unichar); - Rect glyphBounds(skGlyph.fWidth, skGlyph.fHeight); - glyphBounds.translate(totalAdvance + skGlyph.fLeft, skGlyph.fTop); - bounds.unionWith(glyphBounds); + // compute bounds + SkGlyph skGlyph = autoCache.getCache()->getUnicharMetrics(unichar); + Rect glyphBounds(skGlyph.fWidth, skGlyph.fHeight); + glyphBounds.translate(totalAdvance + skGlyph.fLeft, skGlyph.fTop); + bounds.unionWith(glyphBounds); - // advance next character - SkScalar skWidth; - paint.getTextWidths(&glyph, sizeof(glyph), &skWidth, NULL); - totalAdvance += skWidth; - } - bounds.translate(x, y); - canvas->drawText(glyphs.data(), positions.data(), glyphs.size(), paint, x, y, - bounds.left, bounds.top, bounds.right, bounds.bottom, totalAdvance); + // advance next character + SkScalar skWidth; + paint.getTextWidths(&glyph, sizeof(glyph), &skWidth, NULL); + totalAdvance += skWidth; + } + + // apply alignment via x parameter (which JNI layer would have done) + if (paint.getTextAlign() == SkPaint::kCenter_Align) { + x -= totalAdvance / 2; + } else if (paint.getTextAlign() == SkPaint::kRight_Align) { + x -= totalAdvance; + } + + bounds.translate(x, y); + + // Force left alignment, since alignment offset is already baked in + SkPaint alignPaintCopy(paint); + alignPaintCopy.setTextAlign(SkPaint::kLeft_Align); + canvas->drawText(glyphs.data(), positions.data(), glyphs.size(), alignPaintCopy, x, y, + bounds.left, bounds.top, bounds.right, bounds.bottom, totalAdvance); } } /* namespace uirenderer */ diff --git a/libs/hwui/utils/TestUtils.h b/libs/hwui/utils/TestUtils.h index f9fa242ec8336..c5dcf09317b29 100644 --- a/libs/hwui/utils/TestUtils.h +++ b/libs/hwui/utils/TestUtils.h @@ -197,7 +197,7 @@ public: static SkColor interpolateColor(float fraction, SkColor start, SkColor end); static void drawTextToCanvas(TestCanvas* canvas, const char* text, - const SkPaint& inPaint, float x, float y); + const SkPaint& paint, float x, float y); private: static void syncHierarchyPropertiesAndDisplayListImpl(RenderNode* node) {