Closed Bug 1185799 Opened 5 years ago Closed 4 years ago

Make nsICODecoder use only the public Decoder interface

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

(3 files)

Right now, nsICODecoder can *almost* be implemented using the public Decoder interface. The main issue is that it calls Decoder::Write(), a normally internal method, rather than passing a SourceBuffer iterator to its contained decoder. Making that work may be a bit tricky, but it'd be worthwhile, as I think a lot of bugs come from the fact that nsICODecoder takes code paths that no other decoder types take.

If this is fixed, we can also make nsICODecoder use DecoderFactory to instantiate its contained decoders, which is another win for sanity.

In general, the goal is that |friend class nsICODecoder| disappears from the codebase.
Whiteboard: [gfx-noted]
Blocks: 1284031
We have to do this now, because it's blocking an important improvement to SourceBuffer and StreamingLexer. See bug 1284031. Fortunately, in the time between when this bug was filed and now, implementing this has gotten a lot simpler.
Depends on: 1284032
In this first part we add a method to DecoderFactory which can construct a
decoder for the kinds of images you can find in ICO resources - PNGs and BMPs -
and automatically transfer settings from the ICO decoder itself. We then switch
the ICO decoder over to use this DecoderFactory method, instead of constructing
decoders itself.
Attachment #8767446 - Flags: review?(edwin)
In this part we make nsBMPDecoder and nsPNGDecoder stop being friends with
nsICODecoder. nsBMPDecoder doesn't need to be friends with nsICODecoder at all
now that we made the changes in part 1, so that's straightforward. nsPNGDecoder
exposes a method that nsICODecoder needs to call which is currently private
(IsValidICO()), so this patch makes that method public. I've also moved the
implementation of IsValidICO() from the header to the source file because it
feels too complex for a header file method.
Attachment #8767447 - Flags: review?(edwin)
Here's the main event. You might've noticed that in part 1, we started creating
a SourceBuffer for the contained decoders, even though it wasn't used. (I did
that there so we wouldn't have to change DecoderFactory again in this part.) In
this part we start interacting with the contained decoders via the normal public
API.

We use SourceBuffer::Append() to add data to the contained decoder's
SourceBuffer, and then call Decoder::Decode() to decode the data we just
appended. SourceBuffer has a facility for scheduling async tasks to resume
decoding when new data arrives in the SourceBuffer (generally from the network),
but there's already a SourceBuffer for the outer nsICODecoder handling that, so
we pass an IResumable that does nothing to the contained decoder's Decode()
method. That really takes care of it; the only other thing we have to do is call
SourceBuffer::Complete() when the nsICODecoder is done writing to it.

For efficiency, I'm also calling SourceBuffer::ExpectLength() in this patch. We
know how large the ICO resource (i.e., the PNG or BMP embedded in the ICO) that
we're decoding is, so we can let the SourceBuffer know and it can set its
initial capacity to minimize allocations.

It's probably worth noting that this change makes us copy the data for the
resource from the outer nsICODecoder's SourceBuffer into the contained decoder's
SourceBuffer. Since ICO resources are usually very small, this shouldn't have a
significant impact, but it's something we should revisit; I'll file a followup.
This small inefficiency is well worth it because adding a yield feature to
StreamingLexer, as discussed in bug 1284031, will result in an absolutely
*massive* increase in efficiency of our playback of animated images.
Attachment #8767448 - Flags: review?(edwin)
Assignee: nobody → seth
Blocks: 1284034
Try looks good. Bug 1284034 is the followup where we'll eliminate the extra copy; there's a proposed approach in that bug. (It requires fixing some more issues with nsICODecoder, of course. We're gradually getting it into shape.)
Comment on attachment 8767448 [details] [diff] [review]
(Part 3) - Make nsICODecoder use only the public Decoder interface for writing to its contained decoder.

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

lgtm
Attachment #8767448 - Flags: review?(edwin) → review+
Pushed by mfowler@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/07a67db040dc
(Part 1) - Use DecoderFactory to construct nsICODecoder's contained decoder. r=edwin
https://hg.mozilla.org/integration/mozilla-inbound/rev/090ab64054fd
(Part 2) - Make nsBMPDecoder and nsPNGDecoder no longer friends with nsICODecoder. r=edwin
https://hg.mozilla.org/integration/mozilla-inbound/rev/038d7b021492
(Part 3) - Make nsICODecoder use only the public Decoder interface for writing to its contained decoder. r=edwin
You need to log in before you can comment on or make changes to this bug.