Combine native sampling with sampling

src/codec/SkSampledCodec.cpp

Index: src/codec/SkSampledCodec.cpp
diff --git a/src/codec/SkSampledCodec.cpp b/src/codec/SkSampledCodec.cpp
index 229555b9bf549998a434b2339fac2eb5618586a6..e534f0348876d832c09f7ebf62bb8b1c78d25c06 100644
--- a/src/codec/SkSampledCodec.cpp
+++ b/src/codec/SkSampledCodec.cpp
@@ -7,37 +7,72 @@
 
 #include "SkCodec.h"
 #include "SkCodecPriv.h"
+#include "SkMath.h"
 #include "SkSampledCodec.h"
 
+#include <initializer_list>
+
 SkSampledCodec::SkSampledCodec(SkCodec* codec)
     : INHERITED(codec->getInfo())
     , fCodec(codec)
 {}
 
-SkISize SkSampledCodec::onGetSampledDimensions(int sampleSize) const {
+SkISize SkSampledCodec::accountForNativeScaling(int* sampleSizePtr, int* nativeSampleSize) const {
+    SkISize preSampledSize = fCodec->getInfo().dimensions();
+    int sampleSize = *sampleSizePtr;
+    if (nativeSampleSize) {
+        *nativeSampleSize = 1;
+    }
+
     // Fast path for when we are not scaling.
     if (1 == sampleSize) {
-        return fCodec->getInfo().dimensions();
+        return preSampledSize;
     }
 
-    const int width = fCodec->getInfo().width();
-    const int height = fCodec->getInfo().height();
-
-    // Check if the codec can provide the scaling natively.

[msarett, 2015/10/29 22:36:33]

I'm quite happy that this block of code is gone.

[scroggo_chromium, 2015/11/02 21:22:10]

Haha, yeah, I think the new code is much cleaner.

-    float scale = get_scale_from_sample_size(sampleSize);
-    SkSize idealSize = SkSize::Make(scale * (float) width, scale * (float) height);
-    SkISize nativeSize = fCodec->getScaledDimensions(scale);
-    float widthDiff = SkTAbs(((float) nativeSize.width()) - idealSize.width());
-    float heightDiff = SkTAbs(((float) nativeSize.height()) - idealSize.height());
-    // Native scaling is preferred to sampling.  If we can scale natively to
-    // within one of the ideal value, we should choose to scale natively.
-    if (widthDiff < 1.0f && heightDiff < 1.0f) {
-        return nativeSize;
+    // Only JPEG supports native downsampling.
+    if (fCodec->getEncodedFormat() == kJPEG_SkEncodedFormat) {

[msarett, 2015/10/29 22:36:33]

Admitting that we know a little bit about the internals of jpeg in this class
doesn't seem so bad to me.  Maybe we want to keep this confined to our strange
Android support classes or maybe we could benefit from this elsewhere?

[scroggo_chromium, 2015/11/02 21:22:10]

We can certainly consider adding this elsewhere, but I see no rush to.

+        // See if libjpeg supports this scale directly
+        switch (sampleSize) {
+            case 2:
+            case 4:
+            case 8:
+                // This class does not need to do any sampling.
+                *sampleSizePtr = 1;
+                return fCodec->getScaledDimensions(get_scale_from_sample_size(sampleSize));
+            default:
+                break;
+        }
+
+        // Check if sampleSize is a multiple of something libjpeg can support.
+        int remainder;
+        for (int supportedSampleSize : { 8 , 4 , 2 }) {
+            int actualSampleSize;
+            SkTDivMod(sampleSize, supportedSampleSize, &actualSampleSize, &remainder);
+            if (0 == remainder) {
+                float scale = get_scale_from_sample_size(supportedSampleSize);
+
+                // fCodec will scale to this size.
+                preSampledSize = fCodec->getScaledDimensions(scale);
+
+                // And then this class will sample it.
+                *sampleSizePtr = actualSampleSize;
+                if (nativeSampleSize) {
+                    *nativeSampleSize = supportedSampleSize;
+                }
+                break;
+            }
+        }
     }
 
-    // Provide the scaling by sampling.
-    return SkISize::Make(get_scaled_dimension(width, sampleSize),
-            get_scaled_dimension(height, sampleSize));
+    return preSampledSize;
+}
+
+SkISize SkSampledCodec::onGetSampledDimensions(int sampleSize) const {
+    // FIXME: Should this (or the baseclass) check for sample size of 1?
+    // I guess it would be a dumb client to call with sampleSize of 1...

[msarett, 2015/10/29 22:36:33]

I think it's worth thinking about.  We call this with sampleSize = 1 in our test
code.  Are we a dumb client?

Unfortunately, we have three different entry points to this code. 
getSampledDimensions(), onGetSampledDimensions(), and accountForNativeScaling().
 Seems a shame to have to check in all 3, but they all would benefit from a
check.

[scroggo_chromium, 2015/11/02 21:22:10]

Well, that makes a good point. If we don't special case it, we're assuming the
client will special case. I think it does make sense to add here.

Done. As long as I'm making the check, I made it <= 1, catching anyone who uses
a negative number or 0.

EDIT: I just realized that we already do this check - you had it in
onGetSampledDimensions, and I moved it to accountForNativeSampling. But the
other caller of accountForNativeSampling knows it has a sampleSize > 1, and I
think it makes more sense to move this into the base class, skipping ever
calling onGetSampledDimensions. This means SkWebpAdapterCodec will also do the
check.

+    const SkISize size = this->accountForNativeScaling(&sampleSize);
+    return SkISize::Make(get_scaled_dimension(size.width(), sampleSize),
+                         get_scaled_dimension(size.height(), sampleSize));
 }
 
 SkCodec::Result SkSampledCodec::onGetAndroidPixels(const SkImageInfo& info, void* pixels,
@@ -59,6 +94,7 @@ SkCodec::Result SkSampledCodec::onGetAndroidPixels(const SkImageInfo& info, void
 
     // We are performing a subset decode.
     int sampleSize = options.fSampleSize;
+    // FIXME: Can we avoid redoing this?

[msarett, 2015/10/29 22:36:33]

I don't think that this is a redo.

We only make this call here on a subset decode.  And I would assume the client
is calling getSampledSubsetDimensions() on a subset decode.

So this is probably the first time we calculate this.

[scroggo_chromium, 2015/11/02 21:22:10]

Good point. Removed.

     SkISize scaledSize = this->onGetSampledDimensions(sampleSize);
     if (!fCodec->dimensionsSupported(scaledSize)) {
         // If the native codec does not support the requested scale, scale by sampling.
@@ -107,31 +143,46 @@ SkCodec::Result SkSampledCodec::onGetAndroidPixels(const SkImageInfo& info, void
 
 SkCodec::Result SkSampledCodec::sampledDecode(const SkImageInfo& info, void* pixels,
         size_t rowBytes, AndroidOptions& options) {
+    // We should only call this function when sampling.
+    SkASSERT(options.fSampleSize > 1);
+
     // Create options struct for the codec.
     SkCodec::Options sampledOptions;
     sampledOptions.fZeroInitialized = options.fZeroInitialized;
 
+    // FIXME: This was already called by onGetAndroidPixels. Can we reduce that?

[msarett, 2015/10/29 22:36:33]

If we are performing a subset decode, we have already called this in
onGetAndroidPixels().  If we are not performing a subset decode, we have
probably already called with in getScaledDimensions().

This feels like one of those cases (similar to SkCodec) where we should think
about caching, but haven't done it yet.  I'm not sure how much it's worth
compared to decoding an image.

+    int sampleSize = options.fSampleSize;
+    int nativeSampleSize;
+    SkISize nativeSize = this->accountForNativeScaling(&sampleSize, &nativeSampleSize);
+
     // Check if there is a subset.
     SkIRect subset;
     int subsetY = 0;
-    int subsetWidth = fCodec->getInfo().width();
-    int subsetHeight = fCodec->getInfo().height();
+    int subsetWidth = nativeSize.width();
+    int subsetHeight = nativeSize.height();
     if (options.fSubset) {
         // We will need to know about subsetting in the y-dimension in order to use the
         // scanline decoder.
+        // Update the subset to account for scaling done by fCodec.
         SkIRect* subsetPtr = options.fSubset;
-        subsetY = subsetPtr->y();
-        subsetWidth = subsetPtr->width();
-        subsetHeight = subsetPtr->height();
+
+        // Do the divide ourselves, instead of calling get_scaled_dimension. If
+        // X and Y are 0, they should remain 0, rather than being upgraded to 1
+        // due to being smaller than the sampleSize.
+        const int subsetX = subsetPtr->x() / nativeSampleSize;
+        subsetY = subsetPtr->y() / nativeSampleSize;
+
+        subsetWidth = get_scaled_dimension(subsetPtr->width(), nativeSampleSize);
+        subsetHeight = get_scaled_dimension(subsetPtr->height(), nativeSampleSize);
 
         // The scanline decoder only needs to be aware of subsetting in the x-dimension.
-        subset.setXYWH(subsetPtr->x(), 0, subsetWidth, fCodec->getInfo().height());
+        subset.setXYWH(subsetX, 0, subsetWidth, nativeSize.height());
         sampledOptions.fSubset = ⊂
     }
 
     // Start the scanline decode.
     SkCodec::Result result = fCodec->startScanlineDecode(
-            info.makeWH(fCodec->getInfo().width(), fCodec->getInfo().height()), &sampledOptions,
+            info.makeWH(nativeSize.width(), nativeSize.height()), &sampledOptions,
             options.fColorPtr, options.fColorCount);
     if (SkCodec::kSuccess != result) {
         return result;