Move all knowledge of X sampling into SkScaledCodec

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 "SkJpegCodec.h"
 #include "SkJpegDecoderMgr.h"
@@ skipping 137 matching lines @@
 {}
 
 /*
  * 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;
 
 }
+
+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) {
         num = 7;
     } else if (desiredScale > 0.625f) {
         num = 6;
     } else if (desiredScale > 0.5f) {
         num = 5;
     } else if (desiredScale > 0.375f) {
         num = 4;
     } else if (desiredScale > 0.25f) {
         num = 3;
     } else if (desiredScale > 0.125f) {
         num = 2;
     } else {
         num = 1;
     }
 
     // Set up a fake decompress struct in order to use libjpeg to calculate output dimensions
     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;
-    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);
 }
 
 bool SkJpegCodec::onRewind() {
     JpegDecoderMgr* decoderMgr = nullptr;
     if (!ReadHeader(this->stream(), nullptr, &decoderMgr)) {
         return fDecoderMgr->returnFalse("could not rewind");
     }
@@ skipping 61 matching lines @@
             return true;
         default:
             return false;
     }
 }
 
 /*
  * 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;
 }
 
 /*
  * Performs the jpeg decode
  */
 SkCodec::Result SkJpegCodec::onGetPixels(const SkImageInfo& dstInfo,
                                          void* dst, size_t dstRowBytes,
                                          const Options& options, SkPMColor*, int*) {
     if (options.fSubset) {
         // Subsets are not supported.
         return kUnimplemented;
     }
 
     // Get a pointer to the decompress info since we will use it quite frequently
     jpeg_decompress_struct* dinfo = fDecoderMgr->dinfo();
 
     // Set the jump location for libjpeg errors
     if (setjmp(fDecoderMgr->getJmpBuf())) {
         return fDecoderMgr->returnFailure("setjmp", kInvalidInput);
     }
 
     // Check if we can decode to the requested destination and set the output color space
     if (!this->setOutputColorSpace(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);
     }
 
     // The recommended output buffer height should always be 1 in high quality modes.
     // If it's not, we want to know because it means our strategy is not optimal.
     SkASSERT(1 == dinfo->rec_outbuf_height);
 
     // Perform the decode a single row at a time
@@ skipping 30 matching lines @@
         }
 
         // Move to the next row
         dstRow = SkTAddOffset<JSAMPLE>(dstRow, dstRowBytes);
     }
     jpeg_finish_decompress(dinfo);
 
     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:
             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:
             // This function should only be called if the colorType is supported by jpeg
             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,
         const Options& options, SkPMColor ctable[], int* ctableCount) {
     // Set the jump location for libjpeg errors
     if (setjmp(fDecoderMgr->getJmpBuf())) {
         SkCodecPrintf("setjmp: Error from libjpeg\n");
         return kInvalidInput;
     }
 
     // Check if we can decode to the requested destination and set the output color space
     if (!this->setOutputColorSpace(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())) {
         SkCodecPrintf("start decompress failed\n");
         return kInvalidInput;
     }
 
     return kSuccess;
 }
 
@@ skipping 67 matching lines @@
     // Set the jump location for libjpeg errors
     if (setjmp(fDecoderMgr->getJmpBuf())) {
         return fDecoderMgr->returnFailure("setjmp", kInvalidInput);
     }
 
     jpeg_skip_scanlines(fDecoderMgr->dinfo(), count);
 
     return kSuccess;
 }