Scanline decoding for jpeg

src/codec/SkJpegCodec.cpp

Index: src/codec/SkJpegCodec.cpp
diff --git a/src/codec/SkJpegCodec.cpp b/src/codec/SkJpegCodec.cpp
index a4ad76651797e1e99dd2c0742e97212d3896339b..8529717e2862528d020cc6955dce1d183307991b 100644
--- a/src/codec/SkJpegCodec.cpp
+++ b/src/codec/SkJpegCodec.cpp
@@ -61,7 +61,7 @@ SkSwizzler::SrcConfig get_src_config(const jpeg_decompress_struct& dinfo) {
  * @param width the number of pixels in the row that is being converted
  *              CMYK is stored as four bytes per pixel
  */
-static void convert_CMYK_to_RGB(uint8_t* row, uint32_t width) {
+static void convert_CMYK_to_RGB(uint8_t* row, size_t rowBytes) {

[scroggo, 2015/04/24 13:14:28]

It seems very weird to me to pass rowBytes here. Why not stick with width?

[msarett, 2015/04/24 15:08:44]

Yeah you are absolutely right.  I made the change because I felt it was more
convenient.  Looking back, it is just as convenient to pass width and certainly
more clear.

     // We will implement a crude conversion from CMYK -> RGB using formulas
     // from easyrgb.com.
     //
@@ -103,6 +103,7 @@ static void convert_CMYK_to_RGB(uint8_t* row, uint32_t width) {
     // R = C * K / 255
     // G = M * K / 255
     // B = Y * K / 255
+    uint32_t width = rowBytes / 4;
     for (uint32_t x = 0; x < width; x++, row += 4) {
         row[0] = SkMulDiv255Round(row[0], row[3]);
         row[1] = SkMulDiv255Round(row[1], row[3]);
@@ -168,6 +169,8 @@ SkJpegCodec::SkJpegCodec(const SkImageInfo& srcInfo, SkStream* stream,
         JpegDecoderMgr* decoderMgr)
     : INHERITED(srcInfo, stream)
     , fDecoderMgr(decoderMgr)
+    , fSwizzler(NULL)
+    , fSrcRowBytes(0)
 {}
 
 /*
@@ -227,23 +230,73 @@ static bool conversion_possible(const SkImageInfo& dst,
 }
 
 /*
- * Performs the jpeg decode
+ * Handles rewinding the input stream if it is necessary
  */
-SkCodec::Result SkJpegCodec::onGetPixels(const SkImageInfo& dstInfo,
-                                         void* dst, size_t dstRowBytes,
-                                         const Options& options, SkPMColor*, int*) {
-    // Rewind the stream if needed
+bool SkJpegCodec::handleRewind() {
     SkCodec::RewindState rewindState = this->rewindIfNeeded();
     if (rewindState == kCouldNotRewind_RewindState) {
-        return kCouldNotRewind;
+        return fDecoderMgr->returnFalse("could not rewind");
     } else if (rewindState == kRewound_RewindState) {
         JpegDecoderMgr* decoderMgr = NULL;
         if (!ReadHeader(this->stream(), NULL, &decoderMgr)) {
-            return kCouldNotRewind;
+            return fDecoderMgr->returnFalse("could not rewind");
         }
         SkASSERT(NULL != decoderMgr);
         fDecoderMgr.reset(decoderMgr);
     }
+    return true;

[scroggo, 2015/04/24 13:14:28]

nit: I think this should go inside the else statement, since no other paths lead
to it.

[msarett, 2015/04/24 15:08:44]

Hmmm I'm not quite sure I understand, but let me know if you like this better.

[scroggo, 2015/04/24 15:23:13]

Oh no, I was confused. I like it better the first way. I forgot there was a
third option for rewindState. Which reminds me that I think a switch statement
is more clear here.

[msarett, 2015/04/24 15:40:13]

Done.

+}
+
+/*
+ * Checks if we can scale to the requested dimensions and performs the scaling
+ */
+bool SkJpegCodec::handleScaling(const SkImageInfo& dstInfo) {

[scroggo, 2015/04/24 13:14:28]

You only need width and height here. Why not pass those two as parameters
instead?

Also, can we just name this "scale"? (or maybe something like
"scaleToDimensions"?)

[msarett, 2015/04/24 15:08:44]

Agreed.

+    // libjpeg can scale to 1/1, 1/2, 1/4, and 1/8
+    SkASSERT(1 == fDecoderMgr->dinfo()->scale_num);
+    SkASSERT(1 == fDecoderMgr->dinfo()->scale_denom);
+    jpeg_calc_output_dimensions(fDecoderMgr->dinfo());
+    const uint32_t dstWidth = dstInfo.width();
+    const uint32_t dstHeight = dstInfo.height();
+    while (fDecoderMgr->dinfo()->output_width != dstWidth ||
+            fDecoderMgr->dinfo()->output_height != dstHeight) {
+
+        // Return a failure if we have tried all of the possible scales
+        if (8 == fDecoderMgr->dinfo()->scale_denom ||
+                dstWidth > fDecoderMgr->dinfo()->output_width ||
+                dstHeight > fDecoderMgr->dinfo()->output_height) {
+            return fDecoderMgr->returnFalse("could not scale to requested dimensions");
+        }
+
+        // Try the next scale
+        fDecoderMgr->dinfo()->scale_denom *= 2;
+        jpeg_calc_output_dimensions(fDecoderMgr->dinfo());
+    }
+    return true;
+}
+
+/*
+ * Create the swizzler based on the encoded format
+ */
+void SkJpegCodec::initializeSwizzler(const SkImageInfo& dstInfo,
+                                     void* dst, size_t dstRowBytes,
+                                     const Options& options) {
+    SkSwizzler::SrcConfig srcConfig = get_src_config(*fDecoderMgr->dinfo());
+    fSwizzler.reset(SkSwizzler::CreateSwizzler(srcConfig, NULL, dstInfo, dst, dstRowBytes,
+            options.fZeroInitialized));
+    fSrcRowBytes = SkSwizzler::BytesPerPixel(srcConfig) * dstInfo.width();
+}
+
+/*
+ * Performs the jpeg decode
+ */
+SkCodec::Result SkJpegCodec::onGetPixels(const SkImageInfo& dstInfo,
+                                         void* dst, size_t dstRowBytes,
+                                         const Options& options, SkPMColor*, int*) {
+
+    // Rewind the stream if needed
+    if (!this->handleRewind()) {
+        fDecoderMgr->returnFailure("could not rewind stream", kCouldNotRewind);
+    }
 
     // Get a pointer to the decompress info since we will use it quite frequently
     jpeg_decompress_struct* dinfo = fDecoderMgr->dinfo();
@@ -257,25 +310,10 @@ SkCodec::Result SkJpegCodec::onGetPixels(const SkImageInfo& dstInfo,
     if (!conversion_possible(dstInfo, this->getInfo())) {
         return fDecoderMgr->returnFailure("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 == dinfo->scale_num);
-    SkASSERT(1 == dinfo->scale_denom);
-    jpeg_calc_output_dimensions(dinfo);
-    const uint32_t dstWidth = dstInfo.width();
-    const uint32_t dstHeight = dstInfo.height();
-    while (dinfo->output_width != dstWidth || dinfo->output_height != dstHeight) {
 
-        // Return a failure if we have tried all of the possible scales
-        if (8 == dinfo->scale_denom ||
-                dstWidth > dinfo->output_width ||
-                dstHeight > dinfo->output_height) {
-            return fDecoderMgr->returnFailure("cannot scale to requested dims", kInvalidScale);
-        }
-
-        // Try the next scale
-        dinfo->scale_denom *= 2;
-        jpeg_calc_output_dimensions(dinfo);
+    // Perform the necessary scaling
+    if (!this->handleScaling(dstInfo)) {
+        fDecoderMgr->returnFailure("cannot scale to requested dims", kInvalidScale);
     }
 
     // Now, given valid output dimensions, we can start the decompress
@@ -284,13 +322,10 @@ SkCodec::Result SkJpegCodec::onGetPixels(const SkImageInfo& dstInfo,
     }
 
     // Create the swizzler
-    SkSwizzler::SrcConfig srcConfig = get_src_config(*dinfo);
-    SkAutoTDelete<SkSwizzler> swizzler(SkSwizzler::CreateSwizzler(srcConfig, NULL, dstInfo, dst,
-            dstRowBytes, options.fZeroInitialized));
-    if (NULL == swizzler) {
+    this->initializeSwizzler(dstInfo, dst, dstRowBytes, options);
+    if (NULL == fSwizzler) {
         return fDecoderMgr->returnFailure("getSwizzler", kInvalidInput);

[scroggo, 2015/04/24 13:14:28]

This is not new to this change, but I think kInvalidInput might not be correct
here. It means the stream was not valid. But initializing the swizzler does not
involve reading the stream, right? In SkPngCodec, I returned kUnimplemented,
thinking it would mean we had not implemented something in the swizzler, but
maybe it should be kInvalidParameters, meaning the row bytes or dst info was not
supported? I guess it's possible that the SrcConfig was invalid, which might
mean the stream is no good?

[msarett, 2015/04/24 15:08:44]

I think kUnimplemented sounds like the right choice until we can prove its
something else.  At least it is good to be consistent.

     }
-    const 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
@@ -299,29 +334,29 @@ SkCodec::Result SkJpegCodec::onGetPixels(const SkImageInfo& dstInfo,
     SkASSERT(rowsPerDecode <= 4);
 
     // Create a buffer to contain decoded rows (libjpeg requires a 2D array)
-    const uint32_t srcRowBytes = srcBytesPerPixel * dstWidth;
-    SkAutoTDeleteArray<uint8_t> srcBuffer(SkNEW_ARRAY(uint8_t, srcRowBytes * rowsPerDecode));
+    SkAutoTDeleteArray<uint8_t> srcBuffer(SkNEW_ARRAY(uint8_t, fSrcRowBytes * rowsPerDecode));

[scroggo, 2015/04/24 13:14:28]

Maybe SkASSERT(fSrcRowBytes != 0)

[msarett, 2015/04/24 15:08:44]

Done.

     JSAMPLE* srcRows[4];
     uint8_t* srcPtr = srcBuffer.get();
     for (uint8_t i = 0; i < rowsPerDecode; i++) {
         srcRows[i] = (JSAMPLE*) srcPtr;
-        srcPtr += srcRowBytes;
+        srcPtr += fSrcRowBytes;
     }
 
     // Ensure that we loop enough times to decode all of the rows
     // libjpeg will prevent us from reading past the bottom of the image
+    uint32_t dstHeight = dstInfo.height();
     for (uint32_t y = 0; y < dstHeight + rowsPerDecode - 1; y += rowsPerDecode) {
         // Read rows of the image
         uint32_t rowsDecoded = jpeg_read_scanlines(dinfo, srcRows, rowsPerDecode);
 
         // Convert to RGB if necessary
         if (JCS_CMYK == dinfo->out_color_space) {
-            convert_CMYK_to_RGB(srcRows[0], dstWidth * rowsDecoded);
+            convert_CMYK_to_RGB(srcRows[0], fSrcRowBytes * rowsDecoded);
         }
 
         // Swizzle to output destination
         for (uint32_t i = 0; i < rowsDecoded; i++) {
-            swizzler->next(srcRows[i]);
+            fSwizzler->next(srcRows[i]);
         }
 
         // If we cannot read enough rows, assume the input is incomplete
@@ -331,7 +366,7 @@ SkCodec::Result SkJpegCodec::onGetPixels(const SkImageInfo& dstInfo,
             // the fill color for opaque images.  If the destination is kGray,
             // the low 8 bits of SK_ColorBLACK will be used.  Conveniently,
             // these are zeros, which is the representation for black in kGray.
-            SkSwizzler::Fill(swizzler->getDstRow(), dstInfo, dstRowBytes,
+            SkSwizzler::Fill(fSwizzler->getDstRow(), dstInfo, dstRowBytes,
                     dstHeight - y - rowsDecoded, SK_ColorBLACK, NULL);
 
             // Prevent libjpeg from failing on incomplete decode
@@ -346,3 +381,120 @@ SkCodec::Result SkJpegCodec::onGetPixels(const SkImageInfo& dstInfo,
 
     return kSuccess;
 }
+
+/*
+ * Enable scanline decoding for jpegs
+ */
+class SkJpegScanlineDecoder : public SkScanlineDecoder {
+public:
+    SkJpegScanlineDecoder(const SkImageInfo& dstInfo, SkJpegCodec* codec)
+        : INHERITED(dstInfo)
+        , fCodec(codec)
+    {
+        fStorage.reset(fCodec->fSrcRowBytes);
+        fSrcRow = static_cast<uint8_t*>(fStorage.get());
+    }
+
+    SkImageGenerator::Result onGetScanlines(void* dst, int count, size_t rowBytes) override {
+        // Set the jump location for libjpeg errors
+        if (setjmp(fCodec->fDecoderMgr->getJmpBuf())) {
+            return fCodec->fDecoderMgr->returnFailure("setjmp", SkImageGenerator::kInvalidInput);
+        }
+
+        // Read rows one at a time
+        for (int y = 0; y < count; y++) {
+            // Read row of the image
+            uint32_t rowsDecoded = jpeg_read_scanlines(fCodec->fDecoderMgr->dinfo(), &fSrcRow, 1);

[msarett, 2015/04/22 19:52:21]

Here we intentionally decode rows one at a time.  In the regular decoder, we
decode rows in chunks of 1, 2, or 4 depending on a recommendation from the
decoder.

I chose one at a time because it's simpler to implement and **I think** it fits
better with the goal of the scanline decoder.  But we have the option to change
this to be more similar to the original decoder.

+            if (rowsDecoded != 1) {
+                SkSwizzler::Fill(dst, this->dstInfo(), rowBytes, count - y, SK_ColorBLACK, NULL);
+                return SkImageGenerator::kIncompleteInput;
+            }
+
+            // Convert to RGB if necessary
+            if (JCS_CMYK == fCodec->fDecoderMgr->dinfo()->out_color_space) {
+                convert_CMYK_to_RGB(fSrcRow, fCodec->fSrcRowBytes);
+            }
+
+            // Swizzle to output destination
+            fCodec->fSwizzler->setDstRow(dst);
+            fCodec->fSwizzler->next(fSrcRow);
+            dst = SkTAddOffset<void>(dst, rowBytes);
+        }
+
+        return SkImageGenerator::kSuccess;
+    }
+
+    SkImageGenerator::Result onSkipScanlines(int count) override {
+        // Set the jump location for libjpeg errors
+        if (setjmp(fCodec->fDecoderMgr->getJmpBuf())) {
+            return fCodec->fDecoderMgr->returnFailure("setjmp", SkImageGenerator::kInvalidInput);
+        }
+
+        // Read rows but ignore the output
+        for (int y = 0; y < count; y++) {
+            jpeg_read_scanlines(fCodec->fDecoderMgr->dinfo(), &fSrcRow, 1);

[msarett, 2015/04/22 19:52:21]

AFAICT there is not a skip function in libjpeg.

We could read all the rows at once to skip them rather than skip them one at a
time.  The drawback is that we would have to allocate a 2D array of memory large
enough for count rows in order to read them all at once.

I chose this implementation because it seems simpler.  Let me know if you feel
that we should go the other way.

[scroggo, 2015/04/24 13:14:28]

This seems fine to me, until/unless we discover it is a problem. I am also not
aware of a way to skip in libjpeg (and SkImageDecoder_libjpeg also behaves like
this).

+        }
+
+        return SkImageGenerator::kSuccess;
+    }
+
+    void onFinish() override {
+        if (setjmp(fCodec->fDecoderMgr->getJmpBuf())) {
+            SkCodecPrintf("setjmp: Error in libjpeg finish_decompress\n");
+            return;
+        }
+
+        jpeg_finish_decompress(fCodec->fDecoderMgr->dinfo());
+    }
+
+private:
+    SkJpegCodec*        fCodec;     // unowned
+    SkAutoMalloc        fStorage;
+    uint8_t*            fSrcRow;

[scroggo, 2015/04/24 13:14:28]

// pointer into fStorage

[msarett, 2015/04/24 15:08:44]

Done.

+
+    typedef SkScanlineDecoder INHERITED;
+};
+
+SkScanlineDecoder* SkJpegCodec::onGetScanlineDecoder(const SkImageInfo& dstInfo,
+        const Options& options, SkPMColor ctable[], int* ctableCount) {
+
+    // Rewind the stream if needed
+    if (!this->handleRewind()) {
+        SkCodecPrintf("Could not rewind\n");
+        return NULL;
+    }
+
+    // Set the jump location for libjpeg errors
+    if (setjmp(fDecoderMgr->getJmpBuf())) {
+        SkCodecPrintf("setjmp: Error from libjpeg\n");
+        return NULL;
+    }
+
+    // Check if we can decode to the requested destination
+    if (!conversion_possible(dstInfo, this->getInfo())) {
+        SkCodecPrintf("Cannot convert to output type\n");
+        return NULL;
+    }
+
+    // Perform the necessary scaling
+    if (!this->handleScaling(dstInfo)) {
+        SkCodecPrintf("Cannot scale ot output dimensions\n");
+        return NULL;
+    }
+
+    // Now, given valid output dimensions, we can start the decompress
+    if (!jpeg_start_decompress(fDecoderMgr->dinfo())) {
+        SkCodecPrintf("start decompress failed\n");
+        return NULL;
+    }
+
+    // Create the swizzler
+    this->initializeSwizzler(dstInfo, NULL, dstInfo.minRowBytes(), options);
+    if (NULL == fSwizzler) {
+        SkCodecPrintf("Could not create swizzler\n");
+        return NULL;
+    }
+
+    // Return the new scanline decoder
+    return SkNEW_ARGS(SkJpegScanlineDecoder, (dstInfo, this));
+}