From 48fb8d8b30a57da7b9247621bf04f99000f4784e Mon Sep 17 00:00:00 2001 From: Mike Klein Date: Wed, 10 Oct 2018 13:10:24 -0400 Subject: [PATCH] consolidate SkBinaryWriteBuffers It looks like the attached bug is about stack overflow when constructing the two SkBinaryWriteBuffers (on the stack). I can't find any really obvious reason we need to keep the path effect and the mask effect separate here any longer, so I've consolidated into a single buffer and tag for both. Minor refactoring: static, const, etc. Bug: chromium:891693 Cq-Include-Trybots: master.tryserver.blink:linux_trusty_blink_rel Change-Id: I48c411968b62633f13a83e24c3a202a457485b6d Reviewed-on: https://skia-review.googlesource.com/c/161160 Commit-Queue: Mike Klein Reviewed-by: Herb Derby --- src/core/SkRemoteGlyphCache.cpp | 14 +--- src/core/SkScalerContext.cpp | 126 ++++++-------------------------- src/core/SkScalerContext.h | 3 +- src/core/SkWriteBuffer.cpp | 2 +- src/core/SkWriteBuffer.h | 4 +- 5 files changed, 31 insertions(+), 118 deletions(-) diff --git a/src/core/SkRemoteGlyphCache.cpp b/src/core/SkRemoteGlyphCache.cpp index c2c0fa7248..99fbe2f32e 100644 --- a/src/core/SkRemoteGlyphCache.cpp +++ b/src/core/SkRemoteGlyphCache.cpp @@ -17,7 +17,6 @@ #include "SkDraw.h" #include "SkGlyphRun.h" #include "SkGlyphCache.h" -#include "SkPathEffect.h" #include "SkRemoteGlyphCacheImpl.h" #include "SkStrikeCache.h" #include "SkTraceEvent.h" @@ -45,18 +44,11 @@ static SkDescriptor* auto_descriptor_from_desc(const SkDescriptor* source_desc, desc->addEntry(kRec_SkDescriptorTag, sizeof(rec), &rec); } - // Path effect. + // Effects. { uint32_t size; - auto ptr = source_desc->findEntry(kPathEffect_SkDescriptorTag, &size); - if (ptr) desc->addEntry(kPathEffect_SkDescriptorTag, size, ptr); - } - - // Mask filter. - { - uint32_t size; - auto ptr = source_desc->findEntry(kMaskFilter_SkDescriptorTag, &size); - if (ptr) desc->addEntry(kMaskFilter_SkDescriptorTag, size, ptr); + auto ptr = source_desc->findEntry(kEffects_SkDescriptorTag, &size); + if (ptr) { desc->addEntry(kEffects_SkDescriptorTag, size, ptr); } } desc->computeChecksum(); diff --git a/src/core/SkScalerContext.cpp b/src/core/SkScalerContext.cpp index 4325e85d87..f509654d45 100644 --- a/src/core/SkScalerContext.cpp +++ b/src/core/SkScalerContext.cpp @@ -57,9 +57,7 @@ SkScalerContext::SkScalerContext(sk_sp typeface, const SkScalerConte SkDebugf("SkScalerContext checksum %x count %d length %d\n", desc->getChecksum(), desc->getCount(), desc->getLength()); SkDebugf("%s", fRec.dump().c_str()); - SkDebugf(" pathEffect %x maskFilter %x\n", - desc->findEntry(kPathEffect_SkDescriptorTag, nullptr), - desc->findEntry(kMaskFilter_SkDescriptorTag, nullptr)); + SkDebugf(" effects %x\n", desc->findEntry(kEffects_SkDescriptorTag, nullptr)); #endif } @@ -1126,107 +1124,36 @@ SkDescriptor* SkScalerContext::CreateDescriptorAndEffectsUsingPaint( return AutoDescriptorGivenRecAndEffects(rec, *effects, ad); } -static size_t calculate_size_and_flatten( - const SkScalerContextRec& rec, - const SkScalerContextEffects& effects, - SkBinaryWriteBuffer* pathEffectBuffer, - SkBinaryWriteBuffer* maskFilterBuffer) -{ +static size_t calculate_size_and_flatten(const SkScalerContextRec& rec, + const SkScalerContextEffects& effects, + SkBinaryWriteBuffer* effectBuffer) { size_t descSize = sizeof(rec); int entryCount = 1; - if (effects.fPathEffect) { - pathEffectBuffer->writeFlattenable(effects.fPathEffect); - descSize += pathEffectBuffer->bytesWritten(); - entryCount += 1; - } - if (effects.fMaskFilter) { - maskFilterBuffer->writeFlattenable(effects.fMaskFilter); - descSize += maskFilterBuffer->bytesWritten(); + if (effects.fPathEffect || effects.fMaskFilter) { + if (effects.fPathEffect) { effectBuffer->writeFlattenable(effects.fPathEffect); } + if (effects.fMaskFilter) { effectBuffer->writeFlattenable(effects.fMaskFilter); } entryCount += 1; + descSize += effectBuffer->bytesWritten(); } descSize += SkDescriptor::ComputeOverhead(entryCount); return descSize; } -#ifdef SK_DEBUG - #define TEST_DESC -#endif - -#ifdef TEST_DESC -static void test_desc(const SkScalerContextRec& rec, - const SkScalerContextEffects& effects, - SkBinaryWriteBuffer* peBuffer, - SkBinaryWriteBuffer* mfBuffer, - const SkDescriptor* desc) { - // Check that we completely write the bytes in desc (our key), and that - // there are no uninitialized bytes. If there were, then we would get - // false-misses (or worse, false-hits) in our fontcache. - // - // We do this buy filling 2 others, one with 0s and the other with 1s - // and create those, and then check that all 3 are identical. - SkAutoDescriptor ad1(desc->getLength()); - SkAutoDescriptor ad2(desc->getLength()); - SkDescriptor* desc1 = ad1.getDesc(); - SkDescriptor* desc2 = ad2.getDesc(); - - memset(desc1, 0x00, desc->getLength()); - memset(desc2, 0xFF, desc->getLength()); - - desc1->init(); - desc2->init(); - desc1->addEntry(kRec_SkDescriptorTag, sizeof(rec), &rec); - desc2->addEntry(kRec_SkDescriptorTag, sizeof(rec), &rec); - - auto add_flattenable = [](SkDescriptor* desc, uint32_t tag, - SkBinaryWriteBuffer* buffer) { - buffer->writeToMemory(desc->addEntry(tag, buffer->bytesWritten(), nullptr)); - }; - - if (effects.fPathEffect) { - add_flattenable(desc1, kPathEffect_SkDescriptorTag, peBuffer); - add_flattenable(desc2, kPathEffect_SkDescriptorTag, peBuffer); - } - if (effects.fMaskFilter) { - add_flattenable(desc1, kMaskFilter_SkDescriptorTag, mfBuffer); - add_flattenable(desc2, kMaskFilter_SkDescriptorTag, mfBuffer); - } - - SkASSERT(desc->getLength() == desc1->getLength()); - SkASSERT(desc->getLength() == desc2->getLength()); - desc1->computeChecksum(); - desc2->computeChecksum(); - SkASSERT(!memcmp(desc, desc1, desc->getLength())); - SkASSERT(!memcmp(desc, desc2, desc->getLength())); -} -#endif - -void generate_descriptor( - const SkScalerContextRec& rec, - const SkScalerContextEffects& effects, - SkBinaryWriteBuffer* pathEffectBuffer, - SkBinaryWriteBuffer* maskFilterBuffer, - SkDescriptor* desc) -{ +static void generate_descriptor(const SkScalerContextRec& rec, + const SkBinaryWriteBuffer& effectBuffer, + SkDescriptor* desc) { desc->init(); desc->addEntry(kRec_SkDescriptorTag, sizeof(rec), &rec); - auto add = [&desc](uint32_t tag, SkBinaryWriteBuffer* buffer) { - buffer->writeToMemory(desc->addEntry(tag, buffer->bytesWritten(), nullptr)); - }; - - if (effects.fPathEffect) { - add(kPathEffect_SkDescriptorTag, pathEffectBuffer); - } - if (effects.fMaskFilter) { - add(kMaskFilter_SkDescriptorTag, maskFilterBuffer); + if (effectBuffer.bytesWritten() > 0) { + effectBuffer.writeToMemory(desc->addEntry(kEffects_SkDescriptorTag, + effectBuffer.bytesWritten(), + nullptr)); } desc->computeChecksum(); -#ifdef TEST_DESC - test_desc(rec, effects, pathEffectBuffer, maskFilterBuffer, desc); -#endif } SkDescriptor* SkScalerContext::AutoDescriptorGivenRecAndEffects( @@ -1234,11 +1161,10 @@ SkDescriptor* SkScalerContext::AutoDescriptorGivenRecAndEffects( const SkScalerContextEffects& effects, SkAutoDescriptor* ad) { - SkBinaryWriteBuffer peBuffer, mfBuffer; + SkBinaryWriteBuffer buf; - ad->reset(calculate_size_and_flatten(rec, effects, &peBuffer, &mfBuffer)); - - generate_descriptor(rec, effects, &peBuffer, &mfBuffer, ad->getDesc()); + ad->reset(calculate_size_and_flatten(rec, effects, &buf)); + generate_descriptor(rec, buf, ad->getDesc()); return ad->getDesc(); } @@ -1247,27 +1173,23 @@ std::unique_ptr SkScalerContext::DescriptorGivenRecAndEffects( const SkScalerContextRec& rec, const SkScalerContextEffects& effects) { - SkBinaryWriteBuffer peBuffer, mfBuffer; + SkBinaryWriteBuffer buf; - auto desc = SkDescriptor::Alloc(calculate_size_and_flatten(rec, effects, &peBuffer, &mfBuffer)); - - generate_descriptor(rec, effects, &peBuffer, &mfBuffer, desc.get()); + auto desc = SkDescriptor::Alloc(calculate_size_and_flatten(rec, effects, &buf)); + generate_descriptor(rec, buf, desc.get()); return desc; } void SkScalerContext::DescriptorBufferGiveRec(const SkScalerContextRec& rec, void* buffer) { - SkScalerContextEffects noEffects; - SkBinaryWriteBuffer peBuffer, mfBuffer; - generate_descriptor(rec, noEffects, &peBuffer, &mfBuffer, (SkDescriptor*)buffer); + generate_descriptor(rec, SkBinaryWriteBuffer{}, (SkDescriptor*)buffer); } bool SkScalerContext::CheckBufferSizeForRec(const SkScalerContextRec& rec, const SkScalerContextEffects& effects, size_t size) { - SkBinaryWriteBuffer peBuffer, mfBuffer; - - return size >= calculate_size_and_flatten(rec, effects, &peBuffer, &mfBuffer); + SkBinaryWriteBuffer buf; + return size >= calculate_size_and_flatten(rec, effects, &buf); } diff --git a/src/core/SkScalerContext.h b/src/core/SkScalerContext.h index 4bce9f49bc..b10d89ef0e 100644 --- a/src/core/SkScalerContext.h +++ b/src/core/SkScalerContext.h @@ -436,8 +436,7 @@ private: }; #define kRec_SkDescriptorTag SkSetFourByteTag('s', 'r', 'e', 'c') -#define kPathEffect_SkDescriptorTag SkSetFourByteTag('p', 't', 'h', 'e') -#define kMaskFilter_SkDescriptorTag SkSetFourByteTag('m', 's', 'k', 'f') +#define kEffects_SkDescriptorTag SkSetFourByteTag('e', 'f', 'c', 't') /////////////////////////////////////////////////////////////////////////////// diff --git a/src/core/SkWriteBuffer.cpp b/src/core/SkWriteBuffer.cpp index 8300f61d4b..2d96b234e5 100644 --- a/src/core/SkWriteBuffer.cpp +++ b/src/core/SkWriteBuffer.cpp @@ -129,7 +129,7 @@ size_t SkBinaryWriteBuffer::writeStream(SkStream* stream, size_t length) { return bytesWritten; } -bool SkBinaryWriteBuffer::writeToStream(SkWStream* stream) { +bool SkBinaryWriteBuffer::writeToStream(SkWStream* stream) const { return fWriter.writeToStream(stream); } diff --git a/src/core/SkWriteBuffer.h b/src/core/SkWriteBuffer.h index 011977ca19..168b518f0f 100644 --- a/src/core/SkWriteBuffer.h +++ b/src/core/SkWriteBuffer.h @@ -122,8 +122,8 @@ public: void writeTypeface(SkTypeface* typeface) override; void writePaint(const SkPaint& paint) override; - bool writeToStream(SkWStream*); - void writeToMemory(void* dst) { fWriter.flatten(dst); } + bool writeToStream(SkWStream*) const; + void writeToMemory(void* dst) const { fWriter.flatten(dst); } void setFactoryRecorder(sk_sp); void setTypefaceRecorder(sk_sp);