Rework how setting the output size of a decoder works

RESOLVED FIXED in Firefox 51

Status

()

defect
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: seth, Assigned: seth)

Tracking

unspecified
mozilla51
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox51 fixed)

Details

Attachments

(5 attachments)

So remember that formula we used for Decoder::OutputSize() in bug 1291054? The formula that every decoder was already using? Turns out it's not valid for the ICO decoder, and potentially in error conditions in the GIF decoder, because those decoders *destroy* the Downscaler object we were using to get the target size. This is an existing problem that we were avoiding essentially by luck, but I managed to tickle it in bug 1291045, so we need to fix it now.

Rather than introduce hacks in those two cases I'm going to fix it right, by storing the output size independently of the Downscaler object. Downscaler is obsolete anyway; it's only used for decoders that haven't been converted to SurfacePipe yet, and, well, this one remaining use of storing the target size for downscaling, which is hanging on uselessly like an appendix.
The reason I tickled this in bug 1291045, by the way, is that an ICO decoder contains either a BMP decoder or a PNG decoder, and prior to that bug, the BMP or PNG decoder was responsible for inserting the surface that was being decoded into the surface cache. After bug 1291045 part 3, the DecodingTask that wraps the ICO decoder is responsible for that job. When a surface gets inserted into the surface cache, the output size of the decoder is part of the key, and DecodingTask gets that by calling OutputSize() on the *ICO decoder*, not the BMP or PNG decoder it contains, so the fact that the ICO decoder was breaking OutputSize() suddenly became visible.
So here's a new and saner API. SetOutputSize() now just sets |mOutputSize|.
|mOutputSize| is a Maybe because semantically, metadata decodes don't have
sizes. We also run decoders in tests without running a metadata decode first, so
there are still cases where we don't know the output size up front. We might
want to change that just to remove one more use-case we have to support, but for
now let's leave that stuff as it is.

One ugly thing is ExplicitOutputSize(), which returns the value passed to
SetOutputSize(), or Nothing() if SetOutputSize() wasn't explicitly called. This
exists just to allow the ICO decoder (once again, the troublemaker!) access to
this information before PostSize() has been called. I added the boolean to
ensure that ExplicitOutputSize() always returns the same thing (i.e., it doesn't
return Nothing() before PostSize() and Some() afterwards); not doing that seems
to me like a potential footgun for the unwary. (This bug wouldn't exist if such
footguns weren't a real possibility!)
Attachment #8778215 - Flags: review?(edwin)
This propagates the changes to DecoderFactory. DecoderFactory::CreateDecoder()
is only called in situations where we know the output size for sure, so I made
it a required argument now. That required propagating the API change out to
RasterImage as well. I think this is a clear improvement.
Attachment #8778217 - Flags: review?(edwin)
Ugh, the ICO decoder. Hopefully it's clear how the new code maps onto the old
code. We use the output size instead of the downscaler's target size, and we use
ExplicitOutputSize() because we need to call this before PostSize() gets called;
after all, in the code that's being changed here, we're actually figuring out
the intrinsic size so we *can* call PostSize()!.
Attachment #8778218 - Flags: review?(edwin)
As long as we're at it, let's remove the few remaining Downscaler references
that exist in SurfacePipe decoders. They're not long for this world anyway. The
|mDownscaler.reset()| in the GIF decoder doesn't really make sense in a
SurfacePipe world. (And doesn't prevent any kind of catastrophic failure, since
SurfacePipe will fail gracefully if this case happens, even in release builds.)
Attachment #8778221 - Flags: review?(edwin)
Thanks for the quick reviews, Edwin!
Pushed by seth.bugzilla@blackhail.net:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ba7bfe146f7f
(Part 1a) - Replace Decoder::SetTargetSize() with Decoder::SetOutputSize(). r=edwin
https://hg.mozilla.org/integration/mozilla-inbound/rev/3ec9f4bad96e
(Part 1b) - Update DecoderFactory to use SetOutputSize(), and propagate the changes to RasterImage. r=edwin
https://hg.mozilla.org/integration/mozilla-inbound/rev/a0050f7cc0cb
(Part 1c) - Use ExplicitOutputSize() instead of Downscaler::TargetSize() in nsICODecoder. r=edwin
https://hg.mozilla.org/integration/mozilla-inbound/rev/c7cb28cfd0b5
(Part 2) - Remove remaining references to Downscaler in the SurfacePipe decoders. r=edwin
It occurred to me that code in DecoderFactory::CreateDecoderForICOResource()
makes it *appear* as though we could create an ICO resource decoder during a
metadata decode, but we can't (and shouldn't, for performance reasons). Let's go
ahead and assert that that's true. We assume that it is in this bug, or else
we'd trigger an assert by calling SetOutputSize() on a metadata decoder, so we
should make that assumption explicit.
Pushed by seth.bugzilla@blackhail.net:
https://hg.mozilla.org/integration/mozilla-inbound/rev/aef42c5d2329
(Followup) - Add an assert that we don't create a decoder for an ICO resource during a metadata decode. r=me
You need to log in before you can comment on or make changes to this bug.