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..8462f9ed945fa9f080e41448ddd73581896168bc 100644
--- a/src/images/SkImageDecoder_libjpeg.cpp
+++ b/src/images/SkImageDecoder_libjpeg.cpp
@@ -55,6 +55,14 @@ static void overwrite_mem_buffer_size(j_decompress_ptr cinfo) {
 //////////////////////////////////////////////////////////////////////////
 //////////////////////////////////////////////////////////////////////////
 
+static void initialize_info(jpeg_decompress_struct* cinfo, skjpeg_source_mgr* src_mgr) {

[mtklein, 2013/08/05 22:31:09]

cinfo is a pretty cryptic name.  even just decompressor / fDecompressor would be
a step up I think.  I have a real hard time with names involving "info". 
Everything is info...  Is this just canonically called cinfo?

[scroggo, 2013/08/06 15:06:27]

Agreed. Yes, the name is canonical.

+    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:
@@ -106,9 +114,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 +199,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 +309,130 @@ 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) {

[mtklein, 2013/08/05 22:31:09]

can we make this guy take a const&?

[scroggo, 2013/08/06 15:06:27]

The Skia style guide states to use a pointer when you're going to modify an
object/struct.

[mtklein, 2013/08/06 15:22:48]

Right.  It looks to me like it's only being read here?

[scroggo, 2013/08/06 15:29:46]

No, the second switch statement will modify it.

+    SkASSERT(cinfo != NULL);
+
+    SrcDepth srcDepth = k32Bit_SrcDepth;
+    if (JCS_GRAYSCALE == cinfo->jpeg_color_space) {
+        srcDepth = k8BitGray_SrcDepth;
+    }
+
+    SkBitmap::Config config = this->getPrefConfig(srcDepth, false);

[mtklein, 2013/08/05 22:31:09]

Would help my reading to add a reminder what the false argument means here.

[scroggo, 2013/08/06 15:06:27]

Done.

+    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;
+    }
+
+    switch (cinfo->jpeg_color_space) {
+        case JCS_CMYK:
+            // Fall through.
+        case JCS_YCCK:

[scroggo, 2013/08/05 21:47:51]

In the old code, JCS_YCCK results in a request for JCS_RGB, which libjpeg
refuses to do.

+            // 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;

[mtklein, 2013/08/05 22:31:09]

just for consistency i'd add a break above or remove this one.

[scroggo, 2013/08/06 15:06:27]

Done.

+    }
+    return config;
+}
+
+#ifdef ANDROID_RGB
+/**
+ *  FIXME: Improve this name. Here is what this function does:

[scroggo, 2013/08/05 21:47:51]

Suggestions?

+ *      - Set cinfo->dither_mode
+ *      - change the out color space custom color spaces.
+ */
+static void apply_dither_mode(jpeg_decompress_struct* cinfo, SkBitmap::Config config,

[mtklein, 2013/08/05 22:31:09]

funky arg indent?

[mtklein, 2013/08/05 22:31:09]

maybe adjustOutColorSpaceForDither?

[scroggo, 2013/08/06 15:06:27]

How about adjust_out_color_space_and_dither?

[scroggo, 2013/08/06 15:06:27]

Done.

+                       const SkImageDecoder& decoder) {
+    SkASSERT(cinfo != NULL);
+    cinfo->dither_mode = JDITHER_NONE;
+    if (JCS_CMYK == cinfo->out_color_space) {

[scroggo, 2013/08/05 21:47:51]

This check was not happening in tile based decode.

+        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 +441,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 +452,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 +466,16 @@ 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;

[scroggo, 2013/08/05 21:47:51]

scale_num defaults to 1, so this line is unnecessary.

[mtklein, 2013/08/05 22:31:09]

maybe add an assert like in the tiled code?

[scroggo, 2013/08/06 15:06:27]

Done.

     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;

[scroggo, 2013/08/05 21:47:51]

This line would incorrectly change out_color_space to JCS_RGB even if
jpeg_color_space was JCS_CMYK (or JCS_CYYK), resulting in libjpeg failing. The
new code fixes this.

-        }
-    } else if (config != SkBitmap::kARGB_8888_Config &&
-               config != SkBitmap::kARGB_4444_Config &&
-               config != SkBitmap::kRGB_565_Config) {
-        config = SkBitmap::kARGB_8888_Config;
-    }
+    SkBitmap::Config config = this->getBitmapConfig(&cinfo);

[mtklein, 2013/08/05 22:31:09]

const?

[scroggo, 2013/08/06 15:06:27]

Done.

 
 #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;
-        }
-    }
+    apply_dither_mode(&cinfo, config, *this);
 #endif
 
     if (1 == sampleSize && SkImageDecoder::kDecodeBounds_Mode == mode) {
@@ -552,8 +633,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 +658,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 +672,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 +702,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 +711,11 @@ bool SkJPEGImageDecoder::onDecodeSubset(SkBitmap* bm, const SkIRect& region) {
     int requestedSampleSize = this->getSampleSize();
     cinfo->scale_denom = requestedSampleSize;
 
-    if (this->getPreferQualityOverSpeed()) {

[scroggo, 2013/08/05 21:47:51]

The new method checks #ifdef's where appropraiate.

-        cinfo->dct_method = JDCT_ISLOW;
-    } else {
-        cinfo->dct_method = JDCT_IFAST;
-    }
-
-    SkBitmap::Config config = this->getPrefConfig(k32Bit_SrcDepth, false);

[scroggo, 2013/08/05 21:47:51]

The new code has the behavior used in onDecode, so it respects a request for A8,
when appropriate.

-    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;

[scroggo, 2013/08/05 21:47:51]

Likewise, the new code uses a valid color space.

+    set_dct_method(*this, cinfo);
 
+    SkBitmap::Config config = this->getBitmapConfig(cinfo);

[mtklein, 2013/08/05 22:31:09]

const?

[scroggo, 2013/08/06 15:06:27]

Done.

 #ifdef ANDROID_RGB
-    cinfo->dither_mode = JDITHER_NONE;
-    if (SkBitmap::kARGB_8888_Config == config) {

[scroggo, 2013/08/05 21:47:51]

The factored out function checks for cmyk.

-        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;
-        }
-    }
+    apply_dither_mode(cinfo, config, *this);
 #endif
 
     int startX = rect.fLeft;