Closed Bug 1292505 Opened 4 years ago Closed 4 years ago

Rework how setting the output size of a decoder works


(Core :: ImageLib, defect)

Not set



Tracking Status
firefox51 --- fixed


(Reporter: seth, Assigned: seth)




(5 files)

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
(Part 1a) - Replace Decoder::SetTargetSize() with Decoder::SetOutputSize(). r=edwin
(Part 1b) - Update DecoderFactory to use SetOutputSize(), and propagate the changes to RasterImage. r=edwin
(Part 1c) - Use ExplicitOutputSize() instead of Downscaler::TargetSize() in nsICODecoder. r=edwin
(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
(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.