1282566 - Use StreamingLexer in the PNG decoder

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[Seth Fowler [:seth] [:s2h]]

The PNG decoder does its own buffering and the like. So why use StreamingLexer in the PNG decoder?

Well, it turns out that we need to add the ability to yield from StreamingLexer without reading all the data. (Right now it's guaranteed to consume all the data you give it.) This can be implemented most sanely by letting StreamingLexer itself read from the SourceBufferIterator where its input ultimately comes from. However, this means that Decoder::Decode() would need to work differently for Decoder subclasses that use StreamingLexer than for ones that don't. I'd rather avoid that type of confusion, so I think it's better to just use StreamingLexer everywhere.

Additionally, the PNG decoder *will* actually begin to take advantage of StreamingLexer once yielding is supported. It's just that I can't implement yielding cleanly without the PNG decoder already using StreamingLexer, and we need to break the circular dependency somehow.

Comment 1

[Seth Fowler [:seth] [:s2h]]

(In reply to Seth Fowler [:seth] [:s2h] from comment #0)
> The PNG decoder does its own buffering and the like. So why use
> StreamingLexer in the PNG decoder?

This should really have read "libpng does its own buffering".

Comment 2

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 1) - Use png_process_data_pause for early exits in nsPNGDecoder.

To make this cleaner, let's start using png_process_data_pause for early exits
in nsPNGDecoder. png_process_data_pause requires us to save data in some cases,
but we don't actually have to save anything for the places we're using it in
this patch, because in these situations we are terminating the decoder anyway,
so we won't be reading any more data.

This restores us to a situation where longjmp() is only used for error
conditions.

Comment 3

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 2) - Remove some unused fields from nsPNGDecoder.

This is just a little cleanup patch. I noticed there are some totally unused
fields still lingering around that we can get rid of.

Comment 4

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 3) - Use StreamingLexer in the PNG decoder.

Here's the main patch. I believe you're familiar with the general StreamingLexer
pattern, but if not there is documentation on the class in StreamingLexer.h, and
a good simple example of usage in nsIconDecoder.h/.cpp.

All that we do here is tell StreamingLexer to read the input in one gigantic
unbuffered read. The read is SIZE_MAX, so we'll obviously finish decoding long
before we get to that point, which is fine; ReadPNGData() will put us into a
terminal state of some sort by checking what the status of the decoder is after
each call to png_process_data(). If we somehow do manage to read the entire
address space, FinishedPNGData() will catch it and make the decode fail.

Note that we don't manually call PostDataError() anymore if we detect an error
at setjmp(); instead we handle that generically based on the terminal state in
WriteInternal(), which is the pattern used by all other StreamingLexer-based
decoders.

We obviously don't get much out of using StreamingLexer this way, but as
mentioned in comment 0, once StreamingLexer can yield we'll get more benefits
out of it. (In particular, we'll gain the ability to decode PNG animations one
frame at a time.)

Comment 5

[Seth Fowler [:seth] [:s2h]]

Try job here:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3108fb314d98

Comment 6

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Comment on attachment 8765605 [details] [diff] [review]
(Part 3) - Use StreamingLexer in the PNG decoder.

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

Took a bit of puzzling through StreamingLexer, but LGTM.

::: image/decoders/nsPNGDecoder.cpp
@@ +5,5 @@
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
>  #include "ImageLogging.h" // Must appear first
> +
> +#include <cstdint>

nit: group with the #include <algorithm> below.

Comment 7

[Seth Fowler [:seth] [:s2h]]

(In reply to Edwin Flores [:eflores] [:edwin] from comment #6)
> nit: group with the #include <algorithm> below.

Thanks for the review, Edwin! I'll make that change.

Comment 8

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 3) - Use StreamingLexer in the PNG decoder.

Addressed review comments and rebased against central.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/748f5424739f
(Part 1) - Use png_process_data_pause for early exits in nsPNGDecoder. r=edwin
https://hg.mozilla.org/integration/mozilla-inbound/rev/442c01c74c5f
(Part 2) - Remove some unused fields from nsPNGDecoder. r=edwin
https://hg.mozilla.org/integration/mozilla-inbound/rev/3405db522027
(Part 3) - Use StreamingLexer in the PNG decoder. r=edwin

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/748f5424739f
https://hg.mozilla.org/mozilla-central/rev/442c01c74c5f
https://hg.mozilla.org/mozilla-central/rev/3405db522027