Closed Bug 1251806 Opened 4 years ago Closed 4 years ago

Fix the check in RasterImage::GetFrameInternal() that determines if the frame covers the entire surface

Categories

(Core :: ImageLib, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox48 --- fixed

People

(Reporter: seth, Assigned: seth)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

In RasterImage::GetFrameInternal() we're checking whether the frame covers the entire surface, but the check is wrong; we check whether the frame has the size that was passed in to GetFrameInternal() - |aSize| - but if substitution occurred when doing the SurfaceCache lookup, the size might be different than |aSize| and yet the frame could still cover the entire surface.

This bug causes us to call CopyFrame() and uselessly copy surfaces that don't need to be copied; when we hit it, it has a significant performance cost.
Here's the patch. It's straightforward; if the frame doesn't cover the actual
surface, it'd have either an offset (which is unchanged from before this patch)
or its actual size would not match its logical size - i.e., imgFrame::GetSize()
and imgFrame::GetImageSize() would return different values. The old code
compared imgFrame::GetSize() and |aSize|, which is wrong if substitution
occurred.
Attachment #8724359 - Flags: review?(tnikkel)
Attachment #8724359 - Flags: review?(tnikkel) → review+
Are all GetFrame and GetFrameAtSize callers coded to deal with getting a surface of any possible size? Maybe we should audit.
(In reply to Timothy Nikkel (:tnikkel) from comment #2)
> Are all GetFrame and GetFrameAtSize callers coded to deal with getting a
> surface of any possible size? Maybe we should audit.

We probably should. A lot of them pass FLAG_SYNC_DECODE, in which case they won't hit this issue. But for ones that don't, they need to not assume that the size of the surface they receive matches the size of the imgIContainer. Ironically enough this is probably actually simpler in most cases, but the code does need to be updated.
Blocks: 1254371
https://hg.mozilla.org/integration/mozilla-inbound/rev/8c056908523ec29f08db9d304bff71f5485a14f2
Bug 1251806 - In RasterImage::GetFrameInternal(), check if the frame covers the actual surface size rather than the requested surface size. r=tn
https://hg.mozilla.org/mozilla-central/rev/8c056908523e
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in before you can comment on or make changes to this bug.