The ICON decoder currently does not support streaming at all, so reworking it to use StreamingLexer would clean up the code and remove a potential footgun. This would be a good bug for a new contributor with good C++ skills.

Note: I'm a bit uncertain about the mDownscaler case in ReadPixels(), esp. the invalidation.

Things I wasn't sure about: - I used "Icon" throughout rather than "ICON", but I can change to the latter if you'd like. - I tried doing an IconMultiChunk test but it failed, so I took it out again. Is that expected? If so, is it because nsIconDecoder cannot do unbuffered reads?

This time with a more sensible log message.

> (part 2) - Add more testing of the ICON decoder BTW, the green.icon file shows up strangely (compressed?) in the diff. It's just the following byte sequence: 100, 100, (0, 255, 0, 255) x 10000

Comment on attachment 8667713 [details] [diff] [review] (part 1) - Use StreamingLexer in the ICON decoder Review of attachment 8667713 [details] [diff] [review]: ----------------------------------------------------------------- ::: image/decoders/nsIconDecoder.cpp @@ +36,5 @@ > nsIconDecoder::WriteInternal(const char* aBuffer, uint32_t aCount) > { > MOZ_ASSERT(!HasError(), "Shouldn't call WriteInternal after error!"); > > + StreamingLexer<State> lexer(Transition::To(State::HEADER, ICON_HEADER_SIZE)); So this is probably why the ICONMultiChunk case was failing for you. (Which indicates that the decoder is buggy; we can't land without that passing!) The multichunk tests deliver their image data a byte at a time - in other words, each invocation of WriteInternal receives only one byte of data. That checks that we buffer correctly and maintain state correctly between invocations of WriteInternal. In a decoder implemented with StreamingLexer, StreamingLexer is responsible for maintaining a lot of that state automatically, but to do that it needs to stay alive between invocations of WriteInternal. So what you need to do here is make the StreamingLexer a class member. See nsICODecoder for an example.

Comment on attachment 8667728 [details] [diff] [review] (part 2) - Add more testing of the ICON decoder Review of attachment 8667728 [details] [diff] [review]: ----------------------------------------------------------------- Looks good! Just need to add the multichunk version of the test (and update part 1 to make sure it passes, of course). ::: image/test/gtest/TestDecoders.cpp @@ +216,5 @@ > { > CheckDecoderMultiChunk(GreenICOTestCase()); > } > > +TEST(ImageDecoders, IconSingleChunk) As mentioned above, it's critical that the multichunk version of the test pass, so we need to add it to this file.

(In reply to Nicholas Nethercote [:njn] from comment #2) > - I used "Icon" throughout rather than "ICON", but I can change to the latter > if you'd like. I think "Icon" is fine.

> I think "Icon" is fine. And it matches "nsIconDecoder".

> So what you need to do here is make the StreamingLexer a > class member. See nsICODecoder for an example. Oh. I wondered by nsICODecoder was like that. Thanks!

Now with the lexer as a member of nsIconDecoder, as suggested.

Now with a working multi-chunk test.

Comment on attachment 8668175 [details] [diff] [review] (part 1) - Use StreamingLexer in the ICON decoder Review of attachment 8668175 [details] [diff] [review]: ----------------------------------------------------------------- Looks good! This is almost ready to go, I think; just needs to be converted to work row-by-row instead of buffering the entire image. ::: image/decoders/nsIconDecoder.cpp @@ +70,5 @@ > +nsIconDecoder::ReadHeader(const char* aData) > +{ > + // Grab the width and height. > + mWidth = (uint8_t)aData[0]; > + mHeight = (uint8_t)aData[1]; Nit: it'd be preferable to use C++-style casts. I'm trying to gradually purge C-style casts from ImageLib, as I've found that using C++-style casts makes the actual behavior of the code more clear. Using constructor syntax (i.e. |uint8_t(foo)|) is also just fine where applicable. @@ +102,5 @@ > } > } > + > + // The input is 32bpp, so we expect 4 bytes of data per pixel. > + return Transition::To(State::PIXELS, mWidth * mHeight * 4); It'd be much better to read a row at a time, instead of reading the entire image at one time. The advantages are that a much smaller buffer is required and total memory usage during decoding is less. Also, by reading a row at a time, you can get rid of the loop in ReadPixels() that copies the bytes row-by-row; you'll always just do a |memcpy(row, aData, aLength)|, where row is either obtained from |mDownscaler->RowBuffer()| (if you have a downscaler) or from |mImageData + bytesPerRow * mCurrentRow| (if you don't). Obviously you'll need to track mCurrentRow. (I realize that for the non-downscaler case, we copy all the bytes at once currently, but I don't think that behavior is worth preserving; no other decoder works that way, and the only reason this one does is that it originally couldn't handle more than one WriteInternal() call.) After making this change, you'll need to add a final state before SUCCESS where you call PostFrameStop() and PostDecodeDone().

Comment on attachment 8668179 [details] [diff] [review] (part 2) - Add more testing of the ICON decoder Review of attachment 8668179 [details] [diff] [review]: ----------------------------------------------------------------- Looks good! I don't think we need any more changes here; this part is ready to go.

I'll fold this into part 1 before landing.

Comment on attachment 8669489 [details] [diff] [review] (part 1b) - Address review comments Review of attachment 8669489 [details] [diff] [review]: ----------------------------------------------------------------- I think we're very close here, but unfortunately this needs one more round of fixes. Feel free to just update part 1b instead of creating part 1c. ::: image/decoders/nsIconDecoder.cpp @@ +25,5 @@ > : Decoder(aImage) > , mLexer(Transition::To(State::HEADER, ICON_HEADER_SIZE)) > , mWidth(-1) > , mHeight(-1) > + , mBytesPerRow(-1) Probably better not to initialize an unsigned value to -1. (We're doing it for mWidth and mHeight, too, now that I think about it...) @@ +54,1 @@ > default: You need a case here for State::FINISH. I'm guessing you didn't notice this issue because we never reach FINISH due to the bug in ReadRowOfPixels() discussed below. @@ +115,5 @@ > LexerTransition<nsIconDecoder::State> > +nsIconDecoder::ReadRowOfPixels(const char* aData, size_t aLength) > +{ > + if (mBytesRead >= mBytesTotal) { > + return Transition::To(State::FINISH, 0); Pretty sure this doesn't work. We won't hit this condition until after we've read the last row and tried to transition to State::ROW_OF_PIXELS again, but we'll never get there because we won't be able to read another mBytesPerRow bytes. Instead we'll think the image is truncated in Decoder::Decode() and try to clean things up as best we can (i.e., Decoder will manually call PostFrameStop() to mark the frame as finished), but we don't want to rely on that cleanup code in the normal case. Instead... @@ +127,5 @@ > + memcpy(mImageData + mBytesRead, aData, mBytesPerRow); > + } > + mBytesRead += mBytesPerRow; > + > + return Transition::To(State::ROW_OF_PIXELS, mBytesPerRow); ...you should decide whether to dispatch to ROW_OF_PIXELS or FINISH here. @@ +138,5 @@ > if (mDownscaler->HasInvalidation()) { > DownscalerInvalidRect invalidRect = mDownscaler->TakeInvalidRect(); > PostInvalidation(invalidRect.mOriginalSizeRect, > Some(invalidRect.mTargetSizeRect)); > } The invalidations should happen in ReadRowOfPixels(), not Finish(). That way we'd repaint as we decode if the icon is received in smaller chunks. (Of course, even after this bug that's unlikely to happen in practice, but it's good to keep things consistent with the other decoders.)

I confirmed that Finish() is now being called. The invaliation in the non-downscaler case is now even less efficient. Did you still want it like this?

Comment on attachment 8669512 [details] [diff] [review] (part 1b) - Address review comments Review of attachment 8669512 [details] [diff] [review]: ----------------------------------------------------------------- Looks good!

(In reply to Nicholas Nethercote [:njn] from comment #16) > The invaliation in the non-downscaler case is now even less efficient. Did > you > still want it like this? It is, yeah. The usual design would be to track the current row (instead of the number of bytes read) and then invalidate just that row. (For the downscaling case, mDownscaler does this for you.) If you want to make nsIconDecoder work like that, that'd be great! That's really the correct design. I don't think it's urgent to fix, though, because in practice we'll get the data in one WriteInternal call for nsIconDecoder, and Decoder merges invalidations aggressively - we only *actually* send the invalidations to the layout system and trigger painting if Decoder has to block to wait for more data from the network. So it's up to you whether to fix it - it'd be better, but it's by no means critical.

Might as well do it properly, if you don't mind going around the merry-go-round one more time. As well as enabling precise invalidation, tracking the current row makes the code nicer than tracking the number of bytes read.

Comment on attachment 8669521 [details] [diff] [review] (part 1b) - Address review comments Review of attachment 8669521 [details] [diff] [review]: ----------------------------------------------------------------- Looks great! Thanks for seeing this all the way through to the end. This is a huge improvement over the original version of this decoder.

https://hg.mozilla.org/integration/mozilla-inbound/rev/b5311d1b6451 https://hg.mozilla.org/integration/mozilla-inbound/rev/8214a32070c3

https://hg.mozilla.org/mozilla-central/rev/b5311d1b6451 https://hg.mozilla.org/mozilla-central/rev/8214a32070c3