Create an SkAndroidCodec API separate from SkCodec

Create an SkAndroidCodec API separate from SkCodec

We will implement this API using SkCodecs.

SkAndroidCodecs will be used to implement the
BitmapRegionDecoder Java API (and possibly
BitmapFactory).

BUG=skia:

Committed: https://skia.googlesource.com/skia/+/3d9d7a7213748761d46ade183e11aa7cb55313c1

[msarett, 2015-10-16 18:29:02]

Patchset #1 (id:1) has been deleted

[msarett, 2015-10-16 18:42:15]

msarett@google.com changed reviewers:
+ reed@google.com, scroggo@google.com

[msarett, 2015-10-16 18:42:16]

https://codereview.chromium.org/1406223002/diff/20001/dm/DMSrcSink.cpp
File dm/DMSrcSink.cpp (right):

https://codereview.chromium.org/1406223002/diff/20001/dm/DMSrcSink.cpp#newcod...
dm/DMSrcSink.cpp:619: SkImageInfo decodeInfo = codec->getInfo();
Much of this code is shared with CodecSrc.  I'm looking at refactoring it.

https://codereview.chromium.org/1406223002/diff/20001/include/codec/SkAndroid...
File include/codec/SkAndroidCodec.h (right):

https://codereview.chromium.org/1406223002/diff/20001/include/codec/SkAndroid...
include/codec/SkAndroidCodec.h:87: bool getSubset(SkIRect* desiredSubset) const;
I could have (and maybe should have) called this getValidSubset().

In my mind, "invalid subsets" are the ones that aren't contained by the image. 
I didn't want to make it sound like this function would correct invalid subsets
for the client.

https://codereview.chromium.org/1406223002/diff/20001/include/codec/SkAndroid...
include/codec/SkAndroidCodec.h:110: , fColorPtr(nullptr)
At one point, you mentioned that we might want to move colorPtr and
colorCountPtr into Options in SkCodec.  I thought we could go ahead and see how
it looks in SkAndroidCodec.

https://codereview.chromium.org/1406223002/diff/20001/src/codec/SkAndroidCode...
File src/codec/SkAndroidCodec.cpp (right):

https://codereview.chromium.org/1406223002/diff/20001/src/codec/SkAndroidCode...
src/codec/SkAndroidCodec.cpp:85: if (!pixels) {
There are some error cases that we don't check here because SkCodec will catch
them later anyway (missing color table etc.).  Not sure if this is ok.

https://codereview.chromium.org/1406223002/diff/20001/src/codec/SkWebpAdapter...
File src/codec/SkWebpAdapterCodec.cpp (right):

https://codereview.chromium.org/1406223002/diff/20001/src/codec/SkWebpAdapter...
src/codec/SkWebpAdapterCodec.cpp:29: int width = SkTMax(1,
SkScalarRoundToInt(scale * (float) subset.width()));
Alternatively, we could use get_scaled_dimension() and be more uniform with
SkSampledCodec.

[scroggo, 2015-10-16 21:13:55]

looks on the right track to me. I'll take a deeper look on Monday, but here are
my higher level thoughts.

https://codereview.chromium.org/1406223002/diff/20001/dm/DM.cpp
File dm/DM.cpp (right):

https://codereview.chromium.org/1406223002/diff/20001/dm/DM.cpp#newcode372
dm/DM.cpp:372: push_android_codec_src(path, mode, colorTypes[i], sampleSize);
I wonder how much memory we would save DM by creating a single SkData per image
file, and reusing it with each CodecSrc, ImageSrc, and AndroidSrc that tests the
same image? Shouldn't go in this CL, but something to think about.

https://codereview.chromium.org/1406223002/diff/20001/dm/DMSrcSink.cpp
File dm/DMSrcSink.cpp (right):

https://codereview.chromium.org/1406223002/diff/20001/dm/DMSrcSink.cpp#newcod...
dm/DMSrcSink.cpp:752: if (nullptr == codec) {
Maybe this reproduces the old code, but is it necessary? Don't we only create
the Src if the trial run succeeded?

Oh, I think I'm confusing this with nanobench...

https://codereview.chromium.org/1406223002/diff/20001/include/codec/SkAndroid...
File include/codec/SkAndroidCodec.h (right):

https://codereview.chromium.org/1406223002/diff/20001/include/codec/SkAndroid...
include/codec/SkAndroidCodec.h:87: bool getSubset(SkIRect* desiredSubset) const;
On 2015/10/16 18:42:16, msarett wrote:
> I could have (and maybe should have) called this getValidSubset().
> 
> In my mind, "invalid subsets" are the ones that aren't contained by the image.

> I didn't want to make it sound like this function would correct invalid
subsets
> for the client.

I think it's odd that this one is different from the one on SkCodec. Are you
saying that it is named differently but it means the same thing?

Maybe we should come up with a better name for both of them. How about
"getSupportedSubset"?

https://codereview.chromium.org/1406223002/diff/20001/include/codec/SkAndroid...
include/codec/SkAndroidCodec.h:98: *  @return           Size of the scaled
partial image.  Or zero size
Alternatively, we could return a boolean, and modify a passed in SkISize.

https://codereview.chromium.org/1406223002/diff/20001/include/codec/SkAndroid...
include/codec/SkAndroidCodec.h:185: SkCodec::Result getAndroidPixels(const
SkImageInfo& info, void* pixels, size_t rowBytes,
I don't think it's necessary to put "Android" in these names. (It seems a little
redundant to call SkAndroidCodec::getAndroidPixels (with an AndroidOptions, no
less!))

https://codereview.chromium.org/1406223002/diff/20001/include/codec/SkAndroid...
include/codec/SkAndroidCodec.h:213: const SkImageInfo& fInfo;
Maybe a note that this will always be the info of the contained SkCodec? It
actually occurred to me independently (I glossed over it here, and thought about
it later) that this could be a const reference, but it's not something I would
typically see. I thought maybe it's relying on the implementation of SkCodec,
but given that SkCodec::getInfo returns a const reference, maybe this is fine
(just cannot change SkCodec's return value!_

https://codereview.chromium.org/1406223002/diff/20001/src/codec/SkAndroidCode...
File src/codec/SkAndroidCodec.cpp (right):

https://codereview.chromium.org/1406223002/diff/20001/src/codec/SkAndroidCode...
src/codec/SkAndroidCodec.cpp:15: // FIXME: As Leon has mentioned elsewhere,
surely there is also a maximum sampleSize?
I've seen some really high ones. We just need to make sure that we do not crash
or behave badly if the size is too large.

https://codereview.chromium.org/1406223002/diff/20001/src/codec/SkAndroidCode...
src/codec/SkAndroidCodec.cpp:52: if (!is_valid_sample_size(sampleSize)) {
Should we instead treat 0 as 1, as Android does?

https://codereview.chromium.org/1406223002/diff/20001/src/codec/SkAndroidCode...
src/codec/SkAndroidCodec.cpp:85: if (!pixels) {
On 2015/10/16 18:42:16, msarett wrote:
> There are some error cases that we don't check here because SkCodec will catch
> them later anyway (missing color table etc.).  Not sure if this is ok.

Seems fine to me.

https://codereview.chromium.org/1406223002/diff/20001/src/codec/SkAndroidCode...
src/codec/SkAndroidCodec.cpp:99: supportedSize =
this->getSampledDimensions(options->fSampleSize);
I hope these calls are cheap - it seems like a client may have already called
them once to determine the size to use. (They probably are, but it's something
to think about.)

https://codereview.chromium.org/1406223002/diff/20001/src/codec/SkScaledCodec...
File src/codec/SkScaledCodec.cpp (right):

https://codereview.chromium.org/1406223002/diff/20001/src/codec/SkScaledCodec...
src/codec/SkScaledCodec.cpp:17: static bool is_in_range(int coord, int offset,
int length) {
I still need to look over this file in more detail...

https://codereview.chromium.org/1406223002/diff/20001/src/codec/SkScaledCodec.h
File src/codec/SkScaledCodec.h (right):

https://codereview.chromium.org/1406223002/diff/20001/src/codec/SkScaledCodec...
src/codec/SkScaledCodec.h:17: class SkSampledCodec : public SkAndroidCodec {
I think this should be in the file SkSampledCodec.h, or it should be called
SkScaledCodec.

https://codereview.chromium.org/1406223002/diff/20001/src/codec/SkWebpAdapter...
File src/codec/SkWebpAdapterCodec.cpp (right):

https://codereview.chromium.org/1406223002/diff/20001/src/codec/SkWebpAdapter...
src/codec/SkWebpAdapterCodec.cpp:29: int width = SkTMax(1,
SkScalarRoundToInt(scale * (float) subset.width()));
On 2015/10/16 18:42:16, msarett wrote:
> Alternatively, we could use get_scaled_dimension() and be more uniform with
> SkSampledCodec.

I suppose this mimics the old behavior? I don't have a strong preference.

[msarett, 2015-10-19 16:02:15]

Patchset #2 (id:40001) has been deleted

[msarett, 2015-10-19 16:06:10]

https://codereview.chromium.org/1406223002/diff/20001/dm/DM.cpp
File dm/DM.cpp (right):

https://codereview.chromium.org/1406223002/diff/20001/dm/DM.cpp#newcode372
dm/DM.cpp:372: push_android_codec_src(path, mode, colorTypes[i], sampleSize);
On 2015/10/16 21:13:55, scroggo wrote:
> I wonder how much memory we would save DM by creating a single SkData per
image
> file, and reusing it with each CodecSrc, ImageSrc, and AndroidSrc that tests
the
> same image? Shouldn't go in this CL, but something to think about.

Acknowledged.

https://codereview.chromium.org/1406223002/diff/20001/dm/DMSrcSink.cpp
File dm/DMSrcSink.cpp (right):

https://codereview.chromium.org/1406223002/diff/20001/dm/DMSrcSink.cpp#newcod...
dm/DMSrcSink.cpp:752: if (nullptr == codec) {
On 2015/10/16 21:13:55, scroggo wrote:
> Maybe this reproduces the old code, but is it necessary? Don't we only create
> the Src if the trial run succeeded?
> 
> Oh, I think I'm confusing this with nanobench...

We'll create the AndroidCodecSrc before verifying that we can create an
AndroidCodec.

We check this in onDraw(), but size() will actually be called first to create
the SkCanvas.

Of course, we should always succeed when creating an AndroidCodec since this
test is only enabled for webp, jpeg, and png - but I think it's safer to check
than to seg fault.

That way we will fail when we fail to create an AndroidCodec in onDraw().

https://codereview.chromium.org/1406223002/diff/20001/include/codec/SkAndroid...
File include/codec/SkAndroidCodec.h (right):

https://codereview.chromium.org/1406223002/diff/20001/include/codec/SkAndroid...
include/codec/SkAndroidCodec.h:87: bool getSubset(SkIRect* desiredSubset) const;
On 2015/10/16 21:13:55, scroggo wrote:
> On 2015/10/16 18:42:16, msarett wrote:
> > I could have (and maybe should have) called this getValidSubset().
> > 
> > In my mind, "invalid subsets" are the ones that aren't contained by the
image.
> 
> > I didn't want to make it sound like this function would correct invalid
> subsets
> > for the client.
> 
> I think it's odd that this one is different from the one on SkCodec. Are you
> saying that it is named differently but it means the same thing?
> 
> Maybe we should come up with a better name for both of them. How about
> "getSupportedSubset"?

Yes they are the same.

I like getSupportedSubset().  I'll add a TODO for SkCodec.h.

https://codereview.chromium.org/1406223002/diff/20001/include/codec/SkAndroid...
include/codec/SkAndroidCodec.h:98: *  @return           Size of the scaled
partial image.  Or zero size
On 2015/10/16 21:13:55, scroggo wrote:
> Alternatively, we could return a boolean, and modify a passed in SkISize.

Agreed.

I went for this approach for consistency with getScaledDimensions() and
getSampledDimensions().

I think returning a bool is probably a cleaner way to indicate an error.  I
would be in favor of a separate change to all of these APIs, but don't feel too
strongly.

https://codereview.chromium.org/1406223002/diff/20001/include/codec/SkAndroid...
include/codec/SkAndroidCodec.h:185: SkCodec::Result getAndroidPixels(const
SkImageInfo& info, void* pixels, size_t rowBytes,
On 2015/10/16 21:13:55, scroggo wrote:
> I don't think it's necessary to put "Android" in these names. (It seems a
little
> redundant to call SkAndroidCodec::getAndroidPixels (with an AndroidOptions, no
> less!))

Yeah you're right.  I was struggling to come up with unique names from SkCodec. 
I'll add FIXMEs.

https://codereview.chromium.org/1406223002/diff/20001/include/codec/SkAndroid...
include/codec/SkAndroidCodec.h:213: const SkImageInfo& fInfo;
On 2015/10/16 21:13:55, scroggo wrote:
> Maybe a note that this will always be the info of the contained SkCodec? It
> actually occurred to me independently (I glossed over it here, and thought
about
> it later) that this could be a const reference, but it's not something I would
> typically see. I thought maybe it's relying on the implementation of SkCodec,
> but given that SkCodec::getInfo returns a const reference, maybe this is fine
> (just cannot change SkCodec's return value!_

Will add a note.  It may be a bit dangerous to depend on SkCodec in this way.

https://codereview.chromium.org/1406223002/diff/20001/src/codec/SkAndroidCode...
File src/codec/SkAndroidCodec.cpp (right):

https://codereview.chromium.org/1406223002/diff/20001/src/codec/SkAndroidCode...
src/codec/SkAndroidCodec.cpp:52: if (!is_valid_sample_size(sampleSize)) {
On 2015/10/16 21:13:55, scroggo wrote:
> Should we instead treat 0 as 1, as Android does?

I'm not sure.  It's annoying to catch...I've added code in three places.

But it's probably best to be robust and not depend on SkBitmapRegionDecoder to
catch this.

https://codereview.chromium.org/1406223002/diff/20001/src/codec/SkAndroidCode...
src/codec/SkAndroidCodec.cpp:99: supportedSize =
this->getSampledDimensions(options->fSampleSize);
On 2015/10/16 21:13:55, scroggo wrote:
> I hope these calls are cheap - it seems like a client may have already called
> them once to determine the size to use. (They probably are, but it's something
> to think about.)

They are not trivial, but I *think* they are cheap compared to decoding an
image.

Regardless, I am moving them out of this function.

I think the tradeoff is similar to calling dimensionsSupported in
SkCodec::getPixels().  It is a bit redundant, but good to catch kInvalidScale
before calling onGetPixels().

In this class, I don't think we are moving toward a goal of catching these types
of errors before calling onGetAndroidPixels() (especially because
onGetAndroidPixels() calls SkCodec::getPixels()).  So we might as well move this
check out of here and be less redundant.

https://codereview.chromium.org/1406223002/diff/20001/src/codec/SkScaledCodec.h
File src/codec/SkScaledCodec.h (right):

https://codereview.chromium.org/1406223002/diff/20001/src/codec/SkScaledCodec...
src/codec/SkScaledCodec.h:17: class SkSampledCodec : public SkAndroidCodec {
On 2015/10/16 21:13:55, scroggo wrote:
> I think this should be in the file SkSampledCodec.h, or it should be called
> SkScaledCodec.

Agreed.  I didn't want to modify and rename a file at the same time, but given
that I moved it anyway and it looks completely different, I should go ahead and
rename it.

https://codereview.chromium.org/1406223002/diff/20001/src/codec/SkWebpAdapter...
File src/codec/SkWebpAdapterCodec.cpp (right):

https://codereview.chromium.org/1406223002/diff/20001/src/codec/SkWebpAdapter...
src/codec/SkWebpAdapterCodec.cpp:29: int width = SkTMax(1,
SkScalarRoundToInt(scale * (float) subset.width()));
On 2015/10/16 21:13:55, scroggo wrote:
> On 2015/10/16 18:42:16, msarett wrote:
> > Alternatively, we could use get_scaled_dimension() and be more uniform with
> > SkSampledCodec.
> 
> I suppose this mimics the old behavior? I don't have a strong preference.

Using get_scaled_dimension() would actually be consistent with the old behavior.
 I think that's a good reason to use that.

[scroggo, 2015-10-19 20:03:20]

https://codereview.chromium.org/1406223002/diff/20001/dm/DMSrcSink.cpp
File dm/DMSrcSink.cpp (right):

https://codereview.chromium.org/1406223002/diff/20001/dm/DMSrcSink.cpp#newcod...
dm/DMSrcSink.cpp:752: if (nullptr == codec) {
On 2015/10/19 16:06:10, msarett wrote:
> On 2015/10/16 21:13:55, scroggo wrote:
> > Maybe this reproduces the old code, but is it necessary? Don't we only
create
> > the Src if the trial run succeeded?
> > 
> > Oh, I think I'm confusing this with nanobench...
> 
> We'll create the AndroidCodecSrc before verifying that we can create an
> AndroidCodec.
> 
> We check this in onDraw(), but size() will actually be called first to create
> the SkCanvas.
> 
> Of course, we should always succeed when creating an AndroidCodec since this
> test is only enabled for webp, jpeg, and png - but I think it's safer to check
> than to seg fault.
> 
> That way we will fail when we fail to create an AndroidCodec in onDraw().

sgtm

https://codereview.chromium.org/1406223002/diff/20001/src/codec/SkAndroidCode...
File src/codec/SkAndroidCodec.cpp (right):

https://codereview.chromium.org/1406223002/diff/20001/src/codec/SkAndroidCode...
src/codec/SkAndroidCodec.cpp:52: if (!is_valid_sample_size(sampleSize)) {
On 2015/10/19 16:06:10, msarett wrote:
> On 2015/10/16 21:13:55, scroggo wrote:
> > Should we instead treat 0 as 1, as Android does?
> 
> I'm not sure.  It's annoying to catch...I've added code in three places.
> 
> But it's probably best to be robust and not depend on SkBitmapRegionDecoder to
> catch this.

Haha, agreed, it is annoying to catch it in three places. Since the Android API
only passes it once, maybe this would be simpler by handling it upstack...

https://codereview.chromium.org/1406223002/diff/20001/src/codec/SkScaledCodec...
File src/codec/SkScaledCodec.cpp (right):

https://codereview.chromium.org/1406223002/diff/20001/src/codec/SkScaledCodec...
src/codec/SkScaledCodec.cpp:45: SkISize
SkSampledCodec::onGetSampledSubsetDimensions(int sampleSize, const SkIRect&
subset) const {
Interesting - this function is no longer virtual (this code is just inside
getSampledSubsetDimensions directly). Probably makes sense since we only have
two implementations and they're happy with the same answer. It might be a little
weird though, since it makes an assumption about what its subclass can do.

https://codereview.chromium.org/1406223002/diff/60001/src/codec/SkScaledCodec...
File src/codec/SkScaledCodec.cpp (left):

https://codereview.chromium.org/1406223002/diff/60001/src/codec/SkScaledCodec...
src/codec/SkScaledCodec.cpp:68: // Native scaling is preferred to sampling.  If
we can scale natively to
It looks like this comment didn't survive, but I think it's still applicable. I
was a bit surprised that we required it to be so close to native in order to use
native, but I think that makes sense because

1. We told them the best native options, and they refused it if they weren't
within 1 (maybe we should be even more strict - only use native if they
specified it exactly, but I suppose this allows for rounding differences?)
2. We can get closer with sampling

https://codereview.chromium.org/1406223002/diff/60001/src/codec/SkScaledCodec...
File src/codec/SkScaledCodec.cpp (right):

https://codereview.chromium.org/1406223002/diff/60001/src/codec/SkScaledCodec...
src/codec/SkScaledCodec.cpp:56: SkCodec::Result result = fCodec->getPixels(info,
pixels, rowBytes, &codecOptions,
Why not check fCodec->dimensionsSupported()? If true, you can just return
fCodec->getPixels(), and if not, you can go on to sample.

https://codereview.chromium.org/1406223002/diff/80001/src/codec/SkScaledCodec...
File src/codec/SkScaledCodec.cpp (right):

https://codereview.chromium.org/1406223002/diff/80001/src/codec/SkScaledCodec...
src/codec/SkScaledCodec.cpp:54: if (!subset || subset->size() ==
fCodec->getInfo().dimensions()) {
What if the codecs dimensions are W x H, and subset is (1, 1, W+1, H+1)? It
looks like we did not check for subset being within range?

https://codereview.chromium.org/1406223002/diff/80001/src/codec/SkScaledCodec...
src/codec/SkScaledCodec.cpp:56: SkCodec::Result result = fCodec->getPixels(info,
pixels, rowBytes, &codecOptions,
Why not check fCodec->dimensionsSupported()?

https://codereview.chromium.org/1406223002/diff/80001/src/codec/SkScaledCodec...
src/codec/SkScaledCodec.cpp:82: SkCodec::Result result =
fCodec->startScanlineDecode(info.makeWH(scaledSize.width(),
Why not check fCodec->dimensionsSupported()?

https://codereview.chromium.org/1406223002/diff/80001/src/codec/SkScaledCodec...
src/codec/SkScaledCodec.cpp:94: switch (fCodec->getScanlineOrder()) {
I think it might be helpful to have a comment that states that after this point,
we are only concerned with subsetting (either fCodec handled the scale, or no
scale was necessary).

https://codereview.chromium.org/1406223002/diff/80001/src/codec/SkScaledCodec...
src/codec/SkScaledCodec.cpp:109: case SkCodec::kBottomUp_SkScanlineOrder:
I think these we can leave out entirely for now, since they are only for BMP and
GIF?

https://codereview.chromium.org/1406223002/diff/80001/src/codec/SkScaledCodec...
src/codec/SkScaledCodec.cpp:161: info.makeWH(fCodec->getInfo().width(),
fCodec->getInfo().height()), &sampledOptions,
IIUC, this does not combine native sampling with sampling. Is that a TODO?

https://codereview.chromium.org/1406223002/diff/80001/src/codec/SkScaledCodec...
src/codec/SkScaledCodec.cpp:184: int y0 = get_start_coord(sampleY);
Maybe a comment that y0 is the start *after* any subsetting is considered? Or a
name that makes that clear? (Maybe it's obvious, but I got confused down
below...)

https://codereview.chromium.org/1406223002/diff/80001/src/codec/SkScaledCodec...
src/codec/SkScaledCodec.cpp:249: SkAutoMalloc storage((subsetHeight - y0) *
rowBytes);
I'm tempted to tell you to create a const local variable set to subsetHeight -
y0, although i'm not sure what to call it... linesNeeded?

https://codereview.chromium.org/1406223002/diff/80001/src/codec/SkScaledCodec...
src/codec/SkScaledCodec.cpp:258: void* pixelPtr = pixels;
Any opposition to just reusing pixel, rather than creating an extra variable?
(No one is going to need the original value of pixels, are they?)

https://codereview.chromium.org/1406223002/diff/80001/src/codec/SkScaledCodec...
src/codec/SkScaledCodec.cpp:259: while (y < dstHeight) {
I think this makes more sense as a for loop.

https://codereview.chromium.org/1406223002/diff/80001/src/codec/SkScaledCodec...
src/codec/SkScaledCodec.cpp:260: memcpy(pixelPtr, storagePtr,
info.minRowBytes());
Maybe call this once outside the loop? (Hopefully the compiler will figure that
out for you, so maybe I'm getting ahead of myself...)

[msarett, 2015-10-19 21:39:22]

https://codereview.chromium.org/1406223002/diff/20001/src/codec/SkAndroidCode...
File src/codec/SkAndroidCodec.cpp (right):

https://codereview.chromium.org/1406223002/diff/20001/src/codec/SkAndroidCode...
src/codec/SkAndroidCodec.cpp:52: if (!is_valid_sample_size(sampleSize)) {
On 2015/10/19 20:03:20, scroggo wrote:
> On 2015/10/19 16:06:10, msarett wrote:
> > On 2015/10/16 21:13:55, scroggo wrote:
> > > Should we instead treat 0 as 1, as Android does?
> > 
> > I'm not sure.  It's annoying to catch...I've added code in three places.
> > 
> > But it's probably best to be robust and not depend on SkBitmapRegionDecoder
to
> > catch this.
> 
> Haha, agreed, it is annoying to catch it in three places. Since the Android
API
> only passes it once, maybe this would be simpler by handling it upstack...

Let's plan to handle it upstack for now.

https://codereview.chromium.org/1406223002/diff/20001/src/codec/SkScaledCodec...
File src/codec/SkScaledCodec.cpp (right):

https://codereview.chromium.org/1406223002/diff/20001/src/codec/SkScaledCodec...
src/codec/SkScaledCodec.cpp:45: SkISize
SkSampledCodec::onGetSampledSubsetDimensions(int sampleSize, const SkIRect&
subset) const {
On 2015/10/19 20:03:20, scroggo wrote:
> Interesting - this function is no longer virtual (this code is just inside
> getSampledSubsetDimensions directly). Probably makes sense since we only have
> two implementations and they're happy with the same answer. It might be a
little
> weird though, since it makes an assumption about what its subclass can do.

Yeah I agree that it makes sense but is a little weird.

My other concern was that it's not as maintainable.  If/when we decide to
add/change a subclass, I'll get to add the virtual back in :).

https://codereview.chromium.org/1406223002/diff/60001/src/codec/SkScaledCodec...
File src/codec/SkScaledCodec.cpp (left):

https://codereview.chromium.org/1406223002/diff/60001/src/codec/SkScaledCodec...
src/codec/SkScaledCodec.cpp:68: // Native scaling is preferred to sampling.  If
we can scale natively to
On 2015/10/19 20:03:20, scroggo wrote:
> It looks like this comment didn't survive, but I think it's still applicable.

I'll add the comment back in.

> I was a bit surprised that we required it to be so close to native in order to
use
> native
> 

So the old code is a bit confusing, we actually still might choose to use the
native scale below.  Notice that the return statement will select whichever is
closer between the native scale and sampling scale.

The reason that the "within 1 check" existed was the following scenario:
originalWidth = 41
scale = 0.25
idealWidth = 10.25
nativeScaledWidth = 11
sampledWidth = 10

And then, without the extra check, we choose sampling because it's closer, even
though, IMO both scales are perfectly acceptable.

In the new scenario it's a little bit different because we know that we can any
support any sampleSize by sampling.  But we will choose to sample only if we
decide that the native scale is unacceptable (off by 1 or more).

https://codereview.chromium.org/1406223002/diff/60001/src/codec/SkScaledCodec...
File src/codec/SkScaledCodec.cpp (right):

https://codereview.chromium.org/1406223002/diff/60001/src/codec/SkScaledCodec...
src/codec/SkScaledCodec.cpp:56: SkCodec::Result result = fCodec->getPixels(info,
pixels, rowBytes, &codecOptions,
On 2015/10/19 20:03:20, scroggo wrote:
> Why not check fCodec->dimensionsSupported()? If true, you can just return
> fCodec->getPixels(), and if not, you can go on to sample.

sgtm

https://codereview.chromium.org/1406223002/diff/80001/src/codec/SkScaledCodec...
File src/codec/SkScaledCodec.cpp (right):

https://codereview.chromium.org/1406223002/diff/80001/src/codec/SkScaledCodec...
src/codec/SkScaledCodec.cpp:54: if (!subset || subset->size() ==
fCodec->getInfo().dimensions()) {
On 2015/10/19 20:03:20, scroggo wrote:
> What if the codecs dimensions are W x H, and subset is (1, 1, W+1, H+1)? It
> looks like we did not check for subset being within range?

Ugh we used to catch this in getAndroidPixels() before I removed code.  I'll put
some code back.

https://codereview.chromium.org/1406223002/diff/80001/src/codec/SkScaledCodec...
src/codec/SkScaledCodec.cpp:56: SkCodec::Result result = fCodec->getPixels(info,
pixels, rowBytes, &codecOptions,
On 2015/10/19 20:03:20, scroggo wrote:
> Why not check fCodec->dimensionsSupported()?

That is better.  I am changing it.

https://codereview.chromium.org/1406223002/diff/80001/src/codec/SkScaledCodec...
src/codec/SkScaledCodec.cpp:82: SkCodec::Result result =
fCodec->startScanlineDecode(info.makeWH(scaledSize.width(),
On 2015/10/19 20:03:20, scroggo wrote:
> Why not check fCodec->dimensionsSupported()?

You're right this is better.

https://codereview.chromium.org/1406223002/diff/80001/src/codec/SkScaledCodec...
src/codec/SkScaledCodec.cpp:94: switch (fCodec->getScanlineOrder()) {
On 2015/10/19 20:03:20, scroggo wrote:
> I think it might be helpful to have a comment that states that after this
point,
> we are only concerned with subsetting (either fCodec handled the scale, or no
> scale was necessary).

Done.

https://codereview.chromium.org/1406223002/diff/80001/src/codec/SkScaledCodec...
src/codec/SkScaledCodec.cpp:109: case SkCodec::kBottomUp_SkScanlineOrder:
On 2015/10/19 20:03:20, scroggo wrote:
> I think these we can leave out entirely for now, since they are only for BMP
and
> GIF?

Done.

https://codereview.chromium.org/1406223002/diff/80001/src/codec/SkScaledCodec...
src/codec/SkScaledCodec.cpp:161: info.makeWH(fCodec->getInfo().width(),
fCodec->getInfo().height()), &sampledOptions,
On 2015/10/19 20:03:20, scroggo wrote:
> IIUC, this does not combine native sampling with sampling. Is that a TODO?

Yes, we've never implemented that, but we need to.

skbug.com/4320

https://codereview.chromium.org/1406223002/diff/80001/src/codec/SkScaledCodec...
src/codec/SkScaledCodec.cpp:184: int y0 = get_start_coord(sampleY);
On 2015/10/19 20:03:20, scroggo wrote:
> Maybe a comment that y0 is the start *after* any subsetting is considered? Or
a
> name that makes that clear? (Maybe it's obvious, but I got confused down
> below...)

Done.

https://codereview.chromium.org/1406223002/diff/80001/src/codec/SkScaledCodec...
src/codec/SkScaledCodec.cpp:249: SkAutoMalloc storage((subsetHeight - y0) *
rowBytes);
On 2015/10/19 20:03:20, scroggo wrote:
> I'm tempted to tell you to create a const local variable set to subsetHeight -
> y0, although i'm not sure what to call it... linesNeeded?

Yeah I think this makes things better.

https://codereview.chromium.org/1406223002/diff/80001/src/codec/SkScaledCodec...
src/codec/SkScaledCodec.cpp:258: void* pixelPtr = pixels;
On 2015/10/19 20:03:20, scroggo wrote:
> Any opposition to just reusing pixel, rather than creating an extra variable?
> (No one is going to need the original value of pixels, are they?)

Not in this case, let's just use pixels.

https://codereview.chromium.org/1406223002/diff/80001/src/codec/SkScaledCodec...
src/codec/SkScaledCodec.cpp:259: while (y < dstHeight) {
On 2015/10/19 20:03:20, scroggo wrote:
> I think this makes more sense as a for loop.

Yes it definitely does.

https://codereview.chromium.org/1406223002/diff/80001/src/codec/SkScaledCodec...
src/codec/SkScaledCodec.cpp:260: memcpy(pixelPtr, storagePtr,
info.minRowBytes());
On 2015/10/19 20:03:20, scroggo wrote:
> Maybe call this once outside the loop? (Hopefully the compiler will figure
that
> out for you, so maybe I'm getting ahead of myself...)

I don't know if the compiler is that smart or not - I wasn't sharp enough to
notice.

I'll make the change, and we can keep it if it doesn't look too confusing.

[scroggo, 2015-10-20 13:51:59]

lgtm

https://codereview.chromium.org/1406223002/diff/100001/dm/DMSrcSink.cpp
File dm/DMSrcSink.cpp (right):

https://codereview.chromium.org/1406223002/diff/100001/dm/DMSrcSink.cpp#newco...
dm/DMSrcSink.cpp:614: Error AndroidCodecSrc::draw(SkCanvas* canvas) const {
Any thoughts on consolidating this code with CodecSrc?

https://codereview.chromium.org/1406223002/diff/100001/dm/DMSrcSink.cpp#newco...
dm/DMSrcSink.cpp:689: bitmap.eraseColor(0);
How will we handle this in BRD? Or is this a result of the fact that we have an
expectation about how we will stitch them together based on the sampled size
rather than based on the sum of the parts?

https://codereview.chromium.org/1406223002/diff/100001/dm/DMSrcSink.cpp#newco...
dm/DMSrcSink.cpp:690: for (uint32_t x = 0; x < divisor; x++) {
Why are x and y uint32_t when divisor is an int? (I think the compiler will
complain about comparing signed and unsigned.)

https://codereview.chromium.org/1406223002/diff/100001/dm/DMSrcSink.h
File dm/DMSrcSink.h (right):

https://codereview.chromium.org/1406223002/diff/100001/dm/DMSrcSink.h#newcode140
dm/DMSrcSink.h:140: AndroidCodecSrc(Path, Mode, CodecSrc::DstColorType, int);
Can you add /* sampleSize */ to int here? (I *think* you can even not put it in
comments, since this is just a declaration...)

https://codereview.chromium.org/1406223002/diff/100001/src/codec/SkAndroidCod...
File src/codec/SkAndroidCodec.cpp (right):

https://codereview.chromium.org/1406223002/diff/100001/src/codec/SkAndroidCod...
src/codec/SkAndroidCodec.cpp:15: // FIXME: As Leon has mentioned elsewhere,
surely there is also a maximum sampleSize?
Maybe it's bigger than the width/height of the image, which will fail later?

https://codereview.chromium.org/1406223002/diff/100001/src/codec/SkAndroidCod...
src/codec/SkAndroidCodec.cpp:85: return
SkISize::Make(get_scaled_dimension(subset.width(), sampleSize),
Maybe a note that this should perhaps be a virtual
(onGetSampledSubsetDimensions) but both subclasses want the same implementation?

https://codereview.chromium.org/1406223002/diff/100001/src/codec/SkScaledCode...
File src/codec/SkScaledCodec.cpp (right):

https://codereview.chromium.org/1406223002/diff/100001/src/codec/SkScaledCode...
src/codec/SkScaledCodec.cpp:199: // Note that we start the loop at one since we
have already copied the first line.
I'm not sure saving updating storagePtr and pixels on the last iteration is
worth the extra maintainability cost (duplicate code, starting the loop at one)

https://codereview.chromium.org/1406223002/diff/100001/tests/CodexTest.cpp
File tests/CodexTest.cpp (right):

https://codereview.chromium.org/1406223002/diff/100001/tests/CodexTest.cpp#ne...
tests/CodexTest.cpp:145: static void test_android_codec(skiatest::Reporter* r,
SkAndroidCodec* codec, SkBitmap& bm,
I suppose here's a case where it would've been handy for SkAndroidCodec and
SkCodec to implement the same interface...

[msarett, 2015-10-20 14:47:28]

Mike, can I get an API review?

https://codereview.chromium.org/1406223002/diff/100001/dm/DMSrcSink.cpp
File dm/DMSrcSink.cpp (right):

https://codereview.chromium.org/1406223002/diff/100001/dm/DMSrcSink.cpp#newco...
dm/DMSrcSink.cpp:614: Error AndroidCodecSrc::draw(SkCanvas* canvas) const {
On 2015/10/20 13:51:59, scroggo wrote:
> Any thoughts on consolidating this code with CodecSrc?

I'm guessing you are referring to sharing code between the draw() functions?

I originally thought that I would be able to factor out a lot more than I was
able to.  A few candidates for refactoring might be creating the SkData and
allocating pixels for the bitmap.  I decided against doing anything because they
are already one line function calls.

I tried to share the color table stuff, but I thought it ended up messier than
it is now.  We need all of those pointers and local variables to be in this
scope.

A lot of the other stuff is very slightly different - probably the reason I
thought it was best to split off AndroidCodecSrc from CodecSrc.  Ex:
SkAndroidCodec::NewFromData instead of SkCodec::NewFromData
getSampledDimensions instead of getScaledDimensions
fSampleSize instead of fScale

I'm a little annoyed at how similar they look, but don't have an immediate
solution.

https://codereview.chromium.org/1406223002/diff/100001/dm/DMSrcSink.cpp#newco...
dm/DMSrcSink.cpp:689: bitmap.eraseColor(0);
On 2015/10/20 13:51:59, scroggo wrote:
> How will we handle this in BRD? Or is this a result of the fact that we have
an
> expectation about how we will stitch them together based on the sampled size
> rather than based on the sum of the parts?

The second one.  As Mike noted, we potentially run into problems stitching the
image back together when we subset before we scale.

https://codereview.chromium.org/1406223002/diff/100001/dm/DMSrcSink.cpp#newco...
dm/DMSrcSink.cpp:690: for (uint32_t x = 0; x < divisor; x++) {
On 2015/10/20 13:51:59, scroggo wrote:
> Why are x and y uint32_t when divisor is an int? (I think the compiler will
> complain about comparing signed and unsigned.)

Thanks, my mistake.

https://codereview.chromium.org/1406223002/diff/100001/dm/DMSrcSink.h
File dm/DMSrcSink.h (right):

https://codereview.chromium.org/1406223002/diff/100001/dm/DMSrcSink.h#newcode140
dm/DMSrcSink.h:140: AndroidCodecSrc(Path, Mode, CodecSrc::DstColorType, int);
On 2015/10/20 13:51:59, scroggo wrote:
> Can you add /* sampleSize */ to int here? (I *think* you can even not put it
in
> comments, since this is just a declaration...)

Yes!

https://codereview.chromium.org/1406223002/diff/100001/src/codec/SkAndroidCod...
File src/codec/SkAndroidCodec.cpp (right):

https://codereview.chromium.org/1406223002/diff/100001/src/codec/SkAndroidCod...
src/codec/SkAndroidCodec.cpp:15: // FIXME: As Leon has mentioned elsewhere,
surely there is also a maximum sampleSize?
On 2015/10/20 13:51:59, scroggo wrote:
> Maybe it's bigger than the width/height of the image, which will fail later?

We actually don't even fail in this case.  We'll always suggest a size of at
least 1, and when we decode, we will modify the input sample size to the true
sample size that we actually need to get an output size of 1.

Assuming the logic that handles this isn't buggy, we can support arbitrarily
large sample sizes and return 1x1 images.  I think this is what SkImageDecoder
tried to do (but didn't always work).  I'm not sure if we want to set the
precedent of supporting this (but IIRC a few bugs were filed because clients
were already expecting this behavior).

https://codereview.chromium.org/1406223002/diff/100001/src/codec/SkAndroidCod...
src/codec/SkAndroidCodec.cpp:85: return
SkISize::Make(get_scaled_dimension(subset.width(), sampleSize),
On 2015/10/20 13:51:59, scroggo wrote:
> Maybe a note that this should perhaps be a virtual
> (onGetSampledSubsetDimensions) but both subclasses want the same
implementation?

Yes I think this is a good idea.

https://codereview.chromium.org/1406223002/diff/100001/src/codec/SkScaledCode...
File src/codec/SkScaledCodec.cpp (right):

https://codereview.chromium.org/1406223002/diff/100001/src/codec/SkScaledCode...
src/codec/SkScaledCodec.cpp:199: // Note that we start the loop at one since we
have already copied the first line.
On 2015/10/20 13:51:59, scroggo wrote:
> I'm not sure saving updating storagePtr and pixels on the last iteration is
> worth the extra maintainability cost (duplicate code, starting the loop at
one)

Agreed.  I would prefer to bet on the compiler anyway.

https://codereview.chromium.org/1406223002/diff/100001/tests/CodexTest.cpp
File tests/CodexTest.cpp (right):

https://codereview.chromium.org/1406223002/diff/100001/tests/CodexTest.cpp#ne...
tests/CodexTest.cpp:145: static void test_android_codec(skiatest::Reporter* r,
SkAndroidCodec* codec, SkBitmap& bm,
On 2015/10/20 13:51:59, scroggo wrote:
> I suppose here's a case where it would've been handy for SkAndroidCodec and
> SkCodec to implement the same interface...

Yes.

Although we would still run into some added complexity because they call
different APIs.

[reed1, 2015-10-21 17:04:47]

lgtm

[msarett, 2015-10-21 17:09:17]

The CQ bit was checked by msarett@google.com

[msarett, 2015-10-21 17:09:17]

The patchset sent to the CQ was uploaded after l-g-t-m from scroggo@google.com
Link to the patchset: https://codereview.chromium.org/1406223002/#ps120001
(title: "Response to Patch Set 4")

[commit-bot: I haz the power, 2015-10-21 17:09:29]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1406223002/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1406223002/120001

[commit-bot: I haz the power, 2015-10-21 17:11:46]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-10-21 17:11:47]

Try jobs failed on following builders:
  Build-Win-MSVC-x86-Debug-Trybot on client.skia.compile (JOB_FAILED,
http://build.chromium.org/p/client.skia.compile/builders/Build-Win-MSVC-x86-D...)

[msarett, 2015-10-21 17:17:38]

The CQ bit was checked by msarett@google.com

[msarett, 2015-10-21 17:17:39]

The patchset sent to the CQ was uploaded after l-g-t-m from scroggo@google.com,
reed@google.com
Link to the patchset: https://codereview.chromium.org/1406223002/#ps140001
(title: "Win bot fix")

[commit-bot: I haz the power, 2015-10-21 17:17:48]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1406223002/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1406223002/140001

[commit-bot: I haz the power, 2015-10-21 17:27:14]

Committed patchset #6 (id:140001) as
https://skia.googlesource.com/skia/+/3d9d7a7213748761d46ade183e11aa7cb55313c1