API to support native scaling by image-generator

include/core/SkImageGenerator.h

Index: include/core/SkImageGenerator.h
diff --git a/include/core/SkImageGenerator.h b/include/core/SkImageGenerator.h
index e04e05ea0b383d59959397bdf12cf7ef94e86218..e4588313e582f86e98f5c6f0f547dd95b4952cd9 100644
--- a/include/core/SkImageGenerator.h
+++ b/include/core/SkImageGenerator.h
@@ -153,6 +153,30 @@ public:
     GrTexture* generateTexture(GrContext*, const GrTextureParams&, const SkIRect* subset = nullptr);
 
     /**
+     *  Some generators can efficiently scale their contents. If this is supported, the generator
+     *  may only support certain scaled dimensions. Call this with the desired scale factor,
+     *  and it will return true if scaling is supported, and in supportedSizes[] it will return
+     *  the two nearest supported dimensions. If scaling is not supported, or scale is invalid

[msarett, 2015/10/13 21:43:22]

Would we want to return one supportedSize that is smaller than the requested
scale and one supportedSize that is larger than the requested scale, not
necessarily the "two nearest"?  Maybe this is a problem if the generator cannot
scale small enough to be smaller than the requested scale? (it should always be
able to scale larger)

Would it useful to the client to guarantee that supportedSizes[0] is the attempt
to scale smaller and supportedSizes[1] is the attempt to scale larger (or
something like that)?

[aleksandar.stojiljkovic, 2015/10/14 15:32:35]

Another option would be to always return equal or nearest larger dimension.
Not sure what would be use case of nearest smaller dimension - is it when space
for pixels is pre allocated and there is a wish to keep it within the byte
count? 
Or, is it for low memory situation where it is OK to do upscaling while
rendering?

[reed1, 2015/10/15 18:21:28]

Yea, I like the idea of having the results be sorted (first one is <= second
one), and if possible, the first one is <= float_scale <= second one.

[reed1, 2015/10/28 18:01:51]

Your last suggestion makes sense to me. I'll update the comment.

[msarett, 2015/10/28 18:27:19]

I'm not sure I see an update to the comment?

+     *  this will return false, and supportedSizes[] will be undefined.

[msarett, 2015/10/13 21:43:22]

Can we define an "invalid scale" as <= 0.0f or > 1.0f? 

[reed1, 2015/10/15 18:21:28]

Done.

+     */
+    bool computeScaledDimensions(SkScalar scale, SkISize supportedSizes[2]);

[aleksandar.stojiljkovic, 2015/10/14 15:32:35]

since getPixels could be already (implicitly though) used for getting downscaled
image pixels, maybe it makes sense to extend this method to ask if in general
SkImageInfo can be supported:
e.g.
There is no API to check if it can be decoded to YUV, just getYUVPlanes:
Inside Skia then, GrTexture* SkImageCacherator::lockAsTexture needs allocate
planes etc from cache before calling getYUVPlanes, and that one could fail (e.g.
JPEGImageDecoder::canDecodeToYUV())

I'm planning to post a review for that case tomorrow - in order to support
RGB565 (partially decoded JPEG frames are having alpha on undecoded tiles) and
RGB565 is only supported once the full data is available and can be decoded.

[reed1, 2015/10/28 18:01:51]

Perhaps we do want more query methods, but this one is different than asking if
a requested imageinfo would work, since it doesn't propose a width/height, but
rather is asking for possible width/height pairs. If we want other types of
queries (e.g. YUV, etc.) I suggest we add those separately as needed. We can
always consolidate methods afterwards if we see the opportunity.

+
+    /**
+     *  Request the pixels, scaled to fit into the specified scaledInfo dimensions.
+     *  If scaledSubset is not null, it specifies the subset of the scaled pixels that should

[msarett, 2015/10/13 21:43:23]

nit: scaledClip

[reed1, 2015/10/28 18:01:51]

Done.

+     *  be returned. It will be in the scaled coordinate system (not the original), and is only
+     *  valid if it is entire contained inside the scaled dimensions...

[msarett, 2015/10/13 21:43:23]

nit: entirely

[reed1, 2015/10/28 18:01:51]

Done.

+     *
+     *  SkIRect scaledBounds = SkIRect::MakeWH(0, 0, scaledInfo.width(), scaledInfo.height());
+     *  bool valid = scaledBounds.contains(scaledClip);
+     *
+     *  If the requested scaledInfo is not supported by the generator, it it encounters an error,

[msarett, 2015/10/13 21:43:23]

nit: it it

[reed1, 2015/10/28 18:01:51]

Done.

+     *  or the scaledClip is invalid, this returns false and the contents of pixels is undefined.

[msarett, 2015/10/13 21:50:31]

In the case of an incomplete image, will this return true or false?  Or would it
be useful to return an SkCodec::Result?  Or the number of lines decoded
successfully (I don't think we provide this right now)?

[reed1, 2015/10/28 18:01:51]

Good question. I *think* at the image-generator level, this is a simpler issue.
Generators are one-shots, in that they are required to always return the same
answer each time. Thus, if the provided data is incomplete (partial), that
specific generator will always return the same number of decoded pixels.

Is it important for the generator to report this? Not sure. It seems like the
answer could be that all we need to handle is what-to-stick in the un-decoded
pixels... transparent or some special color.

[msarett, 2015/10/28 18:27:19]

From what I've seen, *most* of the incomplete images that clients expect us to
decode are almost complete, and it would be best to fill in the remaining pixels
and return true.  So I think this is probably fine - I'm not sure that it's
important for the generator to report this.

Leon and I have discussed failing on images that are missing lots of pixels, as
opposed to missing just a few, but we haven't implemented this yet.  So as of
now, this API may return true and return a mostly blank image.  I think this is
fine for bad images.

I think you're right - the issue that needs to be handled is what-to-stick in
the un-decoded pixels.  Chrome leaves them transparent.  We don't always have
this option because we have more color types (565, Index8, Gray, etc.).  Right
now we decide what to fill with for the client.  As you've mentioned, I think
this is a flaw in the Codec API.  I intend to fix this so the client can provide
a fill color if they desire.

+     */
+    bool generateScaledPixels(const SkImageInfo& scaledInfo, void* pixels, size_t rowBytes,

[msarett, 2015/10/13 21:43:23]

I have one concern about this that I thought of after our meeting.

If we are asking for a scaledClip, the dimensions of pixels will not match the
dimensions of scaledInfo, instead they will match the dimensions of scaledClip.

I think this could be a little confusing.  We should at least make this clear in
the comments.

[scroggo, 2015/10/14 14:11:28]

Agreed. I thought the conclusion we came to was that the generator could choose
to only fill scaledClip, but the client would need to allocate all of
scaledInfo.

[aleksandar.stojiljkovic, 2015/10/27 13:42:25]

Related to 1412423008 and 1403393004 (downsample & RGB565 support during
decode), I think it is better to move the control on allocation to
SkImageGenerator implementation.
That way, SkImageGenerator would't have to pre-allocate memory for pixel data,
and make 2 calls: 1) what can you scale 2) give me scaled.
Instead, seems better that through this API, allocator is supplied (discardable
memory alloc) and scale and some hint about prefered output type - then
generator decides what is the output format, allocates and selects appropriate
config for the decoded bitmap (alpha, rgb565, indexed palette).
For pngs, after completing partial decoding runs, and establishing that there is
no alpha, generator could choose to drop previous 8888 allocation and replace by
565.
For animated gifs, if it chooses to decode to index8, generator could choose to
instantiate new one (with different palette) if current one has palette changed.

[reed1, 2015/10/28 18:01:51]

Ah, I had a different idea.

I think one of the key values of scaling/clipping is so the client can save RAM
(and hopefully go faster because of that). Thus I think the scaledClip
represents exactly how many pixels the client has allocated.

That said, it is confusing, since the caller is sending in a width/height (info)
and then a different width/height associated with the clip.

[reed1, 2015/10/28 18:01:52]

The older android decoders (SkImageDecoder) follow this pattern nearly exactly.
It has also proven to have problems of control and clear-expectations.
ImageGenerator exists partly to precisely change the pattern to one where the
caller is in more control of what they want and where they want it. For example,
the new pattern makes it clear how a caller might re-use a previous allocation
(or part of a previous allocation) without having to trick-out the "allocator"
ahead of time.

[msarett, 2015/10/28 18:27:19]

I think whatever decision is made here should and probably will trickle into the
SkCodec API as well.

+                              const SkIRect* scaledClip);
+
+    /**
      *  If the default image decoder system can interpret the specified (encoded) data, then
      *  this returns a new ImageGenerator for it. Otherwise this returns NULL. Either way
      *  the caller is still responsible for managing their ownership of the data.
@@ -199,6 +223,14 @@ protected:
         return nullptr;
     }
 
+    virtual bool onComputeScaledDimensions(SkScalar scale, SkISize supportedSizes[2]) {
+        return false;
+    }
+    virtual bool onGenerateScaledPixels(const SkImageInfo& info, void* pixels, size_t rowBytes,
+                                        const SkIRect* subset) {
+        return false;
+    }
+
     bool tryGenerateBitmap(SkBitmap* bm, const SkImageInfo* optionalInfo, SkBitmap::Allocator*);
 
 private: