Beginning work to refactor jpeg tile decoding.

Keep common code together, so we can fix bugs in tile and normal decode
simultaneously.

Convert if-then chains to switch statements for readability.

Add getBitmapConfig, common to both normal and tile decode, which
ensures that they behave the same. getBitmapConfig uses the code
originally in onDecode, so subsetting grayscale into A8 now works.

In getBitmapConfig, handle JCS_YCCK properly.

Fix a bug where requesting A8 from a JCS_CMYK image would
result in a total failure to decode, since we would change
out_color_space to an invalid choice.

Factor common code for applying dither and changing the
out_color_space into apply_dither_mode (final name TBD). Skips
CMYK like normal decoding did before.

BUG=skia:1472
BUG=https://b.corp.google.com/issue?id=9466275
BUG=https://b.corp.google.com/issue?id=9189955
R=mtklein@google.com

Review URL: https://codereview.chromium.org/22290002

git-svn-id: http://skia.googlecode.com/svn/trunk@10629 2bbb7eff-a529-9590-31e7-b0007b416f81

1 files changed, 182 insertions, 104 deletions

diff --git a/src/images/SkImageDecoder_libjpeg.cpp b/src/images/SkImageDecoder_libjpeg.cpp
index 914ceb7e8a..788e3526d2 100644
--- a/src/images/SkImageDecoder_libjpeg.cpp
+++ b/src/images/SkImageDecoder_libjpeg.cpp
@@ -30,7 +30,6 @@ extern "C" {
 //#define TIME_DECODE
 
 // this enables our rgb->yuv code, which is faster than libjpeg on ARM
-// disable for the moment, as we have some glitches when width != multiple of 4
 #define WE_CONVERT_TO_YUV
 
 // If ANDROID_RGB is defined by in the jpeg headers it indicates that jpeg offers
@@ -39,7 +38,7 @@ extern "C" {
 //////////////////////////////////////////////////////////////////////////
 //////////////////////////////////////////////////////////////////////////
 
-static void overwrite_mem_buffer_size(j_decompress_ptr cinfo) {
+static void overwrite_mem_buffer_size(jpeg_decompress_struct* cinfo) {
 #ifdef SK_BUILD_FOR_ANDROID
     /* Check if the device indicates that it has a large amount of system memory
      * if so, increase the memory allocation to 30MB instead of the default 5MB.
@@ -55,6 +54,14 @@ static void overwrite_mem_buffer_size(j_decompress_ptr cinfo) {
 //////////////////////////////////////////////////////////////////////////
 //////////////////////////////////////////////////////////////////////////
 
+static void initialize_info(jpeg_decompress_struct* cinfo, skjpeg_source_mgr* src_mgr) {
+    SkASSERT(cinfo != NULL);
+    SkASSERT(src_mgr != NULL);
+    jpeg_create_decompress(cinfo);
+    overwrite_mem_buffer_size(cinfo);
+    cinfo->src = src_mgr;
+}
+
 #ifdef SK_BUILD_FOR_ANDROID
 class SkJPEGImageIndex {
 public:
@@ -69,10 +76,17 @@ public:
 
     ~SkJPEGImageIndex() {
         if (fHuffmanCreated) {
+            // Set to false before calling the libjpeg function, in case
+            // the libjpeg function calls longjmp. Our setjmp handler may
+            // attempt to delete this SkJPEGImageIndex, thus entering this
+            // destructor again. Setting fHuffmanCreated to false first
+            // prevents an infinite loop.
             fHuffmanCreated = false;
             jpeg_destroy_huffman_index(&fHuffmanIndex);
         }
         if (fDecompressStarted) {
+            // Like fHuffmanCreated, set to false before calling libjpeg
+            // function to prevent potential infinite loop.
             fDecompressStarted = false;
             jpeg_finish_decompress(&fCInfo);
         }
@@ -91,6 +105,8 @@ public:
     void destroyInfo() {
         SkASSERT(fInfoInitialized);
         SkASSERT(!fDecompressStarted);
+        // Like fHuffmanCreated, set to false before calling libjpeg
+        // function to prevent potential infinite loop.
         fInfoInitialized = false;
         jpeg_destroy_decompress(&fCInfo);
         SkDEBUGCODE(fReadHeaderSucceeded = false;)
@@ -106,9 +122,7 @@ public:
      */
     bool initializeInfoAndReadHeader() {
         SkASSERT(!fInfoInitialized && !fDecompressStarted);
-        jpeg_create_decompress(&fCInfo);
-        overwrite_mem_buffer_size(&fCInfo);
-        fCInfo.src = &fSrcMgr;
+        initialize_info(&fCInfo, &fSrcMgr);
         fInfoInitialized = true;
         const bool success = (JPEG_HEADER_OK == jpeg_read_header(&fCInfo, true));
         SkDEBUGCODE(fReadHeaderSucceeded = success;)
@@ -193,6 +207,14 @@ private:
     int fImageHeight;
 #endif
 
+    /**
+     *  Determine the appropriate bitmap config and out_color_space based on
+     *  both the preference of the caller and the jpeg_color_space on the
+     *  jpeg_decompress_struct passed in.
+     *  Must be called after jpeg_read_header.
+     */
+    SkBitmap::Config getBitmapConfig(jpeg_decompress_struct*);
+
     typedef SkImageDecoder INHERITED;
 };
 
@@ -295,6 +317,131 @@ static void convert_CMYK_to_RGB(uint8_t* scanline, unsigned int width) {
     }
 }
 
+/**
+ *  Common code for setting the error manager.
+ */
+static void set_error_mgr(jpeg_decompress_struct* cinfo, skjpeg_error_mgr* errorManager) {
+    SkASSERT(cinfo != NULL);
+    SkASSERT(errorManager != NULL);
+    cinfo->err = jpeg_std_error(errorManager);
+    errorManager->error_exit = skjpeg_error_exit;
+}
+
+/**
+ *  Common code for turning off upsampling and smoothing. Turning these
+ *  off helps performance without showing noticable differences in the
+ *  resulting bitmap.
+ */
+static void turn_off_visual_optimizations(jpeg_decompress_struct* cinfo) {
+    SkASSERT(cinfo != NULL);
+    /* this gives about 30% performance improvement. In theory it may
+       reduce the visual quality, in practice I'm not seeing a difference
+     */
+    cinfo->do_fancy_upsampling = 0;
+
+    /* this gives another few percents */
+    cinfo->do_block_smoothing = 0;
+}
+
+/**
+ * Common code for setting the dct method.
+ */
+static void set_dct_method(const SkImageDecoder& decoder, jpeg_decompress_struct* cinfo) {
+    SkASSERT(cinfo != NULL);
+#ifdef DCT_IFAST_SUPPORTED
+    if (decoder.getPreferQualityOverSpeed()) {
+        cinfo->dct_method = JDCT_ISLOW;
+    } else {
+        cinfo->dct_method = JDCT_IFAST;
+    }
+#else
+    cinfo->dct_method = JDCT_ISLOW;
+#endif
+}
+
+SkBitmap::Config SkJPEGImageDecoder::getBitmapConfig(jpeg_decompress_struct* cinfo) {
+    SkASSERT(cinfo != NULL);
+
+    SrcDepth srcDepth = k32Bit_SrcDepth;
+    if (JCS_GRAYSCALE == cinfo->jpeg_color_space) {
+        srcDepth = k8BitGray_SrcDepth;
+    }
+
+    SkBitmap::Config config = this->getPrefConfig(srcDepth, /*hasAlpha*/ false);
+    switch (config) {
+        case SkBitmap::kA8_Config:
+            // Only respect A8 config if the original is grayscale,
+            // in which case we will treat the grayscale as alpha
+            // values.
+            if (cinfo->jpeg_color_space != JCS_GRAYSCALE) {
+                config = SkBitmap::kARGB_8888_Config;
+            }
+            break;
+        case SkBitmap::kARGB_8888_Config:
+            // Fall through.
+        case SkBitmap::kARGB_4444_Config:
+            // Fall through.
+        case SkBitmap::kRGB_565_Config:
+            // These are acceptable destination configs.
+            break;
+        default:
+            // Force all other configs to 8888.
+            config = SkBitmap::kARGB_8888_Config;
+            break;
+    }
+
+    switch (cinfo->jpeg_color_space) {
+        case JCS_CMYK:
+            // Fall through.
+        case JCS_YCCK:
+            // libjpeg cannot convert from CMYK or YCCK to RGB - here we set up
+            // so libjpeg will give us CMYK samples back and we will later
+            // manually convert them to RGB
+            cinfo->out_color_space = JCS_CMYK;
+            break;
+        case JCS_GRAYSCALE:
+            if (SkBitmap::kA8_Config == config) {
+                cinfo->out_color_space = JCS_GRAYSCALE;
+                break;
+            }
+            // The data is JCS_GRAYSCALE, but the caller wants some sort of RGB
+            // config. Fall through to set to the default.
+        default:
+            cinfo->out_color_space = JCS_RGB;
+            break;
+    }
+    return config;
+}
+
+#ifdef ANDROID_RGB
+/**
+ *  Based on the config and dither mode, adjust out_color_space and
+ *  dither_mode of cinfo.
+ */
+static void adjust_out_color_space_and_dither(jpeg_decompress_struct* cinfo,
+                                              SkBitmap::Config config,
+                                              const SkImageDecoder& decoder) {
+    SkASSERT(cinfo != NULL);
+    cinfo->dither_mode = JDITHER_NONE;
+    if (JCS_CMYK == cinfo->out_color_space) {
+        return;
+    }
+    switch(config) {
+        case SkBitmap::kARGB_8888_Config:
+            cinfo->out_color_space = JCS_RGBA_8888;
+            break;
+        case SkBitmap::kRGB_565_Config:
+            cinfo->out_color_space = JCS_RGB_565;
+            if (decoder.getDitherImage()) {
+                cinfo->dither_mode = JDITHER_ORDERED;
+            }
+            break;
+        default:
+            break;
+    }
+}
+#endif
+
 bool SkJPEGImageDecoder::onDecode(SkStream* stream, SkBitmap* bm, Mode mode) {
 #ifdef TIME_DECODE
     SkAutoTime atm("JPEG Decode");
@@ -303,11 +450,10 @@ bool SkJPEGImageDecoder::onDecode(SkStream* stream, SkBitmap* bm, Mode mode) {
     JPEGAutoClean autoClean;
 
     jpeg_decompress_struct  cinfo;
-    skjpeg_error_mgr        errorManager;
     skjpeg_source_mgr       srcManager(stream, this);
 
-    cinfo.err = jpeg_std_error(&errorManager);
-    errorManager.error_exit = skjpeg_error_exit;
+    skjpeg_error_mgr errorManager;
+    set_error_mgr(&cinfo, &errorManager);
 
     // All objects need to be instantiated before this setjmp call so that
     // they will be cleaned up properly if an error occurs.
@@ -315,14 +461,9 @@ bool SkJPEGImageDecoder::onDecode(SkStream* stream, SkBitmap* bm, Mode mode) {
         return return_false(cinfo, *bm, "setjmp");
     }
 
-    jpeg_create_decompress(&cinfo);
+    initialize_info(&cinfo, &srcManager);
     autoClean.set(&cinfo);
 
-    overwrite_mem_buffer_size(&cinfo);
-
-    //jpeg_stdio_src(&cinfo, file);
-    cinfo.src = &srcManager;
-
     int status = jpeg_read_header(&cinfo, true);
     if (status != JPEG_HEADER_OK) {
         return return_false(cinfo, *bm, "read_header");
@@ -334,67 +475,17 @@ bool SkJPEGImageDecoder::onDecode(SkStream* stream, SkBitmap* bm, Mode mode) {
     */
     int sampleSize = this->getSampleSize();
 
-#ifdef DCT_IFAST_SUPPORTED
-    if (this->getPreferQualityOverSpeed()) {
-        cinfo.dct_method = JDCT_ISLOW;
-    } else {
-        cinfo.dct_method = JDCT_IFAST;
-    }
-#else
-    cinfo.dct_method = JDCT_ISLOW;
-#endif
+    set_dct_method(*this, &cinfo);
 
-    cinfo.scale_num = 1;
+    SkASSERT(1 == cinfo.scale_num);
     cinfo.scale_denom = sampleSize;
 
-    /* this gives about 30% performance improvement. In theory it may
-       reduce the visual quality, in practice I'm not seeing a difference
-     */
-    cinfo.do_fancy_upsampling = 0;
-
-    /* this gives another few percents */
-    cinfo.do_block_smoothing = 0;
-
-    SrcDepth srcDepth = k32Bit_SrcDepth;
-    /* default format is RGB */
-    if (cinfo.jpeg_color_space == JCS_CMYK) {
-        // libjpeg cannot convert from CMYK to RGB - here we set up
-        // so libjpeg will give us CMYK samples back and we will
-        // later manually convert them to RGB
-        cinfo.out_color_space = JCS_CMYK;
-    } else if (cinfo.jpeg_color_space == JCS_GRAYSCALE) {
-        cinfo.out_color_space = JCS_GRAYSCALE;
-        srcDepth = k8BitGray_SrcDepth;
-    } else {
-        cinfo.out_color_space = JCS_RGB;
-    }
+    turn_off_visual_optimizations(&cinfo);
 
-    SkBitmap::Config config = this->getPrefConfig(srcDepth, false);
-    // only these make sense for jpegs
-    if (SkBitmap::kA8_Config == config) {
-        if (cinfo.jpeg_color_space != JCS_GRAYSCALE) {
-            // Converting from a non grayscale image to A8 is
-            // not currently supported.
-            config = SkBitmap::kARGB_8888_Config;
-            // Change the output from jpeg back to RGB.
-            cinfo.out_color_space = JCS_RGB;
-        }
-    } else if (config != SkBitmap::kARGB_8888_Config &&
-               config != SkBitmap::kARGB_4444_Config &&
-               config != SkBitmap::kRGB_565_Config) {
-        config = SkBitmap::kARGB_8888_Config;
-    }
+    const SkBitmap::Config config = this->getBitmapConfig(&cinfo);
 
 #ifdef ANDROID_RGB
-    cinfo.dither_mode = JDITHER_NONE;
-    if (SkBitmap::kARGB_8888_Config == config && JCS_CMYK != cinfo.out_color_space) {
-        cinfo.out_color_space = JCS_RGBA_8888;
-    } else if (SkBitmap::kRGB_565_Config == config && JCS_CMYK != cinfo.out_color_space) {
-        cinfo.out_color_space = JCS_RGB_565;
-        if (this->getDitherImage()) {
-            cinfo.dither_mode = JDITHER_ORDERED;
-        }
-    }
+    adjust_out_color_space_and_dither(&cinfo, config, *this);
 #endif
 
     if (1 == sampleSize && SkImageDecoder::kDecodeBounds_Mode == mode) {
@@ -552,8 +643,7 @@ bool SkJPEGImageDecoder::onBuildTileIndex(SkStream* stream, int *width, int *hei
     jpeg_decompress_struct* cinfo = imageIndex->cinfo();
 
     skjpeg_error_mgr sk_err;
-    cinfo->err = jpeg_std_error(&sk_err);
-    sk_err.error_exit = skjpeg_error_exit;
+    set_error_mgr(cinfo, &sk_err);
 
     // All objects need to be instantiated before this setjmp call so that
     // they will be cleaned up properly if an error occurs.
@@ -578,9 +668,13 @@ bool SkJPEGImageDecoder::onBuildTileIndex(SkStream* stream, int *width, int *hei
         return false;
     }
 
-    cinfo->out_color_space = JCS_RGBA_8888;
-    cinfo->do_fancy_upsampling = 0;
-    cinfo->do_block_smoothing = 0;
+    // FIXME: This sets cinfo->out_color_space, which we may change later
+    // based on the config in onDecodeSubset. This should be fine, since
+    // jpeg_init_read_tile_scanline will check out_color_space again after
+    // that change (when it calls jinit_color_deconverter).
+    (void) this->getBitmapConfig(cinfo);
+
+    turn_off_visual_optimizations(cinfo);
 
     // instead of jpeg_start_decompress() we start a tiled decompress
     if (!imageIndex->startTileDecompress()) {
@@ -588,10 +682,15 @@ bool SkJPEGImageDecoder::onBuildTileIndex(SkStream* stream, int *width, int *hei
     }
 
     SkASSERT(1 == cinfo->scale_num);
-    *height = cinfo->output_height;
-    *width = cinfo->output_width;
-    fImageWidth = *width;
-    fImageHeight = *height;
+    fImageWidth = cinfo->output_width;
+    fImageHeight = cinfo->output_height;
+
+    if (width) {
+        *width = fImageWidth;
+    }
+    if (height) {
+        *height = fImageHeight;
+    }
 
     SkDELETE(fImageIndex);
     fImageIndex = imageIndex.detach();
@@ -613,8 +712,8 @@ bool SkJPEGImageDecoder::onDecodeSubset(SkBitmap* bm, const SkIRect& region) {
 
 
     skjpeg_error_mgr errorManager;
-    cinfo->err = jpeg_std_error(&errorManager);
-    errorManager.error_exit = skjpeg_error_exit;
+    set_error_mgr(cinfo, &errorManager);
+
     if (setjmp(errorManager.fJmpBuf)) {
         return false;
     }
@@ -622,32 +721,11 @@ bool SkJPEGImageDecoder::onDecodeSubset(SkBitmap* bm, const SkIRect& region) {
     int requestedSampleSize = this->getSampleSize();
     cinfo->scale_denom = requestedSampleSize;
 
-    if (this->getPreferQualityOverSpeed()) {
-        cinfo->dct_method = JDCT_ISLOW;
-    } else {
-        cinfo->dct_method = JDCT_IFAST;
-    }
-
-    SkBitmap::Config config = this->getPrefConfig(k32Bit_SrcDepth, false);
-    if (config != SkBitmap::kARGB_8888_Config &&
-        config != SkBitmap::kARGB_4444_Config &&
-        config != SkBitmap::kRGB_565_Config) {
-        config = SkBitmap::kARGB_8888_Config;
-    }
-
-    /* default format is RGB */
-    cinfo->out_color_space = JCS_RGB;
+    set_dct_method(*this, cinfo);
 
+    const SkBitmap::Config config = this->getBitmapConfig(cinfo);
 #ifdef ANDROID_RGB
-    cinfo->dither_mode = JDITHER_NONE;
-    if (SkBitmap::kARGB_8888_Config == config) {
-        cinfo->out_color_space = JCS_RGBA_8888;
-    } else if (SkBitmap::kRGB_565_Config == config) {
-        cinfo->out_color_space = JCS_RGB_565;
-        if (this->getDitherImage()) {
-            cinfo->dither_mode = JDITHER_ORDERED;
-        }
-    }
+    adjust_out_color_space_and_dither(cinfo, config, *this);
 #endif
 
     int startX = rect.fLeft;