Closed
Bug 1286165
Opened 8 years ago
Closed 8 years ago
Advance decoders' SourceBufferIterator directly in StreamingLexer
Categories
(Core :: Graphics: ImageLib, defect)
Core
Graphics: ImageLib
Tracking
()
RESOLVED
FIXED
mozilla50
Tracking | Status | |
---|---|---|
firefox50 | --- | fixed |
People
(Reporter: seth, Assigned: seth)
References
Details
Attachments
(3 files)
3.29 KB,
patch
|
eflores
:
review+
|
Details | Diff | Splinter Review |
23.76 KB,
patch
|
eflores
:
review+
n.nethercote
:
review+
|
Details | Diff | Splinter Review |
18.60 KB,
patch
|
n.nethercote
:
review+
|
Details | Diff | Splinter Review |
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 | ||
Comment 1•8 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•8 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•8 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)
Attachment #8770037 -
Flags: review?(edwin) → review+
Attachment #8770042 -
Flags: review?(edwin) → review+
Comment 4•8 years ago
|
||
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 5•8 years ago
|
||
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+
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•8 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
Closed: 8 years ago
status-firefox50:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
You need to log in
before you can comment on or make changes to this bug.
Description
•