Call initializeColorXform inside SkCodec
Make initializeColorXform private, and only call in the base class.
Add virtual method to skip initializeColorXform, for classes that do
not need one.
Change SkCodec::FrameInfo's SkAlphaType to an SkEncodedInfo::Alpha.
This allows proper checking internally whether SkCodec needs to do a
color correct premultiply.
Depends on https://chromium-review.googlesource.com/c/620947, for this
API change.
(Separated from review.skia.org/25746)
Bug: skia:5609
Bug: skia:6839
Change-Id: Icb0d46659c546060c34d32eaf792c86708726c7a
Reviewed-on: https://skia-review.googlesource.com/35880
Reviewed-by: Chris Blume <cblume@chromium.org>
Reviewed-by: Derek Sollenberger <djsollen@google.com>
Reviewed-by: Mike Klein <mtklein@google.com>
Commit-Queue: Leon Scroggins <scroggo@google.com>
diff --git a/include/codec/SkCodec.h b/include/codec/SkCodec.h
index 30bbd0f..5e80f8f 100644
--- a/include/codec/SkCodec.h
+++ b/include/codec/SkCodec.h
@@ -619,7 +619,10 @@
* This is conservative; it will still return non-opaque if e.g. a
* color index-based frame has a color with alpha but does not use it.
*/
+#ifdef SK_LEGACY_FRAME_INFO_ALPHA_TYPE
SkAlphaType fAlphaType;
+#endif
+ SkEncodedInfo::Alpha fAlpha;
/**
* How this frame should be modified before decoding the next one.
@@ -806,8 +809,12 @@
virtual int onOutputScanline(int inputScanline) const;
- bool initializeColorXform(const SkImageInfo& dstInfo,
+ bool initializeColorXform(const SkImageInfo& dstInfo, SkEncodedInfo::Alpha,
SkTransferFunctionBehavior premulBehavior);
+ // Some classes never need a colorXform e.g.
+ // - ICO uses its embedded codec's colorXform
+ // - WBMP is just Black/White
+ virtual bool usesColorXform() const { return true; }
void applyColorXform(void* dst, const void* src, int count, SkAlphaType) const;
void applyColorXform(void* dst, const void* src, int count) const;
diff --git a/src/codec/SkBmpCodec.cpp b/src/codec/SkBmpCodec.cpp
index d8167ef..6df3c93 100644
--- a/src/codec/SkBmpCodec.cpp
+++ b/src/codec/SkBmpCodec.cpp
@@ -620,10 +620,6 @@
SkCodec::Result SkBmpCodec::prepareToDecode(const SkImageInfo& dstInfo,
const SkCodec::Options& options) {
- if (!this->initializeColorXform(dstInfo, options.fPremulBehavior)) {
- return kInvalidConversion;
- }
-
return this->onPrepareToDecode(dstInfo, options);
}
diff --git a/src/codec/SkCodec.cpp b/src/codec/SkCodec.cpp
index 2168ac3..214b157 100644
--- a/src/codec/SkCodec.cpp
+++ b/src/codec/SkCodec.cpp
@@ -218,12 +218,18 @@
const int index = options.fFrameIndex;
if (0 == index) {
if (!this->conversionSupported(info, fEncodedInfo.color(), fEncodedInfo.opaque(),
- fSrcInfo.colorSpace())) {
+ fSrcInfo.colorSpace())
+ || !this->initializeColorXform(info, fEncodedInfo.alpha(), options.fPremulBehavior))
+ {
return kInvalidConversion;
}
return kSuccess;
}
+ if (index < 0) {
+ return kInvalidParameters;
+ }
+
if (options.fSubset || info.dimensions() != fSrcInfo.dimensions()) {
// If we add support for these, we need to update the code that zeroes
// a kRestoreBGColor frame.
@@ -246,47 +252,48 @@
}
const int requiredFrame = frame->getRequiredFrame();
- if (requiredFrame == kNone) {
- return kSuccess;
- }
-
- if (options.fPriorFrame != kNone) {
- // Check for a valid frame as a starting point. Alternatively, we could
- // treat an invalid frame as not providing one, but rejecting it will
- // make it easier to catch the mistake.
- if (options.fPriorFrame < requiredFrame || options.fPriorFrame >= index) {
- return kInvalidParameters;
- }
- const auto* prevFrame = frameHolder->getFrame(options.fPriorFrame);
- switch (prevFrame->getDisposalMethod()) {
- case SkCodecAnimation::DisposalMethod::kRestorePrevious:
+ if (requiredFrame != kNone) {
+ if (options.fPriorFrame != kNone) {
+ // Check for a valid frame as a starting point. Alternatively, we could
+ // treat an invalid frame as not providing one, but rejecting it will
+ // make it easier to catch the mistake.
+ if (options.fPriorFrame < requiredFrame || options.fPriorFrame >= index) {
return kInvalidParameters;
- case SkCodecAnimation::DisposalMethod::kRestoreBGColor:
- // If a frame after the required frame is provided, there is no
- // need to clear, since it must be covered by the desired frame.
- if (options.fPriorFrame == requiredFrame) {
- zero_rect(info, pixels, rowBytes, prevFrame->frameRect());
- }
- break;
- default:
- break;
- }
- return kSuccess;
- }
-
- Options prevFrameOptions(options);
- prevFrameOptions.fFrameIndex = requiredFrame;
- prevFrameOptions.fZeroInitialized = kNo_ZeroInitialized;
- const Result result = this->getPixels(info, pixels, rowBytes, &prevFrameOptions);
- if (result == kSuccess) {
- const auto* prevFrame = frameHolder->getFrame(requiredFrame);
- const auto disposalMethod = prevFrame->getDisposalMethod();
- if (disposalMethod == SkCodecAnimation::DisposalMethod::kRestoreBGColor) {
- zero_rect(info, pixels, rowBytes, prevFrame->frameRect());
+ }
+ const auto* prevFrame = frameHolder->getFrame(options.fPriorFrame);
+ switch (prevFrame->getDisposalMethod()) {
+ case SkCodecAnimation::DisposalMethod::kRestorePrevious:
+ return kInvalidParameters;
+ case SkCodecAnimation::DisposalMethod::kRestoreBGColor:
+ // If a frame after the required frame is provided, there is no
+ // need to clear, since it must be covered by the desired frame.
+ if (options.fPriorFrame == requiredFrame) {
+ zero_rect(info, pixels, rowBytes, prevFrame->frameRect());
+ }
+ break;
+ default:
+ break;
+ }
+ } else {
+ Options prevFrameOptions(options);
+ prevFrameOptions.fFrameIndex = requiredFrame;
+ prevFrameOptions.fZeroInitialized = kNo_ZeroInitialized;
+ const Result result = this->getPixels(info, pixels, rowBytes, &prevFrameOptions);
+ if (result != kSuccess) {
+ return result;
+ }
+ const auto* prevFrame = frameHolder->getFrame(requiredFrame);
+ const auto disposalMethod = prevFrame->getDisposalMethod();
+ if (disposalMethod == SkCodecAnimation::DisposalMethod::kRestoreBGColor) {
+ zero_rect(info, pixels, rowBytes, prevFrame->frameRect());
+ }
}
}
- return result;
+ FrameInfo frameInfo;
+ SkAssertResult(this->getFrameInfo(index, &frameInfo));
+ return this->initializeColorXform(info, frameInfo.fAlpha, options.fPremulBehavior)
+ ? kSuccess : kInvalidConversion;
}
SkCodec::Result SkCodec::getPixels(const SkImageInfo& info, void* pixels, size_t rowBytes,
@@ -609,11 +616,14 @@
}
}
-bool SkCodec::initializeColorXform(const SkImageInfo& dstInfo,
+bool SkCodec::initializeColorXform(const SkImageInfo& dstInfo, SkEncodedInfo::Alpha encodedAlpha,
SkTransferFunctionBehavior premulBehavior) {
fColorXform = nullptr;
fXformOnDecode = false;
- bool needsColorCorrectPremul = needs_premul(dstInfo, fEncodedInfo) &&
+ if (!this->usesColorXform()) {
+ return true;
+ }
+ bool needsColorCorrectPremul = needs_premul(dstInfo.alphaType(), encodedAlpha) &&
SkTransferFunctionBehavior::kRespect == premulBehavior;
if (needs_color_xform(dstInfo, fSrcInfo.colorSpace(), needsColorCorrectPremul)) {
fColorXform = SkColorSpaceXform_Base::New(fSrcInfo.colorSpace(), dstInfo.colorSpace(),
diff --git a/src/codec/SkCodecPriv.h b/src/codec/SkCodecPriv.h
index a737fb7..93d2925 100644
--- a/src/codec/SkCodecPriv.h
+++ b/src/codec/SkCodecPriv.h
@@ -271,9 +271,8 @@
}
}
-static inline bool needs_premul(const SkImageInfo& dstInfo, const SkEncodedInfo& encodedInfo) {
- return kPremul_SkAlphaType == dstInfo.alphaType() &&
- SkEncodedInfo::kUnpremul_Alpha == encodedInfo.alpha();
+static inline bool needs_premul(SkAlphaType dstAT, SkEncodedInfo::Alpha encodedAlpha) {
+ return kPremul_SkAlphaType == dstAT && SkEncodedInfo::kUnpremul_Alpha == encodedAlpha;
}
static inline bool needs_color_xform(const SkImageInfo& dstInfo, const SkColorSpace* srcCS,
diff --git a/src/codec/SkGifCodec.cpp b/src/codec/SkGifCodec.cpp
index 4bd0e17..9f9f8a6 100644
--- a/src/codec/SkGifCodec.cpp
+++ b/src/codec/SkGifCodec.cpp
@@ -146,8 +146,12 @@
frameInfo->fDuration = frameContext->getDuration();
frameInfo->fRequiredFrame = frameContext->getRequiredFrame();
frameInfo->fFullyReceived = frameContext->isComplete();
+#ifdef SK_LEGACY_FRAME_INFO_ALPHA_TYPE
frameInfo->fAlphaType = frameContext->hasAlpha() ? kUnpremul_SkAlphaType
: kOpaque_SkAlphaType;
+#endif
+ frameInfo->fAlpha = frameContext->hasAlpha() ? SkEncodedInfo::kBinary_Alpha
+ : SkEncodedInfo::kOpaque_Alpha;
frameInfo->fDisposalMethod = frameContext->getDisposalMethod();
}
return true;
@@ -226,12 +230,6 @@
SkASSERT(frame->reachedStartOfData());
}
- const auto at = frame->hasAlpha() ? kUnpremul_SkAlphaType : kOpaque_SkAlphaType;
- const auto srcInfo = this->getInfo().makeAlphaType(at);
- if (!this->initializeColorXform(dstInfo, opts.fPremulBehavior)) {
- return gif_error("Cannot convert input type to output type.\n", kInvalidConversion);
- }
-
if (this->xformOnDecode()) {
fXformBuffer.reset(new uint32_t[dstInfo.width()]);
sk_bzero(fXformBuffer.get(), dstInfo.width() * sizeof(uint32_t));
diff --git a/src/codec/SkHeifCodec.cpp b/src/codec/SkHeifCodec.cpp
index 5ccb52a..008c129 100644
--- a/src/codec/SkHeifCodec.cpp
+++ b/src/codec/SkHeifCodec.cpp
@@ -270,10 +270,6 @@
return kUnimplemented;
}
- if (!this->initializeColorXform(dstInfo, options.fPremulBehavior)) {
- return kInvalidConversion;
- }
-
// Check if we can decode to the requested destination and set the output color space
if (!this->setOutputColorFormat(dstInfo)) {
return kInvalidConversion;
@@ -347,10 +343,6 @@
SkCodec::Result SkHeifCodec::onStartScanlineDecode(
const SkImageInfo& dstInfo, const Options& options) {
- if (!this->initializeColorXform(dstInfo, options.fPremulBehavior)) {
- return kInvalidConversion;
- }
-
// Check if we can decode to the requested destination and set the output color space
if (!this->setOutputColorFormat(dstInfo)) {
return kInvalidConversion;
diff --git a/src/codec/SkIcoCodec.h b/src/codec/SkIcoCodec.h
index 62de20f..0b77df5 100644
--- a/src/codec/SkIcoCodec.h
+++ b/src/codec/SkIcoCodec.h
@@ -54,6 +54,8 @@
return true;
}
+ // Handled by the embedded codec.
+ bool usesColorXform() const override { return false; }
private:
Result onStartScanlineDecode(const SkImageInfo& dstInfo,
diff --git a/src/codec/SkJpegCodec.cpp b/src/codec/SkJpegCodec.cpp
index 8f88138..837481c 100644
--- a/src/codec/SkJpegCodec.cpp
+++ b/src/codec/SkJpegCodec.cpp
@@ -581,10 +581,6 @@
return fDecoderMgr->returnFailure("setjmp", kInvalidInput);
}
- if (!this->initializeColorXform(dstInfo, options.fPremulBehavior)) {
- return kInvalidConversion;
- }
-
// Check if we can decode to the requested destination and set the output color space
if (!this->setOutputColorSpace(dstInfo)) {
return fDecoderMgr->returnFailure("setOutputColorSpace", kInvalidConversion);
@@ -690,10 +686,6 @@
return kInvalidInput;
}
- if (!this->initializeColorXform(dstInfo, options.fPremulBehavior)) {
- return kInvalidConversion;
- }
-
// Check if we can decode to the requested destination and set the output color space
if (!this->setOutputColorSpace(dstInfo)) {
return fDecoderMgr->returnFailure("setOutputColorSpace", kInvalidConversion);
diff --git a/src/codec/SkPngCodec.cpp b/src/codec/SkPngCodec.cpp
index c5dae3e..08f23dd 100644
--- a/src/codec/SkPngCodec.cpp
+++ b/src/codec/SkPngCodec.cpp
@@ -263,7 +263,8 @@
if (png_get_tRNS(fPng_ptr, fInfo_ptr, &alphas, &numColorsWithAlpha, nullptr)) {
// If we are performing a color xform, it will handle the premultiply. Otherwise,
// we'll do it here.
- bool premultiply = !this->colorXform() && needs_premul(dstInfo, this->getEncodedInfo());
+ bool premultiply = !this->colorXform() && needs_premul(dstInfo.alphaType(),
+ this->getEncodedInfo().alpha());
// Choose which function to use to create the color table. If the final destination's
// colortype is unpremultiplied, the color table will store unpremultiplied colors.
@@ -980,10 +981,6 @@
// interlaced scanline decoder may need to rewind.
fSwizzler.reset(nullptr);
- if (!this->initializeColorXform(dstInfo, options.fPremulBehavior)) {
- return kInvalidConversion;
- }
-
// If SkColorSpaceXform directly supports the encoded PNG format, we should skip format
// conversion in the swizzler (or skip swizzling altogether).
bool skipFormatConversion = false;
diff --git a/src/codec/SkRawCodec.cpp b/src/codec/SkRawCodec.cpp
index 93c6db5..f739247 100644
--- a/src/codec/SkRawCodec.cpp
+++ b/src/codec/SkRawCodec.cpp
@@ -696,11 +696,6 @@
SkCodec::Result SkRawCodec::onGetPixels(const SkImageInfo& dstInfo, void* dst,
size_t dstRowBytes, const Options& options,
int* rowsDecoded) {
- if (!this->initializeColorXform(dstInfo, options.fPremulBehavior)) {
- SkCodecPrintf("Error: cannot convert input type to output type.\n");
- return kInvalidConversion;
- }
-
SkImageInfo swizzlerInfo = dstInfo;
std::unique_ptr<uint32_t[]> xformBuffer = nullptr;
if (this->colorXform()) {
diff --git a/src/codec/SkWbmpCodec.h b/src/codec/SkWbmpCodec.h
index f26967f..57efe21 100644
--- a/src/codec/SkWbmpCodec.h
+++ b/src/codec/SkWbmpCodec.h
@@ -30,6 +30,8 @@
bool onRewind() override;
bool conversionSupported(const SkImageInfo& dst, SkEncodedInfo::Color srcColor,
bool srcIsOpaque, const SkColorSpace* srcCS) const override;
+ // No need to Xform; all pixels are either black or white.
+ bool usesColorXform() const override { return false; }
private:
/*
* Returns a swizzler on success, nullptr on failure
diff --git a/src/codec/SkWebpCodec.cpp b/src/codec/SkWebpCodec.cpp
index 38feb82..dd87c26 100644
--- a/src/codec/SkWebpCodec.cpp
+++ b/src/codec/SkWebpCodec.cpp
@@ -305,7 +305,11 @@
// libwebp only reports fully received frames for an
// animated image.
frameInfo->fFullyReceived = true;
+#ifdef SK_LEGACY_FRAME_INFO_ALPHA_TYPE
frameInfo->fAlphaType = alpha_type(frame->hasAlpha());
+#endif
+ frameInfo->fAlpha = frame->hasAlpha() ? SkEncodedInfo::kUnpremul_Alpha
+ : SkEncodedInfo::kOpaque_Alpha;
frameInfo->fDisposalMethod = frame->getDisposalMethod();
}
@@ -398,10 +402,6 @@
SkCodec::Result SkWebpCodec::onGetPixels(const SkImageInfo& dstInfo, void* dst, size_t rowBytes,
const Options& options, int* rowsDecodedPtr) {
- if (!this->initializeColorXform(dstInfo, options.fPremulBehavior)) {
- return kInvalidConversion;
- }
-
const int index = options.fFrameIndex;
SkASSERT(0 == index || index < fFrameHolder.size());
diff --git a/tests/CodecAnimTest.cpp b/tests/CodecAnimTest.cpp
index 612a0fa..d28da08 100644
--- a/tests/CodecAnimTest.cpp
+++ b/tests/CodecAnimTest.cpp
@@ -67,8 +67,9 @@
}
DEF_TEST(Codec_frames, r) {
- #define kOpaque kOpaque_SkAlphaType
- #define kUnpremul kUnpremul_SkAlphaType
+ #define kOpaque SkEncodedInfo::kOpaque_Alpha
+ #define kUnpremul SkEncodedInfo::kUnpremul_Alpha
+ #define kBinary SkEncodedInfo::kBinary_Alpha
#define kKeep SkCodecAnimation::DisposalMethod::kKeep
#define kRestoreBG SkCodecAnimation::DisposalMethod::kRestoreBGColor
#define kRestorePrev SkCodecAnimation::DisposalMethod::kRestorePrevious
@@ -78,8 +79,8 @@
// One less than fFramecount, since the first frame is always
// independent.
std::vector<int> fRequiredFrames;
- // Same, since the first frame should match getInfo.
- std::vector<SkAlphaType> fAlphaTypes;
+ // Same, since the first frame should match getEncodedInfo
+ std::vector<SkEncodedInfo::Alpha> fAlphas;
// The size of this one should match fFrameCount for animated, empty
// otherwise.
std::vector<int> fDurations;
@@ -88,15 +89,15 @@
} gRecs[] = {
{ "required.gif", 7,
{ 0, 1, 1, SkCodec::kNone, 4, 4 },
- { kOpaque, kUnpremul, kUnpremul, kOpaque, kOpaque, kOpaque },
+ { kOpaque, kBinary, kBinary, kOpaque, kOpaque, kOpaque },
{ 100, 100, 100, 100, 100, 100, 100 },
0,
{ kKeep, kRestoreBG, kKeep, kKeep, kKeep, kRestoreBG, kKeep } },
{ "alphabetAnim.gif", 13,
{ SkCodec::kNone, 0, 0, 0, 0, 5, 6, SkCodec::kNone,
SkCodec::kNone, SkCodec::kNone, 10, 11 },
- { kUnpremul, kUnpremul, kUnpremul, kUnpremul, kUnpremul, kUnpremul,
- kUnpremul, kUnpremul, kUnpremul, kOpaque, kOpaque, kUnpremul },
+ { kBinary, kBinary, kBinary, kBinary, kBinary, kBinary,
+ kBinary, kBinary, kBinary, kOpaque, kOpaque, kBinary },
{ 100, 100, 100, 100, 100, 100, 100, 100, 100, 100, 100, 100, 100 },
0,
{ kKeep, kRestorePrev, kRestorePrev, kRestorePrev, kRestorePrev,
@@ -115,8 +116,8 @@
{ "randPixelsAnim.gif", 13,
// required frames
{ SkCodec::kNone, 1, 2, 3, 4, 3, 6, 7, 7, 7, 9, 9 },
- { kUnpremul, kUnpremul, kUnpremul, kUnpremul, kUnpremul, kUnpremul,
- kUnpremul, kUnpremul, kUnpremul, kUnpremul, kUnpremul, kUnpremul },
+ { kBinary, kBinary, kBinary, kBinary, kBinary, kBinary,
+ kBinary, kBinary, kBinary, kBinary, kBinary, kBinary },
// durations
{ 0, 1000, 170, 40, 220, 7770, 90, 90, 90, 90, 90, 90, 90 },
// repetition count
@@ -160,6 +161,7 @@
};
#undef kOpaque
#undef kUnpremul
+ #undef kBinary
#undef kKeep
#undef kRestorePrev
#undef kRestoreBG
@@ -204,9 +206,9 @@
continue;
}
- if (rec.fAlphaTypes.size() + 1 != static_cast<size_t>(expected)) {
- ERRORF(r, "'%s' has wrong number entries in fAlphaTypes; expected: %i\tactual: %i",
- rec.fName, expected - 1, rec.fAlphaTypes.size());
+ if (rec.fAlphas.size() + 1 != static_cast<size_t>(expected)) {
+ ERRORF(r, "'%s' has wrong number entries in fAlphas; expected: %i\tactual: %i",
+ rec.fName, expected - 1, rec.fAlphas.size());
continue;
}
@@ -268,19 +270,22 @@
rec.fName, i, rec.fDurations[i], frameInfo.fDuration);
}
- auto to_string = [](SkAlphaType type) {
- switch (type) {
- case kUnpremul_SkAlphaType:
+ auto to_string = [](SkEncodedInfo::Alpha alpha) {
+ switch (alpha) {
+ case SkEncodedInfo::kUnpremul_Alpha:
return "unpremul";
- case kOpaque_SkAlphaType:
+ case SkEncodedInfo::kOpaque_Alpha:
return "opaque";
+ case SkEncodedInfo::kBinary_Alpha:
+ return "binary";
default:
- return "other";
+ SkASSERT(false);
+ return "unknown";
}
};
- auto expectedAlpha = 0 == i ? codec->getInfo().alphaType() : rec.fAlphaTypes[i-1];
- auto alpha = frameInfo.fAlphaType;
+ auto expectedAlpha = 0 == i ? codec->getEncodedInfo().alpha() : rec.fAlphas[i-1];
+ auto alpha = frameInfo.fAlpha;
if (expectedAlpha != alpha) {
ERRORF(r, "%s's frame %i has wrong alpha type! expected: %s\tactual: %s",
rec.fName, i, to_string(expectedAlpha), to_string(alpha));
@@ -314,7 +319,9 @@
auto decode = [&](SkBitmap* bm, int index, int cachedIndex) {
auto decodeInfo = info;
if (index > 0) {
- decodeInfo = info.makeAlphaType(frameInfos[index].fAlphaType);
+ auto alphaType = frameInfos[index].fAlpha == SkEncodedInfo::kOpaque_Alpha
+ ? kOpaque_SkAlphaType : kPremul_SkAlphaType;
+ decodeInfo = info.makeAlphaType(alphaType);
}
bm->allocPixels(decodeInfo);
if (cachedIndex != SkCodec::kNone) {