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

RESOLVED FIXED in Firefox 51

Status

()

Core
ImageLib
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: seth, Assigned: seth)

Tracking

unspecified
mozilla51
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox51 fixed)

Details

Attachments

(5 attachments)

(Assignee)

Description

2 years ago
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.
(Assignee)

Comment 1

2 years ago
Created attachment 8776735 [details] [diff] [review]
(Part 1) - Clean up Decoder's size getters a little bit.

This is just some quick cleanup to improve comments and switch over to Moz2D
types.
Attachment #8776735 - Flags: review?(edwin)
(Assignee)

Comment 2

2 years ago
Created attachment 8776737 [details] [diff] [review]
(Part 2) - Add a Decoder::OutputSize() getter and use it in the decoders.

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)
(Assignee)

Comment 3

2 years ago
Created attachment 8776738 [details] [diff] [review]
(Part 3) - Rename Decoder::GetSize() to Decoder::Size() to be consistent with the style guide.

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)
(Assignee)

Comment 4

2 years ago
Created attachment 8776739 [details] [diff] [review]
(Part 4) - Add Decoder convenience methods for the common case of frame rects that cover the whole image.

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)
(Assignee)

Comment 5

2 years ago
Created attachment 8776740 [details] [diff] [review]
(Part 5) - Remove Decoder::AllocateBasicFrame(), which is dead code.

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+
(Assignee)

Comment 7

2 years ago
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.
(Assignee)

Comment 8

2 years ago
(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.

Comment 9

2 years ago
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
(Assignee)

Updated

2 years ago
Blocks: 1292505
You need to log in before you can comment on or make changes to this bug.