Closed Bug 69534 Opened 24 years ago Closed 24 years ago

Unnecessary Reflows for animated GIF images: eliminate them now.

Categories

(Core :: Layout, defect, P2)

Product:
Core
Component:
Layout
Type:
defect

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla0.8.1

People

(Reporter: attinasi, Assigned: attinasi)

References

Details

(Keywords: perf, topperf)

Attachments

(1 file)

Patch to suppress reflows on animated image notifications if image is fixed width and height
(deleted), patch
Details | Diff | Splinter Review 
nsImageFrame::UpdateImage is called whenever a new frame is to be displayed for
an animated GIF. If the ImageFrame has a fixed width and height, there is no
need to reflow it. Also, even if there are no width and height attributes, we
can check if the iamge size is unchanged and avoid reflowing in that case as well.

This should be a nice performance improvement, since most top100 pages have
animated GIFs and we waste a whole lot of time relowing them when they almost
never actually change size.
Accepting my own bug - marking perf kwd.
Status: NEW → ASSIGNED
Keywords: perf
Priority: -- → P2
Target Milestone: --- → mozilla0.9
Could this be causing bug 68228? If so, its not just a perf problem.
Mats - I'm not sure if this is the same problem, though it looks like it could
be. Can you post a reduced testcase for the problem image in bug 68228? I'll cc
myself on that bug and see if it is fixed too.
I did some performance analysis using a release build on Linux loading cnn.com.
As a test I commented out the code which generated the reflow in
nsImageFrame::UpdateImage. The page rendered correctly without this (Probably
because the table ended up reflowing the image as content as appended.)

cnn.com rendered 10% faster.

A small change to nsImageFrame.cpp to cache the old width and height and only
reflow if it has changed should speed up page loads on pages with animated gifs.

OK, I have a patch that checks to see if an image is fixed size (has explicit
width and height) and bypasses the reflows on notifications if so. A further
refinement is to handle the case where there is no width and height, but we know
that the image has not changed size - I'm working on that.
Hyatt's playing around with similar hacks just now.

/be
I just did this coincidentally.  On jrgm's tests I got about a 4% speedup overall.

I did something a little different though.  You may get better results.

I added code to the beginning of nsImageFrame that asked if you were doing a
resizereflow.  If you were, and your rect's width and height were the same as
the reflow state's computed width and height, I just fill in those values and
return immediately without calling GetDesiredSize, etc.  Avoids thrashing around
asking the image loader questions.

Kevin ran my patch against John's tests and we saw a ~20% improvement in the
loadtimes - more than I expected.

This is good, so I'm gonna go a little further and handle images that do not
have an explicit width and height but we know that the intrinsic size has not
changed. I think David's change will help too, but for a different reason:
whereas my change prevents reflows on animated images, his changes will prevent
reflows that DO get through from doing any unnecessary interactions with the
imageloader.
Keywords: topperf
Are there any cases when moving to the next frame *does* affect the size of an 
image?
Yup, if the <img> has no width or height and the image's next frame is a 
different size, then we have to reflow because the size will change.
I produced a patch coincidentally on the same stuff.  I sent it to you in email,
attinasi and commented on mozilla.performance.  I had some questions about your
patch that I sent to you in email and to mozilla.performance.

sr=hyatt, although it seems that you can also fix the size for percentage
widths/heights as well, and I have questions about values of 0 when used with
width/height.

I'd love to see this checked in.  However, it can be extended as well and pick
up quite a few more pages/reflows.  This turns off reflows for defined
widths/heights (width=N, CSS equivalent, etc).  (Hyatt's comment about
percentages is good too, though you might need to make sure that we do reflow if
the result of the percentage changes).

Jesse Ruderman wrote:
>Are there any cases when moving to the next frame *does* affect the size of an
>image?

Marc Attinasi wrote:
>Yup, if the <img> has no width or height and the image's next frame is a
>different size, then we have to reflow because the size will change.

So, once we have a size for a single frame of an anim, if the next frame _is_
the same size, we should not reflow, right?  I don't think the patch covers that
case.

I'd be happy to see the current patch checked in; but I'd like to see these
other issues (especially the last one) dealt with as well.
For image frames that _do_ change size, would it make sense to make the
allocated size monotonically increasing so that eventually it stops reflowing?
Seems like this would only negatively impact cases where it was used to
intentionally whack the contents around for some DHTML-like effect (and that
wouldn't be done frequently would it?)
Isn't different framesizes _very_ rare? Shouldn't we in those rare cases do the 
RightThing and resize+reflow accordingly? I think it would give a very "buggy" 
look if we never decrease imagesize.
An animated GIF (the vast, vast majority of animated <IMG>
content) cannot change its frame ('screen') size dynamically
at all.  I don't know about other animation formats such as
MNG (I expect not, however).

Adam, is it part of the GIF specification that the individual frames are all the 
same size? I'm not familiar with the details there, but for some reason I 
assumed that each frame could be a different size (maybe I just infered it from 
the code.) Obviously it is an easier problem if the individual images in the 
animation MUST be the same size; in that case we never have to reflow for 
subsequent frames, and I do not have to cache the previous image size either.

In either case, I'm gonna check in the simple change that checks for a fixed 
size (expanding the notion to include percentage width/height as well) and then 
worry about the case of the image size changing. Pavlov told me that his new 
image lib stuff makes this all pretty much obsolete anyway...
Hiya.  It's part of the GIF spec that the dimensions
of the outer frame ('screen size') are specified in the
header and fixed for the duration of the GIF.  Individual
frames that constitute the animation are not necessarily
the same size but are strictly restricted to the area defined
by the outer frame[1], which should not expand and contract.  So
in short, the size of the image is fixed as far as layout is
concerned.

[1] Some broken GIFs have frames which overlap the edges of the
image bounds but that's purely imagelib's problem (it should clip
them, but AFAIK currently crashes) and not yours.

Thanks for the data, Adam. That makes the optimization much easier.

I wonder if theis is now internalized in the new imagelib... We don't need new
dimension notifications for each frame of the animation (in GIF anyweay)
Milestone shift --> Moz 0.8.1
Target Milestone: mozilla0.9 → mozilla0.8.1
Fix checked in: nsImageFrame.h/cpp
Status: ASSIGNED → RESOLVED
Closed: 24 years ago
Resolution: --- → FIXED
Blocks: 71668
Marking verified per last comments.
Status: RESOLVED → VERIFIED
*** Bug 60880 has been marked as a duplicate of this bug. ***
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: