From 35bb74b444bc4a9ed2f437d97c6a943012990fe3 Mon Sep 17 00:00:00 2001 From: msarett Date: Mon, 22 Aug 2016 07:41:28 -0700 Subject: [PATCH] Fix color xform width bug when scaling/subsetting This was not caught by the bots because we don't test color correct modes with our many image decoding tests (takes too long). Adding a unit test. BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2247743002 Review-Url: https://codereview.chromium.org/2247743002 --- src/codec/SkJpegCodec.cpp | 11 ++++++++--- src/codec/SkPngCodec.cpp | 18 +++++++++--------- src/codec/SkPngCodec.h | 2 +- src/codec/SkSwizzler.h | 6 ++++++ tests/CodecTest.cpp | 30 ++++++++++++++++++++++++++++++ 5 files changed, 54 insertions(+), 13 deletions(-) diff --git a/src/codec/SkJpegCodec.cpp b/src/codec/SkJpegCodec.cpp index eb5f6b0387..05f40bdf80 100644 --- a/src/codec/SkJpegCodec.cpp +++ b/src/codec/SkJpegCodec.cpp @@ -495,11 +495,13 @@ int SkJpegCodec::readRows(const SkImageInfo& dstInfo, void* dst, size_t rowBytes uint32_t* swizzleDst = (uint32_t*) dst; size_t decodeDstRowBytes = rowBytes; size_t swizzleDstRowBytes = rowBytes; + int dstWidth = dstInfo.width(); if (fSwizzleSrcRow && fColorXformSrcRow) { decodeDst = (JSAMPLE*) fSwizzleSrcRow; swizzleDst = fColorXformSrcRow; decodeDstRowBytes = 0; swizzleDstRowBytes = 0; + dstWidth = fSwizzler->swizzleWidth(); } else if (fColorXformSrcRow) { decodeDst = (JSAMPLE*) fColorXformSrcRow; swizzleDst = fColorXformSrcRow; @@ -508,6 +510,7 @@ int SkJpegCodec::readRows(const SkImageInfo& dstInfo, void* dst, size_t rowBytes } else if (fSwizzleSrcRow) { decodeDst = (JSAMPLE*) fSwizzleSrcRow; decodeDstRowBytes = 0; + dstWidth = fSwizzler->swizzleWidth(); } for (int y = 0; y < count; y++) { @@ -523,8 +526,7 @@ int SkJpegCodec::readRows(const SkImageInfo& dstInfo, void* dst, size_t rowBytes } if (fColorXform) { - fColorXform->apply(dst, swizzleDst, dstInfo.width(), dstInfo.colorType(), - kOpaque_SkAlphaType); + fColorXform->apply(dst, swizzleDst, dstWidth, dstInfo.colorType(), kOpaque_SkAlphaType); dst = SkTAddOffset(dst, rowBytes); } @@ -590,16 +592,19 @@ SkCodec::Result SkJpegCodec::onGetPixels(const SkImageInfo& dstInfo, } void SkJpegCodec::allocateStorage(const SkImageInfo& dstInfo) { + int dstWidth = dstInfo.width(); + size_t swizzleBytes = 0; if (fSwizzler) { swizzleBytes = get_row_bytes(fDecoderMgr->dinfo()); + dstWidth = fSwizzler->swizzleWidth(); SkASSERT(!fColorXform || SkIsAlign4(swizzleBytes)); } size_t xformBytes = 0; if (kRGBA_F16_SkColorType == dstInfo.colorType()) { SkASSERT(fColorXform); - xformBytes = dstInfo.width() * sizeof(uint32_t); + xformBytes = dstWidth * sizeof(uint32_t); } size_t totalBytes = swizzleBytes + xformBytes; diff --git a/src/codec/SkPngCodec.cpp b/src/codec/SkPngCodec.cpp index 0d13e6ac2e..2b52ab72d7 100644 --- a/src/codec/SkPngCodec.cpp +++ b/src/codec/SkPngCodec.cpp @@ -358,9 +358,8 @@ static bool png_conversion_possible(const SkImageInfo& dst, const SkImageInfo& s } } -void SkPngCodec::allocateStorage(const SkImageInfo& dstInfo) { - const int width = this->getInfo().width(); - size_t colorXformBytes = fColorXform ? width * sizeof(uint32_t) : 0; +void SkPngCodec::allocateStorage() { + size_t colorXformBytes = fColorXform ? fSwizzler->swizzleWidth() * sizeof(uint32_t) : 0; fStorage.reset(SkAlign4(fSrcRowBytes) + colorXformBytes); fSwizzlerSrcRow = fStorage.get(); @@ -384,7 +383,7 @@ public: return kInvalidConversion; } - this->allocateStorage(dstInfo); + this->allocateStorage(); return kSuccess; } @@ -413,7 +412,7 @@ public: fSwizzler->swizzle(swizzlerDstRow, fSwizzlerSrcRow); if (fColorXform) { - fColorXform->apply(dst, (const uint32_t*) swizzlerDstRow, dstInfo.width(), + fColorXform->apply(dst, (const uint32_t*) swizzlerDstRow, fSwizzler->swizzleWidth(), dstInfo.colorType(), xformAlphaType); dst = SkTAddOffset(dst, rowBytes); } @@ -464,7 +463,7 @@ public: return kInvalidConversion; } - this->allocateStorage(dstInfo); + this->allocateStorage(); fCanSkipRewind = true; return SkCodec::kSuccess; } @@ -515,8 +514,9 @@ public: if (fColorXform) { if (fColorXform) { - fColorXform->apply(dst, (const uint32_t*) swizzlerDstRow, dstInfo.width(), - dstInfo.colorType(), xformAlphaType); + fColorXform->apply(dst, (const uint32_t*) swizzlerDstRow, + fSwizzler->swizzleWidth(), dstInfo.colorType(), + xformAlphaType); dst = SkTAddOffset(dst, rowBytes); } } @@ -865,7 +865,7 @@ SkCodec::Result SkPngCodec::onGetPixels(const SkImageInfo& dstInfo, void* dst, return kUnimplemented; } - this->allocateStorage(dstInfo); + this->allocateStorage(); int count = this->readRows(dstInfo, dst, rowBytes, dstInfo.height(), 0); if (count > dstInfo.height()) { *rowsDecoded = count; diff --git a/src/codec/SkPngCodec.h b/src/codec/SkPngCodec.h index b689f6fbae..9d97c719da 100644 --- a/src/codec/SkPngCodec.h +++ b/src/codec/SkPngCodec.h @@ -41,7 +41,7 @@ protected: SkASSERT(fSwizzler); return fSwizzler; } - void allocateStorage(const SkImageInfo& dstInfo); + void allocateStorage(); virtual int readRows(const SkImageInfo& dstInfo, void* dst, size_t rowBytes, int count, int startRow) = 0; diff --git a/src/codec/SkSwizzler.h b/src/codec/SkSwizzler.h index 4845047048..6172af5c43 100644 --- a/src/codec/SkSwizzler.h +++ b/src/codec/SkSwizzler.h @@ -72,6 +72,12 @@ public: */ int sampleX() const { return fSampleX; } + /** + * Returns the actual number of pixels written to destination memory, taking + * scaling, subsetting, and partial frames into account. + */ + int swizzleWidth() const { return fSwizzleWidth; } + private: /** diff --git a/tests/CodecTest.cpp b/tests/CodecTest.cpp index a6b44eb9a7..8023ff219d 100644 --- a/tests/CodecTest.cpp +++ b/tests/CodecTest.cpp @@ -1044,6 +1044,36 @@ DEF_TEST(Codec_jpeg_rewind, r) { REPORTER_ASSERT(r, SkCodec::kSuccess == result); } +static void check_color_xform(skiatest::Reporter* r, const char* path) { + SkAutoTDelete codec(SkAndroidCodec::NewFromStream(resource(path))); + + SkAndroidCodec::AndroidOptions opts; + opts.fSampleSize = 3; + const int subsetWidth = codec->getInfo().width() / 2; + const int subsetHeight = codec->getInfo().height() / 2; + SkIRect subset = SkIRect::MakeWH(subsetWidth, subsetHeight); + opts.fSubset = ⊂ + + const int dstWidth = subsetWidth / opts.fSampleSize; + const int dstHeight = subsetHeight / opts.fSampleSize; + sk_sp data = SkData::MakeFromFileName( + GetResourcePath("icc_profiles/HP_ZR30w.icc").c_str()); + sk_sp colorSpace = SkColorSpace::NewICC(data->data(), data->size()); + SkImageInfo dstInfo = codec->getInfo().makeWH(dstWidth, dstHeight) + .makeColorType(kN32_SkColorType) + .makeColorSpace(colorSpace); + + size_t rowBytes = dstInfo.minRowBytes(); + SkAutoMalloc pixelStorage(dstInfo.getSafeSize(rowBytes)); + SkCodec::Result result = codec->getAndroidPixels(dstInfo, pixelStorage.get(), rowBytes, &opts); + REPORTER_ASSERT(r, SkCodec::kSuccess == result); +} + +DEF_TEST(Codec_ColorXform, r) { + check_color_xform(r, "mandrill_512_q075.jpg"); + check_color_xform(r, "mandrill_512.png"); +} + DEF_TEST(Codec_Png565, r) { // Create an arbitrary 565 bitmap. const char* path = "mandrill_512_q075.jpg";