289763 - GIF: Don't allocate memory for unused areas when compositing

Status: RESOLVED INACTIVE

(Core :: Graphics: ImageLib, defect)

Description

[ArronM (:paper)]

When a multi-frame GIF that requires compositing defines a large image size in
it's header, and then never uses it (all the frames are small and located close
to 0,0), we often get a OOM error and do not display the composited frame(s). 
This is because our compositing frame is defined as the size told to us by the
image header.  Instead of using those numbers, we could use the max (xpos+width)
and  max (ypos+height) to create the compositing frame.

Example:

GIF Header reports:
width=40,000
height=30,000

Frame 1: Pos(0,0) Size(50,50)
Frame 2: Pos(10,10) Size(45,10)

Results Now: Compositing frame will be created with a size of 40000x30000, or
about 3.3 Gigs of memory.  It will most likely fail, and frame 2 will be skipped
over.

What should happen: Compositing frame only needs to be created with a size of
55x50, or 2.6k of memory.

Warning: URL will crash mozilla until Bug 289516 is checked in.

Comment 1

[ArronM (:paper)]

Attachment: 3 Framed, 49173x49429 GIF (~7Gig memory), Drawing Area 21x21

Only the first frame displays on Mozilla and Opera7.  IE6 displays it just
fine.

Comment 2

[ArronM (:paper)]

Attachment: 3 Framed, 2000x4000 (~24M) GIF

Same GIF, except smaller defined area.	Mozilla currently uses about 26M of
memory.

Comment 3

[ArronM (:paper)]

Attachment: WIP

Work In Progress.  Tested only in Windows so far.

Everything appears to work, however I'm still pondering what to do with
nsIImage::DrawTo.  Each platform assumes the destination co-ordinate parameters
are based on the image start.  They do not take into account the x and y
position already present for the frame.  The patch adjusts this by subtracting
at the DrawTo call, but I think the code in DrawTo should be changed to work
correctly.  Unfortunately, that means adding the same code to each platform.

Attachment 181379 [details] memory usage is almost not noticable with this patch (1M),
and Attachment 181378 [details] actually displays all its frames.

Comment 4

[ArronM (:paper)]

To answer my own question:

nsIImage doesn't store x and y offset of the image, gfxImageFrame does. 
Therefore, nsIImage::DrawTo can't do the adjusting calculations.  Which makes my
current patch correct, with the exception of a printf.

Comment 5

[ArronM (:paper)]

Forget what I said about DrawTo.  The correct thing to do is to add the position
adjusting code to gfxImageFrame::DrawTo.

Sorry, having two classes (gfxImageFrame and nsIImage) that have the exact same
function names confused me in previous comments.

Comment 6

[ArronM (:paper)]

Attachment: patch

Not only is memory saved, but we are drawing smaller areas, which will result
in a (very small) speedup for GIFs that use frame offsets and
smaller-than-global-size frames.

Also note that Bug 192790 removes BuildCompositeMask entirely.	In this patch,
I've cleaned up the variable names so I could follow what's going on (and fix
it for adjusting to frame offsets)

Comment 7

[ArronM (:paper)]

Attachment: patch

patch working on Windows and Linux

Comment 8

[Joe Drew (not getting mail)]

Comment on attachment 181490 [details] [diff] [review]
patch

This patch is unfortunately so outdated that I can't evaluate its goodness or badness. This code has changed very substantially in the intervening 6.5 years :(

I think that starting over is probably the best path to victory here.

Comment 9

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee didn't login in Bugzilla in the last 7 months.
:aosmond, could you have a look please?
For more information, please visit auto_nag documentation.