Scanline decoding for bmp

Scanline decoding for bmp

Redesigns SkScanlineDecoder.h to indicate the ordering
in which the scanlines are provided

Refactors SkSwizzler::Fill() to include the zeroInit check
and to actually be correct.

BUG=skia:3257
BUG=skia:4198
Committed: https://skia.googlesource.com/skia/+/5406d6f39ad042e7a0a0d4ea16beca4fe2b66492

[msarett, 2015-08-13 20:55:52]

Patchset #1 (id:1) has been deleted

[msarett, 2015-08-13 21:01:51]

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

[msarett, 2015-08-13 21:01:52]

I'm uploading now in the interest of breaking up this change.  This is not ready
to land as is.

This works properly on a call to getScanlines(height).

However, calling one line at a time results in an upside down image and calling
on smaller chunks results in weird outputs.

I think that the next step is implementing the reordering logic in dm and
SkScaledCodec.

[scroggo, 2015-08-13 21:25:36]

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

https://codereview.chromium.org/1287423002/diff/20001/src/codec/SkBmpCodec.cp...
src/codec/SkBmpCodec.cpp:571: return NULL;
Can this case be reached?

https://codereview.chromium.org/1287423002/diff/20001/src/codec/SkBmpMaskCode...
File src/codec/SkBmpMaskCodec.cpp (right):

https://codereview.chromium.org/1287423002/diff/20001/src/codec/SkBmpMaskCode...
src/codec/SkBmpMaskCodec.cpp:170: fDstInfo = dstInfo;
Can you get this from the base class, which also stores it?

Aha - it looks like you are, and you never use this copy of it. Please remove.

https://codereview.chromium.org/1287423002/diff/20001/src/codec/SkBmpMaskCode...
src/codec/SkBmpMaskCodec.cpp:171: fOpts = options;
Maybe this should be stored in the base class as well?

https://codereview.chromium.org/1287423002/diff/20001/src/codec/SkBmpMaskCode...
src/codec/SkBmpMaskCodec.cpp:179: SkImageInfo rowInfo =
fDstInfo.makeWH(dstInfo().width(), count);
nit: this->dstInfo()...

https://codereview.chromium.org/1287423002/diff/20001/src/codec/SkBmpStandard...
File src/codec/SkBmpStandardCodec.cpp (right):

https://codereview.chromium.org/1287423002/diff/20001/src/codec/SkBmpStandard...
src/codec/SkBmpStandardCodec.cpp:369: SkCodec::Result
SkBmpStandardScanlineDecoder::onStart(const SkImageInfo& dstInfo,
Here's how I see these. Let me know if I'm wrong:

1. All three scanline decoders have the same implementation of onGetScanlines.
2. They have differences in onStart, which are particular to the subclass of
SkBmpCodec.

What if we do the following instead?
- Add a virtual function to SkBmpCodec which does onStart
- Write one scanline decoder which does the following:

SkBmpScanlineDecoder::onStart(...) {
  return fCodec->onStart(...);
}

SkBmpScanlineDecoder::onGetScanlines(...) {
  return fCodec->decode(...);
}

https://codereview.chromium.org/1287423002/diff/20001/src/codec/SkBmpStandard...
src/codec/SkBmpStandardCodec.cpp:417: SkCodec::Result r =
fCodec->decode(rowInfo, dst, rowBytes, fOpts);
This needs to be returned. (probably true for the others)

[msarett, 2015-08-14 15:44:48]

All of these things are still true:

>This works properly on a call to getScanlines(height).
>
>However, calling one line at a time results in an upside
>down image and calling on smaller chunks results in weird
>outputs.
>
>I think that the next step is implementing the reordering >logic in dm and
SkScaledCodec.

But I think this looks a lot nicer after responding to comments and a couple
rebases.

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

https://codereview.chromium.org/1287423002/diff/20001/src/codec/SkBmpCodec.cp...
src/codec/SkBmpCodec.cpp:571: return NULL;
On 2015/08/13 21:25:35, scroggo wrote:
> Can this case be reached?

No. Adding an SkASSERT(false).

https://codereview.chromium.org/1287423002/diff/20001/src/codec/SkBmpMaskCode...
File src/codec/SkBmpMaskCodec.cpp (right):

https://codereview.chromium.org/1287423002/diff/20001/src/codec/SkBmpMaskCode...
src/codec/SkBmpMaskCodec.cpp:170: fDstInfo = dstInfo;
On 2015/08/13 21:25:36, scroggo wrote:
> Can you get this from the base class, which also stores it?
> 
> Aha - it looks like you are, and you never use this copy of it. Please remove.

Done.

https://codereview.chromium.org/1287423002/diff/20001/src/codec/SkBmpMaskCode...
src/codec/SkBmpMaskCodec.cpp:171: fOpts = options;
On 2015/08/13 21:25:36, scroggo wrote:
> Maybe this should be stored in the base class as well?

fOptions is now stored in the base class.

https://codereview.chromium.org/1287423002/diff/20001/src/codec/SkBmpMaskCode...
src/codec/SkBmpMaskCodec.cpp:179: SkImageInfo rowInfo =
fDstInfo.makeWH(dstInfo().width(), count);
On 2015/08/13 21:25:35, scroggo wrote:
> nit: this->dstInfo()...

Done.

https://codereview.chromium.org/1287423002/diff/20001/src/codec/SkBmpStandard...
File src/codec/SkBmpStandardCodec.cpp (right):

https://codereview.chromium.org/1287423002/diff/20001/src/codec/SkBmpStandard...
src/codec/SkBmpStandardCodec.cpp:369: SkCodec::Result
SkBmpStandardScanlineDecoder::onStart(const SkImageInfo& dstInfo,
On 2015/08/13 21:25:36, scroggo wrote:
> Here's how I see these. Let me know if I'm wrong:
> 
> 1. All three scanline decoders have the same implementation of onGetScanlines.
> 2. They have differences in onStart, which are particular to the subclass of
> SkBmpCodec.
> 
> What if we do the following instead?
> - Add a virtual function to SkBmpCodec which does onStart
> - Write one scanline decoder which does the following:
> 
> SkBmpScanlineDecoder::onStart(...) {
>   return fCodec->onStart(...);
> }
> 
> SkBmpScanlineDecoder::onGetScanlines(...) {
>   return fCodec->decode(...);
> }

This is clever!

https://codereview.chromium.org/1287423002/diff/20001/src/codec/SkBmpStandard...
src/codec/SkBmpStandardCodec.cpp:417: SkCodec::Result r =
fCodec->decode(rowInfo, dst, rowBytes, fOpts);
On 2015/08/13 21:25:36, scroggo wrote:
> This needs to be returned. (probably true for the others)

Hmm yes.  Thanks for being a better error checker than my compiler.

[scroggo, 2015-08-14 21:53:01]

https://codereview.chromium.org/1287423002/diff/20001/src/codec/SkBmpStandard...
File src/codec/SkBmpStandardCodec.cpp (right):

https://codereview.chromium.org/1287423002/diff/20001/src/codec/SkBmpStandard...
src/codec/SkBmpStandardCodec.cpp:417: SkCodec::Result r =
fCodec->decode(rowInfo, dst, rowBytes, fOpts);
On 2015/08/14 15:44:48, msarett wrote:
> On 2015/08/13 21:25:36, scroggo wrote:
> > This needs to be returned. (probably true for the others)
> 
> Hmm yes.  Thanks for being a better error checker than my compiler.

I just filed skbug.com/4208, where I proposed doing some sort of automatic
check. The only thing I've found is -fsanitize=return, which does a runtime
check.

https://codereview.chromium.org/1287423002/diff/40001/src/codec/SkBmpCodec.cpp
File src/codec/SkBmpCodec.cpp (right):

https://codereview.chromium.org/1287423002/diff/40001/src/codec/SkBmpCodec.cp...
src/codec/SkBmpCodec.cpp:597: // TODO(msarett): Consider other optimizations for
this codec.
Do you have any optimizations in mind?

https://codereview.chromium.org/1287423002/diff/40001/src/codec/SkBmpCodec.h
File src/codec/SkBmpCodec.h (right):

https://codereview.chromium.org/1287423002/diff/40001/src/codec/SkBmpCodec.h#...
src/codec/SkBmpCodec.h:126: * Allow to BmpCodec subclasses to implement unique
onStart() functions
Maybe mention that this relates to scanline decoding?

https://codereview.chromium.org/1287423002/diff/40001/src/codec/SkBmpCodec.h#...
src/codec/SkBmpCodec.h:128: virtual SkCodec::Result onStart(const SkImageInfo&
dstInfo, const SkCodec::Options& options,
I do not know whether it's good or bad that this shares the same name as
SkScanlineDecoder::onStart.

Would it make sense for onGetPixels to call onStart?

https://codereview.chromium.org/1287423002/diff/40001/src/codec/SkBmpCodec.h#...
src/codec/SkBmpCodec.h:135: virtual Result decode(const SkImageInfo& dstInfo,
void* dst, size_t dstRowBytes,
Can you add a description for these parameters? I think it's interesting (and
not necessarily intuitive) that the height of dstInfo allows the caller to do a
subset (of lines - not in X).

Also, I assume it decodes the next dstInfo.height() lines?

[msarett, 2015-08-17 19:16:13]

https://codereview.chromium.org/1287423002/diff/40001/src/codec/SkBmpCodec.cpp
File src/codec/SkBmpCodec.cpp (right):

https://codereview.chromium.org/1287423002/diff/40001/src/codec/SkBmpCodec.cp...
src/codec/SkBmpCodec.cpp:597: // TODO(msarett): Consider other optimizations for
this codec.
On 2015/08/14 21:53:00, scroggo wrote:
> Do you have any optimizations in mind?

Not in particular.  I just don't think I've analyzed this codec for anything
other than correctness.

Once the design is all set, I think it would be worth looking through and seeing
if there is anything that we can improve.

I think the memory use of the RLE codec is a good candidate.  The header tells
us how big the file is and we currently allocate that many bytes, but now that
we have checkForMoreData(), we can do better.  Also I think bmp swizzles seem
quite trivial and would be candidates for SIMD.

https://codereview.chromium.org/1287423002/diff/40001/src/codec/SkBmpCodec.h
File src/codec/SkBmpCodec.h (right):

https://codereview.chromium.org/1287423002/diff/40001/src/codec/SkBmpCodec.h#...
src/codec/SkBmpCodec.h:126: * Allow to BmpCodec subclasses to implement unique
onStart() functions
On 2015/08/14 21:53:00, scroggo wrote:
> Maybe mention that this relates to scanline decoding?

This is now shared with onGetPixels().

https://codereview.chromium.org/1287423002/diff/40001/src/codec/SkBmpCodec.h#...
src/codec/SkBmpCodec.h:128: virtual SkCodec::Result onStart(const SkImageInfo&
dstInfo, const SkCodec::Options& options,
On 2015/08/14 21:53:01, scroggo wrote:
> I do not know whether it's good or bad that this shares the same name as
> SkScanlineDecoder::onStart.
> 
> Would it make sense for onGetPixels to call onStart?

I think it makes sense to change the name because it's a bit confusing to
duplicate the name of a scanline decoding function.  With the name change, I
think it makes sense to share this function with onGetPixels().

https://codereview.chromium.org/1287423002/diff/40001/src/codec/SkBmpCodec.h#...
src/codec/SkBmpCodec.h:135: virtual Result decode(const SkImageInfo& dstInfo,
void* dst, size_t dstRowBytes,
On 2015/08/14 21:53:01, scroggo wrote:
> Can you add a description for these parameters? I think it's interesting (and
> not necessarily intuitive) that the height of dstInfo allows the caller to do
a
> subset (of lines - not in X).
> 
> Also, I assume it decodes the next dstInfo.height() lines?

Yeah I'm adding comments, this is awkward.  Ideally we would share a decodeRow()
function and do away with this capability (since we are not enabling subsets for
bmp).

decodeRow() isn't a good fit for RLE though.  So I think sticking with decode()
is the best design move.

[msarett, 2015-08-18 15:23:37]

msarett@google.com changed reviewers:
+ djsollen@google.com
- emmaleer@google.com

[msarett, 2015-08-18 15:23:38]

Derek, can you look at the API when you get a chance?

This is still a WIP, but the API changes that I'm proposing are stable.

[djsollen, 2015-08-18 19:25:13]

api 1gtm

[msarett, 2015-08-19 14:05:20]

Patchset #10 (id:200001) has been deleted

[msarett, 2015-08-19 14:22:40]

Leon, this is ready for review when you're back in town :).

https://codereview.chromium.org/1287423002/diff/220001/src/codec/SkScanlineDe...
File src/codec/SkScanlineDecoder.cpp (right):

https://codereview.chromium.org/1287423002/diff/220001/src/codec/SkScanlineDe...
src/codec/SkScanlineDecoder.cpp:92: const SkCodec::Result result =
this->onStart(dstInfo, *options, ctable, ctableCount);
I think we should clean up onStart() in a subsequent CL.  Seems like dstInfo and
options don't need to be parameters since we make them fields in start().

https://codereview.chromium.org/1287423002/diff/220001/src/codec/SkSwizzler.cpp
File src/codec/SkSwizzler.cpp (right):

https://codereview.chromium.org/1287423002/diff/220001/src/codec/SkSwizzler.c...
src/codec/SkSwizzler.cpp:147: static SkSwizzler::ResultAlpha
swizzle_small_index_to_index(
These functions use a similar strategy to the swizzle_bit functions.

https://codereview.chromium.org/1287423002/diff/220001/src/codec/SkSwizzler.c...
src/codec/SkSwizzler.cpp:156: const uint8_t mask = (1 << bpp) - 1;
Unfortunately, these functions need to know the bitsPerPixel.  We could
eliminate a parameter by passing sampleX instead of deltaSrc and offset, but
that would bring more math into these swizzling functions.  I think it's fine as
is, but am happy with it either way.

https://codereview.chromium.org/1287423002/diff/220001/src/codec/SkSwizzler.c...
src/codec/SkSwizzler.cpp:716: void SkSwizzler::Fill(void* dstStartRow, const
SkImageInfo& dstInfo, size_t dstRowBytes,
Sorry I didn't mean to put all of this refactoring of the Fill() function in
this CL.  There are incomplete bmps on the bots, so SkScaledCodec needs to
support filling.  I originally thought SkScaledCodec would call Fill(), but I've
actually implemented it so that is not necessary.

I think a follow up CL will need to test on incomplete images on all types.  I
suspect that we don't handle incomplete pngs gracefully.

https://codereview.chromium.org/1287423002/diff/220001/src/codec/SkSwizzler.c...
src/codec/SkSwizzler.cpp:738: if (SkCodec::kYes_ZeroInitialized == zeroInit && 0
== color) {
This fixes a bug.

We used to check if colorOrIndex was 0 instead of if color is 0.  When the
output color type is kN32, we really need to know if the actual color is 0.

When I follow up with a CL testing incomplete images, I'll add a test.

https://codereview.chromium.org/1287423002/diff/220001/src/codec/SkSwizzler.c...
src/codec/SkSwizzler.cpp:781: if (SkCodec::kYes_ZeroInitialized == zeroInit && 0
== (uint8_t) colorOrIndex) {
This fixes a bug.

Only the low 8-bits of colorOrIndex need to be 0 for this to qualify as a zero
fill.

[msarett, 2015-08-24 19:12:44]

Patchset #12 (id:260001) has been deleted

[scroggo, 2015-08-26 22:40:09]

I looked at these one patch set at a time, so my comments are spread through
them. Sorry about that :(

These changes look generally the right direction.

https://codereview.chromium.org/1287423002/diff/40001/src/codec/SkBmpCodec.cpp
File src/codec/SkBmpCodec.cpp (right):

https://codereview.chromium.org/1287423002/diff/40001/src/codec/SkBmpCodec.cp...
src/codec/SkBmpCodec.cpp:597: // TODO(msarett): Consider other optimizations for
this codec.
On 2015/08/17 19:16:13, msarett wrote:
> On 2015/08/14 21:53:00, scroggo wrote:
> > Do you have any optimizations in mind?
> 
> Not in particular.  I just don't think I've analyzed this codec for anything
> other than correctness.
> 
> Once the design is all set, I think it would be worth looking through and
seeing
> if there is anything that we can improve.
> 
> I think the memory use of the RLE codec is a good candidate.  The header tells
> us how big the file is and we currently allocate that many bytes, but now that
> we have checkForMoreData(), we can do better.  Also I think bmp swizzles seem
> quite trivial and would be candidates for SIMD.

Great. These would be good candidates to specifically mention as TODOs.

https://codereview.chromium.org/1287423002/diff/140001/include/codec/SkScanli...
File include/codec/SkScanlineDecoder.h (right):

https://codereview.chromium.org/1287423002/diff/140001/include/codec/SkScanli...
include/codec/SkScanlineDecoder.h:178: * Upside down bmps and interlaced gifs
are examples.
Should upside down be its own ordering? It seems like that is a special case
where they're still in a simple order, it's just the opposite order.

https://codereview.chromium.org/1287423002/diff/140001/include/codec/SkScanli...
include/codec/SkScanlineDecoder.h:191: kNone_SkScanlineOrder,
I'm not a huge fan of this name, but do not have a suggestion for a better one.

https://codereview.chromium.org/1287423002/diff/140001/src/codec/SkBmpCodec.cpp
File src/codec/SkBmpCodec.cpp (right):

https://codereview.chromium.org/1287423002/diff/140001/src/codec/SkBmpCodec.c...
src/codec/SkBmpCodec.cpp:605: } else {
nit: This else is unnecessary. Typically, this is written as:

if (condition) {
  return enum;
}
return otherEnum;

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

https://codereview.chromium.org/1287423002/diff/140001/src/codec/SkScaledCode...
src/codec/SkScaledCodec.cpp:136: static bool is_row_necessary(int srcY, int
startY, int sampleY) {
nit: Can you add comments explaining this function?

https://codereview.chromium.org/1287423002/diff/160001/src/codec/SkBmpCodec.cpp
File src/codec/SkBmpCodec.cpp (right):

https://codereview.chromium.org/1287423002/diff/160001/src/codec/SkBmpCodec.c...
src/codec/SkBmpCodec.cpp:629: return fCodec->getDstRow(INHERITED::onGetY());
nit: this->INHERITED::onGetY()

https://codereview.chromium.org/1287423002/diff/160001/src/codec/SkBmpCodec.h
File src/codec/SkBmpCodec.h (right):

https://codereview.chromium.org/1287423002/diff/160001/src/codec/SkBmpCodec.h...
src/codec/SkBmpCodec.h:126: virtual SkCodec::Result prepareToDecode(const
SkImageInfo& dstInfo,
Can you add comments explaining the parameters? Does dstInfo still encapsulate
the rows we will decode?

nit: I think we typically make virtual functions protected, rather than private.

https://codereview.chromium.org/1287423002/diff/180001/src/codec/SkBmpCodec.h
File src/codec/SkBmpCodec.h (right):

https://codereview.chromium.org/1287423002/diff/180001/src/codec/SkBmpCodec.h...
src/codec/SkBmpCodec.h:92: int32_t getDstRow(int32_t y, int32_t height);
Can you explain what height means? I suppose it is not the full height of the
image?

https://codereview.chromium.org/1287423002/diff/180001/src/codec/SkSwizzler.h
File src/codec/SkSwizzler.h (right):

https://codereview.chromium.org/1287423002/diff/180001/src/codec/SkSwizzler.h...
src/codec/SkSwizzler.h:193: *  @param deltaSrc bpp * sampleX
It seems a little weird to me that these two parameters are not independent. On
the other hand, I do not see a better way of passing along the required info.

https://codereview.chromium.org/1287423002/diff/220001/src/codec/SkBmpRLECode...
File src/codec/SkBmpRLECodec.cpp (right):

https://codereview.chromium.org/1287423002/diff/220001/src/codec/SkBmpRLECode...
src/codec/SkBmpRLECodec.cpp:64: return decodeRows(dstInfo, dst, dstRowBytes,
opts);
nit: this->decodeRows

https://codereview.chromium.org/1287423002/diff/220001/src/codec/SkSwizzler.cpp
File src/codec/SkSwizzler.cpp (right):

https://codereview.chromium.org/1287423002/diff/220001/src/codec/SkSwizzler.c...
src/codec/SkSwizzler.cpp:147: static SkSwizzler::ResultAlpha
swizzle_small_index_to_index(
On 2015/08/19 14:22:40, msarett wrote:
> These functions use a similar strategy to the swizzle_bit functions.

I'm not sure I understand the comment. Should this comment go in the code? Or
are you proposing merging them somehow?

https://codereview.chromium.org/1287423002/diff/240001/dm/DMSrcSink.cpp
File dm/DMSrcSink.cpp (right):

https://codereview.chromium.org/1287423002/diff/240001/dm/DMSrcSink.cpp#newco...
dm/DMSrcSink.cpp:140: // Visually inspecting very small output images is not
necessary.  We will
+1

[msarett, 2015-08-27 15:00:27]

https://codereview.chromium.org/1287423002/diff/160001/src/codec/SkBmpCodec.h
File src/codec/SkBmpCodec.h (right):

https://codereview.chromium.org/1287423002/diff/160001/src/codec/SkBmpCodec.h...
src/codec/SkBmpCodec.h:126: virtual SkCodec::Result prepareToDecode(const
SkImageInfo& dstInfo,
On 2015/08/26 22:40:09, scroggo wrote:
> Can you add comments explaining the parameters? Does dstInfo still encapsulate
> the rows we will decode?
> 
> nit: I think we typically make virtual functions protected, rather than
private.

Added comments and made this protected.  Is there a reason to make this
protected other than skia convention?  I generally just try to make functions as
hidden as possible.

https://codereview.chromium.org/1287423002/diff/180001/src/codec/SkBmpCodec.h
File src/codec/SkBmpCodec.h (right):

https://codereview.chromium.org/1287423002/diff/180001/src/codec/SkBmpCodec.h...
src/codec/SkBmpCodec.h:92: int32_t getDstRow(int32_t y, int32_t height);
On 2015/08/26 22:40:09, scroggo wrote:
> Can you explain what height means? I suppose it is not the full height of the
> image?

Adding comments.  It used to mean the full height of the image - but for cases
where we are decoding a subset of the image (ex: 1 row for sampling), it means
the height of the subset we are decoding (so it is 1 a lot).

https://codereview.chromium.org/1287423002/diff/180001/src/codec/SkSwizzler.h
File src/codec/SkSwizzler.h (right):

https://codereview.chromium.org/1287423002/diff/180001/src/codec/SkSwizzler.h...
src/codec/SkSwizzler.h:193: *  @param deltaSrc bpp * sampleX
On 2015/08/26 22:40:09, scroggo wrote:
> It seems a little weird to me that these two parameters are not independent.
On
> the other hand, I do not see a better way of passing along the required info.

I agree that it it strange - but I think this is better than passing sampleX and
performing the multiplication each time the rowProc is called.

https://codereview.chromium.org/1287423002/diff/220001/src/codec/SkBmpRLECode...
File src/codec/SkBmpRLECodec.cpp (right):

https://codereview.chromium.org/1287423002/diff/220001/src/codec/SkBmpRLECode...
src/codec/SkBmpRLECodec.cpp:64: return decodeRows(dstInfo, dst, dstRowBytes,
opts);
On 2015/08/26 22:40:09, scroggo wrote:
> nit: this->decodeRows

Done.

https://codereview.chromium.org/1287423002/diff/220001/src/codec/SkSwizzler.cpp
File src/codec/SkSwizzler.cpp (right):

https://codereview.chromium.org/1287423002/diff/220001/src/codec/SkSwizzler.c...
src/codec/SkSwizzler.cpp:147: static SkSwizzler::ResultAlpha
swizzle_small_index_to_index(
On 2015/08/26 22:40:09, scroggo wrote:
> On 2015/08/19 14:22:40, msarett wrote:
> > These functions use a similar strategy to the swizzle_bit functions.
> 
> I'm not sure I understand the comment. Should this comment go in the code? Or
> are you proposing merging them somehow?

I was just trying to provide some context for how I redesigned these in case it
helps with the review.

[scroggo, 2015-08-27 20:14:23]

https://codereview.chromium.org/1287423002/diff/160001/src/codec/SkBmpCodec.h
File src/codec/SkBmpCodec.h (right):

https://codereview.chromium.org/1287423002/diff/160001/src/codec/SkBmpCodec.h...
src/codec/SkBmpCodec.h:126: virtual SkCodec::Result prepareToDecode(const
SkImageInfo& dstInfo,
On 2015/08/27 15:00:27, msarett wrote:
> On 2015/08/26 22:40:09, scroggo wrote:
> > Can you add comments explaining the parameters? Does dstInfo still
encapsulate
> > the rows we will decode?
> > 
> > nit: I think we typically make virtual functions protected, rather than
> private.
> 
> Added comments and made this protected.  Is there a reason to make this
> protected other than skia convention?  I generally just try to make functions
as
> hidden as possible.

Good question. This was just my memory of how we do things; I'm not sure whether
it is Skia-specific.

One answer I've found online (see
http://stackoverflow.com/questions/2170688/private-virtual-method-in-c and
https://isocpp.org/wiki/faq/strange-inheritance#private-virtuals) is that people
find private virtual functions confusing. Apparently there was a recommendation
to use protected virtuals in order to not confuse people, but that has been
reversed.

The confusion comes from the fact that a subclass can override a private virtual
function, but cannot call it (according to that second link).

So the latest recommendation seems to be making a virtual function private,
unless a subclass needs to call it, to follow the general rule of providing the
least access necessary.

That said, I personally do not see how that does any good, at least for a pure
virtual function. If the base class makes a virtual method private, the derived
class can *change* the access level of its overridden version (even to public),
so we didn't really hide anything from the subclass. The one case where we did
hide something is if the derived class is overriding a function with an
implementation - then the subclass cannot call it. Sometimes you want the
subclass to be able to call its INHERITED version, in which case it needs to be
protected (or public).

https://codereview.chromium.org/1287423002/diff/180001/src/codec/SkSwizzler.h
File src/codec/SkSwizzler.h (right):

https://codereview.chromium.org/1287423002/diff/180001/src/codec/SkSwizzler.h...
src/codec/SkSwizzler.h:193: *  @param deltaSrc bpp * sampleX
On 2015/08/27 15:00:27, msarett wrote:
> On 2015/08/26 22:40:09, scroggo wrote:
> > It seems a little weird to me that these two parameters are not independent.
> On
> > the other hand, I do not see a better way of passing along the required
info.
> 
> I agree that it it strange - but I think this is better than passing sampleX
and
> performing the multiplication each time the rowProc is called.

Agreed.

https://codereview.chromium.org/1287423002/diff/280001/dm/DMSrcSink.cpp
File dm/DMSrcSink.cpp (right):

https://codereview.chromium.org/1287423002/diff/280001/dm/DMSrcSink.cpp#newco...
dm/DMSrcSink.cpp:265: return Error::Nonfatal("Test mode not supported");
Wait, why do we require kTopDown? Don't we know how many scanlines we need? It
seems like for kNone we should not have to do anything to support this. For
kOutOfOrder, we would need to write separate code for this, but maybe that could
be a TODO? (FWIW, we might want some common code somewhere that handles that
case.)

https://codereview.chromium.org/1287423002/diff/280001/src/codec/SkBmpCodec.cpp
File src/codec/SkBmpCodec.cpp (right):

https://codereview.chromium.org/1287423002/diff/280001/src/codec/SkBmpCodec.c...
src/codec/SkBmpCodec.cpp:624: return kOutOfOrder_SkScanlineOrder;
If SkScanlineOrder included an option for kBottomUp, we could replace
SkBmpCodec::RowOrder with SkScanlineOrder. What do you think about that option?

https://codereview.chromium.org/1287423002/diff/280001/src/codec/SkBmpCodec.h
File src/codec/SkBmpCodec.h (right):

https://codereview.chromium.org/1287423002/diff/280001/src/codec/SkBmpCodec.h...
src/codec/SkBmpCodec.h:88: * Get the destination row number corresponsing to the
encoded row number.
corresponding*

https://codereview.chromium.org/1287423002/diff/280001/src/codec/SkBmpCodec.h...
src/codec/SkBmpCodec.h:125: *                        the total number of rows
that can be decoded.
will be decoded?

https://codereview.chromium.org/1287423002/diff/280001/src/codec/SkBmpCodec.h...
src/codec/SkBmpCodec.h:126: * @param optoins         Additonal options to pass
to the decoder.
options*

https://codereview.chromium.org/1287423002/diff/280001/src/codec/SkBmpCodec.h...
src/codec/SkBmpCodec.h:127: * @param inputColorPtr   Client provides memory for
a color table.  This
Maybe this should be

Client-provided memory for a color table. Must be enough memory for 256 colors.
etc

https://codereview.chromium.org/1287423002/diff/280001/src/codec/SkBmpMaskCod...
File src/codec/SkBmpMaskCodec.cpp (right):

https://codereview.chromium.org/1287423002/diff/280001/src/codec/SkBmpMaskCod...
src/codec/SkBmpMaskCodec.cpp:22: , fSrcBuffer(SkNEW_ARRAY(uint8_t,
fSrcRowBytes))
Since you wrote this patch, we'ved moved away from SkNEW etc. So this should be
"new uint8_t [fSrcRowBytes]"

https://codereview.chromium.org/1287423002/diff/280001/src/codec/SkBmpRLECode...
File src/codec/SkBmpRLECodec.cpp (right):

https://codereview.chromium.org/1287423002/diff/280001/src/codec/SkBmpRLECode...
src/codec/SkBmpRLECodec.cpp:135: // TODO (msarett): I'm not sure it is smart or
optimal to trust fRLEBytes (read from header)
FWIW, we could use a constant buffer size, like we do for JPEG, and fill it when
necessary.

Or we could only read whatever we think we need for the current calculations.

https://codereview.chromium.org/1287423002/diff/280001/src/codec/SkBmpStandar...
File src/codec/SkBmpStandardCodec.cpp (right):

https://codereview.chromium.org/1287423002/diff/280001/src/codec/SkBmpStandar...
src/codec/SkBmpStandardCodec.cpp:66: return this->decodeIcoMask(dstInfo, dst,
dstRowBytes);
I like this separation!

What happens if a client attempts to row decode an ICO image? Will they just not
get the mask?

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

https://codereview.chromium.org/1287423002/diff/280001/src/codec/SkScaledCode...
src/codec/SkScaledCodec.cpp:150: bool SkScaledCodec::IsCoordNecessary(int
srcCoord, int sampleFactor, int scaledDim) {
Any idea whether calling all these functions slows things down when the sample
size is 1? If so, maybe this should special case if sampleFactor == 1?

[msarett, 2015-08-27 21:21:46]

https://codereview.chromium.org/1287423002/diff/280001/dm/DMSrcSink.cpp
File dm/DMSrcSink.cpp (right):

https://codereview.chromium.org/1287423002/diff/280001/dm/DMSrcSink.cpp#newco...
dm/DMSrcSink.cpp:265: return Error::Nonfatal("Test mode not supported");
On 2015/08/27 20:14:22, scroggo wrote:
> Wait, why do we require kTopDown? Don't we know how many scanlines we need? It
> seems like for kNone we should not have to do anything to support this. For
> kOutOfOrder, we would need to write separate code for this, but maybe that
could
> be a TODO? (FWIW, we might want some common code somewhere that handles that
> case.)

First, this check should be deleted because it is a duplicate of the one in
start_scanline_decoder().

However, I am adding a TODO.  I think you're right that we want common code to
deal with kOutOfOrder etc.  I think you can find that code in SkScaledCodec in
the gif scanline decoder review :/

I'm hesitant to make the change in this patchset because I think we need a more
serious redesign of our test cases here.  If we share the code used in
SkScaledCodec throughout these tests, we may have a bit of duplication of our
test cases.

https://codereview.chromium.org/1287423002/diff/280001/src/codec/SkBmpCodec.cpp
File src/codec/SkBmpCodec.cpp (right):

https://codereview.chromium.org/1287423002/diff/280001/src/codec/SkBmpCodec.c...
src/codec/SkBmpCodec.cpp:624: return kOutOfOrder_SkScanlineOrder;
On 2015/08/27 20:14:22, scroggo wrote:
> If SkScanlineOrder included an option for kBottomUp, we could replace
> SkBmpCodec::RowOrder with SkScanlineOrder. What do you think about that
option?

I think it makes sense to share with SkScanlineOrder.

I'm not exactly sure what is the best way...  We could alternatively just use
kOutOfOrder, and in the context of bmps, that can only mean bottom-up.  But
maybe that is a little confusing?

I'll go ahead and add kBottomUp.  Maybe it's strange to have an additional mode
when I was planning on treating these the same as kOutOfOrder gifs?  But, on the
other hand, it's nice to have the option to end up treating them differently,
and taking advantage of the predictability of bottom-up mode can probably enable
us to perform more optimal bmp decodes.

https://codereview.chromium.org/1287423002/diff/280001/src/codec/SkBmpCodec.h
File src/codec/SkBmpCodec.h (right):

https://codereview.chromium.org/1287423002/diff/280001/src/codec/SkBmpCodec.h...
src/codec/SkBmpCodec.h:88: * Get the destination row number corresponsing to the
encoded row number.
On 2015/08/27 20:14:22, scroggo wrote:
> corresponding*

Sorry.  Fixed!

https://codereview.chromium.org/1287423002/diff/280001/src/codec/SkBmpCodec.h...
src/codec/SkBmpCodec.h:125: *                        the total number of rows
that can be decoded.
On 2015/08/27 20:14:23, scroggo wrote:
> will be decoded?

Done.

https://codereview.chromium.org/1287423002/diff/280001/src/codec/SkBmpCodec.h...
src/codec/SkBmpCodec.h:126: * @param optoins         Additonal options to pass
to the decoder.
On 2015/08/27 20:14:23, scroggo wrote:
> options*

Done.

https://codereview.chromium.org/1287423002/diff/280001/src/codec/SkBmpCodec.h...
src/codec/SkBmpCodec.h:127: * @param inputColorPtr   Client provides memory for
a color table.  This
On 2015/08/27 20:14:22, scroggo wrote:
> Maybe this should be
> 
> Client-provided memory for a color table. Must be enough memory for 256
colors.
> etc

Done.

https://codereview.chromium.org/1287423002/diff/280001/src/codec/SkBmpMaskCod...
File src/codec/SkBmpMaskCodec.cpp (right):

https://codereview.chromium.org/1287423002/diff/280001/src/codec/SkBmpMaskCod...
src/codec/SkBmpMaskCodec.cpp:22: , fSrcBuffer(SkNEW_ARRAY(uint8_t,
fSrcRowBytes))
On 2015/08/27 20:14:23, scroggo wrote:
> Since you wrote this patch, we'ved moved away from SkNEW etc. So this should
be
> "new uint8_t [fSrcRowBytes]"
> 

Done thanks.

https://codereview.chromium.org/1287423002/diff/280001/src/codec/SkBmpRLECode...
File src/codec/SkBmpRLECodec.cpp (right):

https://codereview.chromium.org/1287423002/diff/280001/src/codec/SkBmpRLECode...
src/codec/SkBmpRLECodec.cpp:135: // TODO (msarett): I'm not sure it is smart or
optimal to trust fRLEBytes (read from header)
On 2015/08/27 20:14:23, scroggo wrote:
> FWIW, we could use a constant buffer size, like we do for JPEG, and fill it
when
> necessary.
> 
> Or we could only read whatever we think we need for the current calculations.

Yeah I think both of those are good candidates.

https://codereview.chromium.org/1287423002/diff/280001/src/codec/SkBmpStandar...
File src/codec/SkBmpStandardCodec.cpp (right):

https://codereview.chromium.org/1287423002/diff/280001/src/codec/SkBmpStandar...
src/codec/SkBmpStandardCodec.cpp:66: return this->decodeIcoMask(dstInfo, dst,
dstRowBytes);
On 2015/08/27 20:14:23, scroggo wrote:
> I like this separation!
> 
> What happens if a client attempts to row decode an ICO image? Will they just
not
> get the mask?

Right now they can't row decode an ICO image because we haven't implemented
scanline decoding for icos :).  I kind of punted this problem to when I
implement scanline decoding for icos, but I'm hopeful it won't be too tough to
do properly now that these are separated a little better.

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

https://codereview.chromium.org/1287423002/diff/280001/src/codec/SkScaledCode...
src/codec/SkScaledCodec.cpp:150: bool SkScaledCodec::IsCoordNecessary(int
srcCoord, int sampleFactor, int scaledDim) {
On 2015/08/27 20:14:23, scroggo wrote:
> Any idea whether calling all these functions slows things down when the sample
> size is 1? If so, maybe this should special case if sampleFactor == 1?

We do special case on sample size == 1 by using the native codec (see line 222).
 However, we'll need to revisit this again on gif (because the call on line 222
will not work on interlaced gifs).  This call works on upside down bmps kind of
accidentally.  SkBmpCodec::decodeRows() reverses the rows for you if it is
bottom up (which is how getPixels() uses it).

[msarett, 2015-08-28 12:38:00]

Patchset #14 (id:320001) has been deleted

[msarett, 2015-08-28 12:38:08]

Patchset #14 (id:340001) has been deleted

[scroggo, 2015-08-28 13:28:47]

> Rebase + bug fix in SkScaledCodec

Please try to remember to use separate uploads for rebases. I had to sift
through all of Hal's changes of NULL to nullptr and SkNEW to new to try to find
the interesting change :(

https://codereview.chromium.org/1287423002/diff/280001/dm/DMSrcSink.cpp
File dm/DMSrcSink.cpp (right):

https://codereview.chromium.org/1287423002/diff/280001/dm/DMSrcSink.cpp#newco...
dm/DMSrcSink.cpp:265: return Error::Nonfatal("Test mode not supported");
On 2015/08/27 21:21:46, msarett wrote:
> On 2015/08/27 20:14:22, scroggo wrote:
> > Wait, why do we require kTopDown? Don't we know how many scanlines we need?
It
> > seems like for kNone we should not have to do anything to support this. For
> > kOutOfOrder, we would need to write separate code for this, but maybe that
> could
> > be a TODO? (FWIW, we might want some common code somewhere that handles that
> > case.)
> 
> First, this check should be deleted because it is a duplicate of the one in
> start_scanline_decoder().
> 
> However, I am adding a TODO.  I think you're right that we want common code to
> deal with kOutOfOrder etc.  I think you can find that code in SkScaledCodec in
> the gif scanline decoder review :/
> 
> I'm hesitant to make the change in this patchset because I think we need a
more
> serious redesign of our test cases here.  If we share the code used in
> SkScaledCodec throughout these tests, we may have a bit of duplication of our
> test cases.

sgtm

https://codereview.chromium.org/1287423002/diff/280001/src/codec/SkBmpCodec.cpp
File src/codec/SkBmpCodec.cpp (right):

https://codereview.chromium.org/1287423002/diff/280001/src/codec/SkBmpCodec.c...
src/codec/SkBmpCodec.cpp:624: return kOutOfOrder_SkScanlineOrder;
On 2015/08/27 21:21:46, msarett wrote:
> On 2015/08/27 20:14:22, scroggo wrote:
> > If SkScanlineOrder included an option for kBottomUp, we could replace
> > SkBmpCodec::RowOrder with SkScanlineOrder. What do you think about that
> option?
> 
> I think it makes sense to share with SkScanlineOrder.
> 
> I'm not exactly sure what is the best way...  We could alternatively just use
> kOutOfOrder, and in the context of bmps, that can only mean bottom-up.  But
> maybe that is a little confusing?
> 
> I'll go ahead and add kBottomUp.  Maybe it's strange to have an additional
mode
> when I was planning on treating these the same as kOutOfOrder gifs?  But, on
the
> other hand, it's nice to have the option to end up treating them differently,
> and taking advantage of the predictability of bottom-up mode can probably
enable
> us to perform more optimal bmp decodes.

That would be my preference. We can always treat the two enums the same, in
certain cases.

It might be a little weird that an SkCodec subclass depends on an enum in
SkScanlineDecoder (maybe it belongs somewhere else?), but I think it's fine for
now. We already have some interdependence between the two...

https://codereview.chromium.org/1287423002/diff/280001/src/codec/SkBmpStandar...
File src/codec/SkBmpStandardCodec.cpp (right):

https://codereview.chromium.org/1287423002/diff/280001/src/codec/SkBmpStandar...
src/codec/SkBmpStandardCodec.cpp:66: return this->decodeIcoMask(dstInfo, dst,
dstRowBytes);
On 2015/08/27 21:21:46, msarett wrote:
> On 2015/08/27 20:14:23, scroggo wrote:
> > I like this separation!
> > 
> > What happens if a client attempts to row decode an ICO image? Will they just
> not
> > get the mask?
> 
> Right now they can't row decode an ICO image because we haven't implemented
> scanline decoding for icos :).  I kind of punted this problem to when I
> implement scanline decoding for icos, but I'm hopeful it won't be too tough to
> do properly now that these are separated a little better.

Sgtm. Is there a bug or TODO?

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

https://codereview.chromium.org/1287423002/diff/280001/src/codec/SkScaledCode...
src/codec/SkScaledCodec.cpp:150: bool SkScaledCodec::IsCoordNecessary(int
srcCoord, int sampleFactor, int scaledDim) {
On 2015/08/27 21:21:46, msarett wrote:
> On 2015/08/27 20:14:23, scroggo wrote:
> > Any idea whether calling all these functions slows things down when the
sample
> > size is 1? If so, maybe this should special case if sampleFactor == 1?
> 
> We do special case on sample size == 1 by using the native codec (see line
222).
>  However, we'll need to revisit this again on gif (because the call on line
222
> will not work on interlaced gifs).  This call works on upside down bmps kind
of
> accidentally.  SkBmpCodec::decodeRows() reverses the rows for you if it is
> bottom up (which is how getPixels() uses it).

Oh, that's even better!

https://codereview.chromium.org/1287423002/diff/300001/include/codec/SkScanli...
File include/codec/SkScanlineDecoder.h (right):

https://codereview.chromium.org/1287423002/diff/300001/include/codec/SkScanli...
include/codec/SkScanlineDecoder.h:176: * For this scanline ordering, it may be
advisable to get and skip
Why do they need to skip scanlines one at a time?

https://codereview.chromium.org/1287423002/diff/300001/include/codec/SkScanli...
include/codec/SkScanlineDecoder.h:181: * scanlines where count > 1.
It seems like we *could* make this work, in at least of couple of potential
ways:

Change getScanlines's rowBytes parameter to be signed, so that we can skip
backwards... actually that's no good - we would be subtracting from the
beginning of the memory provided, unless we asked for the end of the
destination, which is just wrong...

Better idea - onGetScanlines could initially add count * rowBytes to dst, and
then subtract rowBytes each time.

I don't know whether we *should* do that, but it seems like an interesting
option.

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

https://codereview.chromium.org/1287423002/diff/360001/src/codec/SkScaledCode...
src/codec/SkScaledCodec.cpp:273: case
SkScanlineDecoder::kBottomUp_SkScanlineOrder:
Is this the bug fix listed in the patch set description?

[msarett, 2015-08-28 14:05:58]

Sorry for adding the fix with the rebase.

https://codereview.chromium.org/1287423002/diff/280001/src/codec/SkBmpStandar...
File src/codec/SkBmpStandardCodec.cpp (right):

https://codereview.chromium.org/1287423002/diff/280001/src/codec/SkBmpStandar...
src/codec/SkBmpStandardCodec.cpp:66: return this->decodeIcoMask(dstInfo, dst,
dstRowBytes);
On 2015/08/28 13:28:47, scroggo wrote:
> On 2015/08/27 21:21:46, msarett wrote:
> > On 2015/08/27 20:14:23, scroggo wrote:
> > > I like this separation!
> > > 
> > > What happens if a client attempts to row decode an ICO image? Will they
just
> > not
> > > get the mask?
> > 
> > Right now they can't row decode an ICO image because we haven't implemented
> > scanline decoding for icos :).  I kind of punted this problem to when I
> > implement scanline decoding for icos, but I'm hopeful it won't be too tough
to
> > do properly now that these are separated a little better.
> 
> Sgtm. Is there a bug or TODO?

Adding a TODO above the function definition.

https://codereview.chromium.org/1287423002/diff/300001/include/codec/SkScanli...
File include/codec/SkScanlineDecoder.h (right):

https://codereview.chromium.org/1287423002/diff/300001/include/codec/SkScanli...
include/codec/SkScanlineDecoder.h:176: * For this scanline ordering, it may be
advisable to get and skip
On 2015/08/28 13:28:47, scroggo wrote:
> Why do they need to skip scanlines one at a time?

They can skip more, but it's a bit a risky, they need to know what they are
doing.  They would be skipping upward from the bottom of the image, which they
may not expect.

https://codereview.chromium.org/1287423002/diff/300001/include/codec/SkScanli...
include/codec/SkScanlineDecoder.h:181: * scanlines where count > 1.
On 2015/08/28 13:28:47, scroggo wrote:
> It seems like we *could* make this work, in at least of couple of potential
> ways:
> 
> Change getScanlines's rowBytes parameter to be signed, so that we can skip
> backwards... actually that's no good - we would be subtracting from the
> beginning of the memory provided, unless we asked for the end of the
> destination, which is just wrong...
> 
> Better idea - onGetScanlines could initially add count * rowBytes to dst, and
> then subtract rowBytes each time.
> 
> I don't know whether we *should* do that, but it seems like an interesting
> option.

So we do invert the rows if they ask for more than one row.  It's just strange
because if they ask for "some" of the rows, they will get the rows from the
bottom of the image (flipped right-side up).

I think this comment is pretty unclear as is.  I've tried to explain things a
little better.

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

https://codereview.chromium.org/1287423002/diff/360001/src/codec/SkScaledCode...
src/codec/SkScaledCodec.cpp:273: case
SkScanlineDecoder::kBottomUp_SkScanlineOrder:
On 2015/08/28 13:28:47, scroggo wrote:
> Is this the bug fix listed in the patch set description?

Yes, sorry this was unclear.

[scroggo, 2015-08-28 14:35:01]

lgtm

https://codereview.chromium.org/1287423002/diff/380001/include/codec/SkScaled...
File include/codec/SkScaledCodec.h (right):

https://codereview.chromium.org/1287423002/diff/380001/include/codec/SkScaled...
include/codec/SkScaledCodec.h:45: * The output can be interpreted as an
x-coordinate or a y-coordinate.
Maybe add comments that these do not need to be (and are not) called if
sampleFactor is 1?

https://codereview.chromium.org/1287423002/diff/380001/include/codec/SkScaled...
include/codec/SkScaledCodec.h:47: static inline int GetStartCoord(int
sampleFactor);
I believe inline functions need to be written in one place in order to inline?

https://codereview.chromium.org/1287423002/diff/380001/src/codec/SkBmpCodec.cpp
File src/codec/SkBmpCodec.cpp (right):

https://codereview.chromium.org/1287423002/diff/380001/src/codec/SkBmpCodec.c...
src/codec/SkBmpCodec.cpp:543: } else {
nit: No need for else.

https://codereview.chromium.org/1287423002/diff/380001/src/codec/SkBmpCodec.c...
src/codec/SkBmpCodec.cpp:544: return height - y - 1;
SkASSERT(kBottomUp == fRowOrder)

https://codereview.chromium.org/1287423002/diff/380001/src/codec/SkBmpRLECode...
File src/codec/SkBmpRLECodec.cpp (right):

https://codereview.chromium.org/1287423002/diff/380001/src/codec/SkBmpRLECode...
src/codec/SkBmpRLECodec.cpp:240: dstRow[SkScaledCodec::GetDstCoord(x, fSampleX)]
=
nit: Maybe pull this function out? i.e.

dstX = SkScaledCodec::GetDstCoord(x, fSampleX)
...
dstRow[dstX] = ...

https://codereview.chromium.org/1287423002/diff/380001/src/codec/SkBmpRLECode...
src/codec/SkBmpRLECodec.cpp:277: fCurrRLEByte = 0;
Should this be done inside initializeStreamBuffer?

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

https://codereview.chromium.org/1287423002/diff/380001/src/codec/SkScaledCode...
src/codec/SkScaledCodec.cpp:260: if (kSuccess != result && kIncompleteInput !=
result) {
It looks like the check for kIncomplete is new?

[msarett, 2015-08-28 15:09:18]

Derek, would you mind taking another look at the API?  I think it's changed a
bit since your 1gtm.

https://codereview.chromium.org/1287423002/diff/380001/include/codec/SkScaled...
File include/codec/SkScaledCodec.h (right):

https://codereview.chromium.org/1287423002/diff/380001/include/codec/SkScaled...
include/codec/SkScaledCodec.h:45: * The output can be interpreted as an
x-coordinate or a y-coordinate.
On 2015/08/28 14:35:01, scroggo wrote:
> Maybe add comments that these do not need to be (and are not) called if
> sampleFactor is 1?

Done.

https://codereview.chromium.org/1287423002/diff/380001/include/codec/SkScaled...
include/codec/SkScaledCodec.h:47: static inline int GetStartCoord(int
sampleFactor);
On 2015/08/28 14:35:01, scroggo wrote:
> I believe inline functions need to be written in one place in order to inline?

You are right that this is silly.  Just typing inline in a bunch of different
places isn't going to do what I *think* we want.

I'm going to move the definitions to the header file, so that the compiler has
the option to inline if it feels like it.

https://codereview.chromium.org/1287423002/diff/380001/src/codec/SkBmpCodec.cpp
File src/codec/SkBmpCodec.cpp (right):

https://codereview.chromium.org/1287423002/diff/380001/src/codec/SkBmpCodec.c...
src/codec/SkBmpCodec.cpp:543: } else {
On 2015/08/28 14:35:01, scroggo wrote:
> nit: No need for else.

Done.

https://codereview.chromium.org/1287423002/diff/380001/src/codec/SkBmpCodec.c...
src/codec/SkBmpCodec.cpp:544: return height - y - 1;
On 2015/08/28 14:35:01, scroggo wrote:
> SkASSERT(kBottomUp == fRowOrder)

Done.

https://codereview.chromium.org/1287423002/diff/380001/src/codec/SkBmpRLECode...
File src/codec/SkBmpRLECodec.cpp (right):

https://codereview.chromium.org/1287423002/diff/380001/src/codec/SkBmpRLECode...
src/codec/SkBmpRLECodec.cpp:240: dstRow[SkScaledCodec::GetDstCoord(x, fSampleX)]
=
On 2015/08/28 14:35:01, scroggo wrote:
> nit: Maybe pull this function out? i.e.
> 
> dstX = SkScaledCodec::GetDstCoord(x, fSampleX)
> ...
> dstRow[dstX] = ...

Done.

https://codereview.chromium.org/1287423002/diff/380001/src/codec/SkBmpRLECode...
src/codec/SkBmpRLECodec.cpp:277: fCurrRLEByte = 0;
On 2015/08/28 14:35:01, scroggo wrote:
> Should this be done inside initializeStreamBuffer?

Yeah I think that's better.

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

https://codereview.chromium.org/1287423002/diff/380001/src/codec/SkScaledCode...
src/codec/SkScaledCodec.cpp:260: if (kSuccess != result && kIncompleteInput !=
result) {
On 2015/08/28 14:35:01, scroggo wrote:
> It looks like the check for kIncomplete is new?

Yeah it is.  If we stop on kIncompleteInput, we will fail to initialize all of
the memory in incomplete images.  If we keep calling getScanlines(), we will
keep returning kIncompleteInput, but eventually fill all of the memory.

I'm not really sold on this being a good strategy.  In my first attempt, I added
a bunch of calls to Fill() in this code, but I found that to be kind of ugly. 
I'm trying to refactor how we call Fill() in another CL - I'm glad you brought
this up because this is another place where what we are doing isn't so great.

https://codereview.chromium.org/1287423002/diff/420001/include/codec/SkScaled...
File include/codec/SkScaledCodec.h (right):

https://codereview.chromium.org/1287423002/diff/420001/include/codec/SkScaled...
include/codec/SkScaledCodec.h:49: static int GetStartCoord(int sampleFactor) {
return sampleFactor / 2 };
It occurs to me that maybe we don't want these implementations visible in the .h
file?  If that is the case, I can move them back to the .cpp file.  And later on
we can investigate if it's actually worth it to move them to SkCodecPriv.h and
try to inline.

[scroggo, 2015-08-28 15:11:23]

lgtm

https://codereview.chromium.org/1287423002/diff/420001/src/codec/SkBmpRLECode...
File src/codec/SkBmpRLECodec.cpp (right):

https://codereview.chromium.org/1287423002/diff/420001/src/codec/SkBmpRLECode...
src/codec/SkBmpRLECodec.cpp:204: int dstX = SkScaledCodec::GetDstCoord(x,
fSampleX);
nit: could be const.

[msarett, 2015-08-28 15:16:29]

Derek, would you mind taking another look at the API?  I think it's changed a
bit since your 1gtm.

https://codereview.chromium.org/1287423002/diff/420001/src/codec/SkBmpRLECode...
File src/codec/SkBmpRLECodec.cpp (right):

https://codereview.chromium.org/1287423002/diff/420001/src/codec/SkBmpRLECode...
src/codec/SkBmpRLECodec.cpp:204: int dstX = SkScaledCodec::GetDstCoord(x,
fSampleX);
On 2015/08/28 15:11:23, scroggo wrote:
> nit: could be const.

Done.

[djsollen, 2015-08-28 20:21:03]

https://codereview.chromium.org/1287423002/diff/440001/include/codec/SkScaled...
File include/codec/SkScaledCodec.h (right):

https://codereview.chromium.org/1287423002/diff/440001/include/codec/SkScaled...
include/codec/SkScaledCodec.h:47: * This does not need to be called and is not
called when sampleFactor == 1.
not sure this comment makes sense when it is a public API.  Are the results
invalid if the sampleFactor is 1?

https://codereview.chromium.org/1287423002/diff/440001/include/codec/SkScaled...
include/codec/SkScaledCodec.h:57: * This does not need to be called and is not
called when sampleFactor == 1.
same here.

https://codereview.chromium.org/1287423002/diff/440001/include/codec/SkScaled...
include/codec/SkScaledCodec.h:59: static int GetDstCoord(int srcCoord, int
sampleFactor) { return srcCoord / sampleFactor; };
why do these functions need to be in the public API anyway?

[msarett, 2015-08-28 20:35:56]

This is much better I think.

[scroggo, 2015-08-28 20:57:57]

On 2015/08/28 20:35:56, msarett wrote:
> This is much better I think.

Agreed. LGTM

[djsollen, 2015-08-31 13:34:59]

public api lgtm

[msarett, 2015-08-31 13:38:36]

The CQ bit was checked by msarett@google.com

[commit-bot: I haz the power, 2015-08-31 13:38:46]

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

[commit-bot: I haz the power, 2015-08-31 13:41:47]

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

[commit-bot: I haz the power, 2015-08-31 13:41:48]

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...)
  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-08-31 13:49:49]

The CQ bit was checked by msarett@google.com

[msarett, 2015-08-31 13:49:50]

The patchset sent to the CQ was uploaded after l-g-t-m from scroggo@google.com,
djsollen@google.com
Link to the patchset: https://codereview.chromium.org/1287423002/#ps480001
(title: "Fix windows errors")

[commit-bot: I haz the power, 2015-08-31 13:49:56]

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

[commit-bot: I haz the power, 2015-08-31 13:55:25]

Committed patchset #20 (id:480001) as
https://skia.googlesource.com/skia/+/5406d6f39ad042e7a0a0d4ea16beca4fe2b66492