From 33a30503d76fdd989358cedd78445ba96bb809dd Mon Sep 17 00:00:00 2001 From: reed Date: Thu, 11 Sep 2014 08:42:36 -0700 Subject: [PATCH] SkData can allocate room for its contents in the same block BUG=skia: R=bungeman@google.com, mtklein@google.com Author: reed@google.com Review URL: https://codereview.chromium.org/560653004 --- include/core/SkData.h | 41 ++++++++++++++--- src/core/SkBitmap.cpp | 4 +- src/core/SkData.cpp | 58 +++++++++++++++++++++---- src/core/SkFlattenableSerialization.cpp | 6 +-- src/core/SkStream.cpp | 35 ++++++++------- src/images/SkDecodingImageGenerator.cpp | 28 ++++++------ src/pdf/SkPDFFont.cpp | 9 ++-- src/pdf/SkPDFGraphicState.cpp | 2 +- src/pdf/SkPDFShader.cpp | 18 +++----- src/sfnt/SkOTUtils.cpp | 4 +- src/utils/SkTextureCompressor.cpp | 15 +++---- 11 files changed, 141 insertions(+), 79 deletions(-) diff --git a/include/core/SkData.h b/include/core/SkData.h index fba2846c70..6da8e11366 100644 --- a/include/core/SkData.h +++ b/include/core/SkData.h @@ -1,4 +1,3 @@ - /* * Copyright 2011 Google Inc. * @@ -6,8 +5,6 @@ * found in the LICENSE file. */ - - #ifndef SkData_DEFINED #define SkData_DEFINED @@ -44,6 +41,19 @@ public: return reinterpret_cast(fPtr); } + /** + * USE WITH CAUTION. + * This call will assert that the refcnt is 1, as a precaution against modifying the + * contents when another client/thread has access to the data. + */ + void* writable_data() { + if (fSize) { + // only assert we're unique if we're not empty + SkASSERT(this->unique()); + } + return fPtr; + } + /** * Helper to copy a range of the data into a caller-provided buffer. * Returns the actual number of bytes copied, after clamping offset and @@ -69,6 +79,12 @@ public: */ static SkData* NewWithCopy(const void* data, size_t length); + /** + * Create a new data with uninitialized contents. The caller should call writable_data() + * to write into the buffer, but this must be done before another ref() is made. + */ + static SkData* NewUninitialized(size_t length); + /** * Create a new dataref by copying the specified c-string * (a null-terminated array of bytes). The returned SkData will have size() @@ -81,8 +97,15 @@ public: * Create a new dataref, taking the data ptr as is, and using the * releaseproc to free it. The proc may be NULL. */ - static SkData* NewWithProc(const void* data, size_t length, - ReleaseProc proc, void* context); + static SkData* NewWithProc(const void* data, size_t length, ReleaseProc proc, void* context); + + /** + * Call this when the data parameter is already const and will outlive the lifetime of the + * SkData. Suitable for with const globals. + */ + static SkData* NewWithoutCopy(const void* data, size_t length) { + return NewWithProc(data, length, NULL, NULL); + } /** * Create a new dataref from a pointer allocated by malloc. The Data object @@ -130,16 +153,22 @@ private: ReleaseProc fReleaseProc; void* fReleaseProcContext; - const void* fPtr; + void* fPtr; size_t fSize; SkData(const void* ptr, size_t size, ReleaseProc, void* context); + SkData(size_t size); // inplace new/delete virtual ~SkData(); + virtual void internal_dispose() const SK_OVERRIDE; + // Called the first time someone calls NewEmpty to initialize the singleton. static SkData* NewEmptyImpl(); static void DeleteEmpty(SkData*); + // shared internal factory + static SkData* PrivateNewWithCopy(const void* srcOrNull, size_t length); + typedef SkRefCnt INHERITED; }; diff --git a/src/core/SkBitmap.cpp b/src/core/SkBitmap.cpp index e28c6fdc34..f0ad029bea 100644 --- a/src/core/SkBitmap.cpp +++ b/src/core/SkBitmap.cpp @@ -1211,9 +1211,9 @@ bool SkBitmap::ReadRawPixels(SkReadBuffer* buffer, SkBitmap* bitmap) { return false; } - char* dst = (char*)sk_malloc_throw(ramSize); + SkAutoDataUnref data(SkData::NewUninitialized(ramSize)); + char* dst = (char*)data->writable_data(); buffer->readByteArray(dst, snugSize); - SkAutoDataUnref data(SkData::NewFromMalloc(dst, ramSize)); if (snugSize != ramSize) { const char* srcRow = dst + snugRB * (height - 1); diff --git a/src/core/SkData.cpp b/src/core/SkData.cpp index c65328720a..a7a3d8d559 100644 --- a/src/core/SkData.cpp +++ b/src/core/SkData.cpp @@ -11,19 +11,49 @@ #include "SkReadBuffer.h" #include "SkWriteBuffer.h" +static void sk_inplace_sentinel_releaseproc(const void*, size_t, void*) { + // we should never get called, as we are just a sentinel + sk_throw(); +} + SkData::SkData(const void* ptr, size_t size, ReleaseProc proc, void* context) { - fPtr = ptr; + fPtr = const_cast(ptr); fSize = size; fReleaseProc = proc; fReleaseProcContext = context; } +// This constructor means we are inline with our fPtr's contents. Thus we set fPtr +// to point right after this. We also set our releaseproc to sk_inplace_sentinel_releaseproc, +// since we need to handle "delete" ourselves. See internal_displose(). +// +SkData::SkData(size_t size) { + fPtr = (char*)(this + 1); // contents are immediately after this + fSize = size; + fReleaseProc = sk_inplace_sentinel_releaseproc; + fReleaseProcContext = NULL; +} + SkData::~SkData() { if (fReleaseProc) { fReleaseProc(fPtr, fSize, fReleaseProcContext); } } +void SkData::internal_dispose() const { + if (sk_inplace_sentinel_releaseproc == fReleaseProc) { + const_cast(this)->fReleaseProc = NULL; // so we don't call it in our destructor + + this->internal_dispose_restore_refcnt_to_1(); + this->~SkData(); // explicitly call this for refcnt bookkeeping + + sk_free(const_cast(this)); + } else { + this->internal_dispose_restore_refcnt_to_1(); + SkDELETE(this); + } +} + bool SkData::equals(const SkData* other) const { if (NULL == other) { return false; @@ -47,11 +77,24 @@ size_t SkData::copyRange(size_t offset, size_t length, void* buffer) const { return length; } +SkData* SkData::PrivateNewWithCopy(const void* srcOrNull, size_t length) { + if (0 == length) { + return SkData::NewEmpty(); + } + char* storage = (char*)sk_malloc_throw(sizeof(SkData) + length); + SkData* data = new (storage) SkData(length); + if (srcOrNull) { + memcpy(data->writable_data(), srcOrNull, length); + } + return data; +} + /////////////////////////////////////////////////////////////////////////////// SkData* SkData::NewEmptyImpl() { return new SkData(NULL, 0, NULL, NULL); } + void SkData::DeleteEmpty(SkData* ptr) { SkDELETE(ptr); } SkData* SkData::NewEmpty() { @@ -68,14 +111,13 @@ SkData* SkData::NewFromMalloc(const void* data, size_t length) { return new SkData(data, length, sk_free_releaseproc, NULL); } -SkData* SkData::NewWithCopy(const void* data, size_t length) { - if (0 == length) { - return SkData::NewEmpty(); - } +SkData* SkData::NewWithCopy(const void* src, size_t length) { + SkASSERT(src); + return PrivateNewWithCopy(src, length); +} - void* copy = sk_malloc_throw(length); // balanced in sk_free_releaseproc - memcpy(copy, data, length); - return new SkData(copy, length, sk_free_releaseproc, NULL); +SkData* SkData::NewUninitialized(size_t length) { + return PrivateNewWithCopy(NULL, length); } SkData* SkData::NewWithProc(const void* data, size_t length, diff --git a/src/core/SkFlattenableSerialization.cpp b/src/core/SkFlattenableSerialization.cpp index b33bca6dbb..31602079a3 100644 --- a/src/core/SkFlattenableSerialization.cpp +++ b/src/core/SkFlattenableSerialization.cpp @@ -15,9 +15,9 @@ SkData* SkValidatingSerializeFlattenable(SkFlattenable* flattenable) { SkWriteBuffer writer(SkWriteBuffer::kValidation_Flag); writer.writeFlattenable(flattenable); size_t size = writer.bytesWritten(); - void* data = sk_malloc_throw(size); - writer.writeToMemory(data); - return SkData::NewFromMalloc(data, size); + SkData* data = SkData::NewUninitialized(size); + writer.writeToMemory(data->writable_data()); + return data; } SkFlattenable* SkValidatingDeserializeFlattenable(const void* data, size_t size, diff --git a/src/core/SkStream.cpp b/src/core/SkStream.cpp index ca9f51f061..b04bc162d2 100644 --- a/src/core/SkStream.cpp +++ b/src/core/SkStream.cpp @@ -69,9 +69,9 @@ SkData* SkStream::readData() { if (0 == size) { return SkData::NewEmpty(); } else { - void* buffer = sk_malloc_throw(size); - this->read(buffer, size); - return SkData::NewFromMalloc(buffer, size); + SkData* data = SkData::NewUninitialized(size); + this->read(data->writable_data(), size); + return data; } } @@ -308,7 +308,7 @@ static SkData* newFromParams(const void* src, size_t size, bool copyData) { if (copyData) { return SkData::NewWithCopy(src, size); } else { - return SkData::NewWithProc(src, size, NULL, NULL); + return SkData::NewWithoutCopy(src, size); } } @@ -318,7 +318,7 @@ SkMemoryStream::SkMemoryStream() { } SkMemoryStream::SkMemoryStream(size_t size) { - fData = SkData::NewFromMalloc(sk_malloc_throw(size), size); + fData = SkData::NewUninitialized(size); fOffset = 0; } @@ -654,12 +654,12 @@ void SkDynamicMemoryWStream::padToAlign4() SkData* SkDynamicMemoryWStream::copyToData() const { if (NULL == fCopy) { - void* buffer = sk_malloc_throw(fBytesWritten); - this->copyTo(buffer); - fCopy = SkData::NewFromMalloc(buffer, fBytesWritten); + SkData* data = SkData::NewUninitialized(fBytesWritten); + // be sure to call copyTo() before we assign to fCopy + this->copyTo(data->writable_data()); + fCopy = data; } - fCopy->ref(); - return fCopy; + return SkRef(fCopy); } void SkDynamicMemoryWStream::invalidateCopy() { @@ -891,11 +891,12 @@ SkData* SkCopyStreamToData(SkStream* stream) { if (stream->hasLength()) { const size_t length = stream->getLength(); - SkAutoMalloc dst(length); - if (stream->read(dst.get(), length) != length) { - return NULL; + SkData* data = SkData::NewUninitialized(length); + if (stream->read(data->writable_data(), length) != length) { + data->unref(); + data = NULL; } - return SkData::NewFromMalloc(dst.detach(), length); + return data; } SkDynamicMemoryWStream tempStream; @@ -922,11 +923,9 @@ SkStreamRewindable* SkStreamRewindableFromSkStream(SkStream* stream) { if (stream->hasPosition()) { // If stream has length, but can't rewind. length -= stream->getPosition(); } - SkAutoMalloc allocMemory(length); - SkDEBUGCODE(size_t read =) stream->read(allocMemory.get(), length); + SkAutoTUnref data(SkData::NewUninitialized(length)); + SkDEBUGCODE(size_t read =) stream->read(data->writable_data(), length); SkASSERT(length == read); - SkAutoTUnref data( - SkData::NewFromMalloc(allocMemory.detach(), length)); return SkNEW_ARGS(SkMemoryStream, (data.get())); } SkDynamicMemoryWStream tempStream; diff --git a/src/images/SkDecodingImageGenerator.cpp b/src/images/SkDecodingImageGenerator.cpp index 1d91bcc0f9..359eb370ad 100644 --- a/src/images/SkDecodingImageGenerator.cpp +++ b/src/images/SkDecodingImageGenerator.cpp @@ -132,22 +132,20 @@ DecodingImageGenerator::~DecodingImageGenerator() { SkData* DecodingImageGenerator::onRefEncodedData() { // This functionality is used in `gm --serialize` // Does not encode options. - if (fData != NULL) { - return SkSafeRef(fData); - } - // TODO(halcanary): SkStreamRewindable needs a refData() function - // which returns a cheap copy of the underlying data. - if (!fStream->rewind()) { - return NULL; - } - size_t length = fStream->getLength(); - if (0 == length) { - return NULL; + if (NULL == fData) { + // TODO(halcanary): SkStreamRewindable needs a refData() function + // which returns a cheap copy of the underlying data. + if (!fStream->rewind()) { + return NULL; + } + size_t length = fStream->getLength(); + if (0 == length) { + return NULL; + } + fData = SkData::NewUninitialized(length); + SkCheckResult(fStream->read(fData->writable_data(), length), length); } - void* buffer = sk_malloc_flags(length, 0); - SkCheckResult(fStream->read(buffer, length), length); - fData = SkData::NewFromMalloc(buffer, length); - return SkSafeRef(fData); + return SkRef(fData); } bool DecodingImageGenerator::onGetPixels(const SkImageInfo& info, diff --git a/src/pdf/SkPDFFont.cpp b/src/pdf/SkPDFFont.cpp index 9e13053c60..b4e6d98722 100644 --- a/src/pdf/SkPDFFont.cpp +++ b/src/pdf/SkPDFFont.cpp @@ -204,16 +204,15 @@ static SkData* handle_type1_stream(SkStream* srcStream, size_t* headerLen, SkASSERT(length > 0); SkASSERT(length + (2 * kPFBSectionHeaderLength) <= srcLen); - SkAutoTMalloc buffer(length); + SkData* data = SkData::NewUninitialized(length); const uint8_t* const srcHeader = src + kPFBSectionHeaderLength; // There is a six-byte section header before header and data // (but not trailer) that we're not going to copy. - const uint8_t* const srcData - = srcHeader + *headerLen + kPFBSectionHeaderLength; + const uint8_t* const srcData = srcHeader + *headerLen + kPFBSectionHeaderLength; const uint8_t* const srcTrailer = srcData + *headerLen; - uint8_t* const resultHeader = buffer.get(); + uint8_t* const resultHeader = (uint8_t*)data->writable_data(); uint8_t* const resultData = resultHeader + *headerLen; uint8_t* const resultTrailer = resultData + *dataLen; @@ -223,7 +222,7 @@ static SkData* handle_type1_stream(SkStream* srcStream, size_t* headerLen, memcpy(resultData, srcData, *dataLen); memcpy(resultTrailer, srcTrailer, *trailerLen); - return SkData::NewFromMalloc(buffer.detach(), length); + return data; } // A PFA has to be converted for PDF. diff --git a/src/pdf/SkPDFGraphicState.cpp b/src/pdf/SkPDFGraphicState.cpp index bd8afe0684..7664ed699b 100644 --- a/src/pdf/SkPDFGraphicState.cpp +++ b/src/pdf/SkPDFGraphicState.cpp @@ -123,7 +123,7 @@ SkPDFObject* SkPDFGraphicState::GetInvertFunction() { static const char psInvert[] = "{1 exch sub}"; // Do not copy the trailing '\0' into the SkData. SkAutoTUnref psInvertStream( - SkData::NewWithCopy(psInvert, strlen(psInvert))); + SkData::NewWithoutCopy(psInvert, strlen(psInvert))); invertFunction = new SkPDFStream(psInvertStream.get()); invertFunction->insertInt("FunctionType", 4); diff --git a/src/pdf/SkPDFShader.cpp b/src/pdf/SkPDFShader.cpp index afe6bb06b3..d9b701c081 100644 --- a/src/pdf/SkPDFShader.cpp +++ b/src/pdf/SkPDFShader.cpp @@ -1160,10 +1160,8 @@ SkPDFImageShader::SkPDFImageShader(SkPDFShader::State* state) : fState(state) { fState.get()->fImage.unlockPixels(); } -SkPDFStream* SkPDFFunctionShader::makePSFunction(const SkString& psCode, - SkPDFArray* domain) { - SkAutoDataUnref funcData(SkData::NewWithCopy(psCode.c_str(), - psCode.size())); +SkPDFStream* SkPDFFunctionShader::makePSFunction(const SkString& psCode, SkPDFArray* domain) { + SkAutoDataUnref funcData(SkData::NewWithCopy(psCode.c_str(), psCode.size())); SkPDFStream* result = new SkPDFStream(funcData.get()); result->insertInt("FunctionType", 4); result->insert("Domain", domain); @@ -1171,14 +1169,12 @@ SkPDFStream* SkPDFFunctionShader::makePSFunction(const SkString& psCode, return result; } -SkPDFShader::ShaderCanonicalEntry::ShaderCanonicalEntry(SkPDFObject* pdfShader, - const State* state) - : fPDFShader(pdfShader), - fState(state) { -} +SkPDFShader::ShaderCanonicalEntry::ShaderCanonicalEntry(SkPDFObject* pdfShader, const State* state) + : fPDFShader(pdfShader) + , fState(state) +{} -bool SkPDFShader::ShaderCanonicalEntry::operator==( - const ShaderCanonicalEntry& b) const { +bool SkPDFShader::ShaderCanonicalEntry::operator==(const ShaderCanonicalEntry& b) const { return fPDFShader == b.fPDFShader || (fState != NULL && b.fState != NULL && *fState == *b.fState); } diff --git a/src/sfnt/SkOTUtils.cpp b/src/sfnt/SkOTUtils.cpp index e76d1da08c..0e009528b4 100644 --- a/src/sfnt/SkOTUtils.cpp +++ b/src/sfnt/SkOTUtils.cpp @@ -83,8 +83,8 @@ SkData* SkOTUtils::RenameFont(SkStream* fontData, const char* fontName, int font size_t originalDataSize = fontData->getLength() - oldNameTablePhysicalSize; size_t newDataSize = originalDataSize + nameTablePhysicalSize; - SK_OT_BYTE* data = static_cast(sk_malloc_throw(newDataSize)); - SkAutoTUnref rewrittenFontData(SkData::NewFromMalloc(data, newDataSize)); + SkAutoTUnref rewrittenFontData(SkData::NewUninitialized(newDataSize)); + SK_OT_BYTE* data = static_cast(rewrittenFontData->writable_data()); if (fontData->read(data, oldNameTableOffset) < oldNameTableOffset) { return NULL; diff --git a/src/utils/SkTextureCompressor.cpp b/src/utils/SkTextureCompressor.cpp index 07b58c5a60..799eadc84b 100644 --- a/src/utils/SkTextureCompressor.cpp +++ b/src/utils/SkTextureCompressor.cpp @@ -173,7 +173,7 @@ bool CompressBufferToFormat(uint8_t* dst, const uint8_t* src, SkColorType srcCol return false; } -SkData *CompressBitmapToFormat(const SkBitmap &bitmap, Format format) { +SkData* CompressBitmapToFormat(const SkBitmap &bitmap, Format format) { SkAutoLockPixels alp(bitmap); int compressedDataSize = GetCompressedDataSize(format, bitmap.width(), bitmap.height()); @@ -182,15 +182,14 @@ SkData *CompressBitmapToFormat(const SkBitmap &bitmap, Format format) { } const uint8_t* src = reinterpret_cast(bitmap.getPixels()); - uint8_t* dst = reinterpret_cast(sk_malloc_throw(compressedDataSize)); + SkData* dst = SkData::NewUninitialized(compressedDataSize); - if (CompressBufferToFormat(dst, src, bitmap.colorType(), bitmap.width(), bitmap.height(), - bitmap.rowBytes(), format)) { - return SkData::NewFromMalloc(dst, compressedDataSize); + if (!CompressBufferToFormat((uint8_t*)dst->writable_data(), src, bitmap.colorType(), + bitmap.width(), bitmap.height(), bitmap.rowBytes(), format)) { + dst->unref(); + dst = NULL; } - - sk_free(dst); - return NULL; + return dst; } SkBlitter* CreateBlitterForFormat(int width, int height, void* compressedBuffer, -- 2.34.1