Advance decoders' SourceBufferIterator directly in StreamingLexer

RESOLVED FIXED in Firefox 50

Status

()

defect
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: seth, Assigned: seth)

Tracking

unspecified
mozilla50
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox50 fixed)

Details

Attachments

(3 attachments)

Assignee

Description

3 years ago
As discussed in bug 1284031 comment 0, we need StreamingLexer to interact with SourceBufferIterator more directly so we can efficiently support yielding, which will allow for massive improvements to animated image playback.

In this bug we'll move the loop that advances a Decoder's SourceBufferIterator from Decoder::Decode() to StreamingLexer.
Assignee

Updated

3 years ago
Depends on: 1285867
Assignee

Updated

3 years ago
Depends on: 1286168
Assignee

Updated

3 years ago
Blocks: 1286175
Assignee

Comment 1

3 years ago
In part 2 we'll start passing an IResumable from Decoder::Decode() all the way
down to StreamingLexer. Since we don't always need an IResumable, we don't
really want to impose the requirement that we have one, so let's make
SourceBufferIterator and SourceBuffer allow a null IResumable. This will be
particularly handy in the case of nsICODecoder, which up to now has been
pointlessly allocating an IResumable that doesn't do anything.
Attachment #8770037 - Flags: review?(edwin)
Assignee

Comment 2

3 years ago
Here's the meat of the patch. We want to move the loop that advances the
SourceBufferIterator from Decoder::Decode() into StreamingLexer. To keep the
individual steps in this patch series management, I've implemented this as a
second overload to Lex() which wraps the original one. Later on, when the time
comes to implement yield support, these things will become more tightly coupled.

All of the decoders need to be updated to call the new Lex() overload, and we
need the ability to pass an IResumable to them. This means we need to touch
DoDecode()'s interface yet again. Sorry, Edwin. =)
Attachment #8770042 - Flags: review?(n.nethercote)
Attachment #8770042 - Flags: review?(edwin)
Assignee

Comment 3

3 years ago
Here's an update to the StreamingLexer tests to cover the new Lex() overload. I
needed some IResumable helper implementations for these tests; they're in
Common.h because we'll want to use them in the upcoming SourceBuffer test suite,
too.
Attachment #8770043 - Flags: review?(n.nethercote)
Comment on attachment 8770042 [details] [diff] [review]
(Part 2) - Advance decoders' SourceBufferIterator directly in StreamingLexer.

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

rs=me
Attachment #8770042 - Flags: review?(n.nethercote) → review+
Comment on attachment 8770043 [details] [diff] [review]
(Part 3) - Add StreamingLexer tests for the SourceBufferIterator version of Lex().

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

rs=me
Attachment #8770043 - Flags: review?(n.nethercote) → review+

Comment 6

3 years ago
Pushed by mfowler@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ab4ae8b37941
(Part 1) - Allow advancing a SourceBufferIterator without an IResumable. r=edwin
https://hg.mozilla.org/integration/mozilla-inbound/rev/e2069b87af4c
(Part 2) - Advance decoders' SourceBufferIterator directly in StreamingLexer. r=edwin,njn
https://hg.mozilla.org/integration/mozilla-inbound/rev/bf3df70f738e
(Part 3) - Add StreamingLexer tests for the SourceBufferIterator version of Lex(). r=njn

Comment 7

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/ab4ae8b37941
https://hg.mozilla.org/mozilla-central/rev/e2069b87af4c
https://hg.mozilla.org/mozilla-central/rev/bf3df70f738e
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
You need to log in before you can comment on or make changes to this bug.