If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Use StreamingLexer in the ICON decoder

RESOLVED FIXED in Firefox 44

Status

()

Core
ImageLib
RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: seth, Assigned: njn, Mentored)

Tracking

unspecified
mozilla44
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox44 fixed)

Details

(Whiteboard: [gfx-noted])

Attachments

(3 attachments, 5 obsolete attachments)

(Reporter)

Description

2 years ago
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.
Whiteboard: [gfx-noted]
(Assignee)

Comment 1

2 years ago
Created attachment 8667713 [details] [diff] [review]
(part 1) - Use StreamingLexer in the ICON decoder

Note: I'm a bit uncertain about the mDownscaler case in ReadPixels(), esp. the
invalidation.
Attachment #8667713 - Flags: review?(seth)
(Assignee)

Updated

2 years ago
Assignee: nobody → n.nethercote
Status: NEW → ASSIGNED
(Assignee)

Comment 2

2 years ago
Created attachment 8667727 [details] [diff] [review]
(part 1) - Use StreamingLexer in the ICON decoder

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?
Attachment #8667727 - Flags: review?(seth)
(Assignee)

Comment 3

2 years ago
Created attachment 8667728 [details] [diff] [review]
(part 2) - Add more testing of the ICON decoder

This time with a more sensible log message.
Attachment #8667728 - Flags: review?(seth)
(Assignee)

Updated

2 years ago
Attachment #8667727 - Attachment is obsolete: true
Attachment #8667727 - Flags: review?(seth)
(Assignee)

Comment 4

2 years ago
> (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
(Reporter)

Comment 5

2 years ago
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.
Attachment #8667713 - Flags: review?(seth)
(Reporter)

Comment 6

2 years ago
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.
Attachment #8667728 - Flags: review?(seth) → review+
(Reporter)

Comment 7

2 years ago
(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.
(Assignee)

Comment 8

2 years ago
> I think "Icon" is fine.

And it matches "nsIconDecoder".
(Assignee)

Comment 9

2 years ago
> 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!
(Assignee)

Comment 10

2 years ago
Created attachment 8668175 [details] [diff] [review]
(part 1) - Use StreamingLexer in the ICON decoder

Now with the lexer as a member of nsIconDecoder, as suggested.
Attachment #8668175 - Flags: review?(seth)
(Assignee)

Updated

2 years ago
Attachment #8667713 - Attachment is obsolete: true
(Assignee)

Comment 11

2 years ago
Created attachment 8668179 [details] [diff] [review]
(part 2) - Add more testing of the ICON decoder

Now with a working multi-chunk test.
Attachment #8668179 - Flags: review?(seth)
(Assignee)

Updated

2 years ago
Attachment #8667728 - Attachment is obsolete: true
(Reporter)

Comment 12

2 years ago
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().
Attachment #8668175 - Flags: review?(seth)
(Reporter)

Comment 13

2 years ago
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.
Attachment #8668179 - Flags: review?(seth) → review+
(Assignee)

Comment 14

2 years ago
Created attachment 8669489 [details] [diff] [review]
(part 1b) - Address review comments

I'll fold this into part 1 before landing.
Attachment #8669489 - Flags: review?(seth)
(Reporter)

Comment 15

2 years ago
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.)
Attachment #8669489 - Flags: review?(seth)
(Assignee)

Comment 16

2 years ago
Created attachment 8669512 [details] [diff] [review]
(part 1b) - Address review comments

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?
Attachment #8669512 - Flags: review?(seth)
(Assignee)

Updated

2 years ago
Attachment #8669489 - Attachment is obsolete: true
(Reporter)

Comment 17

2 years ago
Comment on attachment 8669512 [details] [diff] [review]
(part 1b) - Address review comments

Review of attachment 8669512 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good!
Attachment #8669512 - Flags: review?(seth) → review+
(Reporter)

Comment 18

2 years ago
(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.
(Assignee)

Comment 19

2 years ago
Created attachment 8669521 [details] [diff] [review]
(part 1b) - Address review comments

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.
Attachment #8669521 - Flags: review?(seth)
(Assignee)

Updated

2 years ago
Attachment #8669512 - Attachment is obsolete: true
(Reporter)

Comment 20

2 years ago
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.
Attachment #8669521 - Flags: review?(seth) → review+

Comment 21

2 years ago
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
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox44: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
(Reporter)

Updated

a year ago
Blocks: 1284031
You need to log in before you can comment on or make changes to this bug.