SkScaledCodec class

src/codec/SkJpegCodec.cpp

Index: src/codec/SkJpegCodec.cpp
diff --git a/src/codec/SkJpegCodec.cpp b/src/codec/SkJpegCodec.cpp
index 5acc0b396c54d34abb922f80687104b54f85990a..eeda35ac8509eb768d1a45e764f9a46bed00113c 100644
--- a/src/codec/SkJpegCodec.cpp
+++ b/src/codec/SkJpegCodec.cpp
@@ -189,6 +189,41 @@ SkISize SkJpegCodec::onGetScaledDimensions(float desiredScale) const {
     return SkISize::Make(dinfo.output_width, dinfo.output_height);
 }
 
+SkCodec::Result SkJpegCodec::initializeSwizzler(const SkImageInfo& requestedInfo,
+                                               void* dst, size_t rowBytes,
+                                               const Options& options,
+                                               SkPMColor ctable[],
+                                               int* ctableCount) {
+
+    const SkColorType srcColorType = requestedInfo.colorType();
+    SkSwizzler::SrcConfig srcConfig;
+    switch (srcColorType) {
+        case kGray_8_SkColorType:
+            srcConfig = SkSwizzler::kGray;
+            break;
+        case kRGBA_8888_SkColorType:
+            srcConfig = SkSwizzler::kRGBX;
+            break;
+        case kBGRA_8888_SkColorType:
+            srcConfig = SkSwizzler::kBGRX;
+            break;
+        case kRGB_565_SkColorType:
+            srcConfig = SkSwizzler::kRGB_565;
+            break;
+        default:
+            //would have exited before now if the colorType was supported by jpeg
+            SkASSERT(false);
+    }
+
+    fSwizzler.reset(SkSwizzler::CreateSwizzler(srcConfig, NULL, requestedInfo, dst, rowBytes,
+                                               options.fZeroInitialized, this->getInfo()));
+    if (!fSwizzler) {
+        // FIXME: CreateSwizzler could fail for another reason.
+        return kUnimplemented;
+    }
+    return kSuccess;
+}
+
 /*
  * Handles rewinding the input stream if it is necessary
  */
@@ -287,6 +322,8 @@ bool SkJpegCodec::scaleToDimensions(uint32_t dstWidth, uint32_t dstHeight) {
         if (1 == fDecoderMgr->dinfo()->scale_num ||
                 dstWidth > fDecoderMgr->dinfo()->output_width ||
                 dstHeight > fDecoderMgr->dinfo()->output_height) {
+            this->fDecoderMgr->dinfo()->scale_num = 8;
+            turbo_jpeg_calc_output_dimensions(this->fDecoderMgr->dinfo());
             return fDecoderMgr->returnFalse("could not scale to requested dimensions");
         }
 
@@ -385,7 +422,10 @@ public:
     SkJpegScanlineDecoder(const SkImageInfo& dstInfo, SkJpegCodec* codec)
         : INHERITED(dstInfo)
         , fCodec(codec)
-    {}
+    {
+        fStorage.reset(fCodec->getInfo().width() * dstInfo.minRowBytes());

[scroggo, 2015/07/30 18:28:23]

We only need to do this if there is a swizzler, right?

[emmaleer, 2015/07/30 22:27:56]

Yes, I've added an if statement to only do this if there is a swizzler

+        fSrcRow = static_cast<uint8_t*>(fStorage.get());
+    }
 
     virtual ~SkJpegScanlineDecoder() {
         if (setjmp(fCodec->fDecoderMgr->getJmpBuf())) {
@@ -404,9 +444,16 @@ public:
         if (setjmp(fCodec->fDecoderMgr->getJmpBuf())) {
             return fCodec->fDecoderMgr->returnFailure("setjmp", SkCodec::kInvalidInput);
         }
-
         // Read rows one at a time
-        JSAMPLE* dstRow = (JSAMPLE*) dst;
+        JSAMPLE* dstRow;
+        if (fCodec->fSwizzler) {

[scroggo, 2015/07/30 18:28:23]

So it looks like these introduce a slight performance hit? Maybe we could try
something like the following:

constructor {
  if (fCodec->fSwizzler) {
    mScanlineProc = &decodeAndSwizzle;
  } else {
    mScanlineProc = &decodeDirect;
  }
}

onGetScanlineInternal(void* dstRow) {
  // Does everything inside the loop
}

onGetScanlines() {
  for (int i = 0; i < count; i++) {
    mScanlineProc;
  }
}

decodeAndSwizzle() {
  // setup
  onGetScanlineInternal
}

decodeDirect() {
  onGetScanlineInternal
}

Not sure if it would be faster, but something to consider. Maybe the difference
is neglible?

[emmaleer, 2015/07/30 22:27:56]

I think it's negligible. Also, I don't think this one specific if should drive a
function change this big, since all if statements decrease performance. 
We could, for example, do the same thing as above, but with different if
statements in onGetScanlines.
(such as "JCS_CMYK == fCodec->fDecoderMgr->dinfo()->out_color_space") 
Adding that change could potentially speed it up for any of the if statements.
Why should we choose to optimize this specific if statement?
I think if SkJpegCodec was performing too slow for users, then maybe we could go
back and optimize if statements. 
But since there is a minimal performance decrease, and the reason why we are
worried about this if statement and not other if statements isn't clear, I don't
think this change is worth it.
(Leon or Matt, if you think this change is worth it still, let me know)  

[msarett, 2015/07/31 12:17:59]

I think Leon's suggestion is a good one.  We did regress performance by 2.5%,
and while it's not awful, it's worth it to see if we can do better.  Taking a
closer look, it seems to me that the two possible causes are:
1) Memory allocation of fSrcRow (it looks like you've already fixed this to only
allocate when we need a swizzler)
2) If statements inside for loop in onGetScanlines()

Unless we gain the whole 2.5% back from the fix to (1), I think we should try
fixing (2) as well.  I think the idea of Leon's design is to factor the:
if (swizzler) {
 // code
} else {
  //other code
}
outside of the for loop, so we only check the branch once instead of height
times.  It seems logical to me.

[emmaleer, 2015/07/31 18:41:56]

So I re-ran the performance tests, after changing fSrcRow to only be allocated
if we are sampling, and the code is now faster than master by 0.61%. So that
change seemed to fix the slow down! 
I've added the new performance results to the same google-doc.

[scroggo, 2015/07/31 19:31:58]

That's great! I'm a little surprised that it is faster than master, though. That
is comparing running nanobench on JPEG, without used the scaled codec? (If it is
using the scaled codec, the difference may be us sampling versus letting JPEG
sample - not that I expect us to be faster there either, necessarily.)

Glad you tried the simple change and tested it!

[emmaleer, 2015/07/31 20:43:09]

Yes, it is comparing running nanobench on JPEG, without using the scaled codec.

+            // write data to storage row, then sample using swizzler         
+            dstRow = fSrcRow;
+        } else {
+            // write data directly to dst
+            dstRow = (JSAMPLE*) dst;
+        }
+
         for (int y = 0; y < count; y++) {
             // Read row of the image
             uint32_t rowsDecoded =
@@ -423,10 +470,15 @@ public:
                 convert_CMYK_to_RGBA(dstRow, this->dstInfo().width());
             }
 
-            // Move to the next row
-            dstRow = SkTAddOffset<JSAMPLE>(dstRow, rowBytes);
+            if(fCodec->fSwizzler) {
+                // use swizzler to sample row
+                fCodec->fSwizzler->setDstRow(dst);
+                fCodec->fSwizzler->next(dstRow);
+                dst += rowBytes;

[scroggo, 2015/07/30 18:28:23]

Why not use SkTAddOffset?

[emmaleer, 2015/07/30 22:27:56]

Changed to to SkTAddOffset.

+            } else {
+                dstRow = SkTAddOffset<JSAMPLE>(dstRow, rowBytes);
+            }
         }
-
         return SkCodec::kSuccess;
     }
 
@@ -452,6 +504,8 @@ public:
 
 private:
     SkAutoTDelete<SkJpegCodec> fCodec;
+    SkAutoMalloc               fStorage;

[scroggo, 2015/07/30 18:28:23]

// Only used if sampling is needed.

[emmaleer, 2015/07/30 22:27:56]

Acknowledged.

+    uint8_t*                   fSrcRow;
 
     typedef SkScanlineDecoder INHERITED;
 };
@@ -488,8 +542,18 @@ SkScanlineDecoder* SkJpegCodec::onGetScanlineDecoder(const SkImageInfo& dstInfo,
 
     // Perform the necessary scaling
     if (!codec->scaleToDimensions(dstInfo.width(), dstInfo.height())) {
-        SkCodecPrintf("Cannot scale to output dimensions\n");
-        return NULL;
+        // native scaling to dstInfo dimensions not supported
+
+        if (dstInfo.height() != this->getInfo().height()) {
+            // no non-native height sampling supported
+            return NULL;
+        }
+        // create swizzler for sampling
+        if (codec->initializeSwizzler(dstInfo, NULL, dstInfo.minRowBytes(), options, ctable,
+                                      ctableCount) != kSuccess) {
+            SkCodecPrintf("failed to initialize the swizzler.\n");
+            return NULL;
+        }
     }
 
     // Now, given valid output dimensions, we can start the decompress