GNOME Bugzilla – Bug 106962
GIF animations loaded incorrectly
Last modified: 2010-07-10 04:07:49 UTC
Some (broken) animations do not load at all. In particular, animations with frames not wholly contained within the bounds of the base frame do not get loaded correctly (see Exhibit A). Old versions of GIFBuilder also spit animations with zero-sized frames for "delay only" parts; unfortunately I can't find such an example right now, but this has been tested with them. It may be in the Evolution bugzilla somewhere.
Created attachment 14584 [details] Exhibit A: animation that does not load correctly
Created attachment 14585 [details] [review] Patch with the fix that is in the 1.4 branch; it works very well for GtkHTML.
Thanks for explaining what the patch tries to achieve. Unforunately, I get ** (lt-testpixbuf:21811): WARNING **: Error loading image: Failed to load image '/home/matthias/testimages/exhibit-a.gif': GIF image contained a frame appearing outside the image bounds. on CVS head, with or without your patch. This may be due to improved error checking in gtk-2.0 compared to the 1.4 pixbuf code: 2002-03-15 Matthias Clasen <maclas@gmx.de> * io-gif.c (gif_get_frame_info): Catch invalid frame dimensions.
Oops, I missed that bit. Sorry about that; here is a revised patch. The ChangeLog is this: * gdk-pixbuf/io-gif.c (gif_get_frame_info): Do not bail out on zero-sized frames. GifBuilder may spit such frames for "just delay, no image". Do not bail out on frames whose bounds are partially outside of the base bounds. (clip_frame): New utility function. (gif_fill_in_pixels): Clip to the frame's pixbuf. (gif_get_lzw): Clip to the frame's pixbuf.
Created attachment 14645 [details] [review] Revised patch.
CCing Larry because this affects gtkhtml.
The second patch works well for me. I put some test images on http://primates.ximian.com/~lewing/anim.html While looking at this I notice gdk-pixbuf seems to make the animations run more quickly than mozilla, should I open another bug for that?
Well, this patch makes gdk-pixbuf load many broken gifs, but the price we pay is robustness (which should be bad news for people who want to use it to display images in an html widget inside an email client...) matthias@linux:~/gnome/gtk/head/tests> pixbuf-random seed: 1046391327 the last tested image is saved to the file "pixbuf-random-image" GIF_HEADER GdkPixbuf-CRITICAL **: file gdk-pixbuf.c: line 156 (gdk_pixbuf_new): assertion `width > 0' failed aborting... Aborted Those size checks in gif_get_frame_info were good for something. We cannot simply drop them, because zero-sized pixbufs are not allowed (why ?).
There was a small bug in federico's patch, with the fix. He ended up checking pointer values rather than dereferencing them first. Revised patch attached. This shouldn't be any less robust than the current code. I doubt there is any reason a pixbuf with a zero dimension is prohibited other than someone put a check because they couldn't think of a reason why you'd want one. This patch makes 0x0 frames 1x1 frames to get around the 0 check, but frankly I'd rather load out of spec gif frames with the wrong dimensions rather than not load them at all.
Created attachment 14679 [details] [review] 3rd revision
I'm an idiot. I'm making a new gdk-pixbuf release as we speak. Thanks for catching that, Larry.
Allowing 0 width/height pixbufs seems reasonable logically, but allowing them now seems like a subtle API breakage. A function similar to: gboolean pixbuf_is_skinny (GdkPixbuf *pixbuf) { int width = gdk_pixbuf_get_width (pixbuf); int height = gdk_pixbuf_get_height (pixbuf); return width / height < 1; } seems reasonably likely to exist somewhere, and would be perfectly correct now, and incorrect with the change. Is the change OK for 2.2.x? Almost certainly not. 2.4? 3.0? Even for 3.0 I'm worried about a subtle semantic change like this. Maybe I'm just too paranoid.
The 3rd revision works fine under extended pixbuf-random and pixbuf-randomly-modified testing, so I think it is fine to commit. The gif loader fails in pixbuf-lowmem testing, due to a g_realloc, but this is unrelated to the patch and I will fix it once the patch is committed. Owen, should this go to both branches or do you want to restrict this amount of code change to HEAD ?
If we have multiple people who understand the patch, I think it's OK if it goes onto gtk-2-2. (Note the string change ... since it's seldom user-visible, I think it's OK to simply mail gnome-i18n about it.) I took a quick glance through the patch (just to see what it was about again), and had a few questions that maybe someone could answer: * Why dont' we check context->gif89.transparent anymore? Do we always create an alpha pixbuf? - if (context->gif89.transparent != -1) { * What's up with the changes to the area passed to update_func .. it looks like they changed from being animation relative to being frame relative. Were they just wrong before? Thanks.
I haven't looked at the update_func calls in detail, but I can answer the first question: yes, the gif loader always creates pixbufs with alpha.
Any word on this?
No, I was kind of hoping that you would try to answer Owens question about the update_func changes. What I did so far was to hack io-gif.c to display a red box around the update area, but the result really confused me - I saw lot of boxes in irrelevant areas when trying Larry's marching ant example.
Isn't the update area supposed to be frame-relative? [At least, isn't this what is more convenient for applications?] Maybe I did screw up something with respect to the offsets...
I would think that what it is "supposed to be" is defined by what it is now. We can't change it even if it were more convenient to be elsewise...
Trying to answer Federicos question: The update area may be frame-relative in pixbuf-stable, but in the gtk version frames are no longer part of the api, since they have been replaced be the abstact AnimationIter concept. Thus the update area *must* be relative to the animation.
I looked in detail at the current situation, and it was broken. I've now fixed the update_func() calls to consistently specify the updated rectangle relative to the animation. Still broken is that we use the (small) frame rather than the composited pixbuf as first argument, but that doesn't matter, as the GtkPixbufLoader doesn't look at that pixbuf anyway. I have also incorporated your idea to accept empty frames as transparent 1x1 pixbufs. Still open: dealing with over-large frames.
I've committed a fix for handling oversized frames to HEAD. I've taken a different approach than Federico: read all frames at their original size, but be careful to clip all rectangles to the logical screen size. This approach was easier to understand for me, and I didn't have to mess with the complicated logic for progressively loading interlaced frames... It is a larger change, thus I've not committed it to 2.2, even though I've carefully tested it. Reopen this bug if you want to see the fix in 2.2, Owen.