Implementation of SkBitmapRegionDecoder using SkAndroidCodec

Implementation of SkBitmapRegionDecoder using SkAndroidCodec

Includes testing in DM and nanobench

BUG=skia:

Committed: https://skia.googlesource.com/skia/+/26ad17b8f872fc8bc18df7f49067edbd8b9799e8

[msarett, 2015-10-12 18:42:36]

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

[msarett, 2015-10-12 18:42:36]

And here is the SkBitmapRegionDecoder implementation.

https://codereview.chromium.org/1402863002/diff/1/tools/SkBitmapRegionCanvas.cpp
File tools/SkBitmapRegionCanvas.cpp (left):

https://codereview.chromium.org/1402863002/diff/1/tools/SkBitmapRegionCanvas....
tools/SkBitmapRegionCanvas.cpp:19: * Chooses the correct image subset offsets
and dimensions for the partial decode.
Moved to SkCodecTools.h

The only change is that this is now "inline" instead of "static".

https://codereview.chromium.org/1402863002/diff/1/tools/SkBitmapRegionCanvas.cpp
File tools/SkBitmapRegionCanvas.cpp (right):

https://codereview.chromium.org/1402863002/diff/1/tools/SkBitmapRegionCanvas....
tools/SkBitmapRegionCanvas.cpp:35: // Fix the input sampleSize if necessary.
Android does not set a default sampleSize.  We should fix this is Android, but
also catch it here to be robust.

https://codereview.chromium.org/1402863002/diff/1/tools/SkBitmapRegionCanvas....
tools/SkBitmapRegionCanvas.cpp:136: if (!imageContainsEntireSubset) {
Oops.

https://codereview.chromium.org/1402863002/diff/1/tools/SkBitmapRegionCodec.cpp
File tools/SkBitmapRegionCodec.cpp (right):

https://codereview.chromium.org/1402863002/diff/1/tools/SkBitmapRegionCodec.c...
tools/SkBitmapRegionCodec.cpp:78: // Ask the codec for a scaled subset
Up until here, this implementation is the same as SkBitmapRegionCanvas.

[scroggo, 2015-10-12 19:18:09]

https://codereview.chromium.org/1402863002/diff/1/tools/SkBitmapRegionCodec.cpp
File tools/SkBitmapRegionCodec.cpp (right):

https://codereview.chromium.org/1402863002/diff/1/tools/SkBitmapRegionCodec.c...
tools/SkBitmapRegionCodec.cpp:78: // Ask the codec for a scaled subset
On 2015/10/12 18:42:36, msarett wrote:
> Up until here, this implementation is the same as SkBitmapRegionCanvas.

Would it be possible to pull out a common function? Something like:

bool adjust_rect(SkIRect* subset) {
 ...
}

I guess you have three conditions you want to know:
- No intersection
- partially inside
- Fully inside

Maybe there's a clear way to do this?

https://codereview.chromium.org/1402863002/diff/1/tools/SkBitmapRegionCodec.c...
tools/SkBitmapRegionCodec.cpp:157: if (SkCodec::kSuccess != result &&
SkCodec::kIncompleteInput != result) {
Why is incomplete considered fatal?

https://codereview.chromium.org/1402863002/diff/1/tools/SkBitmapRegionDecoder...
File tools/SkBitmapRegionDecoderInterface.h (right):

https://codereview.chromium.org/1402863002/diff/1/tools/SkBitmapRegionDecoder...
tools/SkBitmapRegionDecoderInterface.h:24: kCodec_Strategy,    // Uses
SkScaledCodec for scaling and subsetting
nit: Maybe this should be kScaledCodec_Strategy

[msarett, 2015-10-21 19:08:34]

Patchset #3 (id:40001) has been deleted

[msarett, 2015-10-21 19:15:10]

Since your last review:

Patch Set 2 responds to comments.

Patch Set 3 rebases on SkAndroidCodec.h

Patch Set 4 renames the strategy kAndroidCodec_Strategy.

https://codereview.chromium.org/1402863002/diff/1/tools/SkBitmapRegionCodec.cpp
File tools/SkBitmapRegionCodec.cpp (right):

https://codereview.chromium.org/1402863002/diff/1/tools/SkBitmapRegionCodec.c...
tools/SkBitmapRegionCodec.cpp:78: // Ask the codec for a scaled subset
On 2015/10/12 19:18:09, scroggo wrote:
> On 2015/10/12 18:42:36, msarett wrote:
> > Up until here, this implementation is the same as SkBitmapRegionCanvas.
> 
> Would it be possible to pull out a common function? Something like:
> 
> bool adjust_rect(SkIRect* subset) {
>  ...
> }
> 
> I guess you have three conditions you want to know:
> - No intersection
> - partially inside
> - Fully inside
> 
> Maybe there's a clear way to do this?

I think this is a little cleaner.  Let me know what you think.

https://codereview.chromium.org/1402863002/diff/1/tools/SkBitmapRegionCodec.c...
tools/SkBitmapRegionCodec.cpp:157: if (SkCodec::kSuccess != result &&
SkCodec::kIncompleteInput != result) {
On 2015/10/12 19:18:09, scroggo wrote:
> Why is incomplete considered fatal?

Success and incomplete are both non-errors.  Alternatively, this could be a
switch statement.

https://codereview.chromium.org/1402863002/diff/1/tools/SkBitmapRegionDecoder...
File tools/SkBitmapRegionDecoderInterface.h (right):

https://codereview.chromium.org/1402863002/diff/1/tools/SkBitmapRegionDecoder...
tools/SkBitmapRegionDecoderInterface.h:24: kCodec_Strategy,    // Uses
SkScaledCodec for scaling and subsetting
On 2015/10/12 19:18:09, scroggo wrote:
> nit: Maybe this should be kScaledCodec_Strategy

Done.

[msarett, 2015-10-21 19:15:42]

Description was changed from

==========
Implementation of SkBitmapRegionDecoder using SkScaledCodec

Includes testing in DM and nanobench

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

to

==========
Implementation of SkBitmapRegionDecoder using SkAndroidCodec

Includes testing in DM and nanobench

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

[scroggo, 2015-10-21 20:56:20]

https://codereview.chromium.org/1402863002/diff/80001/dm/DM.cpp
File dm/DM.cpp (right):

https://codereview.chromium.org/1402863002/diff/80001/dm/DM.cpp#newcode421
dm/DM.cpp:421: folder.append("brd_scaled_codec");
brd_android_codec?

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

https://codereview.chromium.org/1402863002/diff/80001/tools/SkBitmapRegionCod...
tools/SkBitmapRegionCodec.cpp:34: // requested region, not by the size of the
intersection of the region
nit: region -> subset (although, to be fair, this function is called
decodeRegion, to match BRD...)

https://codereview.chromium.org/1402863002/diff/80001/tools/SkBitmapRegionCod...
tools/SkBitmapRegionCodec.cpp:78: //                 decode into and then create
the bitmap after
We certainly can. Using an SkBitmap early on is just a convenience. Instead, we
would need to call SkImageInfo::getSize(SkImageInfo::minRowBytes()) to determine
how much memory to allocate.

https://codereview.chromium.org/1402863002/diff/80001/tools/SkBitmapRegionCod...
tools/SkBitmapRegionCodec.cpp:117: bitmap->eraseColor(0);
Won't this not work for kIndex_8?

https://codereview.chromium.org/1402863002/diff/80001/tools/SkBitmapRegionDec...
File tools/SkBitmapRegionDecoderInterface.cpp (right):

https://codereview.chromium.org/1402863002/diff/80001/tools/SkBitmapRegionDec...
tools/SkBitmapRegionDecoderInterface.cpp:53:
SkAndroidCodec::NewFromStream(streamDeleter.detach());
Since this targets BRD, which always uses an SkMemoryStream/SkData, maybe we
should call NewFromData instead?

https://codereview.chromium.org/1402863002/diff/80001/tools/SkCodecTools.h
File tools/SkCodecTools.h (right):

https://codereview.chromium.org/1402863002/diff/80001/tools/SkCodecTools.h#ne...
tools/SkCodecTools.h:18: kInvalid_SubsetType,
Maybe kOutside?

https://codereview.chromium.org/1402863002/diff/80001/tools/SkCodecTools.h#ne...
tools/SkCodecTools.h:47: // Make sure we don't decode pixels past the edge of
the image or past the edge of the region.
nit: region -> subset

Android called it BitmapRegionDecoder, but "Region" means something different in
Skia (SkRegion). It also means something different in Android (the same as in
Skia, actually). We're stuck with that name for the Java API (and I'm fine to
use it in e.g. BitmapRegionDecoderInterface etc), but I think we should say
"subset" here. (You could argue that subset need not be rectangular, but we
generally think of them as such.)

[msarett, 2015-10-21 22:00:00]

https://codereview.chromium.org/1402863002/diff/80001/dm/DM.cpp
File dm/DM.cpp (right):

https://codereview.chromium.org/1402863002/diff/80001/dm/DM.cpp#newcode421
dm/DM.cpp:421: folder.append("brd_scaled_codec");
On 2015/10/21 20:56:20, scroggo wrote:
> brd_android_codec?

Yes!

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

https://codereview.chromium.org/1402863002/diff/80001/tools/SkBitmapRegionCod...
tools/SkBitmapRegionCodec.cpp:34: // requested region, not by the size of the
intersection of the region
On 2015/10/21 20:56:20, scroggo wrote:
> nit: region -> subset (although, to be fair, this function is called
> decodeRegion, to match BRD...)

Done.

https://codereview.chromium.org/1402863002/diff/80001/tools/SkBitmapRegionCod...
tools/SkBitmapRegionCodec.cpp:78: //                 decode into and then create
the bitmap after
On 2015/10/21 20:56:20, scroggo wrote:
> We certainly can. Using an SkBitmap early on is just a convenience. Instead,
we
> would need to call SkImageInfo::getSize(SkImageInfo::minRowBytes()) to
determine
> how much memory to allocate.

Acknowledged.

https://codereview.chromium.org/1402863002/diff/80001/tools/SkBitmapRegionCod...
tools/SkBitmapRegionCodec.cpp:117: bitmap->eraseColor(0);
On 2015/10/21 20:56:20, scroggo wrote:
> Won't this not work for kIndex_8?

Ah yes...
Will fix.

https://codereview.chromium.org/1402863002/diff/80001/tools/SkBitmapRegionDec...
File tools/SkBitmapRegionDecoderInterface.cpp (right):

https://codereview.chromium.org/1402863002/diff/80001/tools/SkBitmapRegionDec...
tools/SkBitmapRegionDecoderInterface.cpp:53:
SkAndroidCodec::NewFromStream(streamDeleter.detach());
On 2015/10/21 20:56:20, scroggo wrote:
> Since this targets BRD, which always uses an SkMemoryStream/SkData, maybe we
> should call NewFromData instead?

Yeah I think so.

SkDatas are more flexible than SkStreams, and you've already started looking at
implementing some optimizations that take advantage of using an SkData.  Let's
use NewFromData when we can.

https://codereview.chromium.org/1402863002/diff/80001/tools/SkCodecTools.h
File tools/SkCodecTools.h (right):

https://codereview.chromium.org/1402863002/diff/80001/tools/SkCodecTools.h#ne...
tools/SkCodecTools.h:18: kInvalid_SubsetType,
On 2015/10/21 20:56:20, scroggo wrote:
> Maybe kOutside?

Done.

https://codereview.chromium.org/1402863002/diff/80001/tools/SkCodecTools.h#ne...
tools/SkCodecTools.h:47: // Make sure we don't decode pixels past the edge of
the image or past the edge of the region.
On 2015/10/21 20:56:20, scroggo wrote:
> nit: region -> subset
> 
> Android called it BitmapRegionDecoder, but "Region" means something different
in
> Skia (SkRegion). It also means something different in Android (the same as in
> Skia, actually). We're stuck with that name for the Java API (and I'm fine to
> use it in e.g. BitmapRegionDecoderInterface etc), but I think we should say
> "subset" here. (You could argue that subset need not be rectangular, but we
> generally think of them as such.)

Yes thanks!

I generally try to say subset, but I've been slipping up in this class.

[scroggo, 2015-10-22 13:49:02]

lgtm

[msarett, 2015-10-22 13:51:12]

The CQ bit was checked by msarett@google.com

[commit-bot: I haz the power, 2015-10-22 13:51:19]

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

[commit-bot: I haz the power, 2015-10-22 13:53:30]

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

[commit-bot: I haz the power, 2015-10-22 13:53:30]

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

[msarett, 2015-10-22 13:58:00]

The CQ bit was checked by msarett@google.com

[msarett, 2015-10-22 13:58:00]

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/1402863002/#ps120001
(title: "Win bot fix")

[commit-bot: I haz the power, 2015-10-22 13:58:09]

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

[commit-bot: I haz the power, 2015-10-22 14:11:23]

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

[commit-bot: I haz the power, 2015-10-22 14:11:24]

Try jobs failed on following builders:
  Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Debug-Trybot on client.skia (JOB_FAILED,
http://build.chromium.org/p/client.skia/builders/Test-Ubuntu-GCC-GCE-CPU-AVX2...)

[msarett, 2015-10-22 14:18:10]

The CQ bit was checked by msarett@google.com

[msarett, 2015-10-22 14:18:11]

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/1402863002/#ps140001
(title: "Add AndroidCodec strategy to benchmark")

[commit-bot: I haz the power, 2015-10-22 14:18:14]

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

[commit-bot: I haz the power, 2015-10-22 14:29:21]

Committed patchset #7 (id:140001) as
https://skia.googlesource.com/skia/+/26ad17b8f872fc8bc18df7f49067edbd8b9799e8