Move all knowledge of X sampling into SkScaledCodec

src/codec/SkJpegCodec.cpp

Index: src/codec/SkJpegCodec.cpp
diff --git a/src/codec/SkJpegCodec.cpp b/src/codec/SkJpegCodec.cpp
index b4c794c90623111d3835b0673d5ec88eb5c42d32..6311fddf123444908858013ef20fc296f4a4614b 100644
--- a/src/codec/SkJpegCodec.cpp
+++ b/src/codec/SkJpegCodec.cpp
@@ -155,14 +155,22 @@ static int get_row_bytes(const j_decompress_ptr dinfo) {
     return dinfo->output_width * colorBytes;
 
 }
+
+void calc_output_dimensions(jpeg_decompress_struct* dinfo, unsigned int num, unsigned int denom) {
+    dinfo->num_components = 0;

[msarett, 2015/10/01 19:34:34]

This line belongs in onGetScaledDimensions(), I think.  It is a hack that makes
sure jpeg_calc_output_dimensions() does not give an error on our fake dinfo
struct (we tell it there are zero components, so there are no components to
iterate through and verify things about).

I don't think we want to be modifying this value if someone calls this function
with a real dinfo struct.

[scroggo, 2015/10/01 21:16:57]

This function is only ever called on a fake dinfo struct, though. Maybe add a
comment?

[msarett, 2015/10/01 22:53:07]

Comment works for me.

+    dinfo->scale_num = num;
+    dinfo->scale_denom = denom;
+    jpeg_calc_output_dimensions(dinfo);
+}
+
 /*
  * Return a valid set of output dimensions for this decoder, given an input scale
  */
 SkISize SkJpegCodec::onGetScaledDimensions(float desiredScale) const {
     // libjpeg-turbo supports scaling by 1/8, 1/4, 3/8, 1/2, 5/8, 3/4, 7/8, and 1/1, so we will
     // support these as well
-    long num;
-    long denom = 8;
+    unsigned int num;
+    unsigned int denom = 8;
     if (desiredScale > 0.875f) {
         num = 8;
     } else if (desiredScale > 0.75f) {
@@ -187,10 +195,7 @@ SkISize SkJpegCodec::onGetScaledDimensions(float desiredScale) const {
     dinfo.image_width = this->getInfo().width();
     dinfo.image_height = this->getInfo().height();
     dinfo.global_state = fReadyState;
-    dinfo.num_components = 0;
-    dinfo.scale_num = num;
-    dinfo.scale_denom = denom;
-    jpeg_calc_output_dimensions(&dinfo);
+    calc_output_dimensions(&dinfo, num, denom);
 
     // Return the calculated output dimensions for the given scale
     return SkISize::Make(dinfo.output_width, dinfo.output_height);
@@ -272,28 +277,39 @@ bool SkJpegCodec::setOutputColorSpace(const SkImageInfo& dst) {
  * Checks if we can natively scale to the requested dimensions and natively scales the 
  * dimensions if possible
  */
-bool SkJpegCodec::nativelyScaleToDimensions(uint32_t dstWidth, uint32_t dstHeight) {
+bool SkJpegCodec::onDimensionsSupported(const SkISize& size) {
+    if (setjmp(fDecoderMgr->getJmpBuf())) {
+        return fDecoderMgr->returnFalse("onDimensionsSupported/setjmp");
+    }
+
+    const int dstWidth = size.width();
+    const int dstHeight = size.height();
+
+    // Set up a fake decompress struct in order to use libjpeg to calculate output dimensions

[scroggo, 2015/10/01 15:33:14]

The old code crashed for me when I attempted to call both getScaledDimensions
*and* onDimensionsSupported. This surprised me, and maybe I had some other
error, but I think we talked about doing this anyway for some other reason.

[msarett, 2015/10/01 19:34:34]

That's surprising.  Hopefully it's because of the num_components thing I
mentioned above?  Let me know...if not I'd like to track down what was happening
at some point.

It seems a bit weird to me to not directly modify fDecoderMgr->dinfo(), I'd lean
towards doing it that way.

But I'd guess this implementation has identical performance and behavior, so I'm
happy either way.  Especially if the old approach is still crashing.

[scroggo, 2015/10/01 21:16:57]

I agree that it's worth figuring out why the other version doesn't work (I tried
leaving out the modification of num_components; that didn't work, but I can keep
trying.)

[msarett, 2015/10/01 22:53:07]

I'll file a bug on myself.

+    jpeg_decompress_struct dinfo;
+    sk_bzero(&dinfo, sizeof(dinfo));
+    dinfo.image_width = this->getInfo().width();
+    dinfo.image_height = this->getInfo().height();
+    dinfo.global_state = fReadyState;
+
     // 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;
-    jpeg_calc_output_dimensions(fDecoderMgr->dinfo());
-    while (fDecoderMgr->dinfo()->output_width != dstWidth ||
-            fDecoderMgr->dinfo()->output_height != dstHeight) {
+    unsigned int num = 8;
+    const unsigned int denom = 8;
+    calc_output_dimensions(&dinfo, num, denom);
+    while (dinfo.output_width != dstWidth || dinfo.output_height != dstHeight) {
 
         // Return a failure if we have tried all of the possible scales
-        if (1 == fDecoderMgr->dinfo()->scale_num ||
-                dstWidth > fDecoderMgr->dinfo()->output_width ||
-                dstHeight > fDecoderMgr->dinfo()->output_height) {
-            // reset native scale settings on failure because this may be supported by the swizzler 
-            this->fDecoderMgr->dinfo()->scale_num = 8;
-            jpeg_calc_output_dimensions(this->fDecoderMgr->dinfo());
+        if (1 == num || dstWidth > dinfo.output_width || dstHeight > dinfo.output_height) {
             return false;
         }
 
         // Try the next scale
-        fDecoderMgr->dinfo()->scale_num -= 1;
-        jpeg_calc_output_dimensions(fDecoderMgr->dinfo());
+        num -= 1;
+        calc_output_dimensions(&dinfo, num, denom);
     }
+
+    fDecoderMgr->dinfo()->scale_num = num;
+    fDecoderMgr->dinfo()->scale_denom = denom;
     return true;
 }
 
@@ -321,11 +337,6 @@ SkCodec::Result SkJpegCodec::onGetPixels(const SkImageInfo& dstInfo,
         return fDecoderMgr->returnFailure("conversion_possible", kInvalidConversion);
     }
 
-    // Perform the necessary scaling
-    if (!this->nativelyScaleToDimensions(dstInfo.width(), dstInfo.height())) {
-        return fDecoderMgr->returnFailure("cannot scale to requested dims", kInvalidScale);
-    }
-
     // Now, given valid output dimensions, we can start the decompress
     if (!jpeg_start_decompress(dinfo)) {
         return fDecoderMgr->returnFailure("startDecompress", kInvalidInput);
@@ -376,7 +387,13 @@ SkCodec::Result SkJpegCodec::onGetPixels(const SkImageInfo& dstInfo,
     return kSuccess;
 }
 
-SkCodec::Result SkJpegCodec::initializeSwizzler(const SkImageInfo& info, const Options& options) {
+SkSampler* SkJpegCodec::getSampler() {
+    if (fSwizzler) {
+        SkASSERT(fSrcRow && static_cast<uint8_t*>(fStorage.get()) == fSrcRow);
+        return fSwizzler;
+    }
+
+    const SkImageInfo& info = this->dstInfo();
     SkSwizzler::SrcConfig srcConfig;
     switch (info.colorType()) {
         case kGray_8_SkColorType:
@@ -396,13 +413,15 @@ SkCodec::Result SkJpegCodec::initializeSwizzler(const SkImageInfo& info, const O
             SkASSERT(false);
     }
 
-    fSwizzler.reset(SkSwizzler::CreateSwizzler(srcConfig, nullptr, info, options.fZeroInitialized, 
-                                               this->getInfo()));
+    fSwizzler.reset(SkSwizzler::CreateSwizzler(srcConfig, nullptr, info,
+                                               this->options().fZeroInitialized));
     if (!fSwizzler) {
-        return SkCodec::kUnimplemented;
+        return nullptr;
     }
 
-    return kSuccess;
+    fStorage.reset(get_row_bytes(fDecoderMgr->dinfo()));
+    fSrcRow = static_cast<uint8_t*>(fStorage.get());
+    return fSwizzler;
 }
 
 SkCodec::Result SkJpegCodec::onStartScanlineDecode(const SkImageInfo& dstInfo,
@@ -418,25 +437,10 @@ SkCodec::Result SkJpegCodec::onStartScanlineDecode(const SkImageInfo& dstInfo,
         return kInvalidConversion;
     }
 
-    // Perform the necessary scaling
-    if (!this->nativelyScaleToDimensions(dstInfo.width(), dstInfo.height())) {
-        // full native scaling to dstInfo dimensions not supported
-
-        if (!SkScaledCodec::DimensionsSupportedForSampling(this->getInfo(), dstInfo)) {
-            return kInvalidScale;
-        }
-        // create swizzler for sampling
-        Result result = this->initializeSwizzler(dstInfo, options);
-        if (kSuccess != result) {
-            SkCodecPrintf("failed to initialize the swizzler.\n");
-            return result;
-        }
-        fStorage.reset(get_row_bytes(fDecoderMgr->dinfo()));
-        fSrcRow = static_cast<uint8_t*>(fStorage.get());
-    } else {
-        fSrcRow = nullptr;
-        fSwizzler.reset(nullptr);
-    }
+    // Remove objects used for sampling.
+    fSwizzler.reset(nullptr);
+    fSrcRow = nullptr;
+    fStorage.free();

[msarett, 2015/10/01 19:34:34]

I'm a little confused here...

Why can we do this/Why do we want to do this?

[scroggo, 2015/10/01 21:16:57]

Because onGetScanlines changes its behavior based on whether there is an
fSwizzler. If Someone calls start again, we assume that they do not want to use
the same sampling. Maybe this is the wrong assumption, but it seems like it's
the safest.

We could leave the storage (not sure if you meant just that line or all three).
Leaving the storage means we'll be hanging on to memory that may not be needed.
Also, when we ask for sampling again we could be asking for a different color
type, meaning the size needed is different.

[msarett, 2015/10/01 22:53:07]

Thanks!

 
     // Now, given valid output dimensions, we can start the decompress
     if (!jpeg_start_decompress(fDecoderMgr->dinfo())) {