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

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[Seth Fowler [:seth] [:s2h]]

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.

Comment 1

[Seth Fowler [:seth] [:s2h]]

Attachment: In RasterImage::GetFrameInternal(), check if the frame covers the actual surface size rather than the requested surface size.

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.

Comment 2

[Timothy Nikkel (:tnikkel)]

Are all GetFrame and GetFrameAtSize callers coded to deal with getting a surface of any possible size? Maybe we should audit.

Comment 3

[Seth Fowler [:seth] [:s2h]]

(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.

Comment 4

[Seth Fowler [:seth] [:s2h]]

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

Comment 5

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/8c056908523e