SkScaledCodec class

src/codec/SkJpegCodec.cpp

Index: src/codec/SkJpegCodec.cpp
diff --git a/src/codec/SkJpegCodec.cpp b/src/codec/SkJpegCodec.cpp
index e160f0c4e29a124aa09b03fcb6fd8dea37f57b0e..758b6e7a79fc5d838db715305f59c6271fd1bf29 100644
--- a/src/codec/SkJpegCodec.cpp
+++ b/src/codec/SkJpegCodec.cpp
@@ -11,6 +11,7 @@
 #include "SkJpegUtility_codec.h"
 #include "SkCodecPriv.h"
 #include "SkColorPriv.h"
+#include "SkScaledCodec.h"
 #include "SkScanlineDecoder.h"
 #include "SkStream.h"
 #include "SkTemplates.h"
@@ -149,6 +150,14 @@ SkJpegCodec::SkJpegCodec(const SkImageInfo& srcInfo, SkStream* stream,
 {}
 
 /*
+ * Return the row bytes of a particular image type and width
+ */
+static int get_row_bytes(const j_decompress_ptr dinfo) {
+    int colorBytes = (dinfo->out_color_space == JCS_RGB565) ? 2 : dinfo->out_color_components;
+    return dinfo->output_width * colorBytes;
+
+}
+/*
  * Return a valid set of output dimensions for this decoder, given an input scale
  */
 SkISize SkJpegCodec::onGetScaledDimensions(float desiredScale) const {
@@ -272,10 +281,10 @@ bool SkJpegCodec::setOutputColorSpace(const SkImageInfo& dst) {
 }
 
 /*
- * Checks if we can scale to the requested dimensions and scales the dimensions
- * if possible
+ * Checks if we can natively scale to the requested dimensions and natively scales the 
+ * dimensions if possible
  */
-bool SkJpegCodec::scaleToDimensions(uint32_t dstWidth, uint32_t dstHeight) {
+bool SkJpegCodec::nativelyScaleToDimensions(uint32_t dstWidth, uint32_t dstHeight) {
     // libjpeg-turbo can scale to 1/8, 1/4, 3/8, 1/2, 5/8, 3/4, 7/8, and 1/1
     fDecoderMgr->dinfo()->scale_denom = 8;
     fDecoderMgr->dinfo()->scale_num = 8;
@@ -287,7 +296,10 @@ 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) {
-            return fDecoderMgr->returnFalse("could not scale to requested dimensions");
+            // reset native scale settings on failure because this may be supported by the swizzler 
+            this->fDecoderMgr->dinfo()->scale_num = 8;
+            turbo_jpeg_calc_output_dimensions(this->fDecoderMgr->dinfo());
+            return false;
         }
 
         // Try the next scale
@@ -327,7 +339,7 @@ SkCodec::Result SkJpegCodec::onGetPixels(const SkImageInfo& dstInfo,
     }
 
     // Perform the necessary scaling
-    if (!this->scaleToDimensions(dstInfo.width(), dstInfo.height())) {
+    if (!this->nativelyScaleToDimensions(dstInfo.width(), dstInfo.height())) {
         return fDecoderMgr->returnFailure("cannot scale to requested dims", kInvalidScale);
     }
 
@@ -395,6 +407,47 @@ public:
         , fOpts()
     {}
 
+    /*
+    * Return a valid set of output dimensions for this decoder, given an input scale
+    */  
+    SkISize onGetScaledDimensions(float desiredScale) override {
+        return fCodec->onGetScaledDimensions(desiredScale);
+    }
+
+    /*
+     * Create the swizzler based on the encoded format.
+     * The swizzler is only used for sampling in the x direction.
+     */
+
+    SkCodec::Result initializeSwizzler(const SkImageInfo& info, const SkCodec::Options& options) {
+        SkSwizzler::SrcConfig srcConfig;
+        switch (info.colorType()) {
+            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, info, options.fZeroInitialized, 
+                                                   this->getInfo().width()));
+        if (!fSwizzler) {
+            // FIXME: CreateSwizzler could fail for another reason.
+            return SkCodec::kUnimplemented;
+        }
+        return SkCodec::kSuccess;
+    }
+
     SkCodec::Result onStart(const SkImageInfo& dstInfo, const SkCodec::Options& options,
                             SkPMColor ctable[], int* ctableCount) override {
 
@@ -415,8 +468,19 @@ public:
         }
 
         // Perform the necessary scaling
-        if (!fCodec->scaleToDimensions(dstInfo.width(), dstInfo.height())) {
-            return SkCodec::kInvalidScale;
+        if (!fCodec->nativelyScaleToDimensions(dstInfo.width(), dstInfo.height())) {
+            // full native scaling to dstInfo dimensions not supported
+
+            if (!SkScaledCodec::DimensionsSupportedForSampling(this->getInfo(), dstInfo)) {
+                return SkCodec::kInvalidScale;
+            }
+            // create swizzler for sampling
+            if (this->initializeSwizzler(dstInfo, options) != SkCodec::kSuccess) {

[scroggo, 2015/08/06 15:09:11]

initializeSwizzler may return kUnimplemented, and then we compare to kSuccess.
If it is not kSuccess, we return kInvalidScale.

Maybe initializeSwizzler should just return an SkSwizzler (and maybe it should
instead be called createSwizzler), and then we can just compare to NULL to
decide to return kInvalidScale?

[emmaleer, 2015/08/06 18:59:52]

I've changed this to check the result from initializeSwizzler, and if the result
!= kSuccess, return result.
I think this is the same outcome as what you're suggestion would do?
If not, I can change it to your suggestion.

[scroggo, 2015/08/06 20:37:59]

Your approach makes sense. My approach would have meant one less if check, but
this is not a performance-critical piece of the code where we need to eliminate
if checks, IMO.

Returning result is probably more correct, in case it had more knowledge of its
failure.

[emmaleer, 2015/08/07 18:38:56]

Acknowledged.

+                SkCodecPrintf("failed to initialize the swizzler.\n");
+                return SkCodec::kInvalidScale;
+            }
+            fStorage.reset(get_row_bytes(fCodec->fDecoderMgr->dinfo()));
+            fSrcRow = static_cast<uint8_t*>(fStorage.get());
         }

[scroggo, 2015/08/06 15:09:10]

Should we set fSrcRow to NULL otherwise? Same question for the constructor.

That way, if we made a mistake and tried to write to it when we never
initialized it we will see more predictable results (we'll segfault instead of
writing to random memory).

Oh, and we definitely need to reset fSwizzler. If the first call to onStart
requires a swizzler, but the next does not, we will be attempting to swizzle
when we should have just written directly to the output memory. We should also
have a test for this.

[emmaleer, 2015/08/06 18:59:52]

Acknowledged.

 
         // Now, given valid output dimensions, we can start the decompress
@@ -447,9 +511,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 (fSwizzler) {
+            // 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 =
@@ -466,22 +537,26 @@ public:
 
             // Convert to RGBA if necessary
             if (JCS_CMYK == fCodec->fDecoderMgr->dinfo()->out_color_space) {
-                convert_CMYK_to_RGBA(dstRow, this->dstInfo().width());
+                convert_CMYK_to_RGBA(dstRow, fCodec->fDecoderMgr->dinfo()->output_width);
             }
 
-            // Move to the next row
-            dstRow = SkTAddOffset<JSAMPLE>(dstRow, rowBytes);
+            if(fSwizzler) {
+                // use swizzler to sample row
+                fSwizzler->swizzle(dst, dstRow);
+                dst = SkTAddOffset<JSAMPLE>(dst, rowBytes);
+            } else {
+                dstRow = SkTAddOffset<JSAMPLE>(dstRow, rowBytes);
+            }
         }
-
         return SkCodec::kSuccess;
     }
 
 #ifndef TURBO_HAS_SKIP
-#define turbo_jpeg_skip_scanlines(dinfo, count)                              \
-    SkAutoMalloc storage(dinfo->output_width * dinfo->out_color_components); \
-    uint8_t* storagePtr = static_cast<uint8_t*>(storage.get());              \
-    for (int y = 0; y < count; y++) {                                        \
-        turbo_jpeg_read_scanlines(dinfo, &storagePtr, 1);                    \
+#define turbo_jpeg_skip_scanlines(dinfo, count)                                                \
+    SkAutoMalloc storage(get_row_bytes(dinfo));                                                \
+    uint8_t* storagePtr = static_cast<uint8_t*>(storage.get());                                \
+    for (int y = 0; y < count; y++) {                                                          \
+        turbo_jpeg_read_scanlines(dinfo, &storagePtr, 1);                                      \
     }
 #endif
 
@@ -496,9 +571,16 @@ public:
         return SkCodec::kSuccess;
     }
 
+    SkEncodedFormat onGetEncodedFormat() const override {
+        return kJPEG_SkEncodedFormat;
+    }
+
 private:
     SkAutoTDelete<SkJpegCodec> fCodec;
+    SkAutoMalloc               fStorage;    // Only used if sampling is needed
+    uint8_t*                   fSrcRow;     // Only used if sampling is needed
     SkCodec::Options           fOpts;
+    SkAutoTDelete<SkSwizzler>  fSwizzler;
 
     typedef SkScanlineDecoder INHERITED;
 };
@@ -510,6 +592,7 @@ SkScanlineDecoder* SkJpegCodec::NewSDFromStream(SkStream* stream) {
     }
 
     const SkImageInfo& srcInfo = codec->getInfo();
+
     // Return the new scanline decoder
     return SkNEW_ARGS(SkJpegScanlineDecoder, (srcInfo, codec.detach()));
 }