SkJpegCodec

src/codec/SkJpegCodec.cpp

Index: src/codec/SkJpegCodec.cpp
diff --git a/src/codec/SkJpegCodec.cpp b/src/codec/SkJpegCodec.cpp
new file mode 100644
index 0000000000000000000000000000000000000000..e26680a41b46763ceaa8fd55750e13490a3b018c
--- /dev/null
+++ b/src/codec/SkJpegCodec.cpp
@@ -0,0 +1,444 @@
+/*
+ * Copyright 2015 Google Inc.
+ *
+ * Use of this source code is governed by a BSD-style license that can be
+ * found in the LICENSE file.
+ */
+
+#include "SkJpegCodec.h"
+
+#include "SkCodec.h"
+#include "SkJpegAutoClean.h"
+#include "SkJpegCodec.h"
+#include "SkJpegUtility.h"
+#include "SkCodecPriv.h"
+#include "SkColorPriv.h"
+#include "SkStream.h"
+#include "SkTemplates.h"
+#include "SkTypes.h"
+
+#include <stdio.h>
+extern "C" {
+    #include "jerror.h"
+    #include "jpegint.h"
+    #include "jpeglib.h"
+}
+
+// ANDROID_RGB
+// If this is defined in the jpeg headers it indicates that jpeg offers
+// support for two additional formats: JCS_RGBA_8888 and JCS_RGB_565.
+
+/*
+ * Dummy error reporting functions for when the client wants to suppress messages

[scroggo, 2015/04/10 17:19:06]

Instead of using these, use SkCodecPrintf. Then they can be turned on/off
globally with the rest of the codec output.

Ok, now I see that you check SK_PRINT_CODEC_MESSAGES to decide whether to use
this. That seems fine.

[msarett, 2015/04/13 20:54:05]

This actually isn't fine.  When SK_PRINT_CODEC_MESSAGES is defined, compiling
fails because these methods are unused.  I removed this code.  I have yet to see
jpeglib print any messages, so for now, I think are good without this.

[scroggo, 2015/04/14 13:10:32]

No, jpeglib will print messages. Just always use the same function, which will
call SkCodecPrintf, which will do nothing if SK_PRINT_CODEC_MESSAGES is not
defined.

Alternatively, you could surround these functions with:

#ifndef SK_PRINT_CODEC_MESSAGES
...
#endif

+ */
+static void do_nothing_emit_message(jpeg_common_struct*, int) {
+    // Do nothing
+}
+static void do_nothing_output_message(j_common_ptr) {
+    // Do nothing
+}
+
+/*
+ * Choose the size of the memory buffer on Android
+ */
+static void overwrite_mem_buffer_size(jpeg_decompress_struct* jpegInfo) {
+#ifdef SK_BUILD_FOR_ANDROID
+
+// Use 30 MB for devices with a large amount of system memory and 5MB otherwise
+// TODO: (msarett) This matches SkImageDecoder.  Why were these values chosen?

[scroggo, 2015/04/10 17:19:06]

Good question. I am not sure why these values were chosen, or whether they
matter any more (I think even the lowest end devices today have more memory than
our original low memory devices).

What's worse, using the smaller value apparently causes skbug.com/1282 (though I
am not sure why).

[msarett, 2015/04/13 20:54:04]

I'm going to investigate this bug further after submitting my fixes.

+#ifdef ANDROID_LARGE_MEMORY_DEVICE
+    jpegInfo->mem->max_memory_to_use = 30 * 1024 * 1024;
+#else
+    jpegInfo->mem->max_memory_to_use = 5 * 1024 * 1024;
+#endif
+
+#endif // SK_BUILD_FOR_ANDROID
+}
+
+/*
+ * Print informative error messages
+ */
+static void print_jpeg_decoder_errors(const jpeg_decompress_struct& jpegInfo, int width,
+        int height, const char caller[]) {
+    char buffer[JMSG_LENGTH_MAX];
+    jpegInfo.err->format_message((const j_common_ptr) &jpegInfo, buffer);
+    SkCodecPrintf("libjpeg error %d <%s> from %s [%d %d]\n",
+            jpegInfo.err->msg_code, buffer, caller, width, height);
+}
+
+static bool return_false(const jpeg_decompress_struct& jpegInfo, const char caller[]) {
+    print_jpeg_decoder_errors(jpegInfo, jpegInfo.output_width, jpegInfo.output_height, caller);
+    return false;
+}
+
+static SkCodec::Result return_failure(const jpeg_decompress_struct& jpegInfo, const char caller[],
+        SkCodec::Result result) {
+    print_jpeg_decoder_errors(jpegInfo, jpegInfo.output_width, jpegInfo.output_height, caller);
+    return result;
+}
+
+/*
+ * Convert a row of CMYK samples to RGBX in place.
+ * Note that this method moves the row pointer in its processing.

[scroggo, 2015/04/10 17:19:05]

Could you specify what width means?

It appears that width is the number of four byte pixels in row?

Also, it looks like this code came from SkImageDecoder_libjpeg.cpp. Any reason
not to include the comments from the analogous function there?

[msarett, 2015/04/13 20:54:05]

I meant to have a TODO here.  I removed the comments because AFAICT the comments
did not match what the code was doing.  On second look, it turns out that they
make some sense.  I think I understand this well enough now to document what is
going on.

+ */
+static void convert_CMYK_to_RGB(uint8_t* row, unsigned int width) {
+    for (uint32_t x = 0; x < width; x++, row += 4) {
+        row[0] = SkMulDiv255Round(row[0], row[3]);
+        row[1] = SkMulDiv255Round(row[1], row[3]);
+        row[2] = SkMulDiv255Round(row[2], row[3]);
+        row[3] = 0xFF;
+    }
+}
+
+/*
+ * 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* jpegInfo) {
+    SkASSERT(jpegInfo != NULL);
+    // This gives about 30% performance improvement in SkImageDecoder. In theory, it may
+    // reduce the visual quality.  In practice it's hard to see a difference.

[scroggo, 2015/04/10 17:19:06]

It does. See https://codereview.chromium.org/973563002/#msg14

Go ahead and leave these optimizations out.

[msarett, 2015/04/13 20:54:05]

Done.

+    // TODO (msarett): Measure performance gains for SkCodec.
+    jpegInfo->do_fancy_upsampling = 0;
+
+    // This gives another few percents in SkCodec.
+    // TODO (msarett): Measure performance gains for SkCodec.
+    jpegInfo->do_block_smoothing = 0;
+}
+
+/*
+ * Common code for setting the dct method.
+ */
+static void set_dct_method(jpeg_decompress_struct* jpegInfo) {

[scroggo, 2015/04/10 17:19:06]

Not sure this needs to be its own method, given that it is only used once and it
is so small.

[msarett, 2015/04/13 20:54:05]

Done.

+    SkASSERT(jpegInfo != NULL);
+#ifdef DCT_IFAST_SUPPORTED
+    jpegInfo->dct_method = JDCT_IFAST;
+#else
+    jpegInfo->dct_method = JDCT_ISLOW;
+#endif
+}
+
+/*
+ * Get the preferred color type to decode to based on the properties of the jpeg.
+ * Also, inform libjpeg what format to decode to.
+ */
+SkColorType get_color_type(jpeg_decompress_struct* jpegInfo) {
+    SkASSERT(jpegInfo != NULL);
+
+    switch (jpegInfo->jpeg_color_space) {
+        case JCS_CMYK:
+        case JCS_YCCK:
+            // libjpeg cannot convert from CMYK or YCCK to RGB.
+            // Here, we ask libjpeg to give us CMYK samples back and
+            // we will later manually convert them to RGB.
+            jpegInfo->out_color_space = JCS_CMYK;
+            return kN32_SkColorType;
+        case JCS_GRAYSCALE:
+            jpegInfo->out_color_space = JCS_GRAYSCALE;
+            return kGray_8_SkColorType;
+        case JCS_RGB:

[scroggo, 2015/04/10 17:19:06]

Why did you make this explicit only to fall through to the default?

[msarett, 2015/04/13 20:54:05]

You're right this is unnecessary.

+        default:
+#ifdef ANDROID_RGB
+            jpegInfo->dither_mode = JDITHER_NONE;
+            jpegInfo->out_color_space = JCS_RGBA_8888;
+#else
+            jpegInfo->out_color_space = JCS_RGB;
+#endif
+            // TODO (msarett): SkImageDecoder uses dithering when decoding to 565.
+            // Will we want to dither when we enable decodes to 565?

[scroggo, 2015/04/10 17:19:06]

No. We do not want to do dithering. If the image would require dithering to get
565, we will not support 565.

[msarett, 2015/04/13 20:54:05]

Hmm ok.  Looking at libjpeg, it appears that the default dither mode is
Floyd-Steinberg dithering.  So to avoid doing any dithering at all, we would
have to set dither_mode to JDITHER_NONE for all cases.  To match the behavior of
SkImageDecoder, we would set dither_mode to JDITHER_NONE for the 8888 case,
JDITHER_ORDERED for 565 (simpler dithering algorithm than FS), and leave it as
default for all others.

Let me know what you think it best.

[scroggo, 2015/04/14 13:10:32]

Oh, I meant we would not do dithering after the fact.

I do not think we want to support 565, but I'm not sure about what to do for
8888. What does Chromium do, and what is the default for 8888?

[msarett, 2015/04/14 19:30:36]

The default dithering for all destinations is Floyd-Steinberg.

SkImageDecoder turns off dithering for 8888.
SkImageDecoder uses ordered dithering (faster than FS) for 565.
SkImageDecoder uses the default for other cases (non ANDORID_RGB).

We always use FS dithering and chromium also always uses FS.  I think this is a
good choice?

[scroggo, 2015/04/15 00:31:05]

If Chromium always uses FS, that seems like a good choice. We'll be able to see
the difference in Gold, since SkImageDecoder does not use FS.

+            return kN32_SkColorType;
+    }
+}
+
+/*
+ * Get the config and bytes per pixel of the source data.
+ * Return whether the data is supported.
+ */
+static SkSwizzler::SrcConfig get_src_config(const jpeg_decompress_struct* jpegInfo) {
+    if (JCS_CMYK == jpegInfo->out_color_space) {
+        // In this case we will manually convert the CMYK values to RGBX.
+        return SkSwizzler::kRGBX;
+    } else if (3 == jpegInfo->out_color_components && JCS_RGB == jpegInfo->out_color_space) {

[scroggo, 2015/04/10 17:19:06]

nit: these do not need to be else statements, since each one returns.

[msarett, 2015/04/13 20:54:05]

Done.

+        return SkSwizzler::kRGB;
+#ifdef ANDROID_RGB
+    } else if (JCS_RGBA_8888 == jpegInfo->out_color_space) {
+        return SkSwizzler::kRGBX;
+    } else if (JCS_RGB_565 == jpegInfo->out_color_space) {
+        return SkSwizzler::kRGB_565;
+#endif
+    } else if (1 == jpegInfo->out_color_components && JCS_GRAYSCALE == jpegInfo->out_color_space) {
+        return SkSwizzler::kGray;
+    } else {
+        return SkSwizzler::kUnknown;
+    }
+}
+
+bool SkJpegCodec::IsJpeg(SkStream* stream) {
+    static const uint8_t jpegSig[] = { 0xFF, 0xD8, 0xFF };
+    char buffer[sizeof(jpegSig)];
+    return stream->read(buffer, sizeof(jpegSig)) == sizeof(jpegSig) &&
+            !memcmp(buffer, jpegSig, sizeof(jpegSig));
+}
+
+bool SkJpegCodec::ReadHeader(SkStream* stream, SkCodec** codecOut,
+        jpeg_decompress_struct** jpegInfoOut) {
+    // Initialize a struct that will contain all of the jpeg info as we prepare to decode
+    // Store the struct in a container that automatically frees the memory used
+    JpegAutoClean jpegInfo(
+            (jpeg_decompress_struct*) SkNEW(jpeg_decompress_struct), false);
+
+    // The source manager handles the input stream for libjpeg
+    SkAutoTDelete<skjpeg_source_mgr> srcMgr(SkNEW_ARGS(skjpeg_source_mgr, (stream)));
+    SkAutoTDeleteArray<uint8_t> buffer(SkNEW_ARRAY(uint8_t, srcMgr->kBufferSize));
+    srcMgr->fBuffer = buffer.get();

[scroggo, 2015/04/10 17:19:05]

This seems dangerous. Why did you not make fBuffer fully owned by srcMgr, like
in the old code?

[msarett, 2015/04/13 20:54:04]

The old code allocated fBuffer on the stack, which we cannot do because it is
used in both NewFromStream and onGetPixels.

I think using an AutoTDelete inside the struct is far superior to what I did.  I
guess I just didn't know this was possible in C++.

[scroggo, 2015/04/14 13:10:32]

Let's discuss this further in person, so I can better understand what you did
not think possible, and hopefully I can help you understand.

+
+    // Set up the error manager.  This must be set before the call to jpeg_create_decompress
+    // in case there is a failure in initializing memory in libjpeg.
+    SkAutoTDelete<skjpeg_error_mgr> errorMgr(SkNEW(skjpeg_error_mgr));
+    jpegInfo.get()->err = jpeg_std_error(errorMgr.get());
+    errorMgr->error_exit = skjpeg_err_exit;
+
+    // All objects need to be instantiated before this setjmp call so that
+    // they will be cleaned up properly if an error occurs.
+    if (setjmp(errorMgr.get()->fJmpBuf)) {
+        return return_false(*(jpegInfo.get()), "setjmp");
+    }
+
+    // Initialize the jpeg info struct to prepare to decode
+    jpeg_create_decompress(jpegInfo.get());
+    overwrite_mem_buffer_size(jpegInfo.get());
+    // Set ownership of the source manager and error manager to jpegInfo
+    jpegInfo.get()->src = srcMgr.detach();
+    buffer.detach();
+    errorMgr.detach();
+    jpegInfo.setInit(true);

[scroggo, 2015/04/10 17:19:06]

setInit seems like an odd function to me. It does not change any state of the
jpeg_decompress_struct; it just changes whether we do something later. It seems
cleaner to have a function which calls jpeg_create_decompress, and then sets
fInit to true.

We could take this further, and make the JpegAutoClean hold everything
(source_manager, error_manager,...) instead of passing ownership around. We can
then put the whole JpegAutoClean into an SkAutoTDelete. When we end up creating
the codec, we'll detach it and pass to the codec.

[msarett, 2015/04/13 20:54:05]

Let's take it further.  I think the new version is what you suggested and is
better.

+
+    // Set the warning and error messages if necessary
+#ifndef SK_PRINT_CODEC_MESSAGES
+    jpegInfo.get()->err->emit_message = &do_nothing_emit_message;
+    jpegInfo.get()->err->output_message = &do_nothing_output_message;
+#endif
+
+    // Read the jpeg header
+    int status = jpeg_read_header(jpegInfo.get(), true);
+    if (status != JPEG_HEADER_OK) {
+        return return_false(*(jpegInfo.get()), "read_header");
+    }
+
+    // Choose a method of performing the discrete cosine transform
+    set_dct_method(jpegInfo.get());
+
+    // Optimize for performance over visual quality
+    turn_off_visual_optimizations(jpegInfo.get());
+
+    if (NULL != codecOut) {
+        // Recommend the color type to decode to
+        const SkColorType colorType = get_color_type(jpegInfo.get());
+
+        // Create image info object and the codec
+        const SkImageInfo& imageInfo = SkImageInfo::Make(jpegInfo.get()->image_width,
+                jpegInfo.get()->image_height, colorType, kOpaque_SkAlphaType);
+        *codecOut = SkNEW_ARGS(SkJpegCodec, (imageInfo, stream, jpegInfo.detach()));
+    } else {
+        SkASSERT(NULL != jpegInfoOut);
+        *jpegInfoOut = jpegInfo.detach();
+    }
+    return true;
+}
+
+SkCodec* SkJpegCodec::NewFromStream(SkStream* stream) {
+    SkAutoTDelete<SkStream> streamDeleter(stream);
+    SkCodec* codec = NULL;
+    if (ReadHeader(stream,  &codec, NULL)) {
+        // Codec has taken ownership of the stream, we do not need to delete it
+        SkASSERT(codec);
+        streamDeleter.detach();
+        return codec;
+    }
+    return NULL;
+}
+
+SkJpegCodec::SkJpegCodec(const SkImageInfo& srcInfo, SkStream* stream,
+        jpeg_decompress_struct* jpegInfo)
+    : INHERITED(srcInfo, stream)
+    , fJpegInfo(jpegInfo, true)
+{}
+
+/*
+ * Return a valid set of output dimensions for this decoder, given an input scale
+ */
+SkISize SkJpegCodec::onGetScaledDimensions(float desiredScale) const {
+#ifdef IDCT_SCALING_SUPPORTED
+    // libjpeg supports scaling by 1/1, 1/2, 1/4, and 1/8, so we will support these as well
+    long scale;
+    if (desiredScale > 0.75) {

[scroggo, 2015/04/10 17:19:06]

nit: .75f

[msarett, 2015/04/13 20:54:05]

Done.

+        scale = 1;
+    } else if (desiredScale > 0.375) {
+        scale = 2;
+    } else if (desiredScale > 0.1875) {
+        scale = 4;
+    } else {
+        scale = 8;
+    }
+
+    // Return the calculated output dimensions for the given scale.
+    // This code is risky because it may become incorrect if libjpeg changes how it calculates

[scroggo, 2015/04/10 17:19:06]

I tend to think libjpeg will not change how it makes this calculation, but I
*do* think it would be better to use jpeg_calc_output_dimensions.

What if we change scale_denom, and then change it back?

Also, please add a test that checks that if we attempt to decode with different
scales that the desired dimensions are supported.

The test should not enforce the heuristic about which scale to use depending on
desiredScale (since it is somewhat arbitrary - it may be right to split in the
middle, but maybe another heuristic would be better?); only that the dimensions
we suggested are supported.

[msarett, 2015/04/13 20:54:05]

The compiler complains if I change scale_denom at all since it if part of
fJpegInfo, which is a field.  One option would possibly be to make a copy of
fJpegInfo and call jpeg_calc_output_dimensions on the copy.  I considered this
option, but I tend to think the current approach is better.

I have added the test you suggested.

On the scaling heuristic.  It's worth noting that I chose a different heuristic
than libjpeg does internally.  You can give libjpeg any fraction (scale_num,
scale_denom), and if the fraction is less than or equal to 1/8 you get 1/8, <=
1/4 you get 1/4 etc.  I debated whether to match this or choose another one.  I
guess as you say it is somewhat arbitrary and easy to change.

[scroggo, 2015/04/14 13:10:32]

Yeah, it might just depend on what the client wants. I was initially thinking we
should do something along the lines of libjpeg, so we do not scale down extra
and then scale back up.

On the other hand, you've implemented something that picks the closest, even if
it means scaling down more than necessary, meaning we do the least amount of
(post) scaling possible.

+    // output dimensions.  The ideal way to get the output dimensions would be:
+    // fJpegInfo.get()->scale_denom = scale;
+    // jpeg_calc_output_dimensions(fJpegInfo.get());
+    // However, we cannot do this because this function is const.
+    return SkISize::Make(jdiv_round_up((long) fJpegInfo.get()->image_width, scale),
+                         jdiv_round_up((long) fJpegInfo.get()->image_height, scale));
+#else
+    return this->getInfo().dimensions();
+#endif
+}
+
+/*
+ * Checks if the conversion between the input image and the requested output
+ * image has been implemented
+ */
+static bool conversion_possible(const SkImageInfo& dst,
+                                const SkImageInfo& src) {
+    // Ensure that the profile type is unchanged
+    if (dst.profileType() != src.profileType()) {
+        return false;
+    }
+
+    // Ensure that the alpha type is opaque
+    if (kOpaque_SkAlphaType != dst.alphaType()) {
+        return false;
+    }
+
+    // Always allow kN32 as the color type
+    if (kN32_SkColorType == dst.colorType()) {
+        return true;
+    }
+
+    // Otherwise require that the destination color type match our recommendation
+    return dst.colorType() == src.colorType();
+}
+
+/*
+ * Initiates the jpeg decode
+ */
+SkCodec::Result SkJpegCodec::onGetPixels(const SkImageInfo& dstInfo,
+                                void* dst, size_t dstRowBytes,
+                                const Options& options, SkPMColor*, int*) {
+    // Set a new jump location for libjpeg errors
+    skjpeg_error_mgr* errorMgr = (skjpeg_error_mgr*) fJpegInfo.get()->err;
+    if (setjmp(errorMgr->fJmpBuf)) {
+        return return_failure(*(fJpegInfo.get()), "setjmp", kIncompleteInput);
+    }
+
+    // Rewind the stream if needed
+    SkCodec::RewindState rewindState = this->rewindIfNeeded();
+    if (rewindState == kCouldNotRewind_RewindState) {
+        return kCouldNotRewind;
+    } else if (rewindState == kRewound_RewindState) {
+        jpeg_decompress_struct* jpegInfo = NULL;
+        if (!ReadHeader(this->stream(), NULL, &jpegInfo)) {
+            return kCouldNotRewind;
+        }
+        SkASSERT(NULL != jpegInfo);
+        fJpegInfo.reset(jpegInfo);
+    }
+
+    // Check if we can decode to the requested destination
+    if (!conversion_possible(dstInfo, this->getInfo())) {
+        return return_failure(*(fJpegInfo.get()), "conversion_possible", kInvalidConversion);
+    }
+
+    // Check if we can scale to the requested dimensions
+    // libjpeg can scale to 1/1, 1/2, 1/4, and 1/8
+    SkASSERT(1 == fJpegInfo.get()->scale_num);

[scroggo, 2015/04/10 17:19:06]

If you implement operator->, you can just use

fJpegInfo->scale_num

[msarett, 2015/04/13 20:54:05]

Done.

+    SkASSERT(1 == fJpegInfo.get()->scale_denom);
+    jpeg_calc_output_dimensions(fJpegInfo.get());
+    const uint32_t dstWidth = dstInfo.width();
+    const uint32_t dstHeight = dstInfo.height();
+    while (fJpegInfo.get()->output_width != dstWidth ||
+            fJpegInfo.get()->output_height != dstHeight) {
+
+        // Return a failure if we have tried all of the possible scales
+        if (8 == fJpegInfo.get()->scale_denom) {

[scroggo, 2015/04/10 17:19:06]

It seems like we could also early exit (as a failure) if dstWidth > output_width
|| dstHeight > output_height.

That means potentially extra checks, and potentially less checks, so not sure
the best way to do it.

[msarett, 2015/04/13 20:54:04]

Yeah this is something I thought about and ultimately decided not to implement. 
It helps us fail quicker on bad inputs but adds checks to potentially valid
requests.

Given that you thought about it as well, I will add.

+            return return_failure(*(fJpegInfo.get()), "cannot scale to requested dimensions",
+                    kInvalidScale);
+        }
+
+        // Try the next scale
+        fJpegInfo.get()->scale_denom *= 2;
+        jpeg_calc_output_dimensions(fJpegInfo.get());
+    }
+
+    // Now, given valid output dimensions, we can start the decompress
+    if (!jpeg_start_decompress(fJpegInfo.get())) {
+        return return_failure(*(fJpegInfo.get()), "start_decompress", kInvalidInput);
+    }
+
+    // Create the swizzler
+    SkSwizzler::SrcConfig srcConfig = get_src_config(fJpegInfo.get());

[scroggo, 2015/04/10 17:19:06]

I think this and the following several local variables can be const?

[msarett, 2015/04/13 20:54:05]

Done.

+    SkAutoTDelete<SkSwizzler> swizzler(SkSwizzler::CreateSwizzler(srcConfig, NULL, dstInfo,
+            dst, dstRowBytes, options.fZeroInitialized));
+    if (NULL == swizzler.get()) {
+        return return_failure(*(fJpegInfo.get()), "CreateSwizzler", kInvalidInput);
+    }
+    uint32_t srcBytesPerPixel = SkSwizzler::BytesPerPixel(srcConfig);
+
+    // This is usually 1, but can also be 2 or 4.
+    // If we wanted to always read one row at a time, we could, but we will save space and time
+    // by using the recommendation from libjpeg.
+    uint32_t rowsPerDecode = fJpegInfo.get()->rec_outbuf_height;

[scroggo, 2015/04/10 17:19:06]

We should also implement scanline decoding, although we can leave that to
another CL.

[msarett, 2015/04/13 20:54:05]

Will do.

+    SkASSERT(rowsPerDecode <= 4);
+
+    // Create a buffer to contain decoded rows (libjpeg requires a 2D array)
+    uint32_t srcRowBytes = srcBytesPerPixel * dstWidth;
+    SkAutoTDeleteArray<uint8_t> srcBuffer(SkNEW_ARRAY(uint8_t, srcRowBytes * rowsPerDecode));
+    JSAMPLE* srcRows[4];
+    uint8_t* srcPtr = srcBuffer.get();
+    for (uint8_t i = 0; i < rowsPerDecode; i++) {
+        srcRows[i] = (JSAMPLE*) srcPtr;
+        srcPtr += srcRowBytes;
+    }
+
+    // Ensure that we loop enough times to decode all of the rows
+    // libjpeg will prevent us from reading past the bottom of the image
+    for (uint32_t y = 0; y < dstHeight + rowsPerDecode - 1; y += rowsPerDecode) {
+        // Read rows of the image
+        uint32_t rowsDecoded = jpeg_read_scanlines(fJpegInfo.get(), srcRows, rowsPerDecode);
+
+        // If we cannot read enough rows, assume the input is incomplete
+        if (rowsDecoded < rowsPerDecode &&  y + rowsDecoded < dstHeight) {

[scroggo, 2015/04/10 17:19:06]

extra space here between && and y?

[msarett, 2015/04/13 20:54:05]

Done.

+            // Convert and swizzle the rows that we were able to read
+            if (JCS_CMYK == fJpegInfo.get()->out_color_space) {
+                convert_CMYK_to_RGB(srcRows[0], dstWidth * rowsDecoded);
+            }
+            for (uint32_t i = 0; i < rowsDecoded; i++) {
+                swizzler->next(srcRows[i]);
+            }
+
+            // Fill the remainder of the image with white.
+            // This error handling behavior is unspecified but matches the behavior of
+            // SkImageDecoder.
+            SkSwizzler::Fill(dst, dstInfo, dstRowBytes, y + rowsDecoded, SK_ColorWHITE, NULL);

[scroggo, 2015/04/10 17:19:06]

Don't we use a different color for other images? I tend to think we should
choose the same everywhere.

Fill does not accept kGray, correct?

[msarett, 2015/04/13 20:54:05]

We will consistently fill all opaque images with black.

I added support for kGray to Fill().  I think it makes sense to always fill with
black here as well.

[scroggo, 2015/04/14 13:10:32]

If we want to enforce using the same values everywhere, we could add a static
function which chooses the fill color based on the SkImageInfo.

+
+            // Finish the decode and indicate that the input was incomplete.
+            fJpegInfo.get()->output_scanline = dstHeight;

[scroggo, 2015/04/10 17:19:06]

What does this do?

[msarett, 2015/04/13 20:54:05]

This suppresses a warning message from libjpeg.

"Application transferred too few scanlines"

We will report the warning anyway if SK_PRINT_CODEC_MESSAGES is defined.  I am
adding a comment.

+            jpeg_finish_decompress(fJpegInfo.get());
+            return return_failure(*(fJpegInfo.get()), "Incomplete image data", kIncompleteInput);
+        }
+
+        // Convert to RGB if necessary
+        if (JCS_CMYK == fJpegInfo.get()->out_color_space) {

[scroggo, 2015/04/10 17:19:06]

Isn't this code the same whether we reached the end prematurely or not?

Can we instead always do this first, then check if we're incomplete?

[msarett, 2015/04/13 20:54:05]

Done.

+            convert_CMYK_to_RGB(srcRows[0], dstWidth * rowsDecoded);
+        }
+
+        // Swizzle to output destination
+        for (uint32_t i = 0; i < rowsDecoded; i++) {
+            swizzler->next(srcRows[i]);
+        }
+    }
+    jpeg_finish_decompress(fJpegInfo.get());
+
+    return kSuccess;
+}