From c06f309cf52b885b1b1d98c6b045b120a09f5c54 Mon Sep 17 00:00:00 2001 From: Leon Scroggins Date: Tue, 2 May 2017 17:08:28 +0000 Subject: Revert "Add support for row-by-row jpeg encoding" This reverts commit 9b848d5749c5e34b56f927a3a3374c8ebafbd9db. Reason for revert: ASAN reports leaked memory [1]. Google3 reports a "delete size mismatch" [2], which I suspect is the same issue. [1] https://chromium-swarm.appspot.com/task?id=35e2c9fa9eac6310&refresh=10&show_raw=1 [2] https://test.corp.google.com/ui#cl=154838904&flags=CAMQBQ==&id=OCL:154838904:BASE:154839043:1493741642370:9c96115f&t=//chrome/skia/dm_wrapper:dm_wrapper Original change's description: > Add support for row-by-row jpeg encoding > > Bug: 713862 > Change-Id: I787b7c49662a00b89ae0ef35845dfbd6be3e6fb1 > Reviewed-on: https://skia-review.googlesource.com/14641 > Commit-Queue: Matt Sarett > Reviewed-by: Leon Scroggins > TBR=msarett@google.com,scroggo@google.com,reed@google.com NOPRESUBMIT=true NOTREECHECKS=true NOTRY=true Change-Id: Ic5a8d67e0d4a7733662586055ceff086a2ab335d Reviewed-on: https://skia-review.googlesource.com/15140 Reviewed-by: Leon Scroggins Commit-Queue: Leon Scroggins --- src/images/SkImageEncoder.cpp | 8 +- src/images/SkImageEncoderPriv.h | 7 + src/images/SkJPEGImageEncoder.cpp | 268 +++++++++++++++----------------------- src/images/SkJpegEncoder.h | 59 --------- 4 files changed, 112 insertions(+), 230 deletions(-) delete mode 100644 src/images/SkJpegEncoder.h (limited to 'src/images') diff --git a/src/images/SkImageEncoder.cpp b/src/images/SkImageEncoder.cpp index 23f46e4982..fecadbf82a 100644 --- a/src/images/SkImageEncoder.cpp +++ b/src/images/SkImageEncoder.cpp @@ -6,7 +6,6 @@ */ #include "SkImageEncoderPriv.h" -#include "SkJpegEncoder.h" bool SkEncodeImage(SkWStream* dst, const SkPixmap& src, SkEncodedImageFormat format, int quality) { @@ -17,11 +16,8 @@ bool SkEncodeImage(SkWStream* dst, const SkPixmap& src, return SkEncodeImageWithWIC(dst, src, format, quality); #else switch(format) { - case SkEncodedImageFormat::kJPEG: { - SkJpegEncoder::Options opts; - opts.fQuality = quality; - return SkJpegEncoder::Encode(dst, src, opts); - } + case SkEncodedImageFormat::kJPEG: + return SkEncodeImageAsJPEG(dst, src, quality); case SkEncodedImageFormat::kPNG: return SkEncodeImageAsPNG(dst, src, SkEncodeOptions()); case SkEncodedImageFormat::kWEBP: diff --git a/src/images/SkImageEncoderPriv.h b/src/images/SkImageEncoderPriv.h index 5e57cd128a..540d93093b 100644 --- a/src/images/SkImageEncoderPriv.h +++ b/src/images/SkImageEncoderPriv.h @@ -14,6 +14,13 @@ struct SkEncodeOptions { SkTransferFunctionBehavior fUnpremulBehavior = SkTransferFunctionBehavior::kIgnore; }; +#ifdef SK_HAS_JPEG_LIBRARY + bool SkEncodeImageAsJPEG(SkWStream*, const SkPixmap&, const SkEncodeOptions&); + bool SkEncodeImageAsJPEG(SkWStream*, const SkPixmap&, int quality); +#else + #define SkEncodeImageAsJPEG(...) false +#endif + #ifdef SK_HAS_PNG_LIBRARY bool SkEncodeImageAsPNG(SkWStream*, const SkPixmap&, const SkEncodeOptions&); #else diff --git a/src/images/SkJPEGImageEncoder.cpp b/src/images/SkJPEGImageEncoder.cpp index b875898fc9..764e9c43b0 100644 --- a/src/images/SkJPEGImageEncoder.cpp +++ b/src/images/SkJPEGImageEncoder.cpp @@ -11,8 +11,6 @@ #include "SkColorPriv.h" #include "SkImageEncoderFns.h" -#include "SkImageInfoPriv.h" -#include "SkJpegEncoder.h" #include "SkJPEGWriteUtility.h" #include "SkStream.h" #include "SkTemplates.h" @@ -24,144 +22,122 @@ extern "C" { #include "jerror.h" } -// This warning triggers false postives way too often in here. -#if defined(__GNUC__) && !defined(__clang__) - #pragma GCC diagnostic ignored "-Wclobbered" -#endif - -class SkJpegEncoderMgr : SkNoncopyable { -public: - - /* - * Create the decode manager - * Does not take ownership of stream - */ - static std::unique_ptr Make(SkWStream* stream) { - return std::unique_ptr(new SkJpegEncoderMgr(stream)); - } - - bool setParams(const SkImageInfo& srcInfo); - - jpeg_compress_struct* cinfo() { return &fCInfo; } - - jmp_buf& jmpBuf() { return fErrMgr.fJmpBuf; } - - transform_scanline_proc proc() const { return fProc; } - - ~SkJpegEncoderMgr() { - jpeg_destroy_compress(&fCInfo); - } - -private: - - SkJpegEncoderMgr(SkWStream* stream) - : fDstMgr(stream) - , fProc(nullptr) - { - fCInfo.err = jpeg_std_error(&fErrMgr); - fErrMgr.error_exit = skjpeg_error_exit; - jpeg_create_compress(&fCInfo); - fCInfo.dest = &fDstMgr; - } - - jpeg_compress_struct fCInfo; - skjpeg_error_mgr fErrMgr; - skjpeg_destination_mgr fDstMgr; - transform_scanline_proc fProc; -}; - -bool SkJpegEncoderMgr::setParams(const SkImageInfo& srcInfo) { - J_COLOR_SPACE jpegColorType = JCS_EXT_RGBA; - int numComponents = 0; - switch (srcInfo.colorType()) { +/** + * Returns true if |info| is supported by the jpeg encoder and false otherwise. + * |jpegColorType| will be set to the proper libjpeg-turbo type for input to the library. + * |numComponents| will be set to the number of components in the |jpegColorType|. + * |proc| will be set if we need to pre-convert the input before passing to + * libjpeg-turbo. Otherwise will be set to nullptr. + */ +// TODO (skbug.com/1501): +// Should we fail on non-opaque encodes? +// Or should we change alpha behavior (ex: unpremultiply when the input is premul)? +// Or is ignoring the alpha type and alpha channel ok here? +static bool set_encode_config(J_COLOR_SPACE* jpegColorType, int* numComponents, + transform_scanline_proc* proc, const SkImageInfo& info) { + *proc = nullptr; + switch (info.colorType()) { case kRGBA_8888_SkColorType: - jpegColorType = JCS_EXT_RGBA; - numComponents = 4; - break; + *jpegColorType = JCS_EXT_RGBA; + *numComponents = 4; + return true; case kBGRA_8888_SkColorType: - jpegColorType = JCS_EXT_BGRA; - numComponents = 4; - break; + *jpegColorType = JCS_EXT_BGRA; + *numComponents = 4; + return true; case kRGB_565_SkColorType: - fProc = transform_scanline_565; - jpegColorType = JCS_RGB; - numComponents = 3; - break; + *proc = transform_scanline_565; + *jpegColorType = JCS_RGB; + *numComponents = 3; + return true; case kARGB_4444_SkColorType: - fProc = transform_scanline_444; - jpegColorType = JCS_RGB; - numComponents = 3; - break; + *proc = transform_scanline_444; + *jpegColorType = JCS_RGB; + *numComponents = 3; + return true; case kIndex_8_SkColorType: - fProc = transform_scanline_index8_opaque; - jpegColorType = JCS_RGB; - numComponents = 3; - break; + *proc = transform_scanline_index8_opaque; + *jpegColorType = JCS_RGB; + *numComponents = 3; + return true; case kGray_8_SkColorType: - SkASSERT(srcInfo.isOpaque()); - jpegColorType = JCS_GRAYSCALE; - numComponents = 1; - break; + SkASSERT(info.isOpaque()); + *jpegColorType = JCS_GRAYSCALE; + *numComponents = 1; + return true; case kRGBA_F16_SkColorType: - if (!srcInfo.colorSpace() || !srcInfo.colorSpace()->gammaIsLinear()) { + if (!info.colorSpace() || !info.colorSpace()->gammaIsLinear()) { return false; } - fProc = transform_scanline_F16_to_8888; - jpegColorType = JCS_EXT_RGBA; - numComponents = 4; - break; + *proc = transform_scanline_F16_to_8888; + *jpegColorType = JCS_EXT_RGBA; + *numComponents = 4; + return true; default: return false; } - fCInfo.image_width = srcInfo.width(); - fCInfo.image_height = srcInfo.height(); - fCInfo.in_color_space = jpegColorType; - fCInfo.input_components = numComponents; - jpeg_set_defaults(&fCInfo); - // Tells libjpeg-turbo to compute optimal Huffman coding tables - // for the image. This improves compression at the cost of - // slower encode performance. - fCInfo.optimize_coding = TRUE; - return true; } -class SkJpegEncoder_Base : public SkJpegEncoder { -public: - SkJpegEncoder_Base(std::unique_ptr encoderMgr, const SkPixmap& src); - - bool onEncodeRows(int numRows); +bool SkEncodeImageAsJPEG(SkWStream* stream, const SkPixmap& pixmap, const SkEncodeOptions& opts) { + if (SkTransferFunctionBehavior::kRespect == opts.fUnpremulBehavior) { + // Respecting the transfer function requries a color space. It's not actually critical + // in the jpeg case (since jpegs are opaque), but Skia color correct behavior generally + // requires pixels to be tagged with color spaces. + if (!pixmap.colorSpace() || (!pixmap.colorSpace()->gammaCloseToSRGB() && + !pixmap.colorSpace()->gammaIsLinear())) { + return false; + } + } -private: - std::unique_ptr fEncoderMgr; - SkPixmap fSrc; - int fCurrRow; - SkAutoTMalloc fStorage; -}; + return SkEncodeImageAsJPEG(stream, pixmap, 100); +} -std::unique_ptr SkJpegEncoder::Make(SkWStream* dst, const SkPixmap& src, - const Options& options) { - if (!SkImageInfoIsValidAllowNumericalCS(src.info()) || !src.addr() || - src.rowBytes() < src.info().minRowBytes()) { - return nullptr; +bool SkEncodeImageAsJPEG(SkWStream* stream, const SkPixmap& pixmap, int quality) { + if (!pixmap.addr()) { + return false; } + jpeg_compress_struct cinfo; + skjpeg_error_mgr sk_err; + skjpeg_destination_mgr sk_wstream(stream); - std::unique_ptr encoderMgr = SkJpegEncoderMgr::Make(dst); - if (setjmp(encoderMgr->jmpBuf())) { - return nullptr; + // Declare before calling setjmp. + SkAutoTMalloc storage; + + cinfo.err = jpeg_std_error(&sk_err); + sk_err.error_exit = skjpeg_error_exit; + if (setjmp(sk_err.fJmpBuf)) { + return false; } - if (!encoderMgr->setParams(src.info())) { - return nullptr; + J_COLOR_SPACE jpegColorSpace; + int numComponents; + transform_scanline_proc proc; + if (!set_encode_config(&jpegColorSpace, &numComponents, &proc, pixmap.info())) { + return false; } - jpeg_set_quality(encoderMgr->cinfo(), options.fQuality, TRUE); - jpeg_start_compress(encoderMgr->cinfo(), TRUE); + jpeg_create_compress(&cinfo); + cinfo.dest = &sk_wstream; + cinfo.image_width = pixmap.width(); + cinfo.image_height = pixmap.height(); + cinfo.input_components = numComponents; + cinfo.in_color_space = jpegColorSpace; + + jpeg_set_defaults(&cinfo); - if (src.colorSpace()) { - sk_sp icc = icc_from_color_space(*src.colorSpace()); + // Tells libjpeg-turbo to compute optimal Huffman coding tables + // for the image. This improves compression at the cost of + // slower encode performance. + cinfo.optimize_coding = TRUE; + jpeg_set_quality(&cinfo, quality, TRUE /* limit to baseline-JPEG values */); + + jpeg_start_compress(&cinfo, TRUE); + + if (pixmap.colorSpace()) { + sk_sp icc = icc_from_color_space(*pixmap.colorSpace()); if (icc) { // Create a contiguous block of memory with the icc signature followed by the profile. sk_sp markerData = @@ -173,68 +149,30 @@ std::unique_ptr SkJpegEncoder::Make(SkWStream* dst, const SkPixma *ptr++ = 1; // Out of one total markers. memcpy(ptr, icc->data(), icc->size()); - jpeg_write_marker(encoderMgr->cinfo(), kICCMarker, markerData->bytes(), - markerData->size()); + jpeg_write_marker(&cinfo, kICCMarker, markerData->bytes(), markerData->size()); } } - return std::unique_ptr(new SkJpegEncoder_Base(std::move(encoderMgr), src)); -} - - -SkJpegEncoder_Base::SkJpegEncoder_Base(std::unique_ptr encoderMgr, - const SkPixmap& src) - : fEncoderMgr(std::move(encoderMgr)) - , fSrc(src) - , fCurrRow(0) - , fStorage(fEncoderMgr->proc() ? fEncoderMgr->cinfo()->input_components*src.width() : 0) -{} - -bool SkJpegEncoder::encodeRows(int numRows) { - return ((SkJpegEncoder_Base*) this)->onEncodeRows(numRows); -} - -bool SkJpegEncoder_Base::onEncodeRows(int numRows) { - SkASSERT(numRows > 0 && fCurrRow < fSrc.height()); - if (numRows <= 0 || fCurrRow >= fSrc.height()) { - return false; - } - - if (fCurrRow + numRows > fSrc.height()) { - numRows = fSrc.height() - fCurrRow; - } - - if (setjmp(fEncoderMgr->jmpBuf())) { - // Short circuit any future calls after failing. - fCurrRow = fSrc.height(); - return false; + if (proc) { + storage.reset(numComponents * pixmap.width()); } - const void* srcRow = fSrc.addr(0, fCurrRow); - const SkPMColor* colors = fSrc.ctable() ? fSrc.ctable()->readColors() : nullptr; - for (int i = 0; i < numRows; i++) { + const void* srcRow = pixmap.addr(); + const SkPMColor* colors = pixmap.ctable() ? pixmap.ctable()->readColors() : nullptr; + while (cinfo.next_scanline < cinfo.image_height) { JSAMPLE* jpegSrcRow = (JSAMPLE*) srcRow; - if (fEncoderMgr->proc()) { - fEncoderMgr->proc()((char*)fStorage.get(), (const char*)srcRow, fSrc.width(), - fEncoderMgr->cinfo()->input_components, colors); - jpegSrcRow = fStorage.get(); + if (proc) { + proc((char*)storage.get(), (const char*)srcRow, pixmap.width(), numComponents, colors); + jpegSrcRow = storage.get(); } - jpeg_write_scanlines(fEncoderMgr->cinfo(), &jpegSrcRow, 1); - srcRow = SkTAddOffset(srcRow, fSrc.rowBytes()); + (void) jpeg_write_scanlines(&cinfo, &jpegSrcRow, 1); + srcRow = SkTAddOffset(srcRow, pixmap.rowBytes()); } - fCurrRow += numRows; - if (fCurrRow == fSrc.height()) { - jpeg_finish_compress(fEncoderMgr->cinfo()); - } + jpeg_finish_compress(&cinfo); + jpeg_destroy_compress(&cinfo); return true; } - -bool SkJpegEncoder::Encode(SkWStream* dst, const SkPixmap& src, const Options& options) { - auto encoder = SkJpegEncoder::Make(dst, src, options); - return encoder && encoder->encodeRows(src.height()); -} - #endif diff --git a/src/images/SkJpegEncoder.h b/src/images/SkJpegEncoder.h deleted file mode 100644 index 0c41acc8fe..0000000000 --- a/src/images/SkJpegEncoder.h +++ /dev/null @@ -1,59 +0,0 @@ -/* - * Copyright 2017 Google Inc. - * - * Use of this source code is governed by a BSD-style license that can be - * found in the LICENSE file. - */ - -#ifndef SkJpegEncoder_DEFINED -#define SkJpegEncoder_DEFINED - -#include "SkPixmap.h" -#include "SkTypes.h" - -class SkWStream; - -class SkJpegEncoder : SkNoncopyable { -public: - - // TODO (skbug.com/1501): - // Since jpegs are always opaque, this encoder ignores the alpha channel and treats the - // pixels as opaque. - // Another possible behavior is to blend the pixels onto opaque black. We'll need to add - // an option for this - and an SkTransferFunctionBehavior. - - struct Options { - /** - * |fQuality| must be in [0, 100] where 0 corresponds to the lowest quality. - */ - int fQuality = 100; - }; - - /** - * Encode the |src| pixels to the |dst| stream. - * |options| may be used to control the encoding behavior. - * - * Returns true on success. Returns false on an invalid or unsupported |src|. - */ - static bool Encode(SkWStream* dst, const SkPixmap& src, const Options& options); - - /** - * Create a jpeg encoder that will encode the |src| pixels to the |dst| stream. - * |options| may be used to control the encoding behavior. - * - * |dst| is unowned but must remain valid for the lifetime of the object. - * - * This returns nullptr on an invalid or unsupported |src|. - */ - static std::unique_ptr Make(SkWStream* dst, const SkPixmap& src, - const Options& options); - - /** - * Encode |numRows| rows of input. If the caller requests more rows than are remaining - * in the src, this will encode all of the remaining rows. |numRows| must be greater - * than zero. - */ - bool encodeRows(int numRows); -}; - -#endif -- cgit v1.2.3