Add scaled subset API to SkCodec

include/codec/SkCodec.h

Index: include/codec/SkCodec.h
diff --git a/include/codec/SkCodec.h b/include/codec/SkCodec.h
index 8b6e2101672e84490cd607da75c106dff231da69..9e366367098771d990ab99cb0e01472f2b3f8137 100644
--- a/include/codec/SkCodec.h
+++ b/include/codec/SkCodec.h
@@ -71,25 +71,6 @@ public:
     }
 
     /**
-     *  Return (via desiredSubset) a subset which can decoded from this codec,
-     *  or false if this codec cannot decode subsets or anything similar to
-     *  desiredSubset.
-     *
-     *  @param desiredSubset In/out parameter. As input, a desired subset of
-     *      the original bounds (as specified by getInfo). If true is returned,
-     *      desiredSubset may have been modified to a subset which is
-     *      supported. Although a particular change may have been made to
-     *      desiredSubset to create something supported, it is possible other
-     *      changes could result in a valid subset.
-     *      If false is returned, desiredSubset's value is undefined.
-     *  @return true if this codec supports decoding desiredSubset (as
-     *      returned, potentially modified)
-     */
-    bool getValidSubset(SkIRect* desiredSubset) const {
-        return this->onGetValidSubset(desiredSubset);
-    }
-
-    /**
      *  Format of the encoded data.
      */
     SkEncodedFormat getEncodedFormat() const { return this->onGetEncodedFormat(); }
@@ -162,15 +143,48 @@ public:
     struct Options {
         Options()
             : fZeroInitialized(kNo_ZeroInitialized)
-            , fSubset(NULL)
+            , fSubset(nullptr)
+            , fScaledDimensions(nullptr)
+            , fScaledSubset(nullptr)
         {}
 
         ZeroInitialized fZeroInitialized;
+
         /**
-         *  If not NULL, represents a subset of the original image to decode.
-         *  Must be within the bounds returned by getInfo().
-         *  If the EncodedFormat is kWEBP_SkEncodedFormat (the only one which
-         *  currently supports subsets), the top and left values must be even.
+         *  fSubset represents a subset of the original image to decode.
+         *  It must be within the bounds returned by getInfo().
+         *  If the EncodedFormat is kWEBP_SkEncodedFormat, the top and left
+         *  values must be even.
+         *  If fSubset is NULL, we are not performing a subset decode.
+         *
+         *  fScaledDimensions and fScaledSubset are used together to help
+         *  specify a scaled subset decode.  Both should be non-NULL for a
+         *  scaled subset decode, and both should be NULL otherwise.
+         *
+         *  If fScaledDimensions and fScaledSubset are NULL, we are not
+         *  performing a scaled subset decode:
+         *      (1) If fSubset is non-NULL, we are performing an unscaled
+         *          subset decode, and the subset dimensions must match the
+         *          dstInfo dimensions.
+         *      (2) If fSubset is NULL and the dstInfo dimensions do not
+         *          match the original dimensions, we are performing a
+         *          scaled decode.
+         *      (3) If fSubset is NULL and the dstInfo dimensions match the

[scroggo, 2015/10/12 20:47:06]

Maybe it is worth noting that (2) and (3) are the same as if there was no
Options object?

+         *          original dimensions, we are performing an unscaled, full
+         *          image decode.
+         *
+         *  If both are non-NULL we are performing a scaled subset decode:

[scroggo, 2015/10/12 20:47:06]

Which "both"? fScaledDimensions and fScaledSubset?

+         *      fSubset must be non-NULL.
+         *      fScaledSubset must be within the bounds of fScaledDimensions.
+         *      The dimensions of fScaledSubset must match the dimensions
+         *      specified by dstInfo.
+         *
+         *  Usage:
+         *      Call getScaledSubsetDimensions() with the desired fSubset of
+         *      the original image and the desired scale.  The function will
+         *      populate the options object with valid values of
+         *      fScaledDimensions and fScaledSubset that can be decoded
+         *      efficiently.
          *
          *  In getPixels, we will attempt to decode the exact rectangular
          *  subset specified by fSubset.
@@ -184,9 +198,36 @@ public:
          *  to getScanlines().
          */
         SkIRect*        fSubset;
+        SkISize*        fScaledDimensions;
+        SkIRect*        fScaledSubset;
     };
 
     /**
+     *  @param desiredScale The requested scale factor.
+     *  @param options      In/out parameter.
+     *                      options->fSubset (in/out) specifies the

[scroggo, 2015/10/12 20:47:06]

If we wanted to, we could make options an optional parameter (haha) and combine
this with getScaledDimensions. I'm not sure whether that's better. (My suspicion
is that since most codecs do not support subsets at all, it makes sense to keep
them separate, so asking for a scale does not worry about subsetting, but I do
think it's at least worth considering.)

Sort of on the other extreme, it seems like SkCodec needs to do a lot of work to
handle subsets, which are only supported by two subclasses. I wonder if we
should refactor things so that SkCodec doesn't need to know anything about
subsets. The sticky point here is webp, which supports subsets natively instead
of supporting scanline decoding. I think we have the following options:
- This approach, where SkCodec has to deal with subsets
- Sort of a hybrid approach, where SkCodec knows minimally about subsets (in
order to support webp). All of this new functionality goes directly on
SkScaledCodec (which I think will need to pass through to SkWebpCodec). Maybe
SkScaledCodec's API looks very different, as it will be focused on handling
subsets.
- All subset handling is done inside SkScaledCodec. Webp will potentially be
very slow, since we do not have a scanline decoder. (We could write a dumb one
though, which decodes 16 scanlines (the blocks that webp are stored in) and
stores them, and then returns one line at a time).
- Maybe SkScaledCodec has its own interface, for which webp has its own
implementation.

There are probably more options I haven't thought of, and it will probably make
sense to discuss in person.

+     *                      requested subset in terms of the original image
+     *                      dimensions.  This may be modified to a subset
+     *                      that can be decoded more efficiently.
+     *                      options->fScaledDimensions is an output and
+     *                      specifies the best scaled (full image) output
+     *                      dimensions that can be achieved for the
+     *                      desiredScale.
+     *                      options->fScaledSubset is an output and specifies
+     *                      the fSubset in terms of fScaledDimensions.

[scroggo, 2015/10/12 20:47:06]

"in terms of fScaledDimensions" is not intuitive to me.

Do you mean it specifies fSubset after scaling the entire image to
fScaledDimensions? I think something like this may be more clear.

+     *
+     *  The codec may not be able to scale and subset efficiently to the exact
+     *  scale and subset requested, so fScaledDimensions and fScaledSubset may
+     *  be approximate.  These output values are the codec's suggestion for
+     *  the closest valid scaled subset that it can support.
+     *
+     *  @return true if fScaledDimensions and fScaledSubset have been

[scroggo, 2015/10/12 20:47:06]

You've removed getValidSubset, so I assume the intent is to use this function
with desiredScale == 1.0f, but the comments do not describe that use case.

It seems like there should be an option to pass 1.0f and just a subset, and the
caller need never worry about fScaledDimensions or fScaledSubset, since they are
doing neither.

On that note, you do not mention who owns fScaledDimensions or fScaledSubset.
I'm assuming that the client does the following:

Options options;
options.fSubset = &some_rect;
options.fScaledDimensions = &uninitialized_dim;
options.fScaledSubset = &uninitialized_rect;

if (codec->getScaledSubsetDimensions(scale, options)) {
  ...
}

So they handle the pointers.

+     *               successfully set to values that the codec supports.
+     *          false otherwise.
+     */
+    bool getScaledSubsetDimensions(float desiredScale, const Options& options) const;

[reed1, 2015/10/12 20:21:00]

wow, pretty complicated rules for calling this. Are all combos required for
android?
Can we have 2-3 separate calls (w/ diff names / parameters) to distinguish the
different variations?
e.g.
    getScaledDimensions(float, SkIRect* inOut);
    getSubsetDimensions(float, SkIRect* inOut);
   ...

[scroggo, 2015/10/12 20:47:07]

The options object does not appear to be const.

+
+    /**
      *  Decode into the given pixels, a block of memory of size at
      *  least (info.fHeight - 1) * rowBytes + (info.fWidth *
      *  bytesPerPixel)
@@ -407,7 +448,11 @@ protected:
         return this->getInfo().dimensions();
     }
 
-    // FIXME: What to do about subsets??
+    virtual bool onGetScaledSubsetDimensions(float desiredScale, const Options& options) const {
+        // By default, scaled subsetting is not supported.
+        return false;
+    }
+
     /**
      *  Subclasses should override if they support dimensions other than the
      *  srcInfo's.
@@ -416,6 +461,22 @@ protected:
         return false;
     }
 
+    /**
+     *  Subclasses should override depending on how they support subsets.
+     *
+     *  The default implementation only supports subsetting in the
+     *  x-dimension during a scanline decode.
+     */
+    virtual bool onSubsetSupported(const SkIRect&, bool isScanlineDecode);
+
+    /**
+     *  Subclasses should override depenging on how they support scaled subsetting.
+     *
+     *  If the scale is supported, the default implementation will support subsetting
+     *  in the x-dimension during a scanline decode.
+     */
+    virtual bool onScaledSubsetSupported(const Options&, bool isScanlineDecode);
+
     virtual SkEncodedFormat onGetEncodedFormat() const = 0;
 
     /**
@@ -536,6 +597,16 @@ private:
         return dim == fSrcInfo.dimensions() || this->onDimensionsSupported(dim);
     }
 
+    /**
+     *  Return whether decoding this subset is supported.
+     */
+    bool subsetSupported(const SkIRect& subset, bool isScanlineDecode);
+
+    /**
+     *  Return whether a decode with the specified options is supported
+     */
+    bool scaledSubsetSupported(const SkISize& dim, const Options& options, bool isScanlineDecode);
+
     // Methods for scanline decoding.
     virtual SkCodec::Result onStartScanlineDecode(const SkImageInfo& dstInfo,
             const SkCodec::Options& options, SkPMColor ctable[], int* ctableCount) {