GNOME Bugzilla – Bug 655755
First frame's delay used for all frames in GDI+ animation handling
Last modified: 2011-09-30 23:52:42 UTC
Created attachment 193017 [details] Example GIF renderer to simplify debugging. If the GDI+ interface is used for animated GIFs with variable-delay frames, the animation is set up with static-delay frames (all frames have the delay intended for just the first frame). This is caused because io-gdip-utils.c's gdip_bitmap_get_frame_delay gets the array of frame delays, but proceeds to always use the first value out of that array. *delay = *((long *)item->value); should be *delay = ((long *)item->value)[frame_index]; frame_index is not currently known in gdip_bitmap_get_frame_delay, but it is known in the calling context and could be passed in as a parameter (the calling context is looping through the frames and the frame index in the "i" variable). Attached is a short example program that just displays "test.gif" from the working directory. I used http://l.yimg.com/us.yimg.com/i/mesg/emoticons7/6.gif (Yahoo! IM's "bighug" emoticon). On Windows, this example program can be used to illustrate the problem by compiling against gtk+-bundle_2.16.6-200915_win32 (doesn't use GDI+ for image handling, so it displays correctly) and gtk+-bundle_2.16.6-20100207_win32 (changed to using GDI+, so it has the problem). This is causing a much-lamented bug in Win32 Pidgin (bug #11858, http://developer.pidgin.im/ticket/11858). If you need more information, let me know. I probably won't be submitting a patch, though, since I won't be setting up a GTK+ build environment for such a trivially recreatable/fixable bug.
Created attachment 193090 [details] [review] Proposed patch to fix GDI+ animation frame delay So, I lied. Setting up the build environment on Windows for this wasn't as tricky as I expected. Here's a patch that's I've confirmed fixes the issue (using the sample program I provided before). Again, if there is any other information that I can provide, let me know.
Created attachment 193091 [details] [review] Proposed patch to fix GDI+ animation frame delay bugs Second version. This also fixes two other (only tangentially related) GDI+ frame delay bugs I just noticed (2 and 3 below). Now this patch fixes: 1. Using the correct delay for a given frame (instead of always using same delay). 2. If the GIF specifies 0ms delay, give it 100ms instead of 20ms (now matches other animation code) 3. If adjusting the specified frame delay (either 0 or just too short), it was still using the specified frame delay in the frame->elapsed value. I've confirmed all three fixes using images that fall under each condition. If the last two need to be split out into another ticket, let me know. However, they seem related _enough_ to just fix it at the same time.
Hi Keith, thanks for the patch! Just tested and here's some remarks: issue 1: looks good to me issue 2: looks good to me, now matches mozilla behavior issue 3: mozilla[1] uses the following (where their mTimeout matches our frame->delay_time): if (mTimeout >= 0 && mTimeout <= 10) return 100; else return mTimeout; Looking at the GIF89A spec[2], we learn that the Delay Time field is a 16-bit unsigned integer that specifies the delay in hundredths (1/100) of a second. Thus we can expect delay values of 0 ms, 10 ms, 20 ms, etc meaning the above check is correct. Would be great if we can match mozilla behavior here too. Thanks, Dieter [1] http://hg.mozilla.org/mozilla-central/file/7558f15d8241/modules/libpr0n/src/imgFrame.cpp#l706 [2] http://www.w3.org/Graphics/GIF/spec-gif89a.txt
Review of attachment 193091 [details] [review]: I saw Dieter already provided an insightful review of the actual fix. Since the patch needs to be updated anyway, I will point out some minor issues. ::: gdk-pixbuf/io-gdip-utils.c.orig @@ -496,2 +496,3 @@ item = (PropertyItem *)g_try_malloc (item_size); if (Ok == GdipGetPropertyItem ((GpImage *)bitmap, PropertyTagFrameDelay, item_size, item)) { + item_count = item_size/sizeof(long); leave spaces around "/" @@ +499,2 @@ /* PropertyTagFrameDelay. Time delay, in hundredths of a second, between two frames in an animated GIF image. */ + *delay = ((long *)item->value)[(frame < item_count)? frame: item_count-1]; spaces before "?" and around "-" @@ -708,1 +709,1 @@ /* GIF delay is in hundredths, we want thousandths */ not your fault, but while at it you could fix the spelling of "thousands" @@ -716,2 +720,2 @@ */ if (frame->delay_time < 20) here you could use "else if" since we know that 100 > 20
Created attachment 197883 [details] [review] Proposed patch to fix GDI+ animation frame delay bugs (w/ style changes) (In reply to comment #3) > Hi Keith, thanks for the patch! No problem. Glad to do it. :-) > issue 3: mozilla[1] uses the following (where their mTimeout > matches our frame->delay_time): > > if (mTimeout >= 0 && mTimeout <= 10) > return 100; > else > return mTimeout; > > Looking at the GIF89A spec[2], we learn that the Delay Time field > is a 16-bit unsigned integer that specifies the delay in hundredths > (1/100) of a second. Thus we can expect delay values of 0 ms, 10 ms, > 20 ms, etc meaning the above check is correct. > > Would be great if we can match mozilla behavior here too. For what it's worth, shortly after I made that reply, I realized that issue 3 wasn't an issue at all (temporarily mistook total_time for delay_time). total_time isn't modified between where it used to be and where I moved it to. However, I didn't submit a new patch reverting that small change (moving the total_time) because I felt that it more logical to pair that line with the other total_time-related line and because it matched the flow in io-gif.c. I'm getting the "if 0, use 100. If just too fast (10) use 20" logic from io-gif.c. I would think we'd want GIFs to render consistently regardless of which module is used. If it's changed, I'd think it should change in all of the animation code - which would probably be a separate effort/patch? The only difference between the two, anyway, is that mozilla is going against the GIF author's wishes for delay values of 1-9 and we're currently only going against their wishes only for the value of 1. Ideally, I'd think we'd always use the specified delay (unless it's 0, which is invalid), but 1 is too fast for us to handle. Maybe a common adjust_frame_delay function that all of the animation code can call is in order so that these decisions only have to be made in one place. But, again, that's probably a separate effort. (In reply to comment #4) > Review of attachment 193091 [details] [review]: > > leave spaces around "/" done > spaces before "?" and around "-" done. Also added a space before the ":". Since I assume if your coding style dictates one, it dictates the other. > not your fault, but while at it you could fix the spelling of "thousands" But, we _are_ talking about thousandths (x/1000), not thousands (x*1000)... > here you could use "else if" since we know that 100 > 20 Sure could. Had it that way before I saw io-gif.c had it as two separate ifs. Changed it to be consistent. I'll change it back to "else if". Submitting a revised patch with the coding style changes, understanding that there may be more conversation and changes needed based on my replies.
(In reply to comment #5) > I'm getting the "if 0, use 100. If just too fast (10) use 20" logic from > io-gif.c. I would think we'd want GIFs to render consistently regardless of > which module is used. Agreed, being consistent with our own platform is more important here :) > > spaces before "?" and around "-" > > done. Also added a space before the ":". Since I assume if your coding style > dictates one, it dictates the other. Yeah, at least the rest of gdk-pixbuf's codebase seems to do so. > > not your fault, but while at it you could fix the spelling of "thousands" > > But, we _are_ talking about thousandths (x/1000), not thousands (x*1000)... Correct.
Created attachment 197926 [details] [review] Proposed patch to fix GDI+ animation frame delay bugs (w/ style changes) Identical to 197883. Added author, commit message and generated with git format-patch.
Comment on attachment 197926 [details] [review] Proposed patch to fix GDI+ animation frame delay bugs (w/ style changes) [01:38] <dieterv> mclasen: would you have a minute for https://bugzilla.gnome.org/show_bug.cgi?id=655755 ? [01:45] <mclasen> dieterv: not a particular expert on the gdip loader, but that patch looks ok to me, and has already had quite a bit of review [01:45] <mclasen> so I would say you should commit it [01:45] <dieterv> cool, thanks! Pushed as ee81d3fc611a0e64dc55c4f2a2fda73932de2fe6.
Thank you Keith for doing the hard work and thank you Paolo for the review!