Closed Bug 1291054 Opened 4 years ago Closed 4 years ago

Clean up some Decoder methods related to sizes, frame rects, and surface allocation

Categories

(Core :: ImageLib, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla51
Tracking Status
firefox51 --- fixed

People

(Reporter: seth, Assigned: seth)

References

Details

Attachments

(5 files)

We're in the process of reworking how decoders interact with the surface cache (see bug 1291045 for the details for single-frame images). Before writing the more complicated patches, it'd be a good idea to do a little cleanup of the Decoder methods related to image size, frame rects, and surface allocation, since we'll be adding new code that interacts with those methods.
This is just some quick cleanup to improve comments and switch over to Moz2D
types.
Attachment #8776735 - Flags: review?(edwin)
This patch adds an OutputSize() method that returns the size of the surface that
the decoder is going to create. This lets us remove the formula for computing
this from a bunch of places.
Attachment #8776737 - Flags: review?(edwin)
Infallible getters aren't supposed to have |Get| in the name, per our style
guide. Let's rename GetSize() to Size() to make it match up. This also makes it
consistent with OutputSize() and TargetSize().
Attachment #8776738 - Flags: review?(edwin)
Most decoders only use a frame rect that covers the entire image. Let's add some
convenience methods to calculate that in one place. This not only makes the code
a little shorter, but more importantly makes it more obvious what the code is
doing at callsites.
Attachment #8776739 - Flags: review?(edwin)
Patch title says it all. Nothing calls AllocateBasicFrame(), which is a remnant
of the pre-downscale-during-decode days.
Attachment #8776740 - Flags: review?(edwin)
Comment on attachment 8776739 [details] [diff] [review]
(Part 4) - Add Decoder convenience methods for the common case of frame rects that cover the whole image.

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

::: image/decoders/nsPNGDecoder.cpp
@@ -147,5 @@
>    // Check if the image has a transparent color in its palette.
>    if (aFormat == SurfaceFormat::B8G8R8A8) {
>      return TransparencyType::eAlpha;
>    }
> -  if (!IntRect(IntPoint(), Size()).IsEqualEdges(aFrameRect)) {

We can remove the |aFrameRect| arg as well.
Attachment #8776739 - Flags: review?(edwin) → review+
Thanks for the quick reviews!

(In reply to Edwin Flores [:eflores] [:edwin] from comment #6)
> We can remove the |aFrameRect| arg as well.

That's a good point! I'll get that fixed.
(In reply to Edwin Flores [:eflores] [:edwin] from comment #6)
> We can remove the |aFrameRect| arg as well.

Oh wait, I misread it the same way you did. It's still used, alas.
Pushed by seth.bugzilla@blackhail.net:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5e94e5cf47ef
(Part 1) - Clean up Decoder's size getters a little bit. r=edwin
https://hg.mozilla.org/integration/mozilla-inbound/rev/5c5ceb44358d
(Part 2) - Add a Decoder::OutputSize() getter and use it in the decoders. r=edwin
https://hg.mozilla.org/integration/mozilla-inbound/rev/a2f8694f12c8
(Part 3) - Rename Decoder::GetSize() to Decoder::Size() to be consistent with the style guide. r=edwin
https://hg.mozilla.org/integration/mozilla-inbound/rev/21260281d501
(Part 4) - Add Decoder convenience methods for the common case of frame rects that cover the whole image. r=edwin
https://hg.mozilla.org/integration/mozilla-inbound/rev/b617b61db290
(Part 5) - Remove Decoder::AllocateBasicFrame(), which is dead code. r=edwin
Blocks: 1292505
You need to log in before you can comment on or make changes to this bug.