After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 585425 - GIF loader uses about 280M of VmRSS for a 200Kb GIF
GIF loader uses about 280M of VmRSS for a 200Kb GIF
Product: gdk-pixbuf
Classification: Platform
Component: general
git master
Other Linux
: Normal normal
: ---
Assigned To: gdk-pixbuf-maint
: 344731 698770 703794 (view as bug list)
Depends on:
Reported: 2009-06-11 10:48 UTC by Philip Van Hoof
Modified: 2014-10-22 14:23 UTC
See Also:
GNOME target: ---
GNOME version: ---

Image that causes this behaviour (274.42 KB, image/gif)
2009-06-11 10:49 UTC, Philip Van Hoof
Reduce memory usage (2.18 KB, patch)
2012-01-08 12:14 UTC, Denis Pauk
none Details | Review
Test program (7.24 KB, text/x-csrc)
2012-01-08 12:17 UTC, Denis Pauk
Same patch but for current git version (2.16 KB, patch)
2012-01-08 12:47 UTC, Denis Pauk
none Details | Review
Fix memory usage for 2.24.0 (4.10 KB, patch)
2012-01-08 18:41 UTC, Denis Pauk
none Details | Review
Fix memory usage for git master(same as for 2.24.0) (3.97 KB, patch)
2012-01-08 18:52 UTC, Denis Pauk
none Details | Review
Minimize memory usage for gif animation. (4.23 KB, patch)
2013-08-03 07:01 UTC, Denis Pauk
needs-work Details | Review
Updated patch (5.20 KB, patch)
2014-04-26 07:04 UTC, Denis Pauk
none Details | Review
gif: Minimize memory usage for gif animation (5.21 KB, patch)
2014-10-22 10:00 UTC, Bastien Nocera
committed Details | Review

Description Philip Van Hoof 2009-06-11 10:48:22 UTC
Here we have an interesting GIF file that apparently has its Logical Screen Size at 45x73.

Regretfully the people who made the image decided that the individual frames would each be sized 86x13364. Reading the specification if GIF this appears to be possible.

This back-trace however will happen for each and every such frame. Which for the attached image means that it happens ~ 194 times.

---case GIF_GET_LZW
---------gdk_pixbuf_copy at io-gif-animation.c:469

The width and height of these pixbufs are each 86x133364, multiplied by three bytes for the RGB data that's about 3MB per frame. 

Looking at it with valgrind's massif tool, and by looking at /proc/$PID/status I noticed that during image loading tc3.gif (with a GdkPixbufLoader, the file size is is 275Kb in size) VmRSS grows to about 280MB.

Apparently some of the GdkPixbuf instances for compositing the frames together are indeed destroyed, else the memory usage would grow to said 3MB x 194 frames, or about 600MB.

This ain't a leak because once loaded, the VmRss usage goes back to more sane values. My kernel took some time to recover from the abuse on VmRSS, but eventually it prevailed.

I'm a bit reluctant to call this normal ...

In any case, it makes it nearly impossible to use such relatively small GIF files on certain mobile devices. Using +200MB VmRSS on such devices is unacceptable memory consumption.

Regretfully there's no way to detect that this will happen for the file in question.

That's because the "size-prepared" signal will emit with the logical screen size, not with the individual frames' sizes.

(image will be attached shortly after committing this description)
Comment 1 Philip Van Hoof 2009-06-11 10:49:03 UTC
Created attachment 136328 [details]
Image that causes this behaviour
Comment 2 Denis Pauk 2012-01-08 12:14:17 UTC
Created attachment 204830 [details] [review]
Reduce memory usage

For me looks as reduced usage from 892MiB to 231MiB (570MiB in peak).
Comment 3 Denis Pauk 2012-01-08 12:17:51 UTC
Created attachment 204831 [details]
Test program

i looks memory usage in gnome system monitor. I usage ubuntu 11.10(gdk-pixbuf-2.24.0)
Comment 4 Denis Pauk 2012-01-08 12:47:47 UTC
Created attachment 204832 [details] [review]
Same patch but for current git version
Comment 5 Denis Pauk 2012-01-08 18:41:15 UTC
Created attachment 204836 [details] [review]
Fix memory usage for 2.24.0
Comment 6 Denis Pauk 2012-01-08 18:52:52 UTC
Created attachment 204837 [details] [review]
Fix memory usage for git master(same as for 2.24.0)
Comment 7 Denis Pauk 2012-01-08 19:08:24 UTC
After patches looks as used 23.2MiB. I added code for clean cached pixbuf (composited) when cache not used. Now deleted cache if exist next frame with composited pixbuf. And when created many new frame use old frame memory(cache not created) and cache created only for last frame in list.
Comment 8 Denis Pauk 2013-04-06 06:17:16 UTC
Please someone make CR.
Comment 9 Denis Pauk 2013-08-03 07:01:36 UTC
Created attachment 250747 [details] [review]
Minimize memory usage for gif animation.

Have cleaned up white spaces in previous patches, checked changes on current master.

As test program can be used eog, so old test program made absolute also.
Comment 10 Denis Pauk 2013-08-27 19:56:26 UTC
Bug 698770 is duplication of this bug.
Comment 11 Jürg Billeter 2013-08-27 21:34:22 UTC
*** Bug 698770 has been marked as a duplicate of this bug. ***
Comment 12 Claudio Saavedra 2013-09-10 11:39:36 UTC
*** Bug 703794 has been marked as a duplicate of this bug. ***
Comment 13 Bastien Nocera 2014-04-11 07:04:30 UTC
Review of attachment 250747 [details] [review]:

There's many things I don't really understand in this patch.

Why do you go through the list of frames after the previous and the current one in gdk_pixbuf_gif_anim_iter_clean_previous()? Wouldn't poke at the prev and current be enough?
Your changes in frame_composite() can probably use a function, instead of repeating the same changes 3 times.

A more detailed commit message would also be useful.

::: gdk-pixbuf/io-gif-animation.c
@@ +213,3 @@
+        GList *tmp = initial;
+        while (tmp != NULL) {

We prefer for() loops, as in gdk_pixbuf_gif_anim_finalize().

@@ +222,3 @@
+                if (tmp == NULL)
+                        return;
+                GdkPixbufFrame *prev_frame = tmp->data;

Declare new variables at the top of the block, not in the middle of it.

@@ +297,2 @@
                 tmp = tmp->next;
+                if (tmp) {

No need for braces for one-line conditionals.
Comment 14 Denis Pauk 2014-04-26 07:04:18 UTC
Created attachment 275192 [details] [review]
Updated patch

Thank you. I have updated patch with such changes: fix code style and logic for cleanup previous frames.

About for - in such case we don't needed full loop - because mainly we have maximum one not cleaned frame before current and while iterated backward only once.
Comment 15 Bastien Nocera 2014-10-22 10:00:06 UTC
Created attachment 289115 [details] [review]
gif: Minimize memory usage for gif animation

Update the logic to create the next frame.
When the new frame uses a buffer from the previous frame, in retain,
revert or dispose mode, try to reuse the allocated buffer from the
previous frame.

This reduces allocated memory by an order of magnitude.

For other cases when we can't use the old buffer, clean up the previous
buffers after the new frame has been handled.
Comment 16 Bastien Nocera 2014-10-22 10:10:06 UTC
Attachment 289115 [details] pushed as 1d5bd7c - gif: Minimize memory usage for gif animation
Comment 17 Bastien Nocera 2014-10-22 14:23:51 UTC
*** Bug 344731 has been marked as a duplicate of this bug. ***