Closed Bug 1284031 Opened 3 years ago Closed 3 years ago

Make StreamingLexer read from a SourceBufferIterator directly

Categories

(Core :: ImageLib, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla50
Tracking Status
firefox50 --- fixed

People

(Reporter: seth, Assigned: seth)

References

Details

(Whiteboard: [gfx-noted])

Attachments

(4 files, 2 obsolete files)

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.
Depends on: 1185799
Whiteboard: [gfx-noted]
Depends on: 1285865
Depends on: 1285867
Depends on: 1286165
Depends on: 1286175
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.
Attachment #8771219 - Flags: review?(n.nethercote)
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.)
Attachment #8771220 - Flags: review?(n.nethercote)
As promised, here are some extra tests for zero-length states in both buffered
and unbuffered situations.
Attachment #8771221 - Flags: review?(n.nethercote)
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.
Attachment #8771222 - Flags: review?(n.nethercote)
Assignee: nobody → seth
Owing to a rebase mistake, the original part 1a patch failed to update
nsPNGDecoder and nsJPEGDecoder to use the new calling convention. Fixed.
Attachment #8771219 - Attachment is obsolete: true
Attachment #8771219 - Flags: review?(n.nethercote)
Attachment #8771238 - Flags: review?(n.nethercote)
Attachment #8771222 - Flags: review?(n.nethercote) → review+
Attachment #8771221 - Flags: review?(n.nethercote) → review+
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"?
Attachment #8771220 - Flags: review?(n.nethercote) → review+
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.
Attachment #8771238 - Flags: review?(n.nethercote) → review+
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. =)
This new version clarifies the assertions mentioned in the review comments.
Attachment #8771220 - Attachment is obsolete: true
Blocks: 1287246
Pushed by mfowler@mozilla.com:
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
You need to log in before you can comment on or make changes to this bug.