Issue 1332053002: Fill incomplete images in SkCodec parent class

msarett

Patchset #1 (id:1) has been deleted

5 years, 3 months ago (2015-09-10 21:06:29 UTC) #1

msarett

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

5 years, 3 months ago (2015-09-11 15:34:52 UTC) #4

msarett

This CL is very much a work in progress. I normally wouldn't upload this early, ...

5 years, 3 months ago (2015-09-11 15:34:53 UTC) #5

msarett

Just want to make sure that, at this stage, you don't spend time reviewing this. ...

5 years, 3 months ago (2015-09-16 14:25:13 UTC) #6

msarett

This is ready for a review. No rush on this, I know you've been swamped ...

5 years, 3 months ago (2015-09-16 22:24:44 UTC) #9

scroggo

https://codereview.chromium.org/1332053002/diff/100001/dm/DMSrcSink.cpp File dm/DMSrcSink.cpp (right): https://codereview.chromium.org/1332053002/diff/100001/dm/DMSrcSink.cpp#newcode355 dm/DMSrcSink.cpp:355: uint32_t lines = 0; Maybe this should be linesDecoded? ...

5 years, 3 months ago (2015-09-22 18:02:48 UTC) #10

msarett

https://codereview.chromium.org/1332053002/diff/100001/dm/DMSrcSink.cpp File dm/DMSrcSink.cpp (right): https://codereview.chromium.org/1332053002/diff/100001/dm/DMSrcSink.cpp#newcode355 dm/DMSrcSink.cpp:355: uint32_t lines = 0; On 2015/09/22 18:02:48, scroggo wrote: ...

5 years, 3 months ago (2015-09-23 13:22:40 UTC) #11

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

https://codereview.chromium.org/1332053002/diff/100001/dm/DMSrcSink.cpp#newco...
dm/DMSrcSink.cpp:355: uint32_t lines = 0;
On 2015/09/22 18:02:48, scroggo wrote:
> Maybe this should be linesDecoded?

Done.

https://codereview.chromium.org/1332053002/diff/100001/dm/DMSrcSink.cpp#newco...
dm/DMSrcSink.cpp:456: return Error::Nonfatal("Image is incomplete, could not get
scanlines");
On 2015/09/22 18:02:48, scroggo wrote:
> This is going to change the behavior, right? Previously we did the same thing
on
> kSuccess and kIncompleteInput, but now we won't ever draw it and be able to
look
> at it in gold.
> 
> (Maybe this is okay, and correct, since we won't have filled in the rest of
the
> image, so we'll get uninitialized memory?)

Yes I should have been more explicit about behavior changes.

Before this CL, getScanlines() would fill the buffer on kIncompleteInput.  Now
getScanlines() just returns the number of lines successfully decoded and leaves
the remaining lines uninitialized.

Not sure if this is "correct", but this is what I intended.  Clients of
SkScanlineDecoder (SkScaledCodec is currently the only one) can and should fill
the uninitialized lines.  But a side effect is that we can't test the
SkScanlineDecoder on incomplete images from DM, which I think is ok.

https://codereview.chromium.org/1332053002/diff/100001/dm/DMSrcSink.cpp#newco...
dm/DMSrcSink.cpp:498: int lines;
On 2015/09/22 18:02:48, scroggo wrote:
> I would lean towards removing this variable and instead making each of the
below
> lines look like:
> 
> if (decoder->skip/getScanlines() < linesToRead) {
>   return Error::Nonfatal
> }
> 
> The reason is because its value is never reused - you assign to it each time
you
> read from it.

Done.

https://codereview.chromium.org/1332053002/diff/100001/include/codec/SkCodec.h
File include/codec/SkCodec.h (right):

https://codereview.chromium.org/1332053002/diff/100001/include/codec/SkCodec....
include/codec/SkCodec.h:237: uint32_t getFillValue(const SkImageInfo& dstInfo)
const {
On 2015/09/22 18:02:48, scroggo wrote:
> I assume the implementation only needs color type and alpha type?

Yes, I will add a comment explaining this.

As an alternative to using dstInfo as a parameter, I could use a colorType and
and an alphaType.  I don't really have a preference, do you have an opinion on
which is better?

https://codereview.chromium.org/1332053002/diff/100001/include/codec/SkCodec....
include/codec/SkCodec.h:247: void* getFillDst(void* dstStart, size_t rowBytes,
uint32_t decodedScanlines) const {
On 2015/09/22 18:02:48, scroggo wrote:
> Could you add a comment with an overview of what this function does?
> 
> Does this work for all scanline orderings?

Yes this needs clarification.

https://codereview.chromium.org/1332053002/diff/100001/include/codec/SkCodec....
include/codec/SkCodec.h:309: * The will only need to be overridden for images
where the rows are not
On 2015/09/22 18:02:48, scroggo wrote:
> This*

Done.

https://codereview.chromium.org/1332053002/diff/100001/include/codec/SkCodec....
include/codec/SkCodec.h:321: void setIncompleteScanlines(uint32_t
incompleteScanlines) {
On 2015/09/22 18:02:48, scroggo wrote:
> Maybe this should be returned by getPixels (in an out parameter)?
> 
> Oh, wait, this is only needed inside getPixels, right? What if we made it an
out
> parameter on onGetPixels. getPixels does not need to know about it.

Good idea!  I like this better than what I have.

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

https://codereview.chromium.org/1332053002/diff/100001/include/codec/SkScanli...
include/codec/SkScanlineDecoder.h:126: *  @return the number of lines
successfully skipped
On 2015/09/22 18:02:48, scroggo wrote:
> Is this useful to know? It seems like skipping partway is no better than
> skipping none.

Um no this isn't useful.  I'm making this return a bool.

I considered going further and making onSkipScanlines also return a bool, but
this prevents us from updating fCurrScanline.  Any thoughts?

https://codereview.chromium.org/1332053002/diff/100001/include/codec/SkScanli...
include/codec/SkScanlineDecoder.h:233: return this->onGetY(fCurrScanline);
On 2015/09/22 18:02:48, scroggo wrote:
> Could this function be non-virtual, and just depend on this->getScanlineOrder?

Yes I think so!  Will do.

The one thing that may be considered a drawback is that we will need to share
some code that is really only relevant to interlaced gifs.

https://codereview.chromium.org/1332053002/diff/100001/include/codec/SkScanli...
include/codec/SkScanlineDecoder.h:242: int getY(int encodedY) const {
On 2015/09/22 18:02:48, scroggo wrote:
> I find the name encodedY confusing. Maybe srcY? I'm curious why this is
needed,
> but I suppose I'll find out by the time I finish reviewing this...

In order to Fill() for kOutOfOrder images, we need to be able to continue to
iterate over rows, even when fCurrScanline stops advancing.

https://codereview.chromium.org/1332053002/diff/100001/include/codec/SkScanli...
include/codec/SkScanlineDecoder.h:248: return fCodec->getFillValue(dstInfo);
On 2015/09/22 18:02:48, scroggo wrote:
> This stuff will become cleaner once I merge codec and scanline decoder...

:)

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

https://codereview.chromium.org/1332053002/diff/100001/src/codec/SkBmpMaskCod...
src/codec/SkBmpMaskCodec.cpp:99: return y;
On 2015/09/22 18:02:48, scroggo wrote:
> It seems like we could have called Fill here, right? Did you not because this
is
> also used by scanline decoder?
> 
> Wait, I think I follow - getPixels takes care of this for us, right?

Yes.  It was my intention to make getPixels() take care of filling, so all of
the subcodecs do not need to have similar code.

https://codereview.chromium.org/1332053002/diff/100001/src/codec/SkCodec.cpp
File src/codec/SkCodec.cpp (right):

https://codereview.chromium.org/1332053002/diff/100001/src/codec/SkCodec.cpp#...
src/codec/SkCodec.cpp:147: // A return value of kIncompleteInput generally
indicates a truncated image stream.
On 2015/09/22 18:02:48, scroggo wrote:
> generally? Is there an exception?

Nope.  Removing that word.

https://codereview.chromium.org/1332053002/diff/100001/src/codec/SkCodecPriv.h
File src/codec/SkCodecPriv.h (right):

https://codereview.chromium.org/1332053002/diff/100001/src/codec/SkCodecPriv....
src/codec/SkCodecPriv.h:145: static uint32_t
get_color_table_fill_value(SkColorType colorType, const SkPMColor* colorPtr,
On 2015/09/22 18:02:48, scroggo wrote:
> inline, not static

Yes!  I'll go ahead and fix all of these.

https://codereview.chromium.org/1332053002/diff/100001/src/codec/SkCodec_libg...
File src/codec/SkCodec_libgif.cpp (right):

https://codereview.chromium.org/1332053002/diff/100001/src/codec/SkCodec_libg...
src/codec/SkCodec_libgif.cpp:668: if (SkCodec::kSuccess != fCodec->readRow()) {
On 2015/09/22 18:02:48, scroggo wrote:
> Should readRow just return a boolean?

Yes!

https://codereview.chromium.org/1332053002/diff/100001/src/codec/SkCodec_libp...
File src/codec/SkCodec_libpng.cpp (right):

https://codereview.chromium.org/1332053002/diff/100001/src/codec/SkCodec_libp...
src/codec/SkCodec_libpng.cpp:505: // bytes), and if we can't fill the buffer, we
immediately fail.  Making our buffer size
On 2015/09/22 18:02:48, scroggo wrote:
> Does this mean, for example, that we try to read 8192 bytes, but the image
> (incorrectly) only contains half that, which may have been enough to contain a
> non-zero number of lines, we fail when we could have decoded a few more lines
> and then failed?
> 
> I guess the problem is that sk_read_fn has no way to deal with reading partial
> data?

Yes that is exactly what the problem is.

https://codereview.chromium.org/1332053002/diff/100001/src/codec/SkCodec_libp...
src/codec/SkCodec_libpng.cpp:509:
this->setIncompleteScanlines(requestedInfo.height() - fRow);
On 2015/09/22 18:02:48, scroggo wrote:
> I always get confused when dealing with setjmp, so maybe this doesn't work. It
> seems weird to me that fRow is a member variable, but it is only used in this
> function. What if instead you said:
> 
> int row = 0;
> if (setjmp())
>   ....
> 
> for (; row < requestedInfo.height(); row++)
> ...
> 
> 
> (I know fRow is used by scanline decoding, but that tells me it should perhaps
> go on that class, rather than SkPngCodec)

Yes this works!  Clever!

scroggo

Overall, I this is moving in the right direction, though not quite ready to submit. ...

5 years, 2 months ago (2015-09-25 15:55:06 UTC) #12

Overall, I this is moving in the right direction, though not quite ready to
submit. It's giving me thoughts on what we can do once we merge the scanline
decoder with codec, which I may try to get you to let me land first, since I
think it will make some of these changes look interestingly different.

nit on rebasing:

First off, thanks for isolating your rebase patch sets, It makes it easier to
see what changes are yours, versus which changes were made due to a rebase.

If you can avoid rebasing entirely, though, that's even better. Then I can see
the pure diff between the two patch sets, plus any comments in the prior patch
set (with the rebase in between, I get the option of one or the other). I
generally only rebase if I need other changes in order to continue my work, or
if I'm ready to submit. (Maybe one of these was the case; if so, sorry for the
nag!)

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

https://codereview.chromium.org/1332053002/diff/100001/dm/DMSrcSink.cpp#newco...
dm/DMSrcSink.cpp:456: return Error::Nonfatal("Image is incomplete, could not get
scanlines");
On 2015/09/23 13:22:39, msarett wrote:
> On 2015/09/22 18:02:48, scroggo wrote:
> > This is going to change the behavior, right? Previously we did the same
thing
> on
> > kSuccess and kIncompleteInput, but now we won't ever draw it and be able to
> look
> > at it in gold.
> > 
> > (Maybe this is okay, and correct, since we won't have filled in the rest of
> the
> > image, so we'll get uninitialized memory?)
> 
> Yes I should have been more explicit about behavior changes.
> 
> Before this CL, getScanlines() would fill the buffer on kIncompleteInput.  Now
> getScanlines() just returns the number of lines successfully decoded and
leaves
> the remaining lines uninitialized.
> 
> Not sure if this is "correct", but this is what I intended.  Clients of
> SkScanlineDecoder (SkScaledCodec is currently the only one) can and should
fill
> the uninitialized lines. 

> But a side effect is that we can't test the
> SkScanlineDecoder on incomplete images from DM, which I think is ok.

Sure we can. We just need to make DM fill the buffer, now that it knows how many
scanlines were decoded.

https://codereview.chromium.org/1332053002/diff/100001/include/codec/SkCodec.h
File include/codec/SkCodec.h (right):

https://codereview.chromium.org/1332053002/diff/100001/include/codec/SkCodec....
include/codec/SkCodec.h:237: uint32_t getFillValue(const SkImageInfo& dstInfo)
const {
On 2015/09/23 13:22:39, msarett wrote:
> On 2015/09/22 18:02:48, scroggo wrote:
> > I assume the implementation only needs color type and alpha type?
> 
> Yes, I will add a comment explaining this.
> 
> As an alternative to using dstInfo as a parameter, I could use a colorType and
> and an alphaType.  I don't really have a preference, do you have an opinion on
> which is better?

I think only passing the information needed by the function is generally better.

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

https://codereview.chromium.org/1332053002/diff/100001/include/codec/SkScanli...
include/codec/SkScanlineDecoder.h:126: *  @return the number of lines
successfully skipped
On 2015/09/23 13:22:40, msarett wrote:
> On 2015/09/22 18:02:48, scroggo wrote:
> > Is this useful to know? It seems like skipping partway is no better than
> > skipping none.
> 
> Um no this isn't useful.  I'm making this return a bool.
> 
> I considered going further and making onSkipScanlines also return a bool, but
> this prevents us from updating fCurrScanline.  Any thoughts?

I think they should return the same value. If you want onSkipScanlines to return
a bool, you'll just add countLines (like we were doing before).

https://codereview.chromium.org/1332053002/diff/100001/src/codec/SkCodec_libp...
File src/codec/SkCodec_libpng.cpp (right):

https://codereview.chromium.org/1332053002/diff/100001/src/codec/SkCodec_libp...
src/codec/SkCodec_libpng.cpp:505: // bytes), and if we can't fill the buffer, we
immediately fail.  Making our buffer size
On 2015/09/23 13:22:40, msarett wrote:
> On 2015/09/22 18:02:48, scroggo wrote:
> > Does this mean, for example, that we try to read 8192 bytes, but the image
> > (incorrectly) only contains half that, which may have been enough to contain
a
> > non-zero number of lines, we fail when we could have decoded a few more
lines
> > and then failed?
> > 
> > I guess the problem is that sk_read_fn has no way to deal with reading
partial
> > data?
> 
> Yes that is exactly what the problem is.

Could you add that to the description?

https://codereview.chromium.org/1332053002/diff/100001/src/codec/SkCodec_libp...
src/codec/SkCodec_libpng.cpp:509:
this->setIncompleteScanlines(requestedInfo.height() - fRow);
On 2015/09/23 13:22:40, msarett wrote:
> On 2015/09/22 18:02:48, scroggo wrote:
> > I always get confused when dealing with setjmp, so maybe this doesn't work.
It
> > seems weird to me that fRow is a member variable, but it is only used in
this
> > function. What if instead you said:
> > 
> > int row = 0;
> > if (setjmp())
> >   ....
> > 
> > for (; row < requestedInfo.height(); row++)
> > ...
> > 
> > 
> > (I know fRow is used by scanline decoding, but that tells me it should
perhaps
> > go on that class, rather than SkPngCodec)
> 
> Yes this works!  Clever!

Just to make sure, did you test this? Can we add a test to CodexTest?

https://codereview.chromium.org/1332053002/diff/140001/include/codec/SkCodec.h
File include/codec/SkCodec.h (right):

https://codereview.chromium.org/1332053002/diff/140001/include/codec/SkCodec....
include/codec/SkCodec.h:253: * responsibility of the subclass to fill
uninitialized memory.
FWIW, it seems like getY could be used to make the base class do it (not sure
whether that is worth the trouble, if it's only for GIF though).

https://codereview.chromium.org/1332053002/diff/140001/include/codec/SkCodec....
include/codec/SkCodec.h:275: * @param incompleteScanlines When the encoded image
stream is incomplete, this function
As I keep thinking about skbug.com/4284, I wonder if we can get to a point where
all the possible Results returned from getPixels will be returned before calling
onGetPixels, allowing onGetPixels to return a number of lines like
onGetScanlines.

https://codereview.chromium.org/1332053002/diff/140001/include/codec/SkCodec....
include/codec/SkCodec.h:347: uint32_t                fIncompleteScanlines;
I think this no longer needs to be a member variable.

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

https://codereview.chromium.org/1332053002/diff/140001/include/codec/SkScanli...
include/codec/SkScanlineDecoder.h:126: *  @return true if the scanlines were
successfully skipped, false otherwise
I think we should further specify what this might mean. It seems like it means
that the data is incomplete, so it was not able to skip that many lines. Another
possibility appears to be bad parameters or we're going beyond the end of the
image. (On that note, going beyond the end of the image is a user error, but it
doesn't mean the image is incomplete. Not sure what to do there. Too bad for
dumb client?)

https://codereview.chromium.org/1332053002/diff/140001/include/codec/SkScanli...
include/codec/SkScanlineDecoder.h:134: return SkCodec::kInvalidParameters;
This return a bool. Not sure what though.

https://codereview.chromium.org/1332053002/diff/140001/include/codec/SkScanli...
include/codec/SkScanlineDecoder.h:136: uint32_t skippedLines =
this->onSkipScanlines(countLines);
As suggested in the first patch set, this should be something like:

if (!this->onSkipScanlines(countLines)) {
  // In my merge CL (crrev.com/1365313002, which I'm almost ready for you
  // to review), I use -1 as a value to mean that start must be called. I think
  // this would be a case where we might want to reset it to -1.
  return false;
}

fCurrScanline += skippedLines;
return true;

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

https://codereview.chromium.org/1332053002/diff/140001/src/codec/SkBmpMaskCod...
src/codec/SkBmpMaskCodec.cpp:58: *incompleteScanlines = dstInfo.height() - rows;
It's a little weird to me that onGetScanlines returns the number of rows that
were decoded, and onGetPixels returns the number that were *not* decoded. I
think this would be better to return the number successfully decoded. This means
you don't need to subtract here (and presumably, in the other subclasses), but
can subtract once in the base class.

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

https://codereview.chromium.org/1332053002/diff/140001/src/codec/SkBmpRLECode...
src/codec/SkBmpRLECodec.cpp:68: // We do not need to set incompleteScanlines
because decodeRows() will handle
How? It does not look like you do anything with that pointer?

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

https://codereview.chromium.org/1332053002/diff/140001/src/codec/SkCodec.cpp#...
src/codec/SkCodec.cpp:155: void* fillDst = this->getFillDst(pixels, rowBytes,
info.height() - incompleteScanlines);
My mistake, you end up doing another subtract. If you just return the rows
decoded, you could avoid both, right?

https://codereview.chromium.org/1332053002/diff/140001/src/codec/SkCodec_libg...
File src/codec/SkCodec_libgif.cpp (right):

https://codereview.chromium.org/1332053002/diff/140001/src/codec/SkCodec_libg...
src/codec/SkCodec_libgif.cpp:476: if (GIF_ERROR == DGifGetLine(fGif,
fSrcBuffer.get(), fFrameDims.width())) {
nit: This can be one line:

return GIF_ERROR != DGifGetLine(...);

https://codereview.chromium.org/1332053002/diff/140001/src/codec/SkCodec_libp...
File src/codec/SkCodec_libpng.cpp (right):

https://codereview.chromium.org/1332053002/diff/140001/src/codec/SkCodec_libp...
src/codec/SkCodec_libpng.cpp:618: return fRow;
Oh, wait, you said that "this works", and I thought you meant using a local
variable. That would be even better, if setjmp doesn't ruin it for us.

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

https://codereview.chromium.org/1332053002/diff/140001/src/codec/SkScanlineDe...
src/codec/SkScanlineDecoder.cpp:110: //SkASSERT(0 < srcY && srcY <
this->dstInfo().height());
Why is this commented out? Should it return some invalid value in that case? I
guess it already will...

msarett

IMO, this is much cleaner after the rebase! https://codereview.chromium.org/1332053002/diff/100001/dm/DMSrcSink.cpp File dm/DMSrcSink.cpp (right): https://codereview.chromium.org/1332053002/diff/100001/dm/DMSrcSink.cpp#newcode456 dm/DMSrcSink.cpp:456: return ...

5 years, 2 months ago (2015-10-01 12:44:52 UTC) #14

IMO, this is much cleaner after the rebase!

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

https://codereview.chromium.org/1332053002/diff/100001/dm/DMSrcSink.cpp#newco...
dm/DMSrcSink.cpp:456: return Error::Nonfatal("Image is incomplete, could not get
scanlines");
On 2015/09/25 15:55:05, scroggo wrote:
> On 2015/09/23 13:22:39, msarett wrote:
> > On 2015/09/22 18:02:48, scroggo wrote:
> > > This is going to change the behavior, right? Previously we did the same
> thing
> > on
> > > kSuccess and kIncompleteInput, but now we won't ever draw it and be able
to
> > look
> > > at it in gold.
> > > 
> > > (Maybe this is okay, and correct, since we won't have filled in the rest
of
> > the
> > > image, so we'll get uninitialized memory?)
> > 
> > Yes I should have been more explicit about behavior changes.
> > 
> > Before this CL, getScanlines() would fill the buffer on kIncompleteInput. 
Now
> > getScanlines() just returns the number of lines successfully decoded and
> leaves
> > the remaining lines uninitialized.
> > 
> > Not sure if this is "correct", but this is what I intended.  Clients of
> > SkScanlineDecoder (SkScaledCodec is currently the only one) can and should
> fill
> > the uninitialized lines. 
> 
> > But a side effect is that we can't test the
> > SkScanlineDecoder on incomplete images from DM, which I think is ok.
> 
> Sure we can. We just need to make DM fill the buffer, now that it knows how
many
> scanlines were decoded.

kScanlineSubset_Mode now fills on incomplete images, instead of failing.  For
consistency, kScanline_Mode has also been modified to fill incomplete images.

https://codereview.chromium.org/1332053002/diff/100001/include/codec/SkCodec.h
File include/codec/SkCodec.h (right):

https://codereview.chromium.org/1332053002/diff/100001/include/codec/SkCodec....
include/codec/SkCodec.h:237: uint32_t getFillValue(const SkImageInfo& dstInfo)
const {
On 2015/09/25 15:55:05, scroggo wrote:
> On 2015/09/23 13:22:39, msarett wrote:
> > On 2015/09/22 18:02:48, scroggo wrote:
> > > I assume the implementation only needs color type and alpha type?
> > 
> > Yes, I will add a comment explaining this.
> > 
> > As an alternative to using dstInfo as a parameter, I could use a colorType
and
> > and an alphaType.  I don't really have a preference, do you have an opinion
on
> > which is better?
> 
> I think only passing the information needed by the function is generally
better.

Done.

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

https://codereview.chromium.org/1332053002/diff/100001/include/codec/SkScanli...
include/codec/SkScanlineDecoder.h:126: *  @return the number of lines
successfully skipped
On 2015/09/25 15:55:05, scroggo wrote:
> On 2015/09/23 13:22:40, msarett wrote:
> > On 2015/09/22 18:02:48, scroggo wrote:
> > > Is this useful to know? It seems like skipping partway is no better than
> > > skipping none.
> > 
> > Um no this isn't useful.  I'm making this return a bool.
> > 
> > I considered going further and making onSkipScanlines also return a bool,
but
> > this prevents us from updating fCurrScanline.  Any thoughts?
> 
> I think they should return the same value. If you want onSkipScanlines to
return
> a bool, you'll just add countLines (like we were doing before).

Will change, I thought returning different values was a bit strange as well.

On the other hand, we may skip "some" of the count lines before failing.  In
this case, fCurrScanline will not be updated properly.  This is not a big deal
because we are treating failures to skip scanlines as errors.  But if we were,
for some reason, to continue using the scanline decoder after a failed skip,
this would be a bug.

https://codereview.chromium.org/1332053002/diff/100001/src/codec/SkCodec_libp...
File src/codec/SkCodec_libpng.cpp (right):

https://codereview.chromium.org/1332053002/diff/100001/src/codec/SkCodec_libp...
src/codec/SkCodec_libpng.cpp:505: // bytes), and if we can't fill the buffer, we
immediately fail.  Making our buffer size
On 2015/09/25 15:55:05, scroggo wrote:
> On 2015/09/23 13:22:40, msarett wrote:
> > On 2015/09/22 18:02:48, scroggo wrote:
> > > Does this mean, for example, that we try to read 8192 bytes, but the image
> > > (incorrectly) only contains half that, which may have been enough to
contain
> a
> > > non-zero number of lines, we fail when we could have decoded a few more
> lines
> > > and then failed?
> > > 
> > > I guess the problem is that sk_read_fn has no way to deal with reading
> partial
> > > data?
> > 
> > Yes that is exactly what the problem is.
> 
> Could you add that to the description?

Done.

https://codereview.chromium.org/1332053002/diff/100001/src/codec/SkCodec_libp...
src/codec/SkCodec_libpng.cpp:509:
this->setIncompleteScanlines(requestedInfo.height() - fRow);
On 2015/09/25 15:55:05, scroggo wrote:
> On 2015/09/23 13:22:40, msarett wrote:
> > On 2015/09/22 18:02:48, scroggo wrote:
> > > I always get confused when dealing with setjmp, so maybe this doesn't
work.
> It
> > > seems weird to me that fRow is a member variable, but it is only used in
> this
> > > function. What if instead you said:
> > > 
> > > int row = 0;
> > > if (setjmp())
> > >   ....
> > > 
> > > for (; row < requestedInfo.height(); row++)
> > > ...
> > > 
> > > 
> > > (I know fRow is used by scanline decoding, but that tells me it should
> perhaps
> > > go on that class, rather than SkPngCodec)
> > 
> > Yes this works!  Clever!
> 
> Just to make sure, did you test this? Can we add a test to CodexTest?

Yes I tested this.  I have been testing by decoding incomplete pngs in
DMSrcSink.  I intend to upload my test images on the bots when this lands.

Would you be in favor of checking in incomplete images to resources and
asserting that decodes of these images return kIncompleteInput?  I'm not sure
how else to test in CodexTest, but I think the testing in DMSrcSink is
sufficient.

https://codereview.chromium.org/1332053002/diff/140001/include/codec/SkCodec.h
File include/codec/SkCodec.h (right):

https://codereview.chromium.org/1332053002/diff/140001/include/codec/SkCodec....
include/codec/SkCodec.h:253: * responsibility of the subclass to fill
uninitialized memory.
On 2015/09/25 15:55:05, scroggo wrote:
> FWIW, it seems like getY could be used to make the base class do it (not sure
> whether that is worth the trouble, if it's only for GIF though).

We will be able to do this (and I intend to do this) once we combine
SkScanlineDecoder and SkCodec.

https://codereview.chromium.org/1332053002/diff/140001/include/codec/SkCodec....
include/codec/SkCodec.h:275: * @param incompleteScanlines When the encoded image
stream is incomplete, this function
On 2015/09/25 15:55:05, scroggo wrote:
> As I keep thinking about skbug.com/4284, I wonder if we can get to a point
where
> all the possible Results returned from getPixels will be returned before
calling
> onGetPixels, allowing onGetPixels to return a number of lines like
> onGetScanlines.

That would be great if we could do that!  For now, we need this :|.

https://codereview.chromium.org/1332053002/diff/140001/include/codec/SkCodec....
include/codec/SkCodec.h:347: uint32_t                fIncompleteScanlines;
On 2015/09/25 15:55:05, scroggo wrote:
> I think this no longer needs to be a member variable.

Done.

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

https://codereview.chromium.org/1332053002/diff/140001/include/codec/SkScanli...
include/codec/SkScanlineDecoder.h:126: *  @return true if the scanlines were
successfully skipped, false otherwise
On 2015/09/25 15:55:05, scroggo wrote:
> I think we should further specify what this might mean. It seems like it means
> that the data is incomplete, so it was not able to skip that many lines.
Another
> possibility appears to be bad parameters or we're going beyond the end of the
> image. (On that note, going beyond the end of the image is a user error, but
it
> doesn't mean the image is incomplete. Not sure what to do there. Too bad for
> dumb client?)

I will add to the comment.

https://codereview.chromium.org/1332053002/diff/140001/include/codec/SkScanli...
include/codec/SkScanlineDecoder.h:134: return SkCodec::kInvalidParameters;
On 2015/09/25 15:55:05, scroggo wrote:
> This return a bool. Not sure what though.

Making it return false.

https://codereview.chromium.org/1332053002/diff/140001/include/codec/SkScanli...
include/codec/SkScanlineDecoder.h:136: uint32_t skippedLines =
this->onSkipScanlines(countLines);
On 2015/09/25 15:55:05, scroggo wrote:
> As suggested in the first patch set, this should be something like:
> 
> if (!this->onSkipScanlines(countLines)) {
>   // In my merge CL (crrev.com/1365313002, which I'm almost ready for you
>   // to review), I use -1 as a value to mean that start must be called. I
think
>   // this would be a case where we might want to reset it to -1.
>   return false;
> }
> 
> fCurrScanline += skippedLines;
> return true;

Done.

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

https://codereview.chromium.org/1332053002/diff/140001/src/codec/SkBmpMaskCod...
src/codec/SkBmpMaskCodec.cpp:58: *incompleteScanlines = dstInfo.height() - rows;
On 2015/09/25 15:55:05, scroggo wrote:
> It's a little weird to me that onGetScanlines returns the number of rows that
> were decoded, and onGetPixels returns the number that were *not* decoded. I
> think this would be better to return the number successfully decoded. This
means
> you don't need to subtract here (and presumably, in the other subclasses), but
> can subtract once in the base class.

Done.

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

https://codereview.chromium.org/1332053002/diff/140001/src/codec/SkBmpRLECode...
src/codec/SkBmpRLECodec.cpp:68: // We do not need to set incompleteScanlines
because decodeRows() will handle
On 2015/09/25 15:55:05, scroggo wrote:
> How? It does not look like you do anything with that pointer?

We always fill the background on RLE decodes, because RLE encodings sometimes
skip pixels.  I will make the comment more clear.

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

https://codereview.chromium.org/1332053002/diff/140001/src/codec/SkCodec.cpp#...
src/codec/SkCodec.cpp:155: void* fillDst = this->getFillDst(pixels, rowBytes,
info.height() - incompleteScanlines);
On 2015/09/25 15:55:05, scroggo wrote:
> My mistake, you end up doing another subtract. If you just return the rows
> decoded, you could avoid both, right?

Acknowledged.

https://codereview.chromium.org/1332053002/diff/140001/src/codec/SkCodec_libg...
File src/codec/SkCodec_libgif.cpp (right):

https://codereview.chromium.org/1332053002/diff/140001/src/codec/SkCodec_libg...
src/codec/SkCodec_libgif.cpp:476: if (GIF_ERROR == DGifGetLine(fGif,
fSrcBuffer.get(), fFrameDims.width())) {
On 2015/09/25 15:55:06, scroggo wrote:
> nit: This can be one line:
> 
> return GIF_ERROR != DGifGetLine(...);

Done.

https://codereview.chromium.org/1332053002/diff/140001/src/codec/SkCodec_libp...
File src/codec/SkCodec_libpng.cpp (right):

https://codereview.chromium.org/1332053002/diff/140001/src/codec/SkCodec_libp...
src/codec/SkCodec_libpng.cpp:618: return fRow;
On 2015/09/25 15:55:06, scroggo wrote:
> Oh, wait, you said that "this works", and I thought you meant using a local
> variable. That would be even better, if setjmp doesn't ruin it for us.

It does work...For some reason I only thought it would only work in
onGetPixels(), but clearly it will work here as well - we always start over at
zero.

I will fix.

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

https://codereview.chromium.org/1332053002/diff/140001/src/codec/SkScanlineDe...
src/codec/SkScanlineDecoder.cpp:110: //SkASSERT(0 < srcY && srcY <
this->dstInfo().height());
On 2015/09/25 15:55:06, scroggo wrote:
> Why is this commented out? Should it return some invalid value in that case? I
> guess it already will...

Hmmm...  Looks like I commented this out instead of fixing it.  It is fixed now.

https://codereview.chromium.org/1332053002/diff/200001/dm/DMSrcSink.cpp
File dm/DMSrcSink.cpp (right):

https://codereview.chromium.org/1332053002/diff/200001/dm/DMSrcSink.cpp#newco...
dm/DMSrcSink.cpp:352: codec->getScanlines(dst, height, rowBytes);
getScanlines() now fills when it cannot get all of the lines, so we do not need
to inspect the return value.

https://codereview.chromium.org/1332053002/diff/200001/src/codec/SkCodec_libi...
File src/codec/SkCodec_libico.cpp (right):

https://codereview.chromium.org/1332053002/diff/200001/src/codec/SkCodec_libi...
src/codec/SkCodec_libico.cpp:282: *rowsDecoded = info.height();
It's a bit strange when a call to getPixels() calls getPixels() or
getScanlines() on an owned codec.  We don't want to fill from this codec because
the embedded codec already handles filling.

This comes up in SkScaledCodec as well.

scroggo

> This CL also makes an API change to SkScanlineDecoder.h > > SkScanlineDecoder::getScanlines() now returns ...

5 years, 2 months ago (2015-10-01 14:48:32 UTC) #15

> This CL also makes an API change to SkScanlineDecoder.h
> 
> SkScanlineDecoder::getScanlines() now returns the
> number of lines it read successfully, rather than a
> SkCodec::Result enum.  getScanlines() can only fail on
> an incomplete input, in which case it is useful to know
> how many lines were successfully decoded - this provides
> more information than kIncomplete vs kSuccess.
> 
> Clients that use SkScanlineDecoder are able to fill
> uninitialized memory easily when the getScanlines() API
> indicates how many scanlines were decoded successfully.

These comments need to be updated to reflect the fact that SkScanlineDecoder
merged into SkCodec.

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

https://codereview.chromium.org/1332053002/diff/100001/include/codec/SkScanli...
include/codec/SkScanlineDecoder.h:126: *  @return the number of lines
successfully skipped
On 2015/10/01 12:44:51, msarett wrote:
> On 2015/09/25 15:55:05, scroggo wrote:
> > On 2015/09/23 13:22:40, msarett wrote:
> > > On 2015/09/22 18:02:48, scroggo wrote:
> > > > Is this useful to know? It seems like skipping partway is no better than
> > > > skipping none.
> > > 
> > > Um no this isn't useful.  I'm making this return a bool.
> > > 
> > > I considered going further and making onSkipScanlines also return a bool,
> but
> > > this prevents us from updating fCurrScanline.  Any thoughts?
> > 
> > I think they should return the same value. If you want onSkipScanlines to
> return
> > a bool, you'll just add countLines (like we were doing before).
> 
> Will change, I thought returning different values was a bit strange as well.
> 
> On the other hand, we may skip "some" of the count lines before failing.  In
> this case, fCurrScanline will not be updated properly.  This is not a big deal
> because we are treating failures to skip scanlines as errors.  But if we were,
> for some reason, to continue using the scanline decoder after a failed skip,
> this would be a bug.

In general, I think it's better to report failures early. In the case of
skipping some of the lines, but not all, it seems like the next call to either
get or skip should definitely fail, so we might as well report that now.

I thought perhaps we should set fCurrScanline to -1, so the next call will
immediately return false - that would make us fail sooner next time, but the
next call ought to fail anyway, so it might be unnecessary (and perhaps
confusing if someone was debugging and trying to figure out why start had not
been called).

https://codereview.chromium.org/1332053002/diff/100001/src/codec/SkCodec_libp...
File src/codec/SkCodec_libpng.cpp (right):

https://codereview.chromium.org/1332053002/diff/100001/src/codec/SkCodec_libp...
src/codec/SkCodec_libpng.cpp:509:
this->setIncompleteScanlines(requestedInfo.height() - fRow);
On 2015/10/01 12:44:51, msarett wrote:
> On 2015/09/25 15:55:05, scroggo wrote:
> > On 2015/09/23 13:22:40, msarett wrote:
> > > On 2015/09/22 18:02:48, scroggo wrote:
> > > > I always get confused when dealing with setjmp, so maybe this doesn't
> work.
> > It
> > > > seems weird to me that fRow is a member variable, but it is only used in
> > this
> > > > function. What if instead you said:
> > > > 
> > > > int row = 0;
> > > > if (setjmp())
> > > >   ....
> > > > 
> > > > for (; row < requestedInfo.height(); row++)
> > > > ...
> > > > 
> > > > 
> > > > (I know fRow is used by scanline decoding, but that tells me it should
> > perhaps
> > > > go on that class, rather than SkPngCodec)
> > > 
> > > Yes this works!  Clever!
> > 
> > Just to make sure, did you test this? Can we add a test to CodexTest?
> 
> Yes I tested this.  I have been testing by decoding incomplete pngs in
> DMSrcSink.  I intend to upload my test images on the bots when this lands.
> 
> Would you be in favor of checking in incomplete images to resources and
> asserting that decodes of these images return kIncompleteInput?  I'm not sure
> how else to test in CodexTest, but I think the testing in DMSrcSink is
> sufficient.

Possibly - the advantage to including it in CodexTest is that we would be
testing that decoding an incomplete image actually does return kIncomplete.

A disadvantage to relying on DM is that if some decode changed from an error to
a Nonfatal Error it seems like we could miss that.

I do like to be conservative about what we add to our resources directory
though.

https://codereview.chromium.org/1332053002/diff/200001/dm/DMSrcSink.cpp
File dm/DMSrcSink.cpp (right):

https://codereview.chromium.org/1332053002/diff/200001/dm/DMSrcSink.cpp#newco...
dm/DMSrcSink.cpp:358: codec->getScanlines(dstPtr, 1, bitmap.rowBytes());
Don't we want to break out of the loop if 0 is returned?

https://codereview.chromium.org/1332053002/diff/200001/dm/DMSrcSink.cpp#newco...
dm/DMSrcSink.cpp:425: return Error::Nonfatal("Image is incomplete, could not get
scanlines");
Since we've returned an error, we will not be able to look at the output, right?
I think we still want to draw the output.

Same for the rest of our incomplete images.

https://codereview.chromium.org/1332053002/diff/200001/dm/DMSrcSink.cpp#newco...
dm/DMSrcSink.cpp:434: codec->getScanlines(buffer, currentSubsetHeight,
rowBytes);
Note that you do something different if skipScanlines reveals the image to be
incomplete versus when getScanlines does. I think this is the correct behavior
(continue)

https://codereview.chromium.org/1332053002/diff/200001/dm/DMSrcSink.cpp#newco...
dm/DMSrcSink.cpp:438: * we copy all the lines at once becuase when calling
getScanlines for
This isn't due to your CL, but it seems like this comment no longer goes with
the code it applies to?

https://codereview.chromium.org/1332053002/diff/200001/dm/DMSrcSink.cpp#newco...
dm/DMSrcSink.cpp:480: return Error::Nonfatal("Image is incomplete, could not get
scanlines");
Not only was the image incomplete, but it was so incomplete that we did not
decode anything.

https://codereview.chromium.org/1332053002/diff/200001/include/codec/SkCodec.h
File include/codec/SkCodec.h (right):

https://codereview.chromium.org/1332053002/diff/200001/include/codec/SkCodec....
include/codec/SkCodec.h:237: * On a kIncompleteInput, we will fill any
uninitialized scanlines.  We allow the subclass
I think "we" is a little vague here. I think you mean "getPixels"? Does the
scanline decoder also fill uninitialized scanlines?

On that note, I *think* that if we decode a single scanline, we'll fill, but if
we decode zero, it seems like the image is completely broken - should we fill in
that case? (Maybe we have a different answer for scanline decoding versus full
decoding?)

https://codereview.chromium.org/1332053002/diff/200001/include/codec/SkCodec....
include/codec/SkCodec.h:248: uint32_t getFillValue(SkColorType colorType,
SkAlphaType alphaType) const {
Does this method need to be public?

https://codereview.chromium.org/1332053002/diff/200001/include/codec/SkCodec....
include/codec/SkCodec.h:299: *  @return the number of lines successfully decoded
Does this fill if some were decoded? What does it mean if it's less than
countLines? (Please add to comments.)

https://codereview.chromium.org/1332053002/diff/200001/include/codec/SkCodec....
include/codec/SkCodec.h:316: *              Invalid parameters passed to
skipScanlines()
What are invalid parameters?

https://codereview.chromium.org/1332053002/diff/200001/include/codec/SkCodec....
include/codec/SkCodec.h:396: /**
nit: newline between these methods

https://codereview.chromium.org/1332053002/diff/200001/include/codec/SkCodec....
include/codec/SkCodec.h:401: int nextScanline(int srcY) const;
I don't think this name is right. My first thought was something like

  int outputScanline(int inputScanline)

but I'm not sure that's very clear either. You're not returning the "next"
scanline, you're returning the logical output location of the (inputVar)'th row
stored in the encoded data.

https://codereview.chromium.org/1332053002/diff/200001/include/codec/SkCodec....
include/codec/SkCodec.h:508: // FIXME (msarett): We should not reallocate this
memory on every call.
Is the real fix to always override onSkipScanlines? I thought we discussed that
we could almost always do better than this default (we can at least skip
swizzling - only jpeg does not require a swizzle always, but it already has an
override). We could make this pure virtual and force the caller to be smarter. I
think many (maybe all) of the subclasses have their own buffers that they reuse,
so we can always avoid the reallocation.

https://codereview.chromium.org/1332053002/diff/200001/include/codec/SkCodec....
include/codec/SkCodec.h:526: * @param dst            Destination pixel memory
I think this is different depending on the scanline order?

https://codereview.chromium.org/1332053002/diff/200001/src/codec/SkCodec.cpp
File src/codec/SkCodec.cpp (left):

https://codereview.chromium.org/1332053002/diff/200001/src/codec/SkCodec.cpp#...
src/codec/SkCodec.cpp:212: return kScanlineDecodingNotStarted;
Do we still need this enum value?

https://codereview.chromium.org/1332053002/diff/200001/src/codec/SkCodec.cpp
File src/codec/SkCodec.cpp (right):

https://codereview.chromium.org/1332053002/diff/200001/src/codec/SkCodec.cpp#...
src/codec/SkCodec.cpp:226: uint32_t SkCodec::getScanlines(void* dst, int
countLines, size_t rowBytes) {
If we wanted to, we could make getScanlines return a Result (so it could return
errors for invalid params and decoding not started), and onGetScanlines return a
number of lines decoded. Maybe the caller does not care exactly how many lines
were decoded? (On the other hand, maybe it is nice for the client to know how
many lines were decoded, so they can decide whether they want to display the
partial image. getPixels does not provide that information, so maybe this one
shouldn't either?)

https://codereview.chromium.org/1332053002/diff/200001/src/codec/SkCodec.cpp#...
src/codec/SkCodec.cpp:275: return get_output_row_interlaced(srcY,
this->getInfo().height());
It's odd to me that kBottomUp takes the dstInfo into account, but kOutOfOrder
does not.

https://codereview.chromium.org/1332053002/diff/200001/src/codec/SkCodec.cpp#...
src/codec/SkCodec.cpp:298: SkASSERT(1 == linesRequested ||
this->getInfo().height() == linesRequested);
Do you need the second half? It seems like if it's true the first half will be,
too?

https://codereview.chromium.org/1332053002/diff/200001/src/codec/SkCodec_libi...
File src/codec/SkCodec_libico.cpp (right):

https://codereview.chromium.org/1332053002/diff/200001/src/codec/SkCodec_libi...
src/codec/SkCodec_libico.cpp:138: // partially decode this incomplete embedded
image.
Is there anything to partially decode if no embedded codecs were created?

https://codereview.chromium.org/1332053002/diff/200001/src/codec/SkCodec_wbmp...
File src/codec/SkCodec_wbmp.cpp (right):

https://codereview.chromium.org/1332053002/diff/200001/src/codec/SkCodec_wbmp...
src/codec/SkCodec_wbmp.cpp:141: Result rowResult = this->readRow(src.get());
I think this should also return a bool?

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

https://codereview.chromium.org/1332053002/diff/200001/src/codec/SkScaledCode...
src/codec/SkScaledCodec.cpp:209: *rowsDecoded = y + 1;
Why is this y+1 and not y? (Same for other instances below)

https://codereview.chromium.org/1332053002/diff/200001/src/codec/SkScaledCode...
src/codec/SkScaledCodec.cpp:310: int scanlines = (int)
fCodec->getScanlines(storagePtr, srcHeight, rowBytes);
It's weird to me that getScanlines returns one type, which you often cast to an
int. Can you use the same type throughout? Is there a reason they are different?

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

https://codereview.chromium.org/1332053002/diff/200001/src/codec/SkSwizzler.c...
src/codec/SkSwizzler.cpp:725: switch(fillInfo.colorType()) {
nit: space between "switch" and "("

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

https://codereview.chromium.org/1332053002/diff/200001/src/codec/SkSwizzler.h...
src/codec/SkSwizzler.h:145: * Contains the color type of the rows to fill.
And, alpha I assume?

msarett

https://codereview.chromium.org/1332053002/diff/100001/include/codec/SkScanlineDecoder.h File include/codec/SkScanlineDecoder.h (right): https://codereview.chromium.org/1332053002/diff/100001/include/codec/SkScanlineDecoder.h#newcode126 include/codec/SkScanlineDecoder.h:126: * @return the number of lines successfully skipped On ...

5 years, 2 months ago (2015-10-01 18:14:14 UTC) #16

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

https://codereview.chromium.org/1332053002/diff/100001/include/codec/SkScanli...
include/codec/SkScanlineDecoder.h:126: *  @return the number of lines
successfully skipped
On 2015/10/01 14:48:31, scroggo wrote:
> On 2015/10/01 12:44:51, msarett wrote:
> > On 2015/09/25 15:55:05, scroggo wrote:
> > > On 2015/09/23 13:22:40, msarett wrote:
> > > > On 2015/09/22 18:02:48, scroggo wrote:
> > > > > Is this useful to know? It seems like skipping partway is no better
than
> > > > > skipping none.
> > > > 
> > > > Um no this isn't useful.  I'm making this return a bool.
> > > > 
> > > > I considered going further and making onSkipScanlines also return a
bool,
> > but
> > > > this prevents us from updating fCurrScanline.  Any thoughts?
> > > 
> > > I think they should return the same value. If you want onSkipScanlines to
> > return
> > > a bool, you'll just add countLines (like we were doing before).
> > 
> > Will change, I thought returning different values was a bit strange as well.
> > 
> > On the other hand, we may skip "some" of the count lines before failing.  In
> > this case, fCurrScanline will not be updated properly.  This is not a big
deal
> > because we are treating failures to skip scanlines as errors.  But if we
were,
> > for some reason, to continue using the scanline decoder after a failed skip,
> > this would be a bug.
> 
> In general, I think it's better to report failures early. In the case of
> skipping some of the lines, but not all, it seems like the next call to either
> get or skip should definitely fail, so we might as well report that now.
> 
> I thought perhaps we should set fCurrScanline to -1, so the next call will
> immediately return false - that would make us fail sooner next time, but the
> next call ought to fail anyway, so it might be unnecessary (and perhaps
> confusing if someone was debugging and trying to figure out why start had not
> been called).

Gotcha.  For now, I'm happy with it as is.

https://codereview.chromium.org/1332053002/diff/100001/src/codec/SkCodec_libp...
File src/codec/SkCodec_libpng.cpp (right):

https://codereview.chromium.org/1332053002/diff/100001/src/codec/SkCodec_libp...
src/codec/SkCodec_libpng.cpp:509:
this->setIncompleteScanlines(requestedInfo.height() - fRow);
On 2015/10/01 14:48:31, scroggo wrote:
> On 2015/10/01 12:44:51, msarett wrote:
> > On 2015/09/25 15:55:05, scroggo wrote:
> > > On 2015/09/23 13:22:40, msarett wrote:
> > > > On 2015/09/22 18:02:48, scroggo wrote:
> > > > > I always get confused when dealing with setjmp, so maybe this doesn't
> > work.
> > > It
> > > > > seems weird to me that fRow is a member variable, but it is only used
in
> > > this
> > > > > function. What if instead you said:
> > > > > 
> > > > > int row = 0;
> > > > > if (setjmp())
> > > > >   ....
> > > > > 
> > > > > for (; row < requestedInfo.height(); row++)
> > > > > ...
> > > > > 
> > > > > 
> > > > > (I know fRow is used by scanline decoding, but that tells me it should
> > > perhaps
> > > > > go on that class, rather than SkPngCodec)
> > > > 
> > > > Yes this works!  Clever!
> > > 
> > > Just to make sure, did you test this? Can we add a test to CodexTest?
> > 
> > Yes I tested this.  I have been testing by decoding incomplete pngs in
> > DMSrcSink.  I intend to upload my test images on the bots when this lands.
> > 
> > Would you be in favor of checking in incomplete images to resources and
> > asserting that decodes of these images return kIncompleteInput?  I'm not
sure
> > how else to test in CodexTest, but I think the testing in DMSrcSink is
> > sufficient.
> 
> Possibly - the advantage to including it in CodexTest is that we would be
> testing that decoding an incomplete image actually does return kIncomplete.
> 
> A disadvantage to relying on DM is that if some decode changed from an error
to
> a Nonfatal Error it seems like we could miss that.
> 
> I do like to be conservative about what we add to our resources directory
> though.

I will add a single incomplete image to resources and verify that it decodes the
same (and returns kIncomplete) for all of our decoding options.

https://codereview.chromium.org/1332053002/diff/200001/dm/DMSrcSink.cpp
File dm/DMSrcSink.cpp (right):

https://codereview.chromium.org/1332053002/diff/200001/dm/DMSrcSink.cpp#newco...
dm/DMSrcSink.cpp:358: codec->getScanlines(dstPtr, 1, bitmap.rowBytes());
On 2015/10/01 14:48:31, scroggo wrote:
> Don't we want to break out of the loop if 0 is returned?

We could do that.  But then we need to iterate over the remaining lines and fill
line by line.  This duplicates the fill code we already have in SkCodec.cpp. 
See Patch Set 4 for this implementation.

Alternatively, here we keep calling getScanlines, and every time it fails, it
will fill the line.  I know we've discussed that we don't really like this
approach.  I think I prefer it here because it simplifies our test code.  But I
don't feel strongly and would be happy to change.

https://codereview.chromium.org/1332053002/diff/200001/dm/DMSrcSink.cpp#newco...
dm/DMSrcSink.cpp:425: return Error::Nonfatal("Image is incomplete, could not get
scanlines");
On 2015/10/01 14:48:31, scroggo wrote:
> Since we've returned an error, we will not be able to look at the output,
right?
> I think we still want to draw the output.
> 
> Same for the rest of our incomplete images.

Yeah the way the test is written now, we will only see the output if the image
is at least half complete.  We can do better than this.

https://codereview.chromium.org/1332053002/diff/200001/dm/DMSrcSink.cpp#newco...
dm/DMSrcSink.cpp:434: codec->getScanlines(buffer, currentSubsetHeight,
rowBytes);
On 2015/10/01 14:48:31, scroggo wrote:
> Note that you do something different if skipScanlines reveals the image to be
> incomplete versus when getScanlines does. I think this is the correct behavior
> (continue)

Acknowledged.

https://codereview.chromium.org/1332053002/diff/200001/dm/DMSrcSink.cpp#newco...
dm/DMSrcSink.cpp:438: * we copy all the lines at once becuase when calling
getScanlines for
On 2015/10/01 14:48:31, scroggo wrote:
> This isn't due to your CL, but it seems like this comment no longer goes with
> the code it applies to?

Yeah I'm a bit confused by this comment.  Removing it.

https://codereview.chromium.org/1332053002/diff/200001/dm/DMSrcSink.cpp#newco...
dm/DMSrcSink.cpp:480: return Error::Nonfatal("Image is incomplete, could not get
scanlines");
On 2015/10/01 14:48:31, scroggo wrote:
> Not only was the image incomplete, but it was so incomplete that we did not
> decode anything.

This could be improved as well.

https://codereview.chromium.org/1332053002/diff/200001/include/codec/SkCodec.h
File include/codec/SkCodec.h (right):

https://codereview.chromium.org/1332053002/diff/200001/include/codec/SkCodec....
include/codec/SkCodec.h:237: * On a kIncompleteInput, we will fill any
uninitialized scanlines.  We allow the subclass
On 2015/10/01 14:48:31, scroggo wrote:
> I think "we" is a little vague here. I think you mean "getPixels"? Does the
> scanline decoder also fill uninitialized scanlines?
> 
> On that note, I *think* that if we decode a single scanline, we'll fill, but
if
> we decode zero, it seems like the image is completely broken - should we fill
in
> that case? (Maybe we have a different answer for scanline decoding versus full
> decoding?)

The current implementation will always fill for getPixels() and getScanlines(). 
I would argue that being consistent on this is a good security practice. 
"Sometimes" leaving memory uninitialized seems dangerous to me.

https://codereview.chromium.org/1332053002/diff/200001/include/codec/SkCodec....
include/codec/SkCodec.h:248: uint32_t getFillValue(SkColorType colorType,
SkAlphaType alphaType) const {
On 2015/10/01 14:48:31, scroggo wrote:
> Does this method need to be public?

Yes, SkScaledCodec needs to call fCodec->getFillValue().

https://codereview.chromium.org/1332053002/diff/200001/include/codec/SkCodec....
include/codec/SkCodec.h:299: *  @return the number of lines successfully decoded
On 2015/10/01 14:48:31, scroggo wrote:
> Does this fill if some were decoded? What does it mean if it's less than
> countLines? (Please add to comments.)

Yes it does.  Adding the comment.

https://codereview.chromium.org/1332053002/diff/200001/include/codec/SkCodec....
include/codec/SkCodec.h:316: *              Invalid parameters passed to
skipScanlines()
On 2015/10/01 14:48:31, scroggo wrote:
> What are invalid parameters?

Adding more detail to the comment.

https://codereview.chromium.org/1332053002/diff/200001/include/codec/SkCodec....
include/codec/SkCodec.h:396: /**
On 2015/10/01 14:48:31, scroggo wrote:
> nit: newline between these methods

Done.

https://codereview.chromium.org/1332053002/diff/200001/include/codec/SkCodec....
include/codec/SkCodec.h:401: int nextScanline(int srcY) const;
On 2015/10/01 14:48:31, scroggo wrote:
> I don't think this name is right. My first thought was something like
> 
>   int outputScanline(int inputScanline)
> 
> but I'm not sure that's very clear either. You're not returning the "next"
> scanline, you're returning the logical output location of the (inputVar)'th
row
> stored in the encoded data.

Hmmm yeah, it made more sense for these to have the same name when the name was
getY().  I like your suggestion.

https://codereview.chromium.org/1332053002/diff/200001/include/codec/SkCodec....
include/codec/SkCodec.h:508: // FIXME (msarett): We should not reallocate this
memory on every call.
On 2015/10/01 14:48:31, scroggo wrote:
> Is the real fix to always override onSkipScanlines? I thought we discussed
that
> we could almost always do better than this default (we can at least skip
> swizzling - only jpeg does not require a swizzle always, but it already has an
> override). We could make this pure virtual and force the caller to be smarter.
I
> think many (maybe all) of the subclasses have their own buffers that they
reuse,
> so we can always avoid the reallocation.

Yes I agree that the solution is to always override this.  Changing the FIXME.

https://codereview.chromium.org/1332053002/diff/200001/include/codec/SkCodec....
include/codec/SkCodec.h:526: * @param dst            Destination pixel memory
On 2015/10/01 14:48:31, scroggo wrote:
> I think this is different depending on the scanline order?

In the redesign, this is always a pointer to the start of destination memory. 
Adding to the comment.

https://codereview.chromium.org/1332053002/diff/200001/src/codec/SkCodec.cpp
File src/codec/SkCodec.cpp (left):

https://codereview.chromium.org/1332053002/diff/200001/src/codec/SkCodec.cpp#...
src/codec/SkCodec.cpp:212: return kScanlineDecodingNotStarted;
On 2015/10/01 14:48:31, scroggo wrote:
> Do we still need this enum value?

Looks like we don't.

https://codereview.chromium.org/1332053002/diff/200001/src/codec/SkCodec.cpp
File src/codec/SkCodec.cpp (right):

https://codereview.chromium.org/1332053002/diff/200001/src/codec/SkCodec.cpp#...
src/codec/SkCodec.cpp:226: uint32_t SkCodec::getScanlines(void* dst, int
countLines, size_t rowBytes) {
On 2015/10/01 14:48:31, scroggo wrote:
> If we wanted to, we could make getScanlines return a Result (so it could
return
> errors for invalid params and decoding not started), and onGetScanlines return
a
> number of lines decoded. Maybe the caller does not care exactly how many lines
> were decoded? (On the other hand, maybe it is nice for the client to know how
> many lines were decoded, so they can decide whether they want to display the
> partial image. getPixels does not provide that information, so maybe this one
> shouldn't either?)

It's fair to point out that this API change is no longer necessary now that
SkCodec and SkScanlineDecoder are merged.  And now this API provides more
information than getPixels(), which might be weird.

So I do think it is worth considering this API change separately from the rest
of this CL.

I would argue that we should make this change because it provides more useful
information for an intelligent client.  The enum provides more useful
information for a dumb client, which I would argue is less important.

At this point, we are still the only client.  Looking at how we use this API, it
doesn't seem to make a ton of difference either way.

https://codereview.chromium.org/1332053002/diff/200001/src/codec/SkCodec.cpp#...
src/codec/SkCodec.cpp:275: return get_output_row_interlaced(srcY,
this->getInfo().height());
On 2015/10/01 14:48:31, scroggo wrote:
> It's odd to me that kBottomUp takes the dstInfo into account, but kOutOfOrder
> does not.

We can use getInfo() in both places.  This is probably best to avoid confusion
(and bugs).  If we ever wanted to call nextScanline() from onGetPixels for a
kBottomUp image, this would have failed because dstInfo() would have been empty.

https://codereview.chromium.org/1332053002/diff/200001/src/codec/SkCodec.cpp#...
src/codec/SkCodec.cpp:298: SkASSERT(1 == linesRequested ||
this->getInfo().height() == linesRequested);
On 2015/10/01 14:48:31, scroggo wrote:
> Do you need the second half? It seems like if it's true the first half will
be,
> too?

I don't understand.  linesRequested can be 1 or the full height.  I think we
need both.

https://codereview.chromium.org/1332053002/diff/200001/src/codec/SkCodec_libi...
File src/codec/SkCodec_libico.cpp (right):

https://codereview.chromium.org/1332053002/diff/200001/src/codec/SkCodec_libi...
src/codec/SkCodec_libico.cpp:138: // partially decode this incomplete embedded
image.
On 2015/10/01 14:48:31, scroggo wrote:
> Is there anything to partially decode if no embedded codecs were created?

Good point, this comment makes no sense.

https://codereview.chromium.org/1332053002/diff/200001/src/codec/SkCodec_wbmp...
File src/codec/SkCodec_wbmp.cpp (right):

https://codereview.chromium.org/1332053002/diff/200001/src/codec/SkCodec_wbmp...
src/codec/SkCodec_wbmp.cpp:141: Result rowResult = this->readRow(src.get());
On 2015/10/01 14:48:31, scroggo wrote:
> I think this should also return a bool?

Yes it should.

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

https://codereview.chromium.org/1332053002/diff/200001/src/codec/SkScaledCode...
src/codec/SkScaledCodec.cpp:209: *rowsDecoded = y + 1;
On 2015/10/01 14:48:32, scroggo wrote:
> Why is this y+1 and not y? (Same for other instances below)

The failed call to getScanlines(1) will fill the scanline that it failed to get.
 Adding a comment.

https://codereview.chromium.org/1332053002/diff/200001/src/codec/SkScaledCode...
src/codec/SkScaledCodec.cpp:310: int scanlines = (int)
fCodec->getScanlines(storagePtr, srcHeight, rowBytes);
On 2015/10/01 14:48:32, scroggo wrote:
> It's weird to me that getScanlines returns one type, which you often cast to
an
> int. Can you use the same type throughout? Is there a reason they are
different?

Yeah I also thought this was strange.

I feel strongly that it should return an unsigned int.  But unfortunately all of
our other APIs that *should* be unsigned are signed (ex: width, height,
nextScanline etc.)

I think its probably to best to be consistent and use int.  Maybe this is
something that we can clean up in the future (or that we don't care about)?

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

https://codereview.chromium.org/1332053002/diff/200001/src/codec/SkSwizzler.c...
src/codec/SkSwizzler.cpp:725: switch(fillInfo.colorType()) {
On 2015/10/01 14:48:32, scroggo wrote:
> nit: space between "switch" and "("

Done.

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

https://codereview.chromium.org/1332053002/diff/200001/src/codec/SkSwizzler.h...
src/codec/SkSwizzler.h:145: * Contains the color type of the rows to fill.
On 2015/10/01 14:48:32, scroggo wrote:
> And, alpha I assume?

Yes it does, although the alpha type is actually irrelevant to this function.

https://codereview.chromium.org/1332053002/diff/220001/dm/DMSrcSink.cpp
File dm/DMSrcSink.cpp (right):

https://codereview.chromium.org/1332053002/diff/220001/dm/DMSrcSink.cpp#newco...
dm/DMSrcSink.cpp:363: codec->getScanlines(dstPtr, 1, bitmap.rowBytes());
Thanks for your thorough review of how the scanline tests (Scanline,
ScanlineSubset, and Stripe) handle incomplete images.  In the last patch set, I
didn't really feel strongly about whether they should succeed or fail (or even
that they be consistent) - but I think it is worth writing the code so that they
will fill uninitialized memory and succeed.

I have considered two ways to accomplish this:
(1) Call SkSwizzler::Fill() from here.  This is straightforward in most cases
and sometimes complex (ex: filling kOutOfOrder images).  It is also generally a
duplication of code in SkCodec.cpp.
(2) Ignore result values for getScanlines() and skipScanlines().  If images are
incomplete, these calls may fail over and over again, but all the memory will
get filled.

I have chosen to go with (2).  I like that it makes the test code simpler to
write.  I don't feel strongly, so let me know if you prefer (1) or another
option altogether.

scroggo

https://codereview.chromium.org/1332053002/diff/200001/dm/DMSrcSink.cpp File dm/DMSrcSink.cpp (right): https://codereview.chromium.org/1332053002/diff/200001/dm/DMSrcSink.cpp#newcode358 dm/DMSrcSink.cpp:358: codec->getScanlines(dstPtr, 1, bitmap.rowBytes()); On 2015/10/01 18:14:13, msarett wrote: > ...

5 years, 2 months ago (2015-10-01 20:48:58 UTC) #17

https://codereview.chromium.org/1332053002/diff/200001/dm/DMSrcSink.cpp
File dm/DMSrcSink.cpp (right):

https://codereview.chromium.org/1332053002/diff/200001/dm/DMSrcSink.cpp#newco...
dm/DMSrcSink.cpp:358: codec->getScanlines(dstPtr, 1, bitmap.rowBytes());
On 2015/10/01 18:14:13, msarett wrote:
> On 2015/10/01 14:48:31, scroggo wrote:
> > Don't we want to break out of the loop if 0 is returned?
> 
> We could do that.  But then we need to iterate over the remaining lines and
fill
> line by line.  This duplicates the fill code we already have in SkCodec.cpp. 
> See Patch Set 4 for this implementation.
> 
> Alternatively, here we keep calling getScanlines, and every time it fails, it
> will fill the line.  I know we've discussed that we don't really like this
> approach.  I think I prefer it here because it simplifies our test code.  But
I
> don't feel strongly and would be happy to change.

Oh yeah, now I remember talking about this, but I don't remember if we came to a
decision.

I guess I sort of imagine an error to mean that calling getScanlines again does
not work, but you're right, this does simplify things here in this testing code.
Maybe add a comment explaining this?

https://codereview.chromium.org/1332053002/diff/200001/include/codec/SkCodec.h
File include/codec/SkCodec.h (right):

https://codereview.chromium.org/1332053002/diff/200001/include/codec/SkCodec....
include/codec/SkCodec.h:248: uint32_t getFillValue(SkColorType colorType,
SkAlphaType alphaType) const {
On 2015/10/01 18:14:13, msarett wrote:
> On 2015/10/01 14:48:31, scroggo wrote:
> > Does this method need to be public?
> 
> Yes, SkScaledCodec needs to call fCodec->getFillValue().

FWIW, crrev.com/1372973002 makes SkScaledCodec a friend, which I think makes
sense. It was precisely for this kind of thing - SkScaledCodec wants to call a
method on SkCodec that I do not think should be accessible to other callers.
(Why would a normal client of SkCodec need to know the fill value?)

https://codereview.chromium.org/1332053002/diff/200001/src/codec/SkCodec.cpp
File src/codec/SkCodec.cpp (right):

https://codereview.chromium.org/1332053002/diff/200001/src/codec/SkCodec.cpp#...
src/codec/SkCodec.cpp:298: SkASSERT(1 == linesRequested ||
this->getInfo().height() == linesRequested);
On 2015/10/01 18:14:13, msarett wrote:
> On 2015/10/01 14:48:31, scroggo wrote:
> > Do you need the second half? It seems like if it's true the first half will
> be,
> > too?
> 
> I don't understand.  linesRequested can be 1 or the full height.  I think we
> need both.

Oh, I think I was confused. This is just to discourage a dumb client from asking
for more than one line but less than all of them for interlaced GIF?

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

https://codereview.chromium.org/1332053002/diff/200001/src/codec/SkScaledCode...
src/codec/SkScaledCodec.cpp:310: int scanlines = (int)
fCodec->getScanlines(storagePtr, srcHeight, rowBytes);
On 2015/10/01 18:14:14, msarett wrote:
> On 2015/10/01 14:48:32, scroggo wrote:
> > It's weird to me that getScanlines returns one type, which you often cast to
> an
> > int. Can you use the same type throughout? Is there a reason they are
> different?
> 
> Yeah I also thought this was strange.
> 
> I feel strongly that it should return an unsigned int.  But unfortunately all
of
> our other APIs that *should* be unsigned are signed (ex: width, height,
> nextScanline etc.)
> 
> I think its probably to best to be consistent and use int.  Maybe this is
> something that we can clean up in the future (or that we don't care about)?

+1 to consistency. I'm not sure why width and height are stored as ints...

https://codereview.chromium.org/1332053002/diff/220001/dm/DMSrcSink.cpp
File dm/DMSrcSink.cpp (right):

https://codereview.chromium.org/1332053002/diff/220001/dm/DMSrcSink.cpp#newco...
dm/DMSrcSink.cpp:363: codec->getScanlines(dstPtr, 1, bitmap.rowBytes());
On 2015/10/01 18:14:14, msarett wrote:
> Thanks for your thorough review of how the scanline tests (Scanline,
> ScanlineSubset, and Stripe) handle incomplete images.  In the last patch set,
I
> didn't really feel strongly about whether they should succeed or fail (or even
> that they be consistent) - but I think it is worth writing the code so that
they
> will fill uninitialized memory and succeed.
> 
> I have considered two ways to accomplish this:
> (1) Call SkSwizzler::Fill() from here.  This is straightforward in most cases
> and sometimes complex (ex: filling kOutOfOrder images).  It is also generally
a
> duplication of code in SkCodec.cpp.
> (2) Ignore result values for getScanlines() and skipScanlines().  If images
are
> incomplete, these calls may fail over and over again, but all the memory will
> get filled.
> 
> I have chosen to go with (2).  I like that it makes the test code simpler to
> write.  I don't feel strongly, so let me know if you prefer (1) or another
> option altogether.

I mentioned this as a response to an older comment, so sorry for the
duplication: It seems a little weird to me that you can call getScanlines, get 0
for some kind of failure, and then keep calling it. On the other hand, I can see
the convenience. I like the comments here, and there should probably also be
comments on SkCodec::getScanlines.

Thinking about this a little more, it seems a little weird that getScanlines
returns 0 when it actually wrote to countLines lines.

Also, is the number of lines just returned so the client can decide whether they
want to use the incomplete image? If they decided not to, it seems like we
wasted time filling the rest of the image.

https://codereview.chromium.org/1332053002/diff/220001/include/codec/SkCodec.h
File include/codec/SkCodec.h (right):

https://codereview.chromium.org/1332053002/diff/220001/include/codec/SkCodec....
include/codec/SkCodec.h:233: * On a kIncompleteInput, getPixels() and
getScanlines() will fill any uninitialized
Maybe change "kIncompleteInput" to "incomplete input"? getScanlines no longer
returns that enum.

https://codereview.chromium.org/1332053002/diff/220001/include/codec/SkCodec....
include/codec/SkCodec.h:391: *  This will often equal fCurrScanline, but may not
in the case of strangely
I find "often" a little confusing here. You really mean that it's always
fCurrScanline except for those strange types.

https://codereview.chromium.org/1332053002/diff/220001/include/codec/SkCodec....
include/codec/SkCodec.h:403: *  This will often equal inputScanline, but may not
in the case of strangely
I again, I think often is the wrong word to use here.

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

https://codereview.chromium.org/1332053002/diff/220001/src/codec/SkBmpCodec.h...
src/codec/SkBmpCodec.h:137: virtual uint32_t decodeRows(const SkImageInfo&
dstInfo, void* dst, size_t dstRowBytes,
I think these should be "int" to match SkCodec?

https://codereview.chromium.org/1332053002/diff/220001/src/codec/SkCodec_libg...
File src/codec/SkCodec_libgif.h (right):

https://codereview.chromium.org/1332053002/diff/220001/src/codec/SkCodec_libg...
src/codec/SkCodec_libgif.h:137: uint32_t onGetScanlines(void* dst, int count,
size_t rowBytes) override;
More uint32_ts that should be converted to int to match

https://codereview.chromium.org/1332053002/diff/220001/src/codec/SkCodec_libp...
File src/codec/SkCodec_libpng.cpp (right):

https://codereview.chromium.org/1332053002/diff/220001/src/codec/SkCodec_libp...
src/codec/SkCodec_libpng.cpp:507: // instead of png_read_row(s)?  Chromium uses
png_process_data.
png_process_data seems like a good option. As with jpeg, it means we could
special case the SkMemoryStream and provide the SkData directly, saving some
copies. It also matches the way the JavaInputStreamAdaptor in Android works
better. (Plus it matches Chromium, which helps us for merging.)

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

https://codereview.chromium.org/1332053002/diff/220001/src/codec/SkScaledCode...
src/codec/SkScaledCodec.cpp:317: while(y < dstHeight && scanlines > 0) {
nit: while (

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

https://codereview.chromium.org/1332053002/diff/220001/src/codec/SkSwizzler.c...
src/codec/SkSwizzler.cpp:760: uint16_t color = (uint16_t) colorOrIndex;
I take it this changed because color may not be zero now?

msarett

I think this is getting closer. The major remaining question is on the getScanlines() API. ...

5 years, 2 months ago (2015-10-01 22:34:52 UTC) #18

I think this is getting closer.  The major remaining
question is on the getScanlines() API.  Right now it
returns the number of lines successfully decoded, and also
fills the rest of the lines it wasn't able to decode
successfully.  I think the comments in SkCodec.h do a good
job of describing this.

"Thinking about this a little more, it seems a little weird
that getScanlines returns 0 when it actually wrote to
countLines lines."

I have to agree that this is weird.

Alternatives:
(1) We could go back to returning kSuccess,
kIncompleteInput, etc.

This is a good solution if we don't feel that knowing the
number of lines decoded is useful.  You could argue that it
isn't useful since we fill the whole memory block anyway.

I would argue that the client should want to know how many
of the lines that he gets back are actually in the image.
That way the client has the option to throw away the empty
block.

(2) We could not fill the lines.

For the client that throws away the empty block, this would
be the correct behavior.  But I don't like the idea of
leaving memory uninitialized - especially when we can
easily deal with it.

"Also, is the number of lines just returned so the client
can decide whether they want to use the incomplete image?
If they decided not to, it seems like we wasted time
filling the rest of the image."

Yes filling the lines may be a waste of time.

In my mind, incomplete images are an error case, and it is
better to be secure rather than worried about performance.

But maybe they are more common than I think and this is the
wrong approach?

FWIW, my test suite consists of bmps I found online
that were cropped on purpose for testing, images that I
manually cropped, and one authentic image.

DMSrcSink:

A lot of these questions came up because I wrote strange
code in DMSrcSink to take advantage of this "interesting"
behavior.  Maybe that's good because now we are talking
about this behavior?

Or maybe the code in DMSrcSink is the problem? I wouldn't
write code like that for production, so maybe that's a good
argument that it shouldn't be in the tests either?

I actually went back to rewrite the tests to use Fill(),
but I ran into an issue because getFillValue() is no longer
public.  I don’t think it makes sense to make this public
just for our tests.

Maybe we should just fail on incomplete images in our
scanline tests?  This was closer to my original thought,
given that we test filling in codec and scaled codec modes.

--------------------------------------------------

So to summarize my long-winded comments:

I am defending the getScanlines() API as is, but, as
always, I'm opened to good arguments about why I'm wrong.
Also I'm happy to talk in person (or over chat due to the
hurricane) about this.

I feel less strongly about the test code,  I’m just not
sure I should make getFillValue() public in order to write
the tests.  And I’m not sure I should disable incomplete
images completely in the scanline modes.  I’d appreciate
your thoughts on this.

https://codereview.chromium.org/1332053002/diff/200001/dm/DMSrcSink.cpp
File dm/DMSrcSink.cpp (right):

https://codereview.chromium.org/1332053002/diff/200001/dm/DMSrcSink.cpp#newco...
dm/DMSrcSink.cpp:358: codec->getScanlines(dstPtr, 1, bitmap.rowBytes());
On 2015/10/01 20:48:57, scroggo wrote:
> On 2015/10/01 18:14:13, msarett wrote:
> > On 2015/10/01 14:48:31, scroggo wrote:
> > > Don't we want to break out of the loop if 0 is returned?
> > 
> > We could do that.  But then we need to iterate over the remaining lines and
> fill
> > line by line.  This duplicates the fill code we already have in SkCodec.cpp.

> > See Patch Set 4 for this implementation.
> > 
> > Alternatively, here we keep calling getScanlines, and every time it fails,
it
> > will fill the line.  I know we've discussed that we don't really like this
> > approach.  I think I prefer it here because it simplifies our test code. 
But
> I
> > don't feel strongly and would be happy to change.
> 
> Oh yeah, now I remember talking about this, but I don't remember if we came to
a
> decision.
> 
> I guess I sort of imagine an error to mean that calling getScanlines again
does
> not work, but you're right, this does simplify things here in this testing
code.
> Maybe add a comment explaining this?

Acknowledged.

https://codereview.chromium.org/1332053002/diff/200001/include/codec/SkCodec.h
File include/codec/SkCodec.h (right):

https://codereview.chromium.org/1332053002/diff/200001/include/codec/SkCodec....
include/codec/SkCodec.h:248: uint32_t getFillValue(SkColorType colorType,
SkAlphaType alphaType) const {
On 2015/10/01 20:48:57, scroggo wrote:
> On 2015/10/01 18:14:13, msarett wrote:
> > On 2015/10/01 14:48:31, scroggo wrote:
> > > Does this method need to be public?
> > 
> > Yes, SkScaledCodec needs to call fCodec->getFillValue().
> 
> FWIW, crrev.com/1372973002 makes SkScaledCodec a friend, which I think makes
> sense. It was precisely for this kind of thing - SkScaledCodec wants to call a
> method on SkCodec that I do not think should be accessible to other callers.
> (Why would a normal client of SkCodec need to know the fill value?)

That seems like a better solution.  I am adding SkScaledCodec as a friend and
making this function protected.

https://codereview.chromium.org/1332053002/diff/200001/src/codec/SkCodec.cpp
File src/codec/SkCodec.cpp (right):

https://codereview.chromium.org/1332053002/diff/200001/src/codec/SkCodec.cpp#...
src/codec/SkCodec.cpp:298: SkASSERT(1 == linesRequested ||
this->getInfo().height() == linesRequested);
On 2015/10/01 20:48:57, scroggo wrote:
> On 2015/10/01 18:14:13, msarett wrote:
> > On 2015/10/01 14:48:31, scroggo wrote:
> > > Do you need the second half? It seems like if it's true the first half
will
> > be,
> > > too?
> > 
> > I don't understand.  linesRequested can be 1 or the full height.  I think we
> > need both.
> 
> Oh, I think I was confused. This is just to discourage a dumb client from
asking
> for more than one line but less than all of them for interlaced GIF?

Yes!

https://codereview.chromium.org/1332053002/diff/220001/dm/DMSrcSink.cpp
File dm/DMSrcSink.cpp (right):

https://codereview.chromium.org/1332053002/diff/220001/dm/DMSrcSink.cpp#newco...
dm/DMSrcSink.cpp:363: codec->getScanlines(dstPtr, 1, bitmap.rowBytes());
On 2015/10/01 20:48:58, scroggo wrote:
> On 2015/10/01 18:14:14, msarett wrote:
> > Thanks for your thorough review of how the scanline tests (Scanline,
> > ScanlineSubset, and Stripe) handle incomplete images.  In the last patch
set,
> I
> > didn't really feel strongly about whether they should succeed or fail (or
even
> > that they be consistent) - but I think it is worth writing the code so that
> they
> > will fill uninitialized memory and succeed.
> > 
> > I have considered two ways to accomplish this:
> > (1) Call SkSwizzler::Fill() from here.  This is straightforward in most
cases
> > and sometimes complex (ex: filling kOutOfOrder images).  It is also
generally
> a
> > duplication of code in SkCodec.cpp.
> > (2) Ignore result values for getScanlines() and skipScanlines().  If images
> are
> > incomplete, these calls may fail over and over again, but all the memory
will
> > get filled.
> > 
> > I have chosen to go with (2).  I like that it makes the test code simpler to
> > write.  I don't feel strongly, so let me know if you prefer (1) or another
> > option altogether.
> 
> I mentioned this as a response to an older comment, so sorry for the
> duplication: It seems a little weird to me that you can call getScanlines, get
0
> for some kind of failure, and then keep calling it. On the other hand, I can
see
> the convenience. I like the comments here, and there should probably also be
> comments on SkCodec::getScanlines.
> 
> Thinking about this a little more, it seems a little weird that getScanlines
> returns 0 when it actually wrote to countLines lines.
> 
> Also, is the number of lines just returned so the client can decide whether
they
> want to use the incomplete image? If they decided not to, it seems like we
> wasted time filling the rest of the image.

Thanks for your thoughts.  I'll respond to this as a comment on the patch set.

https://codereview.chromium.org/1332053002/diff/220001/include/codec/SkCodec.h
File include/codec/SkCodec.h (right):

https://codereview.chromium.org/1332053002/diff/220001/include/codec/SkCodec....
include/codec/SkCodec.h:233: * On a kIncompleteInput, getPixels() and
getScanlines() will fill any uninitialized
On 2015/10/01 20:48:58, scroggo wrote:
> Maybe change "kIncompleteInput" to "incomplete input"? getScanlines no longer
> returns that enum.

Done.

https://codereview.chromium.org/1332053002/diff/220001/include/codec/SkCodec....
include/codec/SkCodec.h:391: *  This will often equal fCurrScanline, but may not
in the case of strangely
On 2015/10/01 20:48:58, scroggo wrote:
> I find "often" a little confusing here. You really mean that it's always
> fCurrScanline except for those strange types.

Done.

https://codereview.chromium.org/1332053002/diff/220001/include/codec/SkCodec....
include/codec/SkCodec.h:403: *  This will often equal inputScanline, but may not
in the case of strangely
On 2015/10/01 20:48:58, scroggo wrote:
> I again, I think often is the wrong word to use here.

Done.

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

https://codereview.chromium.org/1332053002/diff/220001/src/codec/SkBmpCodec.h...
src/codec/SkBmpCodec.h:137: virtual uint32_t decodeRows(const SkImageInfo&
dstInfo, void* dst, size_t dstRowBytes,
On 2015/10/01 20:48:58, scroggo wrote:
> I think these should be "int" to match SkCodec?

Done.  Looks like I forgot onGetScanlines() as well.

https://codereview.chromium.org/1332053002/diff/220001/src/codec/SkCodec_libg...
File src/codec/SkCodec_libgif.h (right):

https://codereview.chromium.org/1332053002/diff/220001/src/codec/SkCodec_libg...
src/codec/SkCodec_libgif.h:137: uint32_t onGetScanlines(void* dst, int count,
size_t rowBytes) override;
On 2015/10/01 20:48:58, scroggo wrote:
> More uint32_ts that should be converted to int to match

Yep there's a bunch

https://codereview.chromium.org/1332053002/diff/220001/src/codec/SkCodec_libp...
File src/codec/SkCodec_libpng.cpp (right):

https://codereview.chromium.org/1332053002/diff/220001/src/codec/SkCodec_libp...
src/codec/SkCodec_libpng.cpp:507: // instead of png_read_row(s)?  Chromium uses
png_process_data.
On 2015/10/01 20:48:58, scroggo wrote:
> png_process_data seems like a good option. As with jpeg, it means we could
> special case the SkMemoryStream and provide the SkData directly, saving some
> copies. It also matches the way the JavaInputStreamAdaptor in Android works
> better. (Plus it matches Chromium, which helps us for merging.)

Great!  Glad you have some insight into how that API works!  That'll be
something to do when we are ready to start optimizing.

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

https://codereview.chromium.org/1332053002/diff/220001/src/codec/SkScaledCode...
src/codec/SkScaledCodec.cpp:317: while(y < dstHeight && scanlines > 0) {
On 2015/10/01 20:48:58, scroggo wrote:
> nit: while (

Done.

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

https://codereview.chromium.org/1332053002/diff/220001/src/codec/SkSwizzler.c...
src/codec/SkSwizzler.cpp:760: uint16_t color = (uint16_t) colorOrIndex;
On 2015/10/01 20:48:58, scroggo wrote:
> I take it this changed because color may not be zero now?

Yeah this bug has actually been around since you enabled 565 for bmps (we used
to always fill with black even when we should use a color from the color table).
 You'll notice some negative images on Gold.  This is me finally getting around
to fixing it.

scroggo

On 2015/10/01 22:34:52, msarett wrote: > I think this is getting closer. The major remaining ...

5 years, 2 months ago (2015-10-02 19:00:54 UTC) #19

On 2015/10/01 22:34:52, msarett wrote:
> I think this is getting closer.  The major remaining
> question is on the getScanlines() API.  Right now it
> returns the number of lines successfully decoded, and also
> fills the rest of the lines it wasn't able to decode
> successfully.  I think the comments in SkCodec.h do a good
> job of describing this.
> 
> "Thinking about this a little more, it seems a little weird
> that getScanlines returns 0 when it actually wrote to
> countLines lines."
> 
> I have to agree that this is weird.
> 
> Alternatives:
> (1) We could go back to returning kSuccess,
> kIncompleteInput, etc.
> 
> This is a good solution if we don't feel that knowing the
> number of lines decoded is useful.  You could argue that it
> isn't useful since we fill the whole memory block anyway.
> 
> I would argue that the client should want to know how many
> of the lines that he gets back are actually in the image.
> That way the client has the option to throw away the empty
> block.

At least in theory I think it's good to let the client know how many lines were
filled. I still don't know what the client (us, on Android) will choose (which
arguably is a good reason to let them). If our decoders were suspendable (more
on that below), we might want to differentiate between "not enough image data"
and "some incorrect data made us stop" (we don't really distinguish between
those two now, and your CL doesn't change that).

The other important question is whether the SkScaledCodec cares about how many
lines were decoded. It reads the return value, and makes decisions on it, but it
seems like it doesn't actually have to. It could keep calling skip/getScanlines,
like we do in DM. That would simplify the code in SkScaledCodec (even though I
think it's a little weird). (I suppose in that case we wouldn't look at the
returned SkCodec::Result either...)

> 
> (2) We could not fill the lines.
> 
> For the client that throws away the empty block, this would
> be the correct behavior.  But I don't like the idea of
> leaving memory uninitialized - especially when we can
> easily deal with it.

That makes sense. FWIW, Chromium's image decoders behave differently. First and
foremost, they are designed to be suspendable, so if they get partial data, they
assume that more data may come in the future, and draw what they have. (Whereas
Android's/our decoders assume they have all the available data.) In order to
achieve this, they fill *all* the lines first (see
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...).
This is redundant if the image was complete, but prevents uninitialized data.

> 
> "Also, is the number of lines just returned so the client
> can decide whether they want to use the incomplete image?
> If they decided not to, it seems like we wasted time
> filling the rest of the image."
> 
> Yes filling the lines may be a waste of time.
> 
> In my mind, incomplete images are an error case, and it is
> better to be secure rather than worried about performance.
> 
> But maybe they are more common than I think and this is the
> wrong approach?

No, I think that is a good point.

> 
> FWIW, my test suite consists of bmps I found online
> that were cropped on purpose for testing, images that I
> manually cropped, and one authentic image.
> 
> DMSrcSink:
> 
> A lot of these questions came up because I wrote strange
> code in DMSrcSink to take advantage of this "interesting"
> behavior.  Maybe that's good because now we are talking
> about this behavior?
> 
> Or maybe the code in DMSrcSink is the problem? I wouldn't
> write code like that for production, so maybe that's a good
> argument that it shouldn't be in the tests either?

Do you mean how you ignore the return value and keep going? It seems like if
that's a supported use case, it is fine to use it in tests. It also seems like
SkScaledCodec could take advantage of it, too.
> 
> I actually went back to rewrite the tests to use Fill(),
> but I ran into an issue because getFillValue() is no longer
> public.  I don’t think it makes sense to make this public
> just for our tests.

FWIW, we *could* make CodecSrc another friend to SkCodec (we could even do that
only when building for testing, though I don't think we ever do that elsewhere).
The risk there is that someone else writes a class named CodecSrc and gets
access to our internals. I don't think that's too big of a risk though...

> 
> Maybe we should just fail on incomplete images in our
> scanline tests?  This was closer to my original thought,
> given that we test filling in codec and scaled codec modes.

I don't like that because it means we cannot verify that they decoded partial
images look the way they should.

> 
> --------------------------------------------------
> 
> So to summarize my long-winded comments:
> 
> I am defending the getScanlines() API as is, but, as
> always, I'm opened to good arguments about why I'm wrong.
> Also I'm happy to talk in person (or over chat due to the
> hurricane) about this.

I think I'm okay with it, too; it's a little strange, but I'm not sure what
would be better. We'll need API approval anyway, so we can also discuss this
with Derek.
> 
> I feel less strongly about the test code,  I’m just not
> sure I should make getFillValue() public in order to write
> the tests.  And I’m not sure I should disable incomplete
> images completely in the scanline modes.  I’d appreciate
> your thoughts on this.

Agreed on all counts. As stated above, you can use a friend to provide access to
getFillValue (if we make the test code call fill).

msarett

Derek, could you take a look at the API? "Do you mean how you ignore ...

5 years, 2 months ago (2015-10-02 20:52:42 UTC) #20

scroggo

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

5 years, 2 months ago (2015-10-02 20:57:09 UTC) #21

scroggo

On 2015/10/02 20:52:42, msarett wrote: > Derek, could you take a look at the API? ...

5 years, 2 months ago (2015-10-02 20:57:09 UTC) #22

djsollen

https://codereview.chromium.org/1332053002/diff/240001/include/codec/SkCodec.h File include/codec/SkCodec.h (right): https://codereview.chromium.org/1332053002/diff/240001/include/codec/SkCodec.h#newcode283 include/codec/SkCodec.h:283: int getScanlines(void* dst, int countLines, size_t rowBytes); what do ...

5 years, 2 months ago (2015-10-05 12:15:39 UTC) #24

scroggo

https://codereview.chromium.org/1332053002/diff/240001/include/codec/SkCodec.h File include/codec/SkCodec.h (right): https://codereview.chromium.org/1332053002/diff/240001/include/codec/SkCodec.h#newcode283 include/codec/SkCodec.h:283: int getScanlines(void* dst, int countLines, size_t rowBytes); On 2015/10/05 ...

5 years, 2 months ago (2015-10-05 13:12:24 UTC) #25

msarett

This change originated as something we "needed" to do: When SkCodec and SkScanlineDecoder were separate, ...

5 years, 2 months ago (2015-10-05 13:41:09 UTC) #26

reed1

https://codereview.chromium.org/1332053002/diff/240001/include/codec/SkCodec.h File include/codec/SkCodec.h (right): https://codereview.chromium.org/1332053002/diff/240001/include/codec/SkCodec.h#newcode283 include/codec/SkCodec.h:283: int getScanlines(void* dst, int countLines, size_t rowBytes); What is ...

5 years, 2 months ago (2015-10-06 18:40:02 UTC) #29

msarett

https://codereview.chromium.org/1332053002/diff/240001/include/codec/SkCodec.h File include/codec/SkCodec.h (right): https://codereview.chromium.org/1332053002/diff/240001/include/codec/SkCodec.h#newcode283 include/codec/SkCodec.h:283: int getScanlines(void* dst, int countLines, size_t rowBytes); On 2015/10/06 ...

5 years, 2 months ago (2015-10-06 18:47:41 UTC) #30

msarett

I plan to take another look at this, but I thought I'd go ahead and ...

5 years, 2 months ago (2015-10-06 21:45:40 UTC) #32

msarett

This is ready for review! Mike, can you take a look at the API? Leon, ...

5 years, 2 months ago (2015-10-07 14:10:13 UTC) #34

scroggo

High level thoughts: A goal of crrev.com/1372973002 was to make it so that each SkCodec ...

5 years, 2 months ago (2015-10-07 16:04:25 UTC) #35

msarett

So I accidentally deleted Patch Set 11...ugh sorry. I've made the fixes and will respond ...

5 years, 2 months ago (2015-10-07 17:40:31 UTC) #39

msarett

https://codereview.chromium.org/1332053002/diff/320001/src/codec/SkCodec_libgif.cpp File src/codec/SkCodec_libgif.cpp (right): https://codereview.chromium.org/1332053002/diff/320001/src/codec/SkCodec_libgif.cpp#newcode468 src/codec/SkCodec_libgif.cpp:468: fSwizzler->fill(dst, dstInfo.colorType(), dstInfo.height(), dstRowBytes, On 2015/10/07 16:04:24, scroggo wrote: ...

5 years, 2 months ago (2015-10-07 17:40:41 UTC) #40

scroggo

https://codereview.chromium.org/1332053002/diff/420001/include/codec/SkCodec.h File include/codec/SkCodec.h (right): https://codereview.chromium.org/1332053002/diff/420001/include/codec/SkCodec.h#newcode461 include/codec/SkCodec.h:461: * type specified in dstInfo. nit: dstInfo is no ...

5 years, 2 months ago (2015-10-07 18:46:39 UTC) #41

https://codereview.chromium.org/1332053002/diff/420001/include/codec/SkCodec.h
File include/codec/SkCodec.h (right):

https://codereview.chromium.org/1332053002/diff/420001/include/codec/SkCodec....
include/codec/SkCodec.h:461: * type specified in dstInfo.
nit: dstInfo is no longer here. Maybe this should say "depending on colorType"

https://codereview.chromium.org/1332053002/diff/420001/src/codec/SkCodec.cpp
File src/codec/SkCodec.cpp (right):

https://codereview.chromium.org/1332053002/diff/420001/src/codec/SkCodec.cpp#...
src/codec/SkCodec.cpp:186: this->fillIncompleteImage(info.colorType(),
info.alphaType(), pixels, info.width(),
Generally speaking, it is better (less error-prone) to have less parameters to a
function. Sometimes we have to have a lot, but here it seems like we could just
use an SkImageInfo to include info.colorType(), info.alphaType(), info.width(),
and info.height().

A downside is that in some places you don't already have an info to pass, so
you'd have to create a new one.

https://codereview.chromium.org/1332053002/diff/420001/src/codec/SkCodec.cpp#...
src/codec/SkCodec.cpp:267: fCurrScanline += linesDecoded;
Refresh my memory - updating fCurrScanline, even when the image has no more
rows, allows the client to keep calling getScanlines to fill the rest of their
memory, right?

https://codereview.chromium.org/1332053002/diff/420001/src/codec/SkCodec.cpp#...
src/codec/SkCodec.cpp:285: return false;
I notice that incomplete from getScanlines updates fCurrScanline, while in
skipScanlines it does not. Is there a reason for this difference? Maybe this is
because onSkipScanlines does not tell how many it successfully skipped? Are
there any consequences of this? (e.g. if someone keeps calling get/skip after
they've run out of data, does this mean fCurrScanline will get updated
differently, so it may not work right for kOutOfOrder?)

https://codereview.chromium.org/1332053002/diff/420001/src/codec/SkCodec.cpp#...
src/codec/SkCodec.cpp:292: SkASSERT(0 <= inputScanline && inputScanline <
this->getInfo().height());
nit:Do we need this assert in both places?

https://codereview.chromium.org/1332053002/diff/420001/src/codec/SkCodec.cpp#...
src/codec/SkCodec.cpp:315: SkSampler* sampler = this->getSampler(false);
Is this the only place where we need to get the sampler and read its scaled
width? This is a step in the right direction. What if, instead of reading this
value, we just told the sampler to do the filling? It seems like it already
knows its width, so we can just tell it to fill so many lines, and it knows how
to deal with that.

(If there's no sampler, which is just in the case of jpeg, I think, then we'll
have to fall back to calling fill directly, but in that case we know the output
width. Maybe this leads to some duped code?)

https://codereview.chromium.org/1332053002/diff/420001/src/codec/SkCodec.cpp#...
src/codec/SkCodec.cpp:316: width = sampler ? sampler->getScaledWidth() : width;
Alternatively, this could say:

if (sampler) {
  width = sampler->getScaledWidth();
}

Don't know if that makes a big difference. (I think it might be a little clearer
for me, but maybe not a big deal.)

https://codereview.chromium.org/1332053002/diff/420001/src/codec/SkSampler.cpp
File src/codec/SkSampler.cpp (right):

https://codereview.chromium.org/1332053002/diff/420001/src/codec/SkSampler.cp...
src/codec/SkSampler.cpp:13: void SkSampler::Fill(void* dst, SkColorType
colorType, int width, int numRows, size_t rowBytes,
Was this code moved cleanly over from SkSwizzler.cpp? (Or are there differences
I should be looking at?)

msarett

https://codereview.chromium.org/1332053002/diff/420001/include/codec/SkCodec.h File include/codec/SkCodec.h (right): https://codereview.chromium.org/1332053002/diff/420001/include/codec/SkCodec.h#newcode461 include/codec/SkCodec.h:461: * type specified in dstInfo. On 2015/10/07 18:46:39, scroggo ...

5 years, 2 months ago (2015-10-07 20:53:23 UTC) #42

https://codereview.chromium.org/1332053002/diff/420001/include/codec/SkCodec.h
File include/codec/SkCodec.h (right):

https://codereview.chromium.org/1332053002/diff/420001/include/codec/SkCodec....
include/codec/SkCodec.h:461: * type specified in dstInfo.
On 2015/10/07 18:46:39, scroggo wrote:
> nit: dstInfo is no longer here. Maybe this should say "depending on colorType"

Done.  Nice catch!

https://codereview.chromium.org/1332053002/diff/420001/src/codec/SkCodec.cpp
File src/codec/SkCodec.cpp (right):

https://codereview.chromium.org/1332053002/diff/420001/src/codec/SkCodec.cpp#...
src/codec/SkCodec.cpp:186: this->fillIncompleteImage(info.colorType(),
info.alphaType(), pixels, info.width(),
On 2015/10/07 18:46:39, scroggo wrote:
> Generally speaking, it is better (less error-prone) to have less parameters to
a
> function. Sometimes we have to have a lot, but here it seems like we could
just
> use an SkImageInfo to include info.colorType(), info.alphaType(),
info.width(),
> and info.height().
> 
> A downside is that in some places you don't already have an info to pass, so
> you'd have to create a new one.

Yeah I've gone back and forth on this...but I think you're right.  Will change.

https://codereview.chromium.org/1332053002/diff/420001/src/codec/SkCodec.cpp#...
src/codec/SkCodec.cpp:267: fCurrScanline += linesDecoded;
On 2015/10/07 18:46:39, scroggo wrote:
> Refresh my memory - updating fCurrScanline, even when the image has no more
> rows, allows the client to keep calling getScanlines to fill the rest of their
> memory, right?

I like that we update fCurrScanline here to the "right" value regardless of how
many scanlines we are able to get.

I think that we could choose to only update it on success and there would be no
behavior change.

Note that if we fail to decode any lines, fCurrScanline will stay the same.

https://codereview.chromium.org/1332053002/diff/420001/src/codec/SkCodec.cpp#...
src/codec/SkCodec.cpp:285: return false;
On 2015/10/07 18:46:39, scroggo wrote:
> I notice that incomplete from getScanlines updates fCurrScanline, while in
> skipScanlines it does not. Is there a reason for this difference? Maybe this
is
> because onSkipScanlines does not tell how many it successfully skipped?

I had argued that onSkipScanlines() should return an int, so we would be able to
update fCurrScanlines to the "correct" value here.  We decided that it was fine
to return a bool, since it is an error when skipScanlines() fails and we will
need not to use this field again anyway.

> Are
> there any consequences of this? (e.g. if someone keeps calling get/skip after
> they've run out of data, does this mean fCurrScanline will get updated
> differently, so it may not work right for kOutOfOrder?)

There is a subtle distinction here that may be a little bit confusing.

The client can't trust fCurrScanline to keep incrementing on incomplete decodes,
whether they are calling get() or skip().  Once it fails, fCurrScanlines stops
incrementing.  So then, the nextScanline() API stops working (maybe we should
keep incrementing fCurrScanline no matter what, so nextScanline() always works).

In order to fill remaining lines one-by-one the client needs to call
outputScanline(y).  By passing in the y value, it is not depending on
fCurrScanline being correct.

After writing all of that, I think the right thing to do is always update
fCurrScanline by countLines, so it always keeps track of the number of scanlines
that have been asked for.  That way nextScanline() and outputScanline(y) will
both always work properly.  Also, if the client asks for too many lines, we will
catch it.  This would have saved us in the SubsetBench bug.

https://codereview.chromium.org/1332053002/diff/420001/src/codec/SkCodec.cpp#...
src/codec/SkCodec.cpp:292: SkASSERT(0 <= inputScanline && inputScanline <
this->getInfo().height());
On 2015/10/07 18:46:39, scroggo wrote:
> nit:Do we need this assert in both places?

No, you're right we don't.

https://codereview.chromium.org/1332053002/diff/420001/src/codec/SkCodec.cpp#...
src/codec/SkCodec.cpp:315: SkSampler* sampler = this->getSampler(false);
On 2015/10/07 18:46:39, scroggo wrote:
> Is this the only place where we need to get the sampler and read its scaled
> width? This is a step in the right direction. What if, instead of reading this
> value, we just told the sampler to do the filling? It seems like it already
> knows its width, so we can just tell it to fill so many lines, and it knows
how
> to deal with that.
> 
> (If there's no sampler, which is just in the case of jpeg, I think, then we'll
> have to fall back to calling fill directly, but in that case we know the
output
> width. Maybe this leads to some duped code?)

Yes we can do this, but the switch statement will be duplicated.

I do think this might be better...let me know what you think.

https://codereview.chromium.org/1332053002/diff/420001/src/codec/SkCodec.cpp#...
src/codec/SkCodec.cpp:316: width = sampler ? sampler->getScaledWidth() : width;
On 2015/10/07 18:46:39, scroggo wrote:
> Alternatively, this could say:
> 
> if (sampler) {
>   width = sampler->getScaledWidth();
> }
> 
> Don't know if that makes a big difference. (I think it might be a little
clearer
> for me, but maybe not a big deal.)

Will make it clearer.

https://codereview.chromium.org/1332053002/diff/420001/src/codec/SkSampler.cpp
File src/codec/SkSampler.cpp (right):

https://codereview.chromium.org/1332053002/diff/420001/src/codec/SkSampler.cp...
src/codec/SkSampler.cpp:13: void SkSampler::Fill(void* dst, SkColorType
colorType, int width, int numRows, size_t rowBytes,
On 2015/10/07 18:46:39, scroggo wrote:
> Was this code moved cleanly over from SkSwizzler.cpp? (Or are there
differences
> I should be looking at?)

The only difference is that we do not pass in an SkImageInfo in this iteration. 
I am about to change this back anyway :)

scroggo

I want to take another look at this tomorrow, but it looks like the right ...

5 years, 2 months ago (2015-10-07 21:56:06 UTC) #43

msarett

https://codereview.chromium.org/1332053002/diff/440001/include/codec/SkCodec.h File include/codec/SkCodec.h (right): https://codereview.chromium.org/1332053002/diff/440001/include/codec/SkCodec.h#newcode556 include/codec/SkCodec.h:556: * @param dstInfo Contains the destination color type On ...

5 years, 2 months ago (2015-10-07 22:04:22 UTC) #44

scroggo

https://codereview.chromium.org/1332053002/diff/440001/include/codec/SkCodec.h File include/codec/SkCodec.h (right): https://codereview.chromium.org/1332053002/diff/440001/include/codec/SkCodec.h#newcode556 include/codec/SkCodec.h:556: * @param dstInfo Contains the destination color type On ...

5 years, 2 months ago (2015-10-08 13:50:30 UTC) #45

msarett

Mike, can I get an API review? Leon, I have responded to your latest comments. ...

5 years, 2 months ago (2015-10-08 15:33:26 UTC) #46

Mike, can I get an API review?

Leon, I have responded to your latest comments.

https://codereview.chromium.org/1332053002/diff/440001/include/codec/SkCodec.h
File include/codec/SkCodec.h (right):

https://codereview.chromium.org/1332053002/diff/440001/include/codec/SkCodec....
include/codec/SkCodec.h:556: * @param dstInfo        Contains the destination
color type
On 2015/10/08 13:50:30, scroggo wrote:
> On 2015/10/07 22:04:22, msarett wrote:
> > On 2015/10/07 21:56:05, scroggo wrote:
> > > So is the height not used?
> > > 
> > > (Should one of linesRequested or linesDecoded be placed in height? that
> might
> > be
> > > confusing...)
> > 
> > Yeah I can see how it's confusing.
> > 
> > fillIncomplete() will eventually either set the height to linesRemaining
> (which
> > is linesRequested - linesDecoded) or 1 (for kOutOfOrder).
> > 
> > We could go ahead and set the height to linesRemaining before calling this
> > function.  Drawbacks of that are duplicated code, the fact that we may
> overwrite
> > it to 1 anyway, and the fact that we still need to pass linesDecoded and
> > linesRequested as parameters anyway.
> 
> I think it's fine as is. Maybe call out that the height is not used?

Will do.

https://codereview.chromium.org/1332053002/diff/440001/include/codec/SkCodec....
include/codec/SkCodec.h:564: void fillIncompleteImage(void* dst, const
SkImageInfo& dstInfo, size_t rowBytes,
On 2015/10/08 13:50:30, scroggo wrote:
> I think dstInfo should come before dst, so this signature looks more like
> getPixels.

You're right, it should.

I'm changing the order in the Sampler functions as well.

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

https://codereview.chromium.org/1332053002/diff/440001/src/codec/SkBmpRLECode...
src/codec/SkBmpRLECodec.cpp:476: // FIXME: Make SkBmpRLECodec have no knowledge
of sampling.
On 2015/10/08 13:50:30, scroggo wrote:
> We might want the opposite - all sampling is done within this class - it just
> currently has a hybrid where it needs to know what SkScaledCodec is doing.

Done.

https://codereview.chromium.org/1332053002/diff/440001/src/codec/SkCodec.cpp
File src/codec/SkCodec.cpp (right):

https://codereview.chromium.org/1332053002/diff/440001/src/codec/SkCodec.cpp#...
src/codec/SkCodec.cpp:261: const uint32_t linesDecoded =
this->onGetScanlines(dst, countLines, rowBytes);
On 2015/10/08 13:50:30, scroggo wrote:
> This should be an int. (There may be a few other places where you're still
> mixing types.) I think one of our bots will catch you on this, since you're
> comparing unsigned to signed down below.

Yeah it wouldn't surprise me if I haven't caught all of these...  Hopefully the
bots will help.

https://codereview.chromium.org/1332053002/diff/440001/src/codec/SkCodec.cpp#...
src/codec/SkCodec.cpp:307: static void fill_proc(SkSampler* sampler, void* dst,
const SkImageInfo& info, size_t rowBytes,
On 2015/10/08 13:50:30, scroggo wrote:
> This order should probably match getPixels (and fillIncompleteImage, once you
> update it) - info, dst, rowBytes. (I do not feel strongly about the other
> parameters' ordering).

Done.

https://codereview.chromium.org/1332053002/diff/440001/src/codec/SkCodecPriv.h
File src/codec/SkCodecPriv.h (right):

https://codereview.chromium.org/1332053002/diff/440001/src/codec/SkCodecPriv....
src/codec/SkCodecPriv.h:91: inline uint32_t get_output_row_interlaced(uint32_t
encodedRow, uint32_t height) {
On 2015/10/08 13:50:30, scroggo wrote:
> This got moved here, but it looks like it's back to only being called in
> SkCodec_libgif. Can you move it back?

Yes!

https://codereview.chromium.org/1332053002/diff/440001/src/codec/SkCodec_libg...
File src/codec/SkCodec_libgif.cpp (right):

https://codereview.chromium.org/1332053002/diff/440001/src/codec/SkCodec_libg...
src/codec/SkCodec_libgif.cpp:496: uint32_t
SkGifCodec::onGetFillValue(SkColorType colorType, SkAlphaType alphaType) const {
On 2015/10/08 13:50:30, scroggo wrote:
> It looks like there are three classes with similar overrides for this
function.
> Not sure if we can combine them better...
> 
> We could have a virtual accessor for the color table, and then make
getFillValue
> non-virtual:
> 
> getFillValue() {
>   SkColorTable* ct = this->getColorTable(); // calls a virtual
>   if (ct) {
>     return get_color_table_fill_value();
>   }
>   return // base implementation from onGetFillValue
> }
> 
> Think that is better?

It won't quite work because gif may have an fFillIndex, while bmp and png will
just use 0 as the index.

I'll add a FIXME to see about sharing code better, but I don't think it's too
bad right now.

https://codereview.chromium.org/1332053002/diff/440001/src/codec/SkCodec_libg...
src/codec/SkCodec_libgif.cpp:530: SkSampler::Fill(dst,
this->dstInfo().makeWH(this->dstInfo().width(), count), rowBytes,
On 2015/10/08 13:50:30, scroggo wrote:
> nit: some local variables would probably make this easier to read.

Done.

https://codereview.chromium.org/1332053002/diff/440001/src/codec/SkMaskSwizzl...
File src/codec/SkMaskSwizzler.h (right):

https://codereview.chromium.org/1332053002/diff/440001/src/codec/SkMaskSwizzl...
src/codec/SkMaskSwizzler.h:41: void fill(void* dst, const SkImageInfo& info,
size_t rowBytes, uint32_t colorOrIndex,
On 2015/10/08 13:50:30, scroggo wrote:
> Again, I think this parameter order should match getPixels (info, dst,
rowbytes)
> 
> Also, it seems weird that this function needs to be passed so much
information.
> For example, it was created with a particular color type/alpha type, so it
> should only use that one (although I suppose it does not store that info). I
> suppose this allows it to call SkSampler::Fill and share code.

Changed the parameter order.

Yeah on the opposite extreme, the Swizzler could store colorType, alphaType,
zeroInit, and (maybe) rowBytes.  Then we would only need to pass in dst, numRow,
and colorOrIndex.

https://codereview.chromium.org/1332053002/diff/440001/tests/CodexTest.cpp
File tests/CodexTest.cpp (right):

https://codereview.chromium.org/1332053002/diff/440001/tests/CodexTest.cpp#ne...
tests/CodexTest.cpp:301: check(r, "incomplete_images/android.png",
SkISize::Make(1024, 1218), true, false, true, true);
On 2015/10/08 13:50:30, scroggo wrote:
> I think we can add this test without adding a new image. We can use the same
> images that are already checked in, but modify check to truncate the stream if
> (incomplete). (We can then more easily test it on a variety of types as well.)

This is a good idea.

https://codereview.chromium.org/1332053002/diff/440001/tools/SkBitmapRegionDe...
File tools/SkBitmapRegionDecoderInterface.cpp (right):

https://codereview.chromium.org/1332053002/diff/440001/tools/SkBitmapRegionDe...
tools/SkBitmapRegionDecoderInterface.cpp:32: SkAutoTDelete<SkCodec>
codec(SkCodec::NewFromStream(stream));
On 2015/10/08 13:50:30, scroggo wrote:
> nit: This change seems unrelated to the rest of the change.
> 
> I think it's fine to land here, but in the future, for such a large change
> (especially one with this much back and forth, which you may not be able to
> predict ahead of time...), it would make the process easier to pull this into
> its own CL (which I can review once and quickly approve).

Sorry about that.  I think maybe this change used to be combined with another
change where this more relevant.  And then I did bad job of splitting them up.

scroggo

On 2015/10/08 15:33:26, msarett wrote: > Mike, can I get an API review? > > ...

5 years, 2 months ago (2015-10-08 16:42:57 UTC) #47

reed1

api lgtm Still not comfortable w/ not controlling what the "fill-in" color is for non-index-8, ...

5 years, 2 months ago (2015-10-08 20:26:20 UTC) #48