Use new jpeg_crop_scanlines() API to optimize jpeg subset decodes

src/codec/SkJpegCodec.cpp

 /*
  * 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 "SkCodec.h"
 #include "SkMSAN.h"
 #include "SkJpegCodec.h"
@@ skipping 353 matching lines @@
             default:
                 // This function should only be called if the colorType is supported by jpeg
                 SkASSERT(false);
         }
     }
 
     if (JCS_RGB == fDecoderMgr->dinfo()->out_color_space) {
         srcConfig = SkSwizzler::kRGB;
     }
 
-    fSwizzler.reset(SkSwizzler::CreateSwizzler(srcConfig, nullptr, dstInfo, options));
+    Options swizzlerOptions = options;
+    if (options.fSubset) {
+        // Use fSwizzlerSubset if this is a subset decode.  This is necessary in the case
+        // where libjpeg-turbo provides a subset and then we need to subset it further.
+        swizzlerOptions.fSubset = &fSwizzlerSubset;

[scroggo, 2016/02/22 15:55:03]

Should we assert here that fSwizzlerSubset is initialized logically? It looks
like we should never hit a case where we didn't initialize it, but it requires
knowing the callers of this method, and the fact that one of them early exits if
options.fSubset is non-NULL, and the other one initializes fSwizzlerSubset
before calling this method.

[msarett, 2016/02/22 17:38:16]

Yes that is a bit confusing.  I'll add an assert.

+    }
+    fSwizzler.reset(SkSwizzler::CreateSwizzler(srcConfig, nullptr, dstInfo, swizzlerOptions));
     SkASSERT(fSwizzler);
     fStorage.reset(get_row_bytes(fDecoderMgr->dinfo()));
     fSrcRow = fStorage.get();
 }
 
 SkSampler* SkJpegCodec::getSampler(bool createIfNecessary) {
     if (!createIfNecessary || fSwizzler) {
         SkASSERT(!fSwizzler || (fSrcRow && fStorage.get() == fSrcRow));
         return fSwizzler;
     }
@@ skipping 19 matching lines @@
     fSwizzler.reset(nullptr);
     fSrcRow = nullptr;
     fStorage.free();
 
     // Now, given valid output dimensions, we can start the decompress
     if (!jpeg_start_decompress(fDecoderMgr->dinfo())) {
         SkCodecPrintf("start decompress failed\n");
         return kInvalidInput;
     }
 
+    if (options.fSubset) {
+        fSwizzlerSubset = *options.fSubset;
+    }
+
+#ifdef TURBO_HAS_CROP
+    if (options.fSubset) {
+        uint32_t startX = options.fSubset->x();
+        uint32_t width = options.fSubset->width();
+
+        // libjpeg-turbo may need to align startX to a multiple of the IDCT
+        // block size.  If this is the case, it will decrease the value of
+        // startX to the appropriate alignment and also increase the value
+        // of width so that the right edge of the requested subset remains
+        // the same.
+        jpeg_crop_scanline(fDecoderMgr->dinfo(), &startX, &width);
+
+        SkASSERT(startX <= (uint32_t) options.fSubset->x());
+        SkASSERT(width >= (uint32_t) options.fSubset->width());
+        SkASSERT(startX + width >= (uint32_t) options.fSubset->right());
+
+        // Instruct the swizzler (if it is necessary) to further subset the
+        // output provided by libjpeg-turbo
+        fSwizzlerSubset.setXYWH(options.fSubset->x() - startX, 0,

[scroggo, 2016/02/22 15:55:03]

So if I understand correctly, this is set here (rather than inside the if
statement below) so that even when we do not need to do our own subsetting, but
we still need a swizzler for CMYK, initializeSwizzler can blindly use
fSwizzlerSubset?

Should we test CMYK images with subsets that do and do not require further
subsetting?

[msarett, 2016/02/22 17:38:16]

Yes that's correct.
This will be covered by my new implementation of kCroppedScanline_Mode in
DMSrcSink.

[scroggo, 2016/02/22 18:46:47]

Maybe add a comment?

[msarett, 2016/02/22 19:07:43]

Done.

+                options.fSubset->width(), options.fSubset->height());

[scroggo, 2016/02/22 15:55:03]

It seems odd that we ever care about the startY and height here. These don't
actually affect anything, do they?

[msarett, 2016/02/22 17:38:16]

No they don't.  They are ignored by SkSwizzler.  The base class implementation
of startScanlineDecode() requires that they are (0, height), since we don't
need/want to specify vertical subsets here.

[scroggo, 2016/02/22 18:46:47]

Maybe add a comment?

[msarett, 2016/02/22 19:07:43]

Done.

+
+        // We will need a swizzler if libjpeg-turbo cannot provide the exact
+        // subset that we request.  Or if we are converting from CMYK.

[scroggo, 2016/02/22 15:55:03]

This part of the comment seems unnecessary here. It doesn't really affect this
code, and we already have a comment down below where it's relevant.

[msarett, 2016/02/22 17:38:16]

Done.

+        if (startX != (uint32_t) options.fSubset->x() ||
+                width != (uint32_t) options.fSubset->width()) {
+            this->initializeSwizzler(dstInfo, options);
+        }
+    }
+
+    // Make sure we have a swizzler if we are converting from CMYK.
+    if (!fSwizzler && JCS_CMYK == fDecoderMgr->dinfo()->out_color_space) {
+        this->initializeSwizzler(dstInfo, options);
+    }
+#else
     // We will need a swizzler if we are performing a subset decode or
     // converting from CMYK.
     J_COLOR_SPACE colorSpace = fDecoderMgr->dinfo()->out_color_space;
     if (options.fSubset || JCS_CMYK == colorSpace || JCS_RGB == colorSpace) {
         this->initializeSwizzler(dstInfo, options);
     }
+#endif
 
     return kSuccess;
 }
 
 int SkJpegCodec::onGetScanlines(void* dst, int count, size_t dstRowBytes) {
     // Set the jump location for libjpeg errors
     if (setjmp(fDecoderMgr->getJmpBuf())) {
         return fDecoderMgr->returnFailure("setjmp", kInvalidInput);
     }
     // Read rows one at a time
@@ skipping 236 matching lines @@
 
         JDIMENSION linesRead = jpeg_read_raw_data(dinfo, yuv, numRowsPerBlock);
         if (linesRead < remainingRows) {
             // FIXME: Handle incomplete YUV decodes without signalling an error.
             return kInvalidInput;
         }
     }
 
     return kSuccess;
 }