GNOME Bugzilla – Bug 613595
gdk-pixbuf does not manage animation sizes correctly
Last modified: 2012-07-31 14:30:36 UTC
Created attachment 156745 [details] GIF to describe the problem I'll provide an offending GIF as next attachment. The test program takes a filename as parameter and optionally a size and height. It uses a GdkPixbufLoader to load a file and then it prints pixbuf and animation sizes. The pixbuf is the first animation frame. 1. According to the GdkPixbufAnimation documentation ( http://library.gnome.org/devel/gdk-pixbuf/unstable/gdk-pixbuf-animation.html#gdk-pixbuf-animation.description ) every frame has the same size, which does not happen here. 2. As it trusts the first point, GdkPixbufLoader uses pixbuf size to calculate the scale and it fails. Let's suppose that you run the test program with limit of 400x400, I expected to get an animation of 400x400, but I get a pixbuf of 400x400 and a much bigger animation, so when I was expected to make the animation smaller, I made it much bigger because first frame is 45x73 and total animation size is 86x13364. Program output should be: $ /tmp/animation_size /tmp/dancing.gif 400 400 ** (process:27255): DEBUG: max size: 400x400 ** (process:27255): DEBUG: animation: 400x400 ** (process:27255): DEBUG: pixbuf: 209x2 ** Message: animation size (400x400) != pixbuf_size (209x2) And it ends up being: $ /tmp/animation_size /tmp/dancing.gif 400 400 ** (process:27228): DEBUG: max size: 400x400 ** (process:27228): DEBUG: animation: 764x73227 ** (process:27228): DEBUG: pixbuf: 400x400 ** ERROR:../../animation_size.c:58:main: assertion failed: (animation_width <= max_width && animation_height <= max_height)
Created attachment 156746 [details] Offending GIF file
Created attachment 156747 [details] [review] Call size_func in io-gif so that pixbuf is aware of it. This patch makes the GIF loader announce the size of the loaded image.
Created attachment 156748 [details] [review] Try to use animation size in the loader for animations. Pixbuf size can be innacurate for some animations with frames of different sizes.
Created attachment 156749 [details] [review] Fixing doc about relying on pixbuf size for animations
Created attachment 173114 [details] [review] Changed doc of GdkPixbufAnimation about size management
I updated third patch as it was not applying anymore with the new structure.
This doesn't seem quite right to me. "every frame has the same size, which does not happen here." This is something that the gif loader must ensure, really.
(In reply to comment #7) > This doesn't seem quite right to me. > > "every frame has the same size, which does not happen here." > > This is something that the gif loader must ensure, really. So there is a bug anyway, though I think that removing the condition of having all frames with the same size is not that big problem.
(In reply to comment #7) > This doesn't seem quite right to me. > > "every frame has the same size, which does not happen here." > > This is something that the gif loader must ensure, really. We can discuss about two things. One of them is that gdk-pixbuf has a problem when scaling the animations and in this case, the scaling causes that we increase the size (a lot) instead of reducing it. This is fixed by the two first patches, so I really think that at least these two should be applied. Then we have the other problem. In this case, the frames have different size. I don't know if the GIF standard allows having frames with different sizes or not, but removing the size constraint would allow us to save some memory with some animations that could have different frames size and it would ease the development of the different loaders (this would be one and in case we get any new animation format allowing different frames size). Other consideration is that what is working so far, will be working the same way as different sizes is a supercase of same size.
We just talk about this at desktop summit. Remainder ;)
Poking a bit around and testing with the latest master branch: Information about the pixbuf is the same with and without patches: $ G_MESSAGES_DEBUG="all" /tmp/animation_size /tmp/dancing.gif ** (process:9864): DEBUG: animation: 86x13364 ** (process:9864): DEBUG: pixbuf: 45x73 ** Message: animation size (86x13364) != pixbuf_size (45x73) As you can see, the animation and first pixbuf sizes are different and that contradicts the documentation. But I will like to discuss this later. Running the test program without the first two patches: $ G_MESSAGES_DEBUG="all" /tmp/animation_size /tmp/dancing.gif 400 400 ** (process:6199): DEBUG: max size: 400x400 ** (process:6199): DEBUG: animation: 764x73227 ** (process:6199): DEBUG: pixbuf: 400x400 ** ERROR:/tmp/animation_size.c:58:main: assertion failed: (animation_width <= max_width && animation_height <= max_height) Aborted In this case we are telling the loader to restrict the maximum size to 400x400 and given that it is using the first pixbuf to calculate the scaling ratio instead of the animation one, it is even increasing the size, as I already stated in the comment 1. Running it with the first two patches: $ G_MESSAGES_DEBUG="all" /tmp/animation_size /tmp/dancing.gif 400 400 ** (process:2253): DEBUG: max size: 400x400 ** (process:2253): DEBUG: animation: 400x400 ** (process:2253): DEBUG: pixbuf: 209x2 ** Message: animation size (400x400) != pixbuf_size (209x2) In this case, the animation is correctly scaled to its max size. The size of every pixbuf is still different that that's another story. Given these facts I consider that it would be needed to apply both first two patches as soon as possible to properly scale gifs. This bug was created more than two years ago and patches are provided, I think it deserves a bit of love.
About the problem with the different pixbuf sizes. I really think we should consider frames can have different sizes. http://en.wikipedia.org/wiki/GIF#Animated_GIF says that they have "by default" the same size, but it does not say is is mandatory. In cases like the one I am bringing to this bug, having all frames with the same size would be a waste of computation and memory as we would need to create new pixbufs with the right animation size to comply with the "all frames must have the same size". Another reason to consider different sizes is that we already have this case where there are animations with different sizes in different frames. Applications considering this will be already buggy (at least for this case) either if we consider the documentation change or not so it does not harm to do it. If you prefer, we can have the discussion on the mailing list.
Created attachment 214177 [details] [review] Pixbuf shall have at least one pixel width or height when scaling Another problem I found when scaling is that in some cases it can happen that it tries to scale to 0, which is forbidden in some dimension. Example: $ G_MESSAGES_DEBUG="all" /tmp/animation_size /tmp/dancing.gif 40 40 ** (process:24208): DEBUG: max size: 40x40 (process:24208): GdkPixbuf-CRITICAL **: gdk_pixbuf_scale_simple: assertion `dest_height > 0' failed ** ERROR:/tmp/animation_size.c:46:main: assertion failed: (pixbuf) Aborted This patch fixes that and the outcome is: $ G_MESSAGES_DEBUG="all" /tmp/animation_size /tmp/dancing.gif 40 40 ** (process:27792): DEBUG: max size: 40x40 ** (process:27792): DEBUG: animation: 40x40 ** (process:27792): DEBUG: pixbuf: 21x1 ** Message: animation size (40x40) != pixbuf_size (21x1)
(In reply to comment #0) > Created an attachment (id=156745) [details] > GIF to describe the problem Btw, sorry in the first place for the description of this attachment, but it is the test program instead of the offending GIF, which is another one.
Mathias, would you approve this?
Review of attachment 156747 [details] [review]: ok
Review of attachment 156748 [details] [review]: ok
Review of attachment 173114 [details] [review]: ok
Review of attachment 214177 [details] [review]: nice catch
Created attachment 219877 [details] [review] Removed entities from the code markup It is not being expanded and by checking other projects doc (GLib for example) it is more consistent.
Created attachment 219878 [details] Changed doc of GdkPixbufAnimation about size management
Review of attachment 219877 [details] [review]: Sure, ok
Pushed: http://git.gnome.org/browse/gdk-pixbuf/log/ Thanks Matthias!
Created attachment 219914 [details] Too much rounding when calculating the size of the scaled frame This was causing some frames to be 1 pixel bigger than the animation.
I need to reopen this because I brought a regression with one of the patches shoud be fixed with that last patch.
(In reply to comment #25) > I need to reopen this because I brought a regression with one of the patches > shoud be fixed with that last patch. Fixed.