Beginning work to refactor jpeg tile decoding.

src/images/SkImageDecoder_libjpeg.cpp

Index: src/images/SkImageDecoder_libjpeg.cpp
diff --git a/src/images/SkImageDecoder_libjpeg.cpp b/src/images/SkImageDecoder_libjpeg.cpp
index 914ceb7e8aa458a3ea4e011be2db27f73d0ead8a..788e3526d2d9880e62fcb9a8a612e6262765425e 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;