From 33deb7ed4d583c88187ba240efb749e9a1fd6843 Mon Sep 17 00:00:00 2001 From: Leon Scroggins III Date: Wed, 7 Jun 2017 12:31:51 -0400 Subject: Make SkCodec more flexible about its required frame SkCodec sets fRequiredFrame to be the earliest possible frame that a given frame can depend on. e.g. - Frame A fills the screen, Keep - Frame B does not cover A, Keep - Frame C covers B but not A, and is opaque Frame C can depend on either A or B. SkCodec already reports that C depends on A. This CL allows a client of SkCodec to use either A or B to create C. Also expose the DisposalMethod. Since any frame between A and C can be used to create C except for DisposePrevious frames, the client needs to be able to know the disposal method so they do not try to use such a frame to create C. Further, the disposal method can be used to give the client a better idea whether they will continue to need a frame. (e.g. if frame i is DisposePrevious and depends on i-1, the client may not want to steal i-1 to create i, since i+1 may also depend on i-1.) TODO: Share code for decoding prior frames between GIF and WEBP Change-Id: I91a5ae22ba3d8dfbe0bde833fa67ae3da0d81ed6 Reviewed-on: https://skia-review.googlesource.com/13722 Reviewed-by: Mike Reed Reviewed-by: Chris Blume Reviewed-by: Matt Sarett Commit-Queue: Leon Scroggins --- dm/DMSrcSink.cpp | 4 +- gm/animatedGif.cpp | 3 +- include/codec/SkCodec.h | 30 +++--- include/codec/SkCodecAnimation.h | 43 +++++++++ resources/required.gif | Bin 0 -> 733 bytes resources/required.webp | Bin 0 -> 788 bytes src/codec/SkCodecAnimation.h | 69 ------------- src/codec/SkCodecAnimationPriv.h | 32 +++++++ src/codec/SkFrameHolder.h | 3 +- src/codec/SkGifCodec.cpp | 63 ++++++++---- src/codec/SkWebpCodec.cpp | 39 +++++--- tests/CodecAnimTest.cpp | 181 +++++++++++++++++++++++------------ tests/CodecTest.cpp | 3 +- third_party/gif/SkGifImageReader.cpp | 10 +- 14 files changed, 294 insertions(+), 186 deletions(-) create mode 100644 include/codec/SkCodecAnimation.h create mode 100644 resources/required.gif create mode 100644 resources/required.webp delete mode 100644 src/codec/SkCodecAnimation.h create mode 100644 src/codec/SkCodecAnimationPriv.h diff --git a/dm/DMSrcSink.cpp b/dm/DMSrcSink.cpp index 983a0c1b15..4ac7e80626 100644 --- a/dm/DMSrcSink.cpp +++ b/dm/DMSrcSink.cpp @@ -504,9 +504,9 @@ Error CodecSrc::draw(SkCanvas* canvas) const { && priorFramePixels.get()) { // Copy into pixels memcpy(pixels.get(), priorFramePixels.get(), safeSize); - options.fHasPriorFrame = true; + options.fPriorFrame = reqFrame; } else { - options.fHasPriorFrame = false; + options.fPriorFrame = SkCodec::kNone; } SkCodec::Result result = codec->getPixels(decodeInfo, pixels.get(), rowBytes, &options, diff --git a/gm/animatedGif.cpp b/gm/animatedGif.cpp index 5f4fe71cb0..80bcdd9ee2 100644 --- a/gm/animatedGif.cpp +++ b/gm/animatedGif.cpp @@ -53,7 +53,6 @@ private: SkCodec::Options opts; opts.fFrameIndex = frameIndex; - opts.fHasPriorFrame = false; const int requiredFrame = fFrameInfos[frameIndex].fRequiredFrame; if (requiredFrame != SkCodec::kNone) { SkASSERT(requiredFrame >= 0 @@ -62,7 +61,7 @@ private: // For simplicity, do not try to cache old frames if (requiredBitmap.getPixels() && sk_tool_utils::copy_to(&bm, requiredBitmap.colorType(), requiredBitmap)) { - opts.fHasPriorFrame = true; + opts.fPriorFrame = requiredFrame; } } diff --git a/include/codec/SkCodec.h b/include/codec/SkCodec.h index d498007486..640ff468c5 100644 --- a/include/codec/SkCodec.h +++ b/include/codec/SkCodec.h @@ -9,6 +9,7 @@ #define SkCodec_DEFINED #include "../private/SkTemplates.h" +#include "SkCodecAnimation.h" #include "SkColor.h" #include "SkColorSpaceXform.h" #include "SkEncodedImageFormat.h" @@ -249,7 +250,7 @@ public: : fZeroInitialized(kNo_ZeroInitialized) , fSubset(nullptr) , fFrameIndex(0) - , fHasPriorFrame(false) + , fPriorFrame(kNone) , fPremulBehavior(SkTransferFunctionBehavior::kRespect) {} @@ -281,23 +282,19 @@ public: int fFrameIndex; /** - * If true, the dst already contains the prior frame. + * If not kNone, the dst already contains the prior frame at this index. * * Only meaningful for multi-frame images. * * If fFrameIndex needs to be blended with a prior frame (as reported by * getFrameInfo[fFrameIndex].fRequiredFrame), the client can set this to - * either true or false: + * any non-kRestorePrevious frame in [fRequiredFrame, fFrameIndex) to + * indicate that that frame is already in the dst. Options.fZeroInitialized + * is ignored in this case. * - * true means that the prior frame is already in the dst, and this - * codec only needs to decode fFrameIndex and blend it with the dst. - * Options.fZeroInitialized is ignored in this case. - * - * false means that the dst does not contain the prior frame, so this - * codec needs to first decode the prior frame (which in turn may need - * to decode its prior frame). + * If set to kNone, the codec will decode any necessary required frame(s) first. */ - bool fHasPriorFrame; + int fPriorFrame; /** * Indicates whether we should do a linear premultiply or a legacy premultiply. @@ -613,7 +610,11 @@ public: struct FrameInfo { /** * The frame that this frame needs to be blended with, or - * kNone. + * kNone if this frame is independent. + * + * Note that this is the *earliest* frame that can be used + * for blending. Any frame from [fRequiredFrame, i) can be + * used, unless its fDisposalMethod is kRestorePrevious. */ int fRequiredFrame; @@ -635,6 +636,11 @@ public: * color index-based frame has a color with alpha but does not use it. */ SkAlphaType fAlphaType; + + /** + * How this frame should be modified before decoding the next one. + */ + SkCodecAnimation::DisposalMethod fDisposalMethod; }; /** diff --git a/include/codec/SkCodecAnimation.h b/include/codec/SkCodecAnimation.h new file mode 100644 index 0000000000..9a4daff8ae --- /dev/null +++ b/include/codec/SkCodecAnimation.h @@ -0,0 +1,43 @@ +/* + * Copyright 2016 Google Inc. + * + * Use of this source code is governed by a BSD-style license that can be + * found in the LICENSE file. + */ + +#ifndef SkCodecAnimation_DEFINED +#define SkCodecAnimation_DEFINED + +namespace SkCodecAnimation { + /** + * This specifies how the next frame is based on this frame. + * + * Names are based on the GIF 89a spec. + * + * The numbers correspond to values in a GIF. + */ + enum class DisposalMethod { + /** + * The next frame should be drawn on top of this one. + * + * In a GIF, a value of 0 (not specified) is also treated as Keep. + */ + kKeep = 1, + + /** + * Similar to Keep, except the area inside this frame's rectangle + * should be cleared to the BackGround color (transparent) before + * drawing the next frame. + */ + kRestoreBGColor = 2, + + /** + * The next frame should be drawn on top of the previous frame - i.e. + * disregarding this one. + * + * In a GIF, a value of 4 is also treated as RestorePrevious. + */ + kRestorePrevious = 3, + }; +}; +#endif // SkCodecAnimation_DEFINED diff --git a/resources/required.gif b/resources/required.gif new file mode 100644 index 0000000000..91a9fd17e8 Binary files /dev/null and b/resources/required.gif differ diff --git a/resources/required.webp b/resources/required.webp new file mode 100644 index 0000000000..9f9a8f8b8d Binary files /dev/null and b/resources/required.webp differ diff --git a/src/codec/SkCodecAnimation.h b/src/codec/SkCodecAnimation.h deleted file mode 100644 index 46a878a801..0000000000 --- a/src/codec/SkCodecAnimation.h +++ /dev/null @@ -1,69 +0,0 @@ -/* - * Copyright 2016 Google Inc. - * - * Use of this source code is governed by a BSD-style license that can be - * found in the LICENSE file. - */ - -#ifndef SkCodecAnimation_DEFINED -#define SkCodecAnimation_DEFINED - -#include "SkCodec.h" -#include "SkRect.h" - -class SkCodecAnimation { -public: - /** - * This specifies how the next frame is based on this frame. - * - * Names are based on the GIF 89a spec. - * - * The numbers correspond to values in a GIF. - */ - enum DisposalMethod { - /** - * The next frame should be drawn on top of this one. - * - * In a GIF, a value of 0 (not specified) is also treated as Keep. - */ - Keep_DisposalMethod = 1, - - /** - * Similar to Keep, except the area inside this frame's rectangle - * should be cleared to the BackGround color (transparent) before - * drawing the next frame. - */ - RestoreBGColor_DisposalMethod = 2, - - /** - * The next frame should be drawn on top of the previous frame - i.e. - * disregarding this one. - * - * In a GIF, a value of 4 is also treated as RestorePrevious. - */ - RestorePrevious_DisposalMethod = 3, - }; - - /** - * How to blend the current frame. - */ - enum class Blend { - /** - * Blend with the prior frame. This is the typical case, supported - * by all animated image types. - */ - kPriorFrame, - - /** - * Do not blend. - * - * This frames pixels overwrite previous pixels "blending" with - * the background color of transparent. - */ - kBG, - }; - -private: - SkCodecAnimation(); -}; -#endif // SkCodecAnimation_DEFINED diff --git a/src/codec/SkCodecAnimationPriv.h b/src/codec/SkCodecAnimationPriv.h new file mode 100644 index 0000000000..233a79b211 --- /dev/null +++ b/src/codec/SkCodecAnimationPriv.h @@ -0,0 +1,32 @@ +/* + * Copyright 2016 Google Inc. + * + * Use of this source code is governed by a BSD-style license that can be + * found in the LICENSE file. + */ + +#ifndef SkCodecAnimationPriv_DEFINED +#define SkCodecAnimationPriv_DEFINED + +namespace SkCodecAnimation { + /** + * How to blend the current frame. + */ + enum class Blend { + /** + * Blend with the prior frame. This is the typical case, supported + * by all animated image types. + */ + kPriorFrame, + + /** + * Do not blend. + * + * This frames pixels overwrite previous pixels "blending" with + * the background color of transparent. + */ + kBG, + }; + +} +#endif // SkCodecAnimationPriv_DEFINED diff --git a/src/codec/SkFrameHolder.h b/src/codec/SkFrameHolder.h index e92b40f9d8..0f30b65e5a 100644 --- a/src/codec/SkFrameHolder.h +++ b/src/codec/SkFrameHolder.h @@ -10,6 +10,7 @@ #include "SkTypes.h" #include "SkCodecAnimation.h" +#include "SkCodecAnimationPriv.h" /** * Base class for a single frame of an animated image. @@ -23,7 +24,7 @@ public: : fId(id) , fHasAlpha(false) , fRequiredFrame(kUninitialized) - , fDisposalMethod(SkCodecAnimation::Keep_DisposalMethod) + , fDisposalMethod(SkCodecAnimation::DisposalMethod::kKeep) , fDuration(0) , fBlend(SkCodecAnimation::Blend::kPriorFrame) { diff --git a/src/codec/SkGifCodec.cpp b/src/codec/SkGifCodec.cpp index f0495bc4b3..6c4273444c 100644 --- a/src/codec/SkGifCodec.cpp +++ b/src/codec/SkGifCodec.cpp @@ -153,6 +153,7 @@ bool SkGifCodec::onGetFrameInfo(int i, SkCodec::FrameInfo* frameInfo) const { frameInfo->fFullyReceived = frameContext->isComplete(); frameInfo->fAlphaType = frameContext->hasAlpha() ? kUnpremul_SkAlphaType : kOpaque_SkAlphaType; + frameInfo->fDisposalMethod = frameContext->getDisposalMethod(); } return true; } @@ -398,11 +399,24 @@ SkCodec::Result SkGifCodec::decodeFrame(bool firstAttempt, const Options& opts, } } else { // Not independent - if (!opts.fHasPriorFrame) { + // FIXME: Share this code with WEBP + const int reqFrame = frameContext->getRequiredFrame(); + if (opts.fPriorFrame != kNone) { + if (opts.fPriorFrame < reqFrame || opts.fPriorFrame >= frameIndex) { + // Alternatively, we could correct this to kNone. + return kInvalidParameters; + } + const auto* prevFrame = fReader->frameContext(opts.fPriorFrame); + if (prevFrame->getDisposalMethod() + == SkCodecAnimation::DisposalMethod::kRestorePrevious) { + // Similarly, this could be corrected to kNone. + return kInvalidParameters; + } + } else { // Decode that frame into pixels. Options prevFrameOpts(opts); prevFrameOpts.fFrameIndex = frameContext->getRequiredFrame(); - prevFrameOpts.fHasPriorFrame = false; + prevFrameOpts.fPriorFrame = kNone; // The prior frame may have a different color table, so update it and the // swizzler. this->initializeColorTable(dstInfo, prevFrameOpts.fFrameIndex); @@ -424,24 +438,33 @@ SkCodec::Result SkGifCodec::decodeFrame(bool firstAttempt, const Options& opts, this->initializeColorTable(dstInfo, frameIndex); this->initializeSwizzler(dstInfo, frameIndex); } - const auto* prevFrame = fReader->frameContext(frameContext->getRequiredFrame()); - if (prevFrame->getDisposalMethod() == SkCodecAnimation::RestoreBGColor_DisposalMethod) { - SkIRect prevRect = prevFrame->frameRect(); - if (prevRect.intersect(this->getInfo().bounds())) { - // Do the divide ourselves for left and top, since we do not want - // get_scaled_dimension to upgrade 0 to 1. (This is similar to SkSampledCodec's - // sampling of the subset.) - auto left = prevRect.fLeft / fSwizzler->sampleX(); - auto top = prevRect.fTop / fSwizzler->sampleY(); - void* const eraseDst = SkTAddOffset(fDst, top * fDstRowBytes - + left * SkColorTypeBytesPerPixel(dstInfo.colorType())); - auto width = get_scaled_dimension(prevRect.width(), fSwizzler->sampleX()); - auto height = get_scaled_dimension(prevRect.height(), fSwizzler->sampleY()); - // fSwizzler->fill() would fill to the scaled width of the frame, but we want to - // fill to the scaled with of the width of the PRIOR frame, so we do all the - // scaling ourselves and call the static version. - SkSampler::Fill(dstInfo.makeWH(width, height), eraseDst, - fDstRowBytes, this->getFillValue(dstInfo), kNo_ZeroInitialized); + + // If the required frame is RestoreBG, we need to erase it. If a frame after the + // required frame is provided, there is no need to erase, since it will be covered + // anyway. + if (opts.fPriorFrame == reqFrame || opts.fPriorFrame == kNone) { + const auto* prevFrame = fReader->frameContext(reqFrame); + if (prevFrame->getDisposalMethod() + == SkCodecAnimation::DisposalMethod::kRestoreBGColor) { + SkIRect prevRect = prevFrame->frameRect(); + if (prevRect.intersect(this->getInfo().bounds())) { + // Do the divide ourselves for left and top, since we do not want + // get_scaled_dimension to upgrade 0 to 1. (This is similar to + // SkSampledCodec's sampling of the subset.) + const auto sampleX = fSwizzler->sampleX(); + const auto sampleY = fSwizzler->sampleY(); + auto left = prevRect.fLeft / sampleX; + auto top = prevRect.fTop / sampleY; + void* const eraseDst = SkTAddOffset(fDst, top * fDstRowBytes + + left * SkColorTypeBytesPerPixel(dstInfo.colorType())); + auto width = get_scaled_dimension(prevRect.width(), sampleX); + auto height = get_scaled_dimension(prevRect.height(), sampleY); + // fSwizzler->fill() would fill to the scaled width of the frame, but we + // want to fill to the scaled with of the width of the PRIOR frame, so we + // do all the scaling ourselves and call the static version. + SkSampler::Fill(dstInfo.makeWH(width, height), eraseDst, fDstRowBytes, + this->getFillValue(dstInfo), kNo_ZeroInitialized); + } } } filledBackground = true; diff --git a/src/codec/SkWebpCodec.cpp b/src/codec/SkWebpCodec.cpp index 21d45da233..f702a3ae98 100644 --- a/src/codec/SkWebpCodec.cpp +++ b/src/codec/SkWebpCodec.cpp @@ -7,6 +7,8 @@ #include "SkBitmap.h" #include "SkCanvas.h" +#include "SkCodecAnimation.h" +#include "SkCodecAnimationPriv.h" #include "SkCodecPriv.h" #include "SkColorSpaceXform.h" #include "SkRasterPipeline.h" @@ -253,8 +255,8 @@ int SkWebpCodec::onGetFrameCount() { Frame* frame = fFrameHolder.appendNewFrame(iter.has_alpha); frame->setXYWH(iter.x_offset, iter.y_offset, iter.width, iter.height); frame->setDisposalMethod(iter.dispose_method == WEBP_MUX_DISPOSE_BACKGROUND ? - SkCodecAnimation::RestoreBGColor_DisposalMethod : - SkCodecAnimation::Keep_DisposalMethod); + SkCodecAnimation::DisposalMethod::kRestoreBGColor : + SkCodecAnimation::DisposalMethod::kKeep); frame->setDuration(iter.duration); if (WEBP_MUX_BLEND != iter.blend_method) { frame->setBlend(SkCodecAnimation::Blend::kBG); @@ -292,6 +294,7 @@ bool SkWebpCodec::onGetFrameInfo(int i, FrameInfo* frameInfo) const { // animated image. frameInfo->fFullyReceived = true; frameInfo->fAlphaType = alpha_type(frame->hasAlpha()); + frameInfo->fDisposalMethod = frame->getDisposalMethod(); } return true; @@ -437,9 +440,15 @@ SkCodec::Result SkWebpCodec::onGetPixels(const SkImageInfo& dstInfo, void* dst, SkSampler::Fill(dstInfo, dst, rowBytes, 0, options.fZeroInitialized); } } else { - if (!options.fHasPriorFrame) { + // FIXME: Share with GIF + if (options.fPriorFrame != kNone) { + if (options.fPriorFrame < requiredFrame || options.fPriorFrame >= index) { + return kInvalidParameters; + } + } else { Options prevFrameOpts(options); prevFrameOpts.fFrameIndex = requiredFrame; + prevFrameOpts.fPriorFrame = kNone; const auto result = this->getPixels(dstInfo, dst, rowBytes, &prevFrameOpts, nullptr, nullptr); switch (result) { @@ -452,16 +461,20 @@ SkCodec::Result SkWebpCodec::onGetPixels(const SkImageInfo& dstInfo, void* dst, } } - // Dispose bg color - const Frame* priorFrame = fFrameHolder.frame(requiredFrame); - if (priorFrame->getDisposalMethod() == SkCodecAnimation::RestoreBGColor_DisposalMethod) { - // FIXME: If we add support for scaling/subsets, this rectangle needs to be adjusted. - const auto priorRect = priorFrame->frameRect(); - const auto info = dstInfo.makeWH(priorRect.width(), priorRect.height()); - const size_t bpp = SkColorTypeBytesPerPixel(dstInfo.colorType()); - const size_t offset = priorRect.x() * bpp + priorRect.y() * rowBytes; - auto* eraseDst = SkTAddOffset(dst, offset); - SkSampler::Fill(info, eraseDst, rowBytes, 0, kNo_ZeroInitialized); + if (options.fPriorFrame == requiredFrame || options.fPriorFrame == kNone) { + // Dispose bg color + const Frame* priorFrame = fFrameHolder.frame(requiredFrame); + if (priorFrame->getDisposalMethod() + == SkCodecAnimation::DisposalMethod::kRestoreBGColor) { + // FIXME: If we add support for scaling/subsets, this rectangle needs to be + // adjusted. + const auto priorRect = priorFrame->frameRect(); + const auto info = dstInfo.makeWH(priorRect.width(), priorRect.height()); + const size_t bpp = SkColorTypeBytesPerPixel(dstInfo.colorType()); + const size_t offset = priorRect.x() * bpp + priorRect.y() * rowBytes; + auto* eraseDst = SkTAddOffset(dst, offset); + SkSampler::Fill(info, eraseDst, rowBytes, 0, kNo_ZeroInitialized); + } } } diff --git a/tests/CodecAnimTest.cpp b/tests/CodecAnimTest.cpp index 74ee3c63ed..27fe0038b5 100644 --- a/tests/CodecAnimTest.cpp +++ b/tests/CodecAnimTest.cpp @@ -57,37 +57,53 @@ DEF_TEST(Codec_565, r) { SkCodec::Options options; options.fFrameIndex = 1; - options.fHasPriorFrame = false; + options.fPriorFrame = SkCodec::kNone; const auto result = codec->getPixels(info, bm.getPixels(), bm.rowBytes(), &options, nullptr, nullptr); REPORTER_ASSERT(r, result == SkCodec::kSuccess); } +static bool restore_previous(const SkCodec::FrameInfo& info) { + return info.fDisposalMethod == SkCodecAnimation::DisposalMethod::kRestorePrevious; +} DEF_TEST(Codec_frames, r) { - #define kOpaque kOpaque_SkAlphaType - #define kUnpremul kUnpremul_SkAlphaType + #define kOpaque kOpaque_SkAlphaType + #define kUnpremul kUnpremul_SkAlphaType + #define kKeep SkCodecAnimation::DisposalMethod::kKeep + #define kRestoreBG SkCodecAnimation::DisposalMethod::kRestoreBGColor + #define kRestorePrev SkCodecAnimation::DisposalMethod::kRestorePrevious static const struct { - const char* fName; - int fFrameCount; + const char* fName; + int fFrameCount; // One less than fFramecount, since the first frame is always // independent. - std::vector fRequiredFrames; + std::vector fRequiredFrames; // Same, since the first frame should match getInfo. - std::vector fAlphaTypes; + std::vector fAlphaTypes; // The size of this one should match fFrameCount for animated, empty // otherwise. - std::vector fDurations; - int fRepetitionCount; + std::vector fDurations; + int fRepetitionCount; + std::vector fDisposalMethods; } gRecs[] = { + { "required.gif", 7, + { 0, 1, 1, SkCodec::kNone, 4, 4 }, + { kOpaque, kUnpremul, kUnpremul, 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 }, { 100, 100, 100, 100, 100, 100, 100, 100, 100, 100, 100, 100, 100 }, - 0 }, + 0, + { kKeep, kRestorePrev, kRestorePrev, kRestorePrev, kRestorePrev, + kRestoreBG, kKeep, kRestoreBG, kRestoreBG, kKeep, kKeep, + kRestoreBG, kKeep } }, { "randPixelsAnim2.gif", 4, // required frames { 0, 0, 1 }, @@ -96,7 +112,8 @@ DEF_TEST(Codec_frames, r) { // durations { 0, 1000, 170, 40 }, // repetition count - 0 }, + 0, + { kKeep, kKeep, kRestorePrev, kKeep } }, { "randPixelsAnim.gif", 13, // required frames { SkCodec::kNone, 1, 2, 3, 4, 3, 6, 7, 7, 7, 9, 9 }, @@ -105,33 +122,49 @@ DEF_TEST(Codec_frames, r) { // durations { 0, 1000, 170, 40, 220, 7770, 90, 90, 90, 90, 90, 90, 90 }, // repetition count - 0 }, - { "box.gif", 1, {}, {}, {}, 0 }, - { "color_wheel.gif", 1, {}, {}, {}, 0 }, + 0, + { kKeep, kKeep, kKeep, kKeep, kRestoreBG, kRestoreBG, kRestoreBG, + kRestoreBG, kRestorePrev, kRestoreBG, kRestorePrev, kRestorePrev, + kRestorePrev, } }, + { "box.gif", 1, {}, {}, {}, 0, { kKeep } }, + { "color_wheel.gif", 1, {}, {}, {}, 0, { kKeep } }, { "test640x479.gif", 4, { 0, 1, 2 }, { kOpaque, kOpaque, kOpaque }, { 200, 200, 200, 200 }, - SkCodec::kRepetitionCountInfinite }, - { "colorTables.gif", 2, { 0 }, { kOpaque }, { 1000, 1000 }, 5 }, - - { "arrow.png", 1, {}, {}, {}, 0 }, - { "google_chrome.ico", 1, {}, {}, {}, 0 }, - { "brickwork-texture.jpg", 1, {}, {}, {}, 0 }, + SkCodec::kRepetitionCountInfinite, + { kKeep, kKeep, kKeep, kKeep } }, + { "colorTables.gif", 2, { 0 }, { kOpaque }, { 1000, 1000 }, 5, + { kKeep, kKeep } }, + + { "arrow.png", 1, {}, {}, {}, 0, {} }, + { "google_chrome.ico", 1, {}, {}, {}, 0, {} }, + { "brickwork-texture.jpg", 1, {}, {}, {}, 0, {} }, #if defined(SK_CODEC_DECODES_RAW) && (!defined(_WIN32)) - { "dng_with_preview.dng", 1, {}, {}, {}, 0 }, + { "dng_with_preview.dng", 1, {}, {}, {}, 0, {} }, #endif - { "mandrill.wbmp", 1, {}, {}, {}, 0 }, - { "randPixels.bmp", 1, {}, {}, {}, 0 }, - { "yellow_rose.webp", 1, {}, {}, {}, 0 }, + { "mandrill.wbmp", 1, {}, {}, {}, 0, {} }, + { "randPixels.bmp", 1, {}, {}, {}, 0, {} }, + { "yellow_rose.webp", 1, {}, {}, {}, 0, {} }, { "webp-animated.webp", 3, { 0, 1 }, { kOpaque, kOpaque }, - { 1000, 500, 1000 }, SkCodec::kRepetitionCountInfinite }, + { 1000, 500, 1000 }, SkCodec::kRepetitionCountInfinite, + { kKeep, kKeep, kKeep } }, { "blendBG.webp", 7, { 0, SkCodec::kNone, SkCodec::kNone, SkCodec::kNone, 4, 4 }, { kOpaque, kOpaque, kUnpremul, kOpaque, kUnpremul, kUnpremul }, - { 525, 500, 525, 437, 609, 729, 444 }, 7 }, + { 525, 500, 525, 437, 609, 729, 444 }, 7, + { kKeep, kKeep, kKeep, kKeep, kKeep, kKeep, kKeep } }, + { "required.webp", 7, + { 0, 1, 1, SkCodec::kNone, 4, 4 }, + { kOpaque, kUnpremul, kUnpremul, kOpaque, kOpaque, kOpaque }, + { 100, 100, 100, 100, 100, 100, 100 }, + 1, + { kKeep, kRestoreBG, kKeep, kKeep, kKeep, kRestoreBG, kKeep } }, }; #undef kOpaque #undef kUnpremul + #undef kKeep + #undef kRestorePrev + #undef kRestoreBG for (const auto& rec : gRecs) { sk_sp data(GetResourceAsData(rec.fName)); @@ -178,6 +211,13 @@ DEF_TEST(Codec_frames, r) { rec.fName, expected - 1, rec.fAlphaTypes.size()); continue; } + + if (rec.fDisposalMethods.size() != static_cast(expected)) { + ERRORF(r, "'%s' has wrong number entries in fDisposalMethods; " + "expected %i\tactual: %i", + rec.fName, expected, rec.fDisposalMethods.size()); + continue; + } } enum class TestMode { @@ -254,6 +294,8 @@ DEF_TEST(Codec_frames, r) { ERRORF(r, "%s's frame %i has wrong dependency! expected: %i\tactual: %i", rec.fName, i, rec.fRequiredFrames[i-1], frameInfo.fRequiredFrame); } + + REPORTER_ASSERT(r, frameInfo.fDisposalMethod == rec.fDisposalMethods[i]); } if (TestMode::kIndividual == mode) { @@ -261,67 +303,84 @@ DEF_TEST(Codec_frames, r) { continue; } - // Compare decoding in two ways: - // 1. Provide the frame that a frame depends on, so the codec just has to blend. - // (in the array cachedFrames) - // 2. Do not provide the frame that a frame depends on, so the codec has to decode - // all the way back to a key-frame. (in a local variable uncachedFrame) - // The two should look the same. + // Compare decoding in multiple ways: + // - Start from scratch for each frame. |codec| will have to decode the required frame + // (and any it depends on) to decode. This is stored in |cachedFrames|. + // - Provide the frame that a frame depends on, so |codec| just has to blend. + // - Provide a frame after the required frame, which will be covered up by the newest + // frame. + // All should look the same. std::vector cachedFrames(frameCount); const auto info = codec->getInfo().makeColorType(kN32_SkColorType); - auto decode = [&](SkBitmap* bm, bool cached, int index) { + auto decode = [&](SkBitmap* bm, int index, int cachedIndex) { auto decodeInfo = info; if (index > 0) { decodeInfo = info.makeAlphaType(frameInfos[index].fAlphaType); } bm->allocPixels(decodeInfo); - if (cached) { + if (cachedIndex != SkCodec::kNone) { // First copy the pixels from the cached frame - const int requiredFrame = frameInfos[index].fRequiredFrame; - if (requiredFrame != SkCodec::kNone) { - const bool success = sk_tool_utils::copy_to(bm, kN32_SkColorType, - cachedFrames[requiredFrame]); - REPORTER_ASSERT(r, success); - } + const bool success = sk_tool_utils::copy_to(bm, kN32_SkColorType, + cachedFrames[cachedIndex]); + REPORTER_ASSERT(r, success); } SkCodec::Options opts; opts.fFrameIndex = index; - opts.fHasPriorFrame = cached; + opts.fPriorFrame = cachedIndex; const auto result = codec->getPixels(decodeInfo, bm->getPixels(), bm->rowBytes(), &opts, nullptr, nullptr); + if (cachedIndex != SkCodec::kNone && restore_previous(frameInfos[cachedIndex])) { + if (result == SkCodec::kInvalidParameters) { + return true; + } + ERRORF(r, "Using a kRestorePrevious frame as fPriorFrame should fail"); + return false; + } if (result != SkCodec::kSuccess) { - ERRORF(r, "Failed to decode frame %i from %s when %scached, error %i", - index, rec.fName, (cached ? "" : "not "), result); + ERRORF(r, "Failed to decode frame %i from %s when providing prior frame %i, " + "error %i", index, rec.fName, cachedIndex, result); } return result == SkCodec::kSuccess; }; for (int i = 0; i < frameCount; i++) { SkBitmap& cachedFrame = cachedFrames[i]; - if (!decode(&cachedFrame, true, i)) { + if (!decode(&cachedFrame, i, SkCodec::kNone)) { continue; } - SkBitmap uncachedFrame; - if (!decode(&uncachedFrame, false, i)) { + const auto reqFrame = frameInfos[i].fRequiredFrame; + if (reqFrame == SkCodec::kNone) { + // Nothing to compare against. continue; } + for (int j = reqFrame; j < i; j++) { + SkBitmap frame; + if (restore_previous(frameInfos[j])) { + (void) decode(&frame, i, j); + continue; + } + if (!decode(&frame, i, j)) { + continue; + } - // Now verify they're equal. - const size_t rowLen = info.bytesPerPixel() * info.width(); - for (int y = 0; y < info.height(); y++) { - const void* cachedAddr = cachedFrame.getAddr(0, y); - SkASSERT(cachedAddr != nullptr); - const void* uncachedAddr = uncachedFrame.getAddr(0, y); - SkASSERT(uncachedAddr != nullptr); - const bool lineMatches = memcmp(cachedAddr, uncachedAddr, rowLen) == 0; - if (!lineMatches) { - SkString name = SkStringPrintf("cached_%i", i); - write_bm(name.c_str(), cachedFrame); - name = SkStringPrintf("uncached_%i", i); - write_bm(name.c_str(), uncachedFrame); - ERRORF(r, "%s's frame %i is different depending on caching!", rec.fName, i); - break; + // Now verify they're equal. + const size_t rowLen = info.bytesPerPixel() * info.width(); + for (int y = 0; y < info.height(); y++) { + const void* cachedAddr = cachedFrame.getAddr(0, y); + SkASSERT(cachedAddr != nullptr); + const void* addr = frame.getAddr(0, y); + SkASSERT(addr != nullptr); + const bool lineMatches = memcmp(cachedAddr, addr, rowLen) == 0; + if (!lineMatches) { + SkString name = SkStringPrintf("cached_%i", i); + write_bm(name.c_str(), cachedFrame); + name = SkStringPrintf("frame_%i", i); + write_bm(name.c_str(), frame); + ERRORF(r, "%s's frame %i is different (starting from line %i) when " + "providing prior frame %i!", rec.fName, i, y, j); + break; + } } } } diff --git a/tests/CodecTest.cpp b/tests/CodecTest.cpp index ffaeb2b61c..f839aaad80 100644 --- a/tests/CodecTest.cpp +++ b/tests/CodecTest.cpp @@ -1514,7 +1514,8 @@ DEF_TEST(Codec_InvalidAnimated, r) { SkCodec::Options opts; for (int i = 0; static_cast(i) < frameInfos.size(); i++) { opts.fFrameIndex = i; - opts.fHasPriorFrame = frameInfos[i].fRequiredFrame == i - 1; + const auto reqFrame = frameInfos[i].fRequiredFrame; + opts.fPriorFrame = reqFrame == i - 1 ? reqFrame : SkCodec::kNone; auto result = codec->startIncrementalDecode(info, bm.getPixels(), bm.rowBytes(), &opts); if (result != SkCodec::kSuccess) { ERRORF(r, "Failed to start decoding frame %i (out of %i) with error %i\n", i, diff --git a/third_party/gif/SkGifImageReader.cpp b/third_party/gif/SkGifImageReader.cpp index 0666fedbc8..76f3edcef0 100644 --- a/third_party/gif/SkGifImageReader.cpp +++ b/third_party/gif/SkGifImageReader.cpp @@ -616,11 +616,11 @@ bool SkGifImageReader::parse(SkGifImageReader::SkGIFParseQuery query) case 4: // Some specs say that disposal method 3 is "overwrite previous", others that setting // the third bit of the field (i.e. method 4) is. We map both to the same value. - currentFrame->setDisposalMethod(SkCodecAnimation::RestorePrevious_DisposalMethod); + currentFrame->setDisposalMethod(SkCodecAnimation::DisposalMethod::kRestorePrevious); break; default: // Other values use the default. - currentFrame->setDisposalMethod(SkCodecAnimation::Keep_DisposalMethod); + currentFrame->setDisposalMethod(SkCodecAnimation::DisposalMethod::kKeep); break; } currentFrame->setDuration(GETINT16(currentComponent + 1) * 10); @@ -903,7 +903,7 @@ static bool independent(const SkFrame& frame) { } static bool restore_bg(const SkFrame& frame) { - return frame.getDisposalMethod() == SkCodecAnimation::RestoreBGColor_DisposalMethod; + return frame.getDisposalMethod() == SkCodecAnimation::DisposalMethod::kRestoreBGColor; } bool SkGIFFrameContext::onReportsAlpha() const { @@ -935,7 +935,7 @@ void SkFrameHolder::setAlphaAndRequiredFrame(SkFrame* frame) { } const SkFrame* prevFrame = this->getFrame(i-1); - while (prevFrame->getDisposalMethod() == SkCodecAnimation::RestorePrevious_DisposalMethod) { + while (prevFrame->getDisposalMethod() == SkCodecAnimation::DisposalMethod::kRestorePrevious) { const int prevId = prevFrame->frameId(); if (0 == prevId) { frame->setHasAlpha(true); @@ -992,7 +992,7 @@ void SkFrameHolder::setAlphaAndRequiredFrame(SkFrame* frame) { return; } - SkASSERT(prevFrame->getDisposalMethod() == SkCodecAnimation::Keep_DisposalMethod); + SkASSERT(prevFrame->getDisposalMethod() == SkCodecAnimation::DisposalMethod::kKeep); frame->setRequiredFrame(prevFrame->frameId()); frame->setHasAlpha(prevFrame->hasAlpha() || (reportsAlpha && !blendWithPrevFrame)); } -- cgit v1.2.3