bugzilla.mozilla.org has resumed normal operation. Attachments prior to 2014 will be unavailable for a few days. This is tracked in Bug 1475801.
Please report any other irregularities here.

Use StreamingLexer in the PNG decoder

RESOLVED FIXED in Firefox 50

Status

()

Core
ImageLib
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: seth, Assigned: seth)

Tracking

unspecified
mozilla50
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox50 fixed)

Details

Attachments

(3 attachments, 1 obsolete attachment)

(Assignee)

Description

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

Comment 1

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

Comment 2

2 years ago
Created attachment 8765602 [details] [diff] [review]
(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.
Attachment #8765602 - Flags: review?(edwin)
(Assignee)

Comment 3

2 years ago
Created attachment 8765603 [details] [diff] [review]
(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.
Attachment #8765603 - Flags: review?(edwin)
(Assignee)

Comment 4

2 years ago
Created attachment 8765605 [details] [diff] [review]
(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.)
Attachment #8765605 - Flags: review?(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.
Attachment #8765605 - Flags: review?(edwin) → review+
(Assignee)

Comment 7

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

Comment 8

2 years ago
Created attachment 8766493 [details] [diff] [review]
(Part 3) - Use StreamingLexer in the PNG decoder.

Addressed review comments and rebased against central.
(Assignee)

Updated

2 years ago
Attachment #8765605 - Attachment is obsolete: true

Comment 9

2 years ago
Pushed by mfowler@mozilla.com:
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

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/748f5424739f
https://hg.mozilla.org/mozilla-central/rev/442c01c74c5f
https://hg.mozilla.org/mozilla-central/rev/3405db522027
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox50: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
(Assignee)

Updated

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