From 303044579913eacc177d4b28a674121725c565bb Mon Sep 17 00:00:00 2001 From: scroggo Date: Tue, 9 Dec 2014 08:27:37 -0800 Subject: [PATCH] Revert of Replace EncodeBitmap with an interface. (patchset #12 id:210001 of https://codereview.chromium.org/784643002/) Reason for revert: Failing serialization tasks in DM: http://build.chromium.org/p/client.skia/builders/Test-Win8-ShuttleA-GTX660-x86-Debug/builds/352/steps/dm/logs/stdio Original issue's description: > Replace EncodeBitmap with an interface. > > Gives more flexibility to the caller to decide whether to use the > encoded data returned by refEncodedData(). > > Provides an implementation that supports the old version of > SkPicture::serialize(). > > TODO: Update Chrome, so we can remove SK_LEGACY_ENCODE_BITMAP entirely > > BUG=skia:3190 > > Committed: https://skia.googlesource.com/skia/+/0c4aba6edb9900c597359dfa49d3ce4a41bc5dd1 > > Committed: https://skia.googlesource.com/skia/+/02b217f80b01a7dda8493422e5257c36a9ce8464 TBR=reed@google.com,rmistry@google.com NOTREECHECKS=true NOTRY=true BUG=skia:3190 Review URL: https://codereview.chromium.org/783393004 --- dm/DMSerializeTask.cpp | 2 +- gm/gmmain.cpp | 2 +- gyp/skia_for_chromium_defines.gypi | 2 -- include/core/SkPicture.h | 20 +------------ include/core/SkPixelSerializer.h | 52 -------------------------------- include/core/SkWriteBuffer.h | 24 +++++++-------- src/core/SkPicture.cpp | 39 ++---------------------- src/core/SkPictureData.cpp | 6 ++-- src/core/SkPictureData.h | 3 +- src/core/SkWriteBuffer.cpp | 61 +++++++++++++++++--------------------- tests/PictureTest.cpp | 22 +++----------- tools/PictureRenderer.cpp | 24 ++++----------- 12 files changed, 56 insertions(+), 201 deletions(-) delete mode 100644 include/core/SkPixelSerializer.h diff --git a/dm/DMSerializeTask.cpp b/dm/DMSerializeTask.cpp index a3e2503..d8b460d 100644 --- a/dm/DMSerializeTask.cpp +++ b/dm/DMSerializeTask.cpp @@ -21,7 +21,7 @@ void SerializeTask::draw() { SkAutoTUnref recorded(RecordPicture(fGM.get(), NULL/*no BBH*/)); SkDynamicMemoryWStream wStream; - recorded->serialize(&wStream); + recorded->serialize(&wStream, NULL); SkAutoTUnref rStream(wStream.detachAsStream()); SkAutoTUnref reconstructed(SkPicture::CreateFromStream(rStream)); diff --git a/gm/gmmain.cpp b/gm/gmmain.cpp index 46b039d..3eb1247 100644 --- a/gm/gmmain.cpp +++ b/gm/gmmain.cpp @@ -1039,7 +1039,7 @@ public: static SkPicture* stream_to_new_picture(const SkPicture& src) { SkDynamicMemoryWStream storage; - src.serialize(&storage); + src.serialize(&storage, NULL); SkAutoTUnref pictReadback(storage.detachAsStream()); SkPicture* retval = SkPicture::CreateFromStream(pictReadback, &SkImageDecoder::DecodeMemory); diff --git a/gyp/skia_for_chromium_defines.gypi b/gyp/skia_for_chromium_defines.gypi index 2c91d29..aee4112 100644 --- a/gyp/skia_for_chromium_defines.gypi +++ b/gyp/skia_for_chromium_defines.gypi @@ -15,8 +15,6 @@ 'skia_for_chromium_defines': [ 'SK_SUPPORT_LEGACY_TEXTRENDERMODE', 'SK_IGNORE_GPU_LAYER_HOISTING', - # Transition for skbug.com/3190 - 'SK_LEGACY_ENCODE_BITMAP', ], }, } diff --git a/include/core/SkPicture.h b/include/core/SkPicture.h index 88d8b05..f764d34 100644 --- a/include/core/SkPicture.h +++ b/include/core/SkPicture.h @@ -24,7 +24,6 @@ class SkBBoxHierarchy; class SkCanvas; class SkData; class SkPictureData; -class SkPixelSerializer; class SkStream; class SkWStream; @@ -36,8 +35,6 @@ namespace SkRecords { class CollectLayers; }; -//#define SK_LEGACY_ENCODE_BITMAP - /** \class SkPicture The SkPicture class records the drawing commands made to a canvas, to @@ -144,30 +141,15 @@ public: * @param pixelRefOffset DEPRECATED -- caller assumes it will return 0. * @return SkData If non-NULL, holds encoded data representing the passed * in bitmap. The caller is responsible for calling unref(). - * - * TODO: No longer used by SkPicture (except when SK_LEGACY_ENCODE_BITMAP - * is defined. Still used by PDF though. Move into PDF. */ typedef SkData* (*EncodeBitmap)(size_t* pixelRefOffset, const SkBitmap& bm); -#ifdef SK_LEGACY_ENCODE_BITMAP /** * Serialize to a stream. If non NULL, encoder will be used to encode * any bitmaps in the picture. * encoder will never be called with a NULL pixelRefOffset. - * DEPRECATED - use serialize(SkWStream*, SkPixelSerializer* serializer) - * instead. - */ - void serialize(SkWStream* wStream, EncodeBitmap encoder) const; -#endif - - /** - * Serialize to a stream. If non NULL, serializer will be used to serialize - * any bitmaps in the picture. - * - * TODO: Use serializer to serialize SkImages as well. */ - void serialize(SkWStream*, SkPixelSerializer* serializer = NULL) const; + void serialize(SkWStream*, EncodeBitmap encoder = NULL) const; /** * Serialize to a buffer. diff --git a/include/core/SkPixelSerializer.h b/include/core/SkPixelSerializer.h deleted file mode 100644 index 8fc445c..0000000 --- a/include/core/SkPixelSerializer.h +++ /dev/null @@ -1,52 +0,0 @@ -/* - * Copyright 2014 Google Inc. - * - * Use of this source code is governed by a BSD-style license that can be - * found in the LICENSE file. - */ - -#ifndef SkPixelSerializer_DEFINED -#define SkPixelSerializer_DEFINED - -#include "SkRefCnt.h" - -class SkData; -struct SkImageInfo; - -/** - * Interface for serializing pixels, e.g. SkBitmaps in an SkPicture. - */ -class SkPixelSerializer : public SkRefCnt { -public: - virtual ~SkPixelSerializer() {} - - /** - * Call to determine if the client wants to serialize the encoded data. If - * false, serialize another version (e.g. the result of encodePixels). - */ - bool useEncodedData(const void* data, size_t len) { - return this->onUseEncodedData(data, len); - } - - /** - * Call to get the client's version of encoding these pixels. If it - * returns NULL, serialize the raw pixels. - */ - SkData* encodePixels(const SkImageInfo& info, void* pixels, size_t rowBytes) { - return this->onEncodePixels(info, pixels, rowBytes); - } - -protected: - /** - * Return true if you want to serialize the encoded data, false if you want - * another version serialized (e.g. the result of encodePixels). - */ - virtual bool onUseEncodedData(const void* data, size_t len) = 0; - - /** - * If you want to encode these pixels, return the encoded data as an SkData - * Return null if you want to serialize the raw pixels. - */ - virtual SkData* onEncodePixels(const SkImageInfo&, void* pixels, size_t rowBytes) = 0; -}; -#endif // SkPixelSerializer_DEFINED diff --git a/include/core/SkWriteBuffer.h b/include/core/SkWriteBuffer.h index 39739f2..4dbe17b 100644 --- a/include/core/SkWriteBuffer.h +++ b/include/core/SkWriteBuffer.h @@ -12,7 +12,6 @@ #include "SkData.h" #include "SkPath.h" #include "SkPicture.h" -#include "SkPixelSerializer.h" #include "SkRefCnt.h" #include "SkWriter32.h" @@ -88,24 +87,21 @@ public: /** * Set an SkBitmapHeap to store bitmaps rather than flattening. * - * Incompatible with an SkPixelSerializer. If an SkPixelSerializer is set, - * setting an SkBitmapHeap will set the SkPixelSerializer to NULL in release - * and crash in debug. + * Incompatible with an EncodeBitmap function. If an EncodeBitmap function is set, setting an + * SkBitmapHeap will set the function to NULL in release mode and crash in debug. */ void setBitmapHeap(SkBitmapHeap*); /** - * Set an SkPixelSerializer to store an encoded representation of pixels, - * e.g. SkBitmaps. + * Provide a function to encode an SkBitmap to an SkData. writeBitmap will attempt to use + * bitmapEncoder to store the SkBitmap. If the reader does not provide a function to decode, it + * will not be able to restore SkBitmaps, but will still be able to read the rest of the stream. + * bitmapEncoder will never be called with a NULL pixelRefOffset. * - * Calls ref() on the serializer. - * - * TODO: Encode SkImage pixels as well. - * - * Incompatible with the SkBitmapHeap. If an encoder is set fBitmapHeap will - * be set to NULL in release and crash in debug. + * Incompatible with the SkBitmapHeap. If an encoder is set fBitmapHeap will be set to NULL in + * release and crash in debug. */ - void setPixelSerializer(SkPixelSerializer*); + void setBitmapEncoder(SkPicture::EncodeBitmap bitmapEncoder); private: bool isValidating() const { return SkToBool(fFlags & kValidation_Flag); } @@ -118,7 +114,7 @@ private: SkBitmapHeap* fBitmapHeap; SkRefCntSet* fTFSet; - SkAutoTUnref fPixelSerializer; + SkPicture::EncodeBitmap fBitmapEncoder; }; #endif // SkWriteBuffer_DEFINED diff --git a/src/core/SkPicture.cpp b/src/core/SkPicture.cpp index 3296264..0d7773b 100644 --- a/src/core/SkPicture.cpp +++ b/src/core/SkPicture.cpp @@ -1,3 +1,4 @@ + /* * Copyright 2007 The Android Open Source Project * @@ -453,41 +454,7 @@ SkPictureData* SkPicture::Backport(const SkRecord& src, const SkPictInfo& info, return SkNEW_ARGS(SkPictureData, (rec, info, false/*deep copy ops?*/)); } -#ifdef SK_LEGACY_ENCODE_BITMAP -// Helper to support the EncodeBitmap version of serialize. -// Mimics the old behavior of always accepting the encoded data, and encoding -// using EncodeBitmap if there was no encoded data. -class EncodeBitmapSerializer : public SkPixelSerializer { -public: - explicit EncodeBitmapSerializer(SkPicture::EncodeBitmap encoder) - : fEncoder(encoder) - { - SkASSERT(fEncoder); - } - - virtual bool onUseEncodedData(const void*, size_t) SK_OVERRIDE { return true; } - - virtual SkData* onEncodePixels(const SkImageInfo& info, void* pixels, - size_t rowBytes) SK_OVERRIDE { - // Required by signature of EncodeBitmap. - size_t unused; - SkBitmap bm; - bm.installPixels(info, pixels, rowBytes); - return fEncoder(&unused, bm); - } - -private: - SkPicture::EncodeBitmap fEncoder; -}; - -void SkPicture::serialize(SkWStream* wStream, SkPicture::EncodeBitmap encoder) const { - EncodeBitmapSerializer serializer(encoder); - this->serialize(wStream, &serializer); -} - -#endif - -void SkPicture::serialize(SkWStream* stream, SkPixelSerializer* pixelSerializer) const { +void SkPicture::serialize(SkWStream* stream, EncodeBitmap encoder) const { SkPictInfo info; this->createHeader(&info); SkAutoTDelete data(Backport(*fRecord, info, this->drawablePicts(), @@ -496,7 +463,7 @@ void SkPicture::serialize(SkWStream* stream, SkPixelSerializer* pixelSerializer) stream->write(&info, sizeof(info)); if (data) { stream->writeBool(true); - data->serialize(stream, pixelSerializer); + data->serialize(stream, encoder); } else { stream->writeBool(false); } diff --git a/src/core/SkPictureData.cpp b/src/core/SkPictureData.cpp index 938274a..2b22654 100644 --- a/src/core/SkPictureData.cpp +++ b/src/core/SkPictureData.cpp @@ -218,14 +218,14 @@ void SkPictureData::flattenToBuffer(SkWriteBuffer& buffer) const { } void SkPictureData::serialize(SkWStream* stream, - SkPixelSerializer* pixelSerializer) const { + SkPicture::EncodeBitmap encoder) const { write_tag_size(stream, SK_PICT_READER_TAG, fOpData->size()); stream->write(fOpData->bytes(), fOpData->size()); if (fPictureCount > 0) { write_tag_size(stream, SK_PICT_PICTURE_TAG, fPictureCount); for (int i = 0; i < fPictureCount; i++) { - fPictureRefs[i]->serialize(stream, pixelSerializer); + fPictureRefs[i]->serialize(stream, encoder); } } @@ -238,7 +238,7 @@ void SkPictureData::serialize(SkWStream* stream, SkWriteBuffer buffer(SkWriteBuffer::kCrossProcess_Flag); buffer.setTypefaceRecorder(&typefaceSet); buffer.setFactoryRecorder(&factSet); - buffer.setPixelSerializer(pixelSerializer); + buffer.setBitmapEncoder(encoder); this->flattenToBuffer(buffer); diff --git a/src/core/SkPictureData.h b/src/core/SkPictureData.h index ab78f8a..6678806 100644 --- a/src/core/SkPictureData.h +++ b/src/core/SkPictureData.h @@ -15,7 +15,6 @@ class SkData; class SkPictureRecord; -class SkPixelSerializer; class SkReader32; class SkStream; class SkWStream; @@ -65,7 +64,7 @@ public: virtual ~SkPictureData(); - void serialize(SkWStream*, SkPixelSerializer*) const; + void serialize(SkWStream*, SkPicture::EncodeBitmap) const; void flatten(SkWriteBuffer&) const; bool containsBitmaps() const; diff --git a/src/core/SkWriteBuffer.cpp b/src/core/SkWriteBuffer.cpp index bcae1f8..c79d275 100644 --- a/src/core/SkWriteBuffer.cpp +++ b/src/core/SkWriteBuffer.cpp @@ -20,7 +20,8 @@ SkWriteBuffer::SkWriteBuffer(uint32_t flags) , fFactorySet(NULL) , fNamedFactorySet(NULL) , fBitmapHeap(NULL) - , fTFSet(NULL) { + , fTFSet(NULL) + , fBitmapEncoder(NULL) { } SkWriteBuffer::SkWriteBuffer(void* storage, size_t storageSize, uint32_t flags) @@ -29,7 +30,8 @@ SkWriteBuffer::SkWriteBuffer(void* storage, size_t storageSize, uint32_t flags) , fNamedFactorySet(NULL) , fWriter(storage, storageSize) , fBitmapHeap(NULL) - , fTFSet(NULL) { + , fTFSet(NULL) + , fBitmapEncoder(NULL) { } SkWriteBuffer::~SkWriteBuffer() { @@ -170,7 +172,7 @@ void SkWriteBuffer::writeBitmap(const SkBitmap& bitmap) { // SkBitmapHeapReader to read the SkBitmap. False if the bitmap was serialized another way. this->writeBool(useBitmapHeap); if (useBitmapHeap) { - SkASSERT(NULL == fPixelSerializer); + SkASSERT(NULL == fBitmapEncoder); int32_t slot = fBitmapHeap->insert(bitmap); fWriter.write32(slot); // crbug.com/155875 @@ -183,33 +185,25 @@ void SkWriteBuffer::writeBitmap(const SkBitmap& bitmap) { return; } - SkPixelRef* pixelRef = bitmap.pixelRef(); - if (pixelRef) { - // see if the pixelref already has an encoded version - SkAutoDataUnref existingData(bitmap.pixelRef()->refEncodedData()); - if (existingData.get() != NULL) { - // Assumes that if the client did not set a serializer, they are - // happy to get the encoded data. - if (!fPixelSerializer || fPixelSerializer->useEncodedData(existingData->data(), - existingData->size())) { - write_encoded_bitmap(this, existingData, bitmap.pixelRefOrigin()); - return; - } + // see if the pixelref already has an encoded version + if (bitmap.pixelRef()) { + SkAutoDataUnref data(bitmap.pixelRef()->refEncodedData()); + if (data.get() != NULL) { + write_encoded_bitmap(this, data, bitmap.pixelRefOrigin()); + return; } + } - // see if the caller wants to manually encode - if (fPixelSerializer) { - SkASSERT(NULL == fBitmapHeap); - SkAutoLockPixels alp(bitmap); - SkAutoDataUnref data(fPixelSerializer->encodePixels(bitmap.info(), - bitmap.getPixels(), - bitmap.rowBytes())); - if (data.get() != NULL) { - // if we have to "encode" the bitmap, then we assume there is no - // offset to share, since we are effectively creating a new pixelref - write_encoded_bitmap(this, data, SkIPoint::Make(0, 0)); - return; - } + // see if the caller wants to manually encode + if (fBitmapEncoder != NULL) { + SkASSERT(NULL == fBitmapHeap); + size_t offset = 0; // this parameter is deprecated/ignored + // if we have to "encode" the bitmap, then we assume there is no + // offset to share, since we are effectively creating a new pixelref + SkAutoDataUnref data(fBitmapEncoder(&offset, bitmap)); + if (data.get() != NULL) { + write_encoded_bitmap(this, data, SkIPoint::Make(0, 0)); + return; } } @@ -251,15 +245,14 @@ SkRefCntSet* SkWriteBuffer::setTypefaceRecorder(SkRefCntSet* rec) { void SkWriteBuffer::setBitmapHeap(SkBitmapHeap* bitmapHeap) { SkRefCnt_SafeAssign(fBitmapHeap, bitmapHeap); if (bitmapHeap != NULL) { - SkASSERT(NULL == fPixelSerializer); - fPixelSerializer.reset(NULL); + SkASSERT(NULL == fBitmapEncoder); + fBitmapEncoder = NULL; } } -void SkWriteBuffer::setPixelSerializer(SkPixelSerializer* serializer) { - fPixelSerializer.reset(serializer); - if (serializer) { - serializer->ref(); +void SkWriteBuffer::setBitmapEncoder(SkPicture::EncodeBitmap bitmapEncoder) { + fBitmapEncoder = bitmapEncoder; + if (bitmapEncoder != NULL) { SkASSERT(NULL == fBitmapHeap); SkSafeUnref(fBitmapHeap); fBitmapHeap = NULL; diff --git a/tests/PictureTest.cpp b/tests/PictureTest.cpp index ad1459e..7a8c8fa 100644 --- a/tests/PictureTest.cpp +++ b/tests/PictureTest.cpp @@ -22,7 +22,6 @@ #include "SkPictureRecorder.h" #include "SkPictureUtils.h" #include "SkPixelRef.h" -#include "SkPixelSerializer.h" #include "SkRRect.h" #include "SkRandom.h" #include "SkRecord.h" @@ -1443,21 +1442,9 @@ static void test_bad_bitmap() { } #endif -// Encodes to PNG, unless there is already encoded data, in which case that gets -// used. -// FIXME: Share with PictureRenderer.cpp? -class PngPixelSerializer : public SkPixelSerializer { -public: - virtual bool onUseEncodedData(const void*, size_t) SK_OVERRIDE { return true; } - virtual SkData* onEncodePixels(const SkImageInfo& info, void* pixels, - size_t rowBytes) SK_OVERRIDE { - SkBitmap bm; - if (!bm.installPixels(info, pixels, rowBytes)) { - return NULL; - } - return SkImageEncoder::EncodeData(bm, SkImageEncoder::kPNG_Type, 100); - } -}; +static SkData* encode_bitmap_to_data(size_t*, const SkBitmap& bm) { + return SkImageEncoder::EncodeData(bm, SkImageEncoder::kPNG_Type, 100); +} static SkData* serialized_picture_from_bitmap(const SkBitmap& bitmap) { SkPictureRecorder recorder; @@ -1467,8 +1454,7 @@ static SkData* serialized_picture_from_bitmap(const SkBitmap& bitmap) { SkAutoTUnref picture(recorder.endRecording()); SkDynamicMemoryWStream wStream; - PngPixelSerializer serializer; - picture->serialize(&wStream, &serializer); + picture->serialize(&wStream, &encode_bitmap_to_data); return wStream.copyToData(); } diff --git a/tools/PictureRenderer.cpp b/tools/PictureRenderer.cpp index 123cc2c..33766e9 100644 --- a/tools/PictureRenderer.cpp +++ b/tools/PictureRenderer.cpp @@ -28,7 +28,6 @@ #include "SkPictureRecorder.h" #include "SkPictureUtils.h" #include "SkPixelRef.h" -#include "SkPixelSerializer.h" #include "SkScalar.h" #include "SkStream.h" #include "SkString.h" @@ -360,22 +359,10 @@ SkCanvas* RecordPictureRenderer::setupCanvas(int width, int height) { return NULL; } -// Encodes to PNG, unless there is already encoded data, in which case that gets -// used. -// FIXME: Share with PictureTest.cpp? - -class PngPixelSerializer : public SkPixelSerializer { -public: - virtual bool onUseEncodedData(const void*, size_t) SK_OVERRIDE { return true; } - virtual SkData* onEncodePixels(const SkImageInfo& info, void* pixels, - size_t rowBytes) SK_OVERRIDE { - SkBitmap bm; - if (!bm.installPixels(info, pixels, rowBytes)) { - return NULL; - } - return SkImageEncoder::EncodeData(bm, SkImageEncoder::kPNG_Type, 100); - } -}; +// the size_t* parameter is deprecated, so we ignore it +static SkData* encode_bitmap_to_data(size_t*, const SkBitmap& bm) { + return SkImageEncoder::EncodeData(bm, SkImageEncoder::kPNG_Type, 100); +} bool RecordPictureRenderer::render(SkBitmap** out) { SkAutoTDelete factory(this->getFactory()); @@ -391,8 +378,7 @@ bool RecordPictureRenderer::render(SkBitmap** out) { // Record the new picture as a new SKP with PNG encoded bitmaps. SkString skpPath = SkOSPath::Join(fWritePath.c_str(), fInputFilename.c_str()); SkFILEWStream stream(skpPath.c_str()); - PngPixelSerializer serializer; - picture->serialize(&stream, &serializer); + picture->serialize(&stream, &encode_bitmap_to_data); return true; } return false; -- 2.7.4