Closed Bug 1210553 Opened 4 years ago Closed 4 years ago

Remove the alternate flags arguments from SurfaceCache's Lookup functions

Categories

(Core :: ImageLib, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla44
Tracking Status
firefox44 --- fixed

People

(Reporter: seth, Assigned: seth)

References

Details

Attachments

(1 file, 2 obsolete files)

Since bug 1191114, we don't need the alternate flags argument for SurfaceCache's Lookup functions.

We originally used the alternate flags so that we could ignore whether surfaces had premultiplied alpha if the surface contained no transparency, since in that case premultiplied alpha makes no difference. This was necessary because we didn't know whether an image contained transparency until after we had decoded it, so we couldn't simply store it in the SurfaceCache without SurfaceFlags::NO_PREMULTIPLY_ALPHA set if it didn't matter.

However, thanks to bug 1191114, which made us detect transparency during the metadata decode, that has changed. Now we *do* know beforehand if an image may be transparent, so we can clear SurfaceFlags::NO_PREMULTIPLY_ALPHA if we know it doesn't matter, both in RasterImage::Decode() and in RasterImage::LookupFrame(), and we no longer need any special support from the SurfaceCache.
Here's the patch. Pretty straightforward - now we just remove the
NO_PREMULTIPLY_ALPHA flags in both LookupFrame() and Decode() if the image is
opaque.

That means that we can just remove every related to alternate flags from
SurfaceCache.
Attachment #8668636 - Flags: review?(dholbert)
Try looks good.
This version of the patch also removes SurfaceKey::WithNewFlags(), which is now
dead code. It's otherwise identical.
Attachment #8669180 - Flags: review?(dholbert)
Attachment #8668636 - Attachment is obsolete: true
Attachment #8668636 - Flags: review?(dholbert)
Comment on attachment 8669180 [details] [diff] [review]
Remove the alternate flags arguments from SurfaceCache's Lookup functions.

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

r=me, just a few thoughts:

::: image/RasterImage.cpp
@@ +315,5 @@
>  
> +  // If we're opaque, we don't need to care about premultiplied alpha, because
> +  // that can only matter for frames with transparency.
> +  uint32_t flags = IsOpaque() ? aFlags & ~FLAG_DECODE_NO_PREMULTIPLY_ALPHA
> +                              : aFlags;

IMO, it feels a bit simpler/clearer to just directly modify aFlags, e.g.:
  if (IsOpaque()) {
    aFlags &= ~FLAG_DECODE_NO_PREMULTIPLY_ALPHA;
  }

But this is fine as-is, too.

::: image/SurfaceCache.cpp
@@ +665,5 @@
>      }
>  
>      MOZ_ASSERT((matchType == MatchType::EXACT) ==
> +                 (surface->GetSurfaceKey() == aSurfaceKey),
> +               "Result differs in a way other than size");

Does this assertion [which is getting simplified in this patch] still make sense?

The assertion message mentions "in a way other than size", but I'm not clear on how size (or "other than size") factors into the asserted condition.

(aSurfaceKey does have a "mSize" member, but we're not directly looking at it vs. the other member-data here.)
Attachment #8669180 - Flags: review?(dholbert) → review+
(In reply to Daniel Holbert [:dholbert] from comment #5)
> IMO, it feels a bit simpler/clearer to just directly modify aFlags, e.g.:

Yeah, I think that's a good idea. Prevents someone from coming in later and mistakenly using |aFlags| instead of |flags|.

> The assertion message mentions "in a way other than size", but I'm not clear
> on how size (or "other than size") factors into the asserted condition.
> 
> (aSurfaceKey does have a "mSize" member, but we're not directly looking at
> it vs. the other member-data here.)

I think this assertion may need some tweaking, yeah. In all the refactoring it's gotten a bit messed up; it looks like it was wrong before this patch.

What we want to check is basically:

- If matchType == MatchType::EXACT, (surface->GetSurfaceKey() == aSurfaceKey) should be true. We're still checking this correctly.
- If matchType == MatchType::SUBSTITUTE_*, all the non-size fields should match. We're not checking this correctly.

This can be made much cleaner just by using multiple MOZ_ASSERT_IF's, I think.
This updated version of the patch addresses all the comments. Thanks for the review!

I think this needs another try run due to the updated assert.
Attachment #8669180 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/53e486b9137b
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
You need to log in before you can comment on or make changes to this bug.