Beginning work to refactor jpeg tile decoding.

src/images/SkImageDecoder_libjpeg.cpp

 /*
  * Copyright 2007 The Android Open Source Project
  *
  * Use of this source code is governed by a BSD-style license that can be
  * found in the LICENSE file.
  */
 
 
 #include "SkImageDecoder.h"
 #include "SkImageEncoder.h"
@@ skipping 12 matching lines @@
 extern "C" {
     #include "jpeglib.h"
     #include "jerror.h"
 }
 
 // These enable timing code that report milliseconds for an encoding/decoding
 //#define TIME_ENCODE
 //#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

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

Off topic, but maybe we should delete this line, given that it's _not_ disabled.

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

Done. This line has been here since we imported the code from Android, so I'm
not sure when we reenabled it...

 #define WE_CONVERT_TO_YUV
 
 // If ANDROID_RGB is defined by in the jpeg headers it indicates that jpeg offers
 // support for two additional formats (1) JCS_RGBA_8888 and (2) JCS_RGB_565.
 
 //////////////////////////////////////////////////////////////////////////
 //////////////////////////////////////////////////////////////////////////
 
 static void overwrite_mem_buffer_size(j_decompress_ptr cinfo) {

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

Is j_decompress_ptr == jpeg_decompress_struct* ?

If so, let's use just one of them?  (I'd use jpeg_decompress_struct*).

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

Yes. This is the only instance of j_decompress_ptr. Changed.

 #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.
      */
 #ifdef ANDROID_LARGE_MEMORY_DEVICE
     cinfo->mem->max_memory_to_use = 30 * 1024 * 1024;
 #else
     cinfo->mem->max_memory_to_use = 5 * 1024 * 1024;
 #endif
 #endif // SK_BUILD_FOR_ANDROID
 }
 
 //////////////////////////////////////////////////////////////////////////
 //////////////////////////////////////////////////////////////////////////
 
+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:
     SkJPEGImageIndex(SkStream* stream, SkImageDecoder* decoder)
         : fSrcMgr(stream, decoder)
         , fInfoInitialized(false)
         , fHuffmanCreated(false)
         , fDecompressStarted(false)
         {
             SkDEBUGCODE(fReadHeaderSucceeded = false;)
         }
 
     ~SkJPEGImageIndex() {

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

Can you add a note in here about why we set the flags to false before calling
jpeg_*?

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

Done.

         if (fHuffmanCreated) {
             fHuffmanCreated = false;
             jpeg_destroy_huffman_index(&fHuffmanIndex);
         }
         if (fDecompressStarted) {
             fDecompressStarted = false;
             jpeg_finish_decompress(&fCInfo);
         }
         if (fInfoInitialized) {
             this->destroyInfo();
@@ skipping 18 matching lines @@
     /**
      *  Initialize the cinfo struct.
      *  Calls jpeg_create_decompress, makes customizations, and
      *  finally calls jpeg_read_header. Returns true if jpeg_read_header
      *  returns JPEG_HEADER_OK.
      *  If cinfo was already initialized, destroyInfo must be called to
      *  destroy the old one. Must not be called after startTileDecompress.
      */
     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;)
         return success;
     }
 
     jpeg_decompress_struct* cinfo() { return &fCInfo; }

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

these two guys can be const methods?

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

No, these are modified and passed to functions that modify them.

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

just meant

jpeg_decompress_struct* cinfo() const { ... }, etc.

Not important.

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

That gives me the following error:

../../../src/images/SkImageDecoder_libjpeg.cpp:132:53: error: invalid conversion
from 'const jpeg_decompress_struct*' to 'jpeg_decompress_struct*' [-fpermissive]

 
     huffman_index* huffmanIndex() { return &fHuffmanIndex; }
 
     /**
      *  Build the index to be used for tile based decoding.
      *  Must only be called after a successful call to
      *  initializeInfoAndReadHeader and must not be called more
      *  than once.
      */
     bool buildHuffmanIndex() {
@@ skipping 57 matching lines @@
 #endif
     virtual bool onDecode(SkStream* stream, SkBitmap* bm, Mode) SK_OVERRIDE;
 
 private:
 #ifdef SK_BUILD_FOR_ANDROID
     SkJPEGImageIndex* fImageIndex;
     int fImageWidth;
     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;
 };
 
 //////////////////////////////////////////////////////////////////////////
 
 /* Automatically clean up after throwing an exception */
 class JPEGAutoClean {
 public:
     JPEGAutoClean(): cinfo_ptr(NULL) {}
     ~JPEGAutoClean() {
@@ skipping 82 matching lines @@
     //    C = ( (1-C) * (1 - (1-K) + (1-K) ) -> C = 1 - C*K
     // The conversion from CMY->RGB remains the same
     for (unsigned int x = 0; x < width; ++x, scanline += 4) {
         scanline[0] = SkMulDiv255Round(scanline[0], scanline[3]);
         scanline[1] = SkMulDiv255Round(scanline[1], scanline[3]);
         scanline[2] = SkMulDiv255Round(scanline[2], scanline[3]);
         scanline[3] = 255;
     }
 }
 
+/**
+ *  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");
 #endif
 
     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.
     if (setjmp(errorManager.fJmpBuf)) {
         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");
     }
 
     /*  Try to fulfill the requested sampleSize. Since jpeg can do it (when it
         can) much faster that we, just use their num/denom api to approximate
         the size.
     */
     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;
+    turn_off_visual_optimizations(&cinfo);
 
-    /* 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;
-    }
-
-    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) {
         bm->setConfig(config, cinfo.image_width, cinfo.image_height);
         bm->setIsOpaque(config != SkBitmap::kA8_Config);
         return true;
     }
 
     /*  image_width and image_height are the original dimensions, available
         after jpeg_read_header(). To see the scaled dimensions, we have to call
@@ skipping 137 matching lines @@
     return true;
 }
 
 #ifdef SK_BUILD_FOR_ANDROID
 bool SkJPEGImageDecoder::onBuildTileIndex(SkStream* stream, int *width, int *height) {
 
     SkAutoTDelete<SkJPEGImageIndex> imageIndex(SkNEW_ARGS(SkJPEGImageIndex, (stream, this)));
     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.
     if (setjmp(sk_err.fJmpBuf)) {
         return false;
     }
 
     // create the cinfo used to create/build the huffmanIndex
     if (!imageIndex->initializeInfoAndReadHeader()) {
         return false;
     }
 
     if (!imageIndex->buildHuffmanIndex()) {
         return false;
     }
 
     // destroy the cinfo used to create/build the huffman index
     imageIndex->destroyInfo();
 
     // Init decoder to image decode mode
     if (!imageIndex->initializeInfoAndReadHeader()) {
         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()) {
         return false;
     }
 
     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();
 
     return true;
 }
 
 bool SkJPEGImageDecoder::onDecodeSubset(SkBitmap* bm, const SkIRect& region) {
     if (NULL == fImageIndex) {
         return false;
     }
     jpeg_decompress_struct* cinfo = fImageIndex->cinfo();
 
     SkIRect rect = SkIRect::MakeWH(fImageWidth, fImageHeight);
     if (!rect.intersect(region)) {
         // If the requested region is entirely outside the image return false
         return false;
     }
 
 
     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;
     }
 
     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;
-    }
+    set_dct_method(*this, cinfo);
 
-    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.

-
+    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;
     int startY = rect.fTop;
     int width = rect.width();
     int height = rect.height();
 
     jpeg_init_read_tile_scanline(cinfo, fImageIndex->huffmanIndex(),
                                  &startX, &startY, &width, &height);
     int skiaSampleSize = recompute_sampleSize(requestedSampleSize, *cinfo);
@@ skipping 414 matching lines @@
 }
 
 static SkImageEncoder* sk_libjpeg_efactory(SkImageEncoder::Type t) {
     return (SkImageEncoder::kJPEG_Type == t) ? SkNEW(SkJPEGImageEncoder) : NULL;
 }
 
 
 static SkTRegistry<SkImageDecoder*, SkStream*> gDReg(sk_libjpeg_dfactory);
 static SkTRegistry<SkImageDecoder::Format, SkStream*> gFormatReg(get_format_jpeg);
 static SkTRegistry<SkImageEncoder*, SkImageEncoder::Type> gEReg(sk_libjpeg_efactory);