1284031 - Make StreamingLexer read from a SourceBufferIterator directly

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

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

We want to support decoding animated images frame-by-frame, and to do that, StreamingLexer needs to support yielding - that is, returning from Lex() before all the data has been consumed. The problem is that if we implement that right now, the only way it can work is for us to buffer all the data that hasn't been consumed before the yield. In many cases that'll be the majority of the image, so this would result in a *lot* of copying, which is bad for performance.

To avoid that, let's make StreamingLexer read from a SourceBufferIterator directly. We can then modify SourceBufferIterator's API to allow advancing through the SourceBuffer for a fixed number of bytes, instead of all at once, which will allow us to implement yielding efficiently.

Comment 1

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

Attachment: (Part 1a) - Advance SourceBufferIterator in Lex() per-state.

Alright! We're ready to more deeply integrate StreamingLexer with
SourceBufferIterator.

This patches makes us advance the SourceBufferIterator passed to StreamingLexer
for each state individually. Thanks to the changes in bug 1286175, this is
cheap, because we're just reading from SourceBufferIterator's local buffer until
we reach a chunk boundary and it has to talk to the underlying SourceBuffer.
SourceBufferIterator also handles zero-length reads, which simplifies matters
since we don't have to be too clever about when we advance the iterator. (This
is tested in the SourceBuffer GTest suite; we'll test it at the StreamingLexer
level in tests attached to this bug.)

What's really amazing is how much this change cleans up the code! StreamingLexer
is now drastically easier to understand. It's amazing how much complexity arose
from only being able to advance in chunks of unpredictable size.

Comment 2

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

Attachment: (Part 1b) - Remove StreamingLexer tests that use the old API.

Part 1a killed the old StreamingLexer API, so we need to remove the
corresponding tests. (The coverage is the same; there was a version of each test
for each API previously, so we're just removing the old API versions.)

Comment 3

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

Attachment: (Part 2) - Add new StreamingLexer tests for zero-length states.

As promised, here are some extra tests for zero-length states in both buffered
and unbuffered situations.

Comment 4

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

Attachment: (Part 3) - Remove the FromSourceBuffer suffix from StreamingLexer test names.

This is just a little cleanup patch; since we got rid of the duplicated tests,
the "FromSourceBuffer" suffix on the test names is no longer necessary.

Comment 5

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

Attachment: (Part 1a) - Advance SourceBufferIterator in Lex() per-state.

Owing to a rebase mistake, the original part 1a patch failed to update
nsPNGDecoder and nsJPEGDecoder to use the new calling convention. Fixed.

Comment 6

[Nicholas Nethercote [inactive]]

Comment on attachment 8771220 [details] [diff] [review]
(Part 1b) - Remove StreamingLexer tests that use the old API.

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

::: image/test/gtest/TestStreamingLexer.cpp
@@ +96,5 @@
> +    case TestState::THREE:
> +      CheckLexedData(aData, aLength, 9);
> +      return Transition::TerminateSuccess();
> +    default:
> +      MOZ_CRASH("Unknown TestState");

UNBUFFERED, which is a known TestState, would trigger this default case. Change to "Unknown or unhandled TestState"?

Comment 7

[Nicholas Nethercote [inactive]]

Comment on attachment 8771238 [details] [diff] [review]
(Part 1a) - Advance SourceBufferIterator in Lex() per-state.

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

r=me because I see no problems, but I don't know the internals of StreamingLexer.h well and so it wasn't a good review.

Comment 8

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

Thanks for the quick review!

(In reply to Nicholas Nethercote [:njn] from comment #7)
> r=me because I see no problems, but I don't know the internals of
> StreamingLexer.h well and so it wasn't a good review.

The good news is that you sort of didn't have to, since I almost totally rewrote it in this patch. =)

Comment 9

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

Attachment: (Part 1b) - Remove StreamingLexer tests that use the old API.

This new version clarifies the assertions mentioned in the review comments.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/656d5e742049
(Part 1) - Advance SourceBufferIterator in Lex() per-state. r=njn
https://hg.mozilla.org/integration/mozilla-inbound/rev/0284587d752d
(Part 2) - Add new StreamingLexer tests for zero-length states. r=njn
https://hg.mozilla.org/integration/mozilla-inbound/rev/b1e09b3b77dd
(Part 3) - Remove the FromSourceBuffer suffix from StreamingLexer test names. r=njn

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/656d5e742049
https://hg.mozilla.org/mozilla-central/rev/0284587d752d
https://hg.mozilla.org/mozilla-central/rev/b1e09b3b77dd