Refactor SkBitmapRegionDecoderInterface for Android

Refactor SkBitmapRegionDecoderInterface for Android

The result SkBitmap, the pixel allocator, and the alpha
preference need to be communicated from the client to
the region decoder.

BUG=skia:

Committed: https://skia.googlesource.com/skia/+/35e5d1b4495478ca3bede66914ae07f50a447c4d

[msarett, 2015-10-23 16:09:47]

Patchset #1 (id:1) has been deleted

[msarett, 2015-10-23 16:16:08]

Description was changed from

==========
stuff

BUG=skia:
==========

to

==========
We need to split decodeRegion() into two calls.

The first one, prepareRegion() needs to know all of the
decode parameters (subset, sampleSize, colorType etc.) in
order to determine the output SkImageInfo.

It returns the output SkImageInfo, so the client can then
allocate the pixels.

The client then calls decodeRegion().

BUG=skia:
==========

[msarett, 2015-10-23 16:25:50]

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

[msarett, 2015-10-23 16:25:51]

I'm not sure I'm thrilled with how this looks.  Looking for feedback.

Problems:
(1) kOriginal_Strategy is different than kCanvas and kAndroidCodec and does not
really fit the new API well.  I guess this is not that huge of an issue given
that we plan to eventually delete this Strategy.
(2) It is a bit awkward to require that one function is called before another. 
An alternative would be for the first call to be optional and act more like
getScaledDimensions().  But then we have to repeat the work on the actual
decode() call.
(3) In order to not repeat the work, we are caching a bunch of information in
fields.

Maybe it's not so bad as is, or maybe I should rewrite it to be more like
SkCodec?

[scroggo, 2015-10-27 15:00:51]

On 2015/10/23 16:25:51, msarett wrote:
> I'm not sure I'm thrilled with how this looks.  Looking for feedback.
> 
> Problems:
> (1) kOriginal_Strategy is different than kCanvas and kAndroidCodec and does
not
> really fit the new API well.  I guess this is not that huge of an issue given
> that we plan to eventually delete this Strategy.

Why do you think it doesn't fit the new API well? I feel like it actually fits
better, as evidenced by the fact that SkBitmapRegionSampler has a much simpler
implementation. (I suppose that also relates to the fact that we're implementing
an interface that sort of comes from one based on the original strategy).

> (2) It is a bit awkward to require that one function is called before another.

> An alternative would be for the first call to be optional and act more like
> getScaledDimensions().  But then we have to repeat the work on the actual
> decode() call.
> (3) In order to not repeat the work, we are caching a bunch of information in
> fields.

What if you used an SkBitmap::Allocator? That allows the caller to be in charge
of allocating the pixels, but it doesn't require two method calls, so we will
not need to cache all these values.

I could see the argument that we are moving away from SkBitmap in general in
Skia, but this class is already using an SkBitmap (which is necessary for the
current implementation in Android - we should be able to change that in the
future, but I'm not sure I want that to hold this up anyway...), and the Android
glue code already uses Allocators.

> 
> Maybe it's not so bad as is, or maybe I should rewrite it to be more like
> SkCodec?

Could you describe what that would look like? How would it be more like SkCodec?

(Some of my comments below may not be applicable if we change the overall
approach.)

https://codereview.chromium.org/1418093006/diff/20001/bench/BitmapRegionDecod...
File bench/BitmapRegionDecoderBench.cpp (right):

https://codereview.chromium.org/1418093006/diff/20001/bench/BitmapRegionDecod...
bench/BitmapRegionDecoderBench.cpp:75: fBRD->prepareRegion(fSubset, fSampleSize,
fColorType, false, &info);
You could also change this to:

  SkAssertResult(fBRD->prepareRegion(...));

This runs the code in release, and also asserts in debug. You could do the same
below.

https://codereview.chromium.org/1418093006/diff/20001/tools/SkBitmapRegionCan...
File tools/SkBitmapRegionCanvas.cpp (right):

https://codereview.chromium.org/1418093006/diff/20001/tools/SkBitmapRegionCan...
tools/SkBitmapRegionCanvas.cpp:24: if (kIndex_8_SkColorType == dstColorType ||
kGray_8_SkColorType == dstColorType) {
Should we also check for requireUnpremul here? I suppose it could be fine if we
end up not needing to do any drawing? And maybe we could even do index 8 if we
don't need to call cropBitmap? May not be worth doing all the special casing?

https://codereview.chromium.org/1418093006/diff/20001/tools/SkBitmapRegionCod...
File tools/SkBitmapRegionCodec.cpp (right):

https://codereview.chromium.org/1418093006/diff/20001/tools/SkBitmapRegionCod...
tools/SkBitmapRegionCodec.cpp:32: fSubsetType =
adjust_subset_rect(fCodec->getInfo().dimensions(), &fSubset, &outX, &outY);
If you wanted to, you could put an SkIRect on the stack instead of modifying
fSubset directly. That way, if you returned false, fSubset would remain
unmodified. You could take that further and set fSubset to something invalid at
the beginning of this function, so decodeRegion will never be called with some
half-correct fSubset (we'll know to fail as if prepareRegion was never called to
begin with).

https://codereview.chromium.org/1418093006/diff/20001/tools/SkBitmapRegionCod...
tools/SkBitmapRegionCodec.cpp:80: if (bitmap.width() < fOutInfo.width() ||
bitmap.height() < fOutInfo.height() ||
Should this just check whether bitmap.info() == fOutInfo? Down below it looks
like you're assuming that they used fOutInfo

https://codereview.chromium.org/1418093006/diff/20001/tools/SkBitmapRegionDec...
File tools/SkBitmapRegionDecoderInterface.h (right):

https://codereview.chromium.org/1418093006/diff/20001/tools/SkBitmapRegionDec...
tools/SkBitmapRegionDecoderInterface.h:62: * @return true if the desiredSubset
intersects at least part of the image
Should this return a SubsetType?

https://codereview.chromium.org/1418093006/diff/20001/tools/SkBitmapRegionDec...
tools/SkBitmapRegionDecoderInterface.h:82: * @param bitmap Must be large enough
to contain the requested subset.
I find this comment confusing. I guess you're saying that you expect the caller
has called setInfo no matter what, and allocPixels except for kOriginal.

https://codereview.chromium.org/1418093006/diff/20001/tools/SkBitmapRegionDec...
tools/SkBitmapRegionDecoderInterface.h:88: virtual bool decodeRegion(SkBitmap&
bitmap) = 0;
Typically we use a pointer if we are going to modify it.

https://codereview.chromium.org/1418093006/diff/20001/tools/SkBitmapRegionSam...
File tools/SkBitmapRegionSampler.cpp (right):

https://codereview.chromium.org/1418093006/diff/20001/tools/SkBitmapRegionSam...
tools/SkBitmapRegionSampler.cpp:31: dstColorType = kAlpha_8_SkColorType;
note that we'll need to update the glue code in Android, which requests
kAlpha_8. We also may want to update it so that it reports ALPHA_8 (in Java) if
we end up with a bitmap that is kAlpha_8.

https://codereview.chromium.org/1418093006/diff/20001/tools/SkCodecTools.h
File tools/SkCodecTools.h (right):

https://codereview.chromium.org/1418093006/diff/20001/tools/SkCodecTools.h#ne...
tools/SkCodecTools.h:33: inline SkBitmapRegionDecoderInterface::SubsetType
adjust_subset_rect(const SkISize& imageDims,
Should this be a method on SkBitmapRegionDecoderInterface?

[msarett, 2015-10-27 18:12:17]

Description was changed from

==========
We need to split decodeRegion() into two calls.

The first one, prepareRegion() needs to know all of the
decode parameters (subset, sampleSize, colorType etc.) in
order to determine the output SkImageInfo.

It returns the output SkImageInfo, so the client can then
allocate the pixels.

The client then calls decodeRegion().

BUG=skia:
==========

to

==========
The result SkBitmap, the pixel allocator, and the alpha
preference need to be communicated from the client to
the region decoder.

BUG=skia:
==========

[msarett, 2015-10-27 18:16:14]

Patchset #2 (id:40001) has been deleted

[msarett, 2015-10-27 18:17:44]

Here's a completely different approach to changing the
BitmapRegionDecoder API per our discussion in person.

[scroggo, 2015-10-27 18:53:55]

On 2015/10/27 18:17:44, msarett wrote:
> Here's a completely different approach to changing the
> BitmapRegionDecoder API per our discussion in person.

I think this looks a lot better.

> Refactor SkBitmapRegionDecoderInterface for Android
This should go in the change description (this is a little confusing, but as
written it will just go in the emails - the commit won't include it).

https://codereview.chromium.org/1418093006/diff/60001/dm/DMSrcSink.cpp
File dm/DMSrcSink.cpp (right):

https://codereview.chromium.org/1418093006/diff/60001/dm/DMSrcSink.cpp#newcod...
dm/DMSrcSink.cpp:131: // FIXME: Make this a fatal error.
Why is this not fatal, and what do you need to do to make it a fatal error?
(Same below)

https://codereview.chromium.org/1418093006/diff/60001/tools/SkBitmapRegionSam...
File tools/SkBitmapRegionSampler.cpp (right):

https://codereview.chromium.org/1418093006/diff/60001/tools/SkBitmapRegionSam...
tools/SkBitmapRegionSampler.cpp:23: fDecoder->setAllocator(allocator);
I think you need to undo this - setAllocator will increase the ref count on
allocator, and hang on to a pointer to it. If it was on the stack, we cannot
hang on to that pointer (or the ref). So this should be something like:

fDecoder->setAllocator(allocator);
...
const bool success = fDecoder->decodeSubset(...);
fDecoder->setAllocator(nullptr);
return success;

[msarett, 2015-10-27 19:05:23]

Description was changed from

==========
The result SkBitmap, the pixel allocator, and the alpha
preference need to be communicated from the client to
the region decoder.

BUG=skia:
==========

to

==========
Refactor SkBitmapRegionDecoderInterface for Android

The result SkBitmap, the pixel allocator, and the alpha
preference need to be communicated from the client to
the region decoder.

BUG=skia:
==========

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

https://codereview.chromium.org/1418093006/diff/60001/dm/DMSrcSink.cpp
File dm/DMSrcSink.cpp (right):

https://codereview.chromium.org/1418093006/diff/60001/dm/DMSrcSink.cpp#newcod...
dm/DMSrcSink.cpp:131: // FIXME: Make this a fatal error.
On 2015/10/27 18:53:55, scroggo wrote:
> Why is this not fatal, and what do you need to do to make it a fatal error?
> (Same below)

I *think* the only thing that needs to be done is to disable webps for
Canvas_Strategy.  Canvas_Strategy uses a scanline decoder.

I was going to make it fatal in this CL, but I realized that it would require an
additional change and didn't want to add too much in one go.

A bad color conversion in kOriginal_Strategy may also fail here.  I know that
some of the time, they will just decode to a default colorType, but I'm not sure
if it's all of the time.

https://codereview.chromium.org/1418093006/diff/60001/tools/SkBitmapRegionSam...
File tools/SkBitmapRegionSampler.cpp (right):

https://codereview.chromium.org/1418093006/diff/60001/tools/SkBitmapRegionSam...
tools/SkBitmapRegionSampler.cpp:23: fDecoder->setAllocator(allocator);
On 2015/10/27 18:53:55, scroggo wrote:
> I think you need to undo this - setAllocator will increase the ref count on
> allocator, and hang on to a pointer to it. If it was on the stack, we cannot
> hang on to that pointer (or the ref). So this should be something like:
> 
> fDecoder->setAllocator(allocator);
> ...
> const bool success = fDecoder->decodeSubset(...);
> fDecoder->setAllocator(nullptr);
> return success;

Nice catch!

[scroggo, 2015-10-27 19:23:37]

Other than a request for some better documentation, lgtm

https://codereview.chromium.org/1418093006/diff/60001/dm/DMSrcSink.cpp
File dm/DMSrcSink.cpp (right):

https://codereview.chromium.org/1418093006/diff/60001/dm/DMSrcSink.cpp#newcod...
dm/DMSrcSink.cpp:131: // FIXME: Make this a fatal error.
On 2015/10/27 19:06:16, msarett wrote:
> On 2015/10/27 18:53:55, scroggo wrote:
> > Why is this not fatal, and what do you need to do to make it a fatal error?
> > (Same below)
> 
> I *think* the only thing that needs to be done is to disable webps for
> Canvas_Strategy.  Canvas_Strategy uses a scanline decoder.
> 
> I was going to make it fatal in this CL, but I realized that it would require
an
> additional change and didn't want to add too much in one go.
> 
> A bad color conversion in kOriginal_Strategy may also fail here.  I know that
> some of the time, they will just decode to a default colorType, but I'm not
sure
> if it's all of the time.

I'm fine if that is left to a separate change, but can you update the comment
with more details and file a bug?

[msarett, 2015-10-27 19:39:36]

The CQ bit was checked by msarett@google.com

[msarett, 2015-10-27 19:39:37]

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/1418093006/#ps100001
(title: "Better comments")

[commit-bot: I haz the power, 2015-10-27 19:39:50]

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

[commit-bot: I haz the power, 2015-10-27 19:50:28]

Committed patchset #4 (id:100001) as
https://skia.googlesource.com/skia/+/35e5d1b4495478ca3bede66914ae07f50a447c4d