From 924205aaf2e0c3c65dda13e0eaccde3e7b2a5c40 Mon Sep 17 00:00:00 2001 From: "commit-bot@chromium.org" Date: Wed, 28 May 2014 16:16:08 +0000 Subject: [PATCH] Revert of add colortable support to imagegenerator (https://codereview.chromium.org/304443003/) Reason for revert: failing tests Original issue's description: > add colortable support to imagegenerator > > BUG=skia: > > Committed: http://code.google.com/p/skia/source/detail?r=14916 R=halcanary@google.com, scroggo@google.com TBR=halcanary@google.com, scroggo@google.com NOTREECHECKS=true NOTRY=true BUG=skia: Author: reed@google.com Review URL: https://codereview.chromium.org/300873007 git-svn-id: http://skia.googlecode.com/svn/trunk@14917 2bbb7eff-a529-9590-31e7-b0007b416f81 --- gyp/images.gyp | 1 - gyp/skia_for_chromium_defines.gypi | 1 - include/core/SkImageGenerator.h | 37 +++------------ src/images/SkDecodingImageGenerator.cpp | 73 ++++++++++++++++-------------- src/images/SkImageGenerator.cpp | 79 --------------------------------- src/lazy/SkDiscardablePixelRef.cpp | 19 +------- src/lazy/SkDiscardablePixelRef.h | 5 ++- tests/CachedDecodingPixelRefTest.cpp | 10 ++--- tests/DrawBitmapRectTest.cpp | 12 +++-- 9 files changed, 63 insertions(+), 174 deletions(-) delete mode 100644 src/images/SkImageGenerator.cpp diff --git a/gyp/images.gyp b/gyp/images.gyp index 18a18be..0062f24 100644 --- a/gyp/images.gyp +++ b/gyp/images.gyp @@ -71,7 +71,6 @@ '../src/images/SkImageEncoder.cpp', '../src/images/SkImageEncoder_Factory.cpp', '../src/images/SkImageEncoder_argb.cpp', - '../src/images/SkImageGenerator.cpp', '../src/images/SkImageRef.cpp', '../src/images/SkImageRefPool.cpp', '../src/images/SkImageRefPool.h', diff --git a/gyp/skia_for_chromium_defines.gypi b/gyp/skia_for_chromium_defines.gypi index a47b391..c114b71 100644 --- a/gyp/skia_for_chromium_defines.gypi +++ b/gyp/skia_for_chromium_defines.gypi @@ -16,7 +16,6 @@ 'SK_SUPPORT_LEGACY_GETTOPDEVICE', 'SK_SUPPORT_LEGACY_ASIMAGEINFO', 'SK_SUPPORT_LEGACY_N32_NAME', - 'SK_SUPPORT_LEGACY_IMAGEGENERATORAPI', ], }, } diff --git a/include/core/SkImageGenerator.h b/include/core/SkImageGenerator.h index 21ebcbb..f399ab5 100644 --- a/include/core/SkImageGenerator.h +++ b/include/core/SkImageGenerator.h @@ -9,7 +9,6 @@ #define SkImageGenerator_DEFINED #include "SkImageInfo.h" -#include "SkColor.h" class SkBitmap; class SkData; @@ -48,13 +47,6 @@ public: */ virtual ~SkImageGenerator() { } -#ifdef SK_SUPPORT_LEGACY_IMAGEGENERATORAPI - virtual bool refEncodedData() { return this->onRefEncodedData(); } - virtual bool getInfo(SkImageInfo* info) { return this->onGetInfo(info); } - virtual bool getPixels(const SkImageInfo& info, void* pixels, size_t rowBytes) { - return this->onGetPixels(info, pixels, rowBytes, NULL, NULL); - } -#else /** * Return a ref to the encoded (i.e. compressed) representation, * of this data. @@ -62,7 +54,7 @@ public: * If non-NULL is returned, the caller is responsible for calling * unref() on the data when it is finished. */ - SkData* refEncodedData() { return this->onRefEncodedData(); } + virtual SkData* refEncodedData() { return NULL; } /** * Return some information about the image, allowing the owner of @@ -73,7 +65,7 @@ public: * * @return false if anything goes wrong. */ - bool getInfo(SkImageInfo* info); + virtual bool getInfo(SkImageInfo* info) = 0; /** * Decode into the given pixels, a block of memory of size at @@ -91,31 +83,12 @@ public: * different output-configs, which the implementation can * decide to support or not. * - * If info is kIndex8_SkColorType, then the caller must provide storage for up to 256 - * SkPMColor values in ctable. On success the generator must copy N colors into that storage, - * (where N is the logical number of table entries) and set ctableCount to N. - * - * If info is not kIndex8_SkColorType, then the last two parameters may be NULL. If ctableCount - * is not null, it will be set to 0. - * * @return false if anything goes wrong or if the image info is * unsupported. */ - bool getPixels(const SkImageInfo& info, void* pixels, size_t rowBytes, - SkPMColor ctable[], int* ctableCount); - - /** - * Simplified version of getPixels() that asserts that info is NOT kIndex8_SkColorType. - */ - bool getPixels(const SkImageInfo& info, void* pixels, size_t rowBytes); -#endif - -protected: - virtual SkData* onRefEncodedData(); - virtual bool onGetInfo(SkImageInfo* info); - virtual bool onGetPixels(const SkImageInfo& info, - void* pixels, size_t rowBytes, - SkPMColor ctable[], int* ctableCount); + virtual bool getPixels(const SkImageInfo& info, + void* pixels, + size_t rowBytes) = 0; }; #endif // SkImageGenerator_DEFINED diff --git a/src/images/SkDecodingImageGenerator.cpp b/src/images/SkDecodingImageGenerator.cpp index 1f0f706..b3924a7 100644 --- a/src/images/SkDecodingImageGenerator.cpp +++ b/src/images/SkDecodingImageGenerator.cpp @@ -23,6 +23,12 @@ bool equal_modulo_alpha(const SkImageInfo& a, const SkImageInfo& b) { class DecodingImageGenerator : public SkImageGenerator { public: virtual ~DecodingImageGenerator(); + virtual SkData* refEncodedData() SK_OVERRIDE; + // This implementaion of getInfo() always returns true. + virtual bool getInfo(SkImageInfo* info) SK_OVERRIDE; + virtual bool getPixels(const SkImageInfo& info, + void* pixels, + size_t rowBytes) SK_OVERRIDE; SkData* fData; SkStreamRewindable* fStream; @@ -35,18 +41,6 @@ public: const SkImageInfo& info, int sampleSize, bool ditherImage); - -protected: - virtual SkData* onRefEncodedData() SK_OVERRIDE; - virtual bool onGetInfo(SkImageInfo* info) SK_OVERRIDE { - *info = fInfo; - return true; - } - virtual bool onGetPixels(const SkImageInfo& info, - void* pixels, size_t rowBytes, - SkPMColor ctable[], int* ctableCount) SK_OVERRIDE; - -private: typedef SkImageGenerator INHERITED; }; @@ -129,7 +123,14 @@ DecodingImageGenerator::~DecodingImageGenerator() { fStream->unref(); } -SkData* DecodingImageGenerator::onRefEncodedData() { +bool DecodingImageGenerator::getInfo(SkImageInfo* info) { + if (info != NULL) { + *info = fInfo; + } + return true; +} + +SkData* DecodingImageGenerator::refEncodedData() { // This functionality is used in `gm --serialize` // Does not encode options. if (fData != NULL) { @@ -150,15 +151,22 @@ SkData* DecodingImageGenerator::onRefEncodedData() { return SkSafeRef(fData); } -bool DecodingImageGenerator::onGetPixels(const SkImageInfo& info, - void* pixels, size_t rowBytes, - SkPMColor ctableEntries[], int* ctableCount) { +bool DecodingImageGenerator::getPixels(const SkImageInfo& info, + void* pixels, + size_t rowBytes) { + if (NULL == pixels) { + return false; + } if (fInfo != info) { // The caller has specified a different info. This is an // error for this kind of SkImageGenerator. Use the Options // to change the settings. return false; } + if (info.minRowBytes() > rowBytes) { + // The caller has specified a bad rowBytes. + return false; + } SkAssertResult(fStream->rewind()); SkAutoTDelete decoder(SkImageDecoder::Factory(fStream)); @@ -194,20 +202,6 @@ bool DecodingImageGenerator::onGetPixels(const SkImageInfo& info, } else { SkASSERT(check_alpha(info.alphaType(), bitmap.alphaType())); } - - if (kIndex_8_SkColorType == info.colorType()) { - if (kIndex_8_SkColorType != bitmap.colorType()) { - return false; // they asked for Index8, but we didn't receive that from decoder - } - SkColorTable* ctable = bitmap.getColorTable(); - if (NULL == ctable) { - return false; - } - const int count = ctable->count(); - memcpy(ctableEntries, ctable->lockColors(), count * sizeof(SkPMColor)); - ctable->unlockColors(); - *ctableCount = count; - } return true; } @@ -220,6 +214,11 @@ SkImageGenerator* CreateDecodingImageGenerator( const SkDecodingImageGenerator::Options& opts) { SkASSERT(stream); SkAutoTUnref autoStream(stream); // always unref this. + if (opts.fUseRequestedColorType && + (kIndex_8_SkColorType == opts.fRequestedColorType)) { + // We do not support indexed color with SkImageGenerators, + return NULL; + } SkAssertResult(autoStream->rewind()); SkAutoTDelete decoder(SkImageDecoder::Factory(autoStream)); if (NULL == decoder.get()) { @@ -228,16 +227,24 @@ SkImageGenerator* CreateDecodingImageGenerator( SkBitmap bitmap; decoder->setSampleSize(opts.fSampleSize); decoder->setRequireUnpremultipliedColors(opts.fRequireUnpremul); - if (!decoder->decode(stream, &bitmap, SkImageDecoder::kDecodeBounds_Mode)) { + if (!decoder->decode(stream, &bitmap, + SkImageDecoder::kDecodeBounds_Mode)) { return NULL; } - if (kUnknown_SkColorType == bitmap.colorType()) { + if (bitmap.config() == SkBitmap::kNo_Config) { return NULL; } SkImageInfo info = bitmap.info(); - if (opts.fUseRequestedColorType && (opts.fRequestedColorType != info.colorType())) { + if (!opts.fUseRequestedColorType) { + // Use default + if (kIndex_8_SkColorType == bitmap.colorType()) { + // We don't support kIndex8 because we don't support + // colortables in this workflow. + info.fColorType = kN32_SkColorType; + } + } else { if (!bitmap.canCopyTo(opts.fRequestedColorType)) { SkASSERT(bitmap.colorType() != opts.fRequestedColorType); return NULL; // Can not translate to needed config. diff --git a/src/images/SkImageGenerator.cpp b/src/images/SkImageGenerator.cpp deleted file mode 100644 index b898764..0000000 --- a/src/images/SkImageGenerator.cpp +++ /dev/null @@ -1,79 +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. - */ - -#include "SkImageGenerator.h" - -#ifndef SK_SUPPORT_LEGACY_IMAGEGENERATORAPI -bool SkImageGenerator::getInfo(SkImageInfo* info) { - SkImageInfo dummy; - if (NULL == info) { - info = &dummy; - } - return this->onGetInfo(info); -} - -bool SkImageGenerator::getPixels(const SkImageInfo& info, void* pixels, size_t rowBytes, - SkPMColor ctable[], int* ctableCount) { - // Dummy (out-of-range) value to catch subclasses that forgot to respect ctable params. - int localCTableCount = -1; - int* localCTableCountPtr = NULL; - - if (kIndex_8_SkColorType == info.colorType()) { - if (NULL == ctable || NULL == ctableCount) { - return false; - } - } else { - if (ctableCount) { - *ctableCount = 0; - } - ctableCount = NULL; - ctable = NULL; - } - - if (kUnknown_SkColorType == info.colorType()) { - return false; - } - if (NULL == pixels) { - return false; - } - if (rowBytes < info.minRowBytes()) { - return false; - } - - bool success = this->onGetPixels(info, pixels, rowBytes, ctable, localCTableCountPtr); - - if (success && localCTableCountPtr) { - SkASSERT(localCTableCount >= 0 && localCTableCount <= 256); - if (ctableCount) { - *ctableCount = localCTableCount; - } - } - return success; -} - -bool SkImageGenerator::getPixels(const SkImageInfo& info, void* pixels, size_t rowBytes) { - SkASSERT(kIndex_8_SkColorType != info.colorType()); - if (kIndex_8_SkColorType == info.colorType()) { - return false; - } - return this->getPixels(info, pixels, rowBytes, NULL, NULL); -} -#endif - -///////////////////////////////////////////////////////////////////////////////////////////// - -SkData* SkImageGenerator::onRefEncodedData() { - return NULL; -} - -bool SkImageGenerator::onGetInfo(SkImageInfo*) { - return false; -} - -bool SkImageGenerator::onGetPixels(const SkImageInfo&, void*, size_t, SkPMColor*, int*) { - return false; -} diff --git a/src/lazy/SkDiscardablePixelRef.cpp b/src/lazy/SkDiscardablePixelRef.cpp index b0bbd27..ccf812c 100644 --- a/src/lazy/SkDiscardablePixelRef.cpp +++ b/src/lazy/SkDiscardablePixelRef.cpp @@ -60,30 +60,15 @@ bool SkDiscardablePixelRef::onNewLockPixels(LockRec* rec) { } void* pixels = fDiscardableMemory->data(); - const SkImageInfo& info = this->info(); - SkPMColor colors[256]; - int colorCount = 0; - - if (!fGenerator->getPixels(info, pixels, fRowBytes, colors, &colorCount)) { + if (!fGenerator->getPixels(this->info(), pixels, fRowBytes)) { fDiscardableMemory->unlock(); SkDELETE(fDiscardableMemory); fDiscardableMemory = NULL; return false; } - // Note: our ctable is not purgable, as it is not stored in the discardablememory block. - // This is because SkColorTable is refcntable, and therefore our caller could hold onto it - // beyond the scope of a lock/unlock. If we change the API/lifecycle for SkColorTable, we - // could move it into the block, but then again perhaps it is small enough that this doesn't - // really matter. - if (colorCount > 0) { - fCTable.reset(SkNEW_ARGS(SkColorTable, (colors, colorCount))); - } else { - fCTable.reset(NULL); - } - rec->fPixels = pixels; - rec->fColorTable = fCTable.get(); + rec->fColorTable = NULL; rec->fRowBytes = fRowBytes; return true; } diff --git a/src/lazy/SkDiscardablePixelRef.h b/src/lazy/SkDiscardablePixelRef.h index 52a1d6c..e5e1c9f 100644 --- a/src/lazy/SkDiscardablePixelRef.h +++ b/src/lazy/SkDiscardablePixelRef.h @@ -17,6 +17,10 @@ * A PixelRef backed by SkDiscardableMemory, with the ability to * re-generate the pixels (via a SkImageGenerator) if the DM is * purged. + * + * Since SkColorTable is reference-counted, we do not support indexed + * color with this class; there would be no way for the discardable + * memory system to unref the color table. */ class SkDiscardablePixelRef : public SkPixelRef { public: @@ -42,7 +46,6 @@ private: // PixelRef, since the SkBitmap doesn't expect them to change. SkDiscardableMemory* fDiscardableMemory; - SkAutoTUnref fCTable; /* Takes ownership of SkImageGenerator. */ SkDiscardablePixelRef(const SkImageInfo&, SkImageGenerator*, diff --git a/tests/CachedDecodingPixelRefTest.cpp b/tests/CachedDecodingPixelRefTest.cpp index eff77e2..10e33ba 100644 --- a/tests/CachedDecodingPixelRefTest.cpp +++ b/tests/CachedDecodingPixelRefTest.cpp @@ -183,9 +183,7 @@ public: SkASSERT((fType <= kLast_TestType) && (fType >= 0)); } virtual ~TestImageGenerator() { } - -protected: - virtual bool onGetInfo(SkImageInfo* info) SK_OVERRIDE { + virtual bool getInfo(SkImageInfo* info) SK_OVERRIDE { REPORTER_ASSERT(fReporter, NULL != info); if ((NULL == info) || (kFailGetInfo_TestType == fType)) { return false; @@ -196,9 +194,9 @@ protected: info->fAlphaType = kOpaque_SkAlphaType; return true; } - - virtual bool onGetPixels(const SkImageInfo& info, void* pixels, size_t rowBytes, - SkPMColor ctable[], int* ctableCount) SK_OVERRIDE { + virtual bool getPixels(const SkImageInfo& info, + void* pixels, + size_t rowBytes) SK_OVERRIDE { REPORTER_ASSERT(fReporter, pixels != NULL); size_t minRowBytes = static_cast(info.fWidth * info.bytesPerPixel()); diff --git a/tests/DrawBitmapRectTest.cpp b/tests/DrawBitmapRectTest.cpp index aa69de4..f294ae7 100644 --- a/tests/DrawBitmapRectTest.cpp +++ b/tests/DrawBitmapRectTest.cpp @@ -22,16 +22,20 @@ class FailureImageGenerator : public SkImageGenerator { public: FailureImageGenerator() { } virtual ~FailureImageGenerator() { } - -protected: - virtual bool onGetInfo(SkImageInfo* info) SK_OVERRIDE { + virtual bool getInfo(SkImageInfo* info) SK_OVERRIDE { info->fWidth = 100; info->fHeight = 100; info->fColorType = kN32_SkColorType; info->fAlphaType = kPremul_SkAlphaType; return true; } - // default onGetPixels() returns false, which is what we want. + virtual bool getPixels(const SkImageInfo& info, + void* pixels, + size_t rowBytes) SK_OVERRIDE { + // this will deliberately return false if they are asking us + // to decode into pixels. + return false; + } }; // crbug.com/295895 -- 2.7.4