Animated GIF plots should repaint bgcolor after each frame

[kini]

As title. ImageMagick should be called with -dispose Background, because when you create an animation, Sage first creates a bunch of frames, then uses ImageMagick to make an animated GIF out of them. If the frames are not all the same size, then ImageMagick determines the minimum size for the final GIF in which it is possible to accommodate all the frames. The default behavior is to simply draw frame n over frame n-1 without erasing frame n-1. When frame n's image is smaller than frame n-1's image, however, you get garbage in frame n outside the borders of the original image used to create frame n.

Before:

After:

The above image was produced by the following code:

t = var('t')

def frame(theta):
    r = 2
    x = r*(t - sin(t))
    y = r*(1 - cos(t))

    plot = parametric_plot((x, y), (t,0,theta), rgbcolor=hue(0.6))
    krug = circle((2*theta, r), r)
    tocka = point((r*(theta - sin(theta)), r*(1 - cos(theta))), pointsize=30, color='red')
    return plot+krug+tocka;

animacija = animate([frame(x) for x in srange(0.1, 5*pi, 0.1)])

animacija.show(delay=10, iterations=0)

(Thanks to user v-v on Freenode.)

---

Apply trac_11313-imagemagick-options.patch to $SAGE_ROOT/devel/sage.

[kini]

Any ideas on how to doctest this? Doesn't really seem possible, on first glance anyway...

[kcrisman]

Does this slow anything down? animate is already on the slow side...

[kini]

This is a defect, not an enhancement, ticket, so I don't consider potential slowdown to be a problem with the patch. That aside, ImageMagick? is written in C and is pretty fast. If animate is slow it's probably because Sage needs to create every frame separately before using ImageMagick? to put them together. I doubt this patch introduces any slowdowns.

To elaborate on what makes this a defect: when you create an animation, Sage first creates a bunch of frames, then uses ImageMagick? to make an animated GIF out of them. If the frames are not all the same size, then ImageMagick? determines the minimum size for the final GIF in which it is possible to accommodate all the frames. The default behavior is to simply draw frame n over frame n-1 without erasing frame n-1. When frame n's image is smaller than frame n-1's image, however, you get garbage in frame 'n' outside the borders of the original image used to create frame 'n'.

[kini]

Sorry about the spam...

[jhpalmieri]

Can you provide code which illustrates the problem (before the patch) and its solution (after)?

[kini]

Hmm, I've lost track of the code which demonstrated this problem (the code was originally posted on a pastebin by a user on IRC in May). While I wait for a response to the message I sent that user, here's an image showing what animate() currently does with that code:

[kini]

Here we are:

t = var('t')

def frame(theta):
    r = 2
    x = r*(t - sin(t))
    y = r*(1 - cos(t))

    plot = parametric_plot((x, y), (t,0,theta), rgbcolor=hue(0.6))
    krug = circle((2*theta, r), r)
    tocka = point((r*(theta - sin(theta)), r*(1 - cos(theta))), pointsize=30, color='red')
    return plot+krug+tocka;

animacija = animate([frame(x) for x in srange(0.1, 5*pi, 0.1)])

animacija.show(delay=10, iterations=0)

Credit to user v-v on freenode. This produces the image shown above on Sage 4.7 (for example on sagenb.org). My local alpha2 (old, I know...) does something kind of alarming... I'll test with the latest dev version once I get it built.

[jhpalmieri]

Minor point: the patch doesn't apply cleanly to Sage 4.7.1.rc0, but that's easy to fix. More importantly, with Safari 5.1 on a Mac, the area which is no longer part of the picture displays as a black box. On Firefox or Chrome, it's white (or maybe the default background color?), but it looks bad on Safari. I don't know if this is a bug in how Safari displays animated gifs or if we can avoid it somehow.

[kini]

The "something kind of alarming" appears to be caused by #11170 - the GIF blows up in filesize by a factor of 50 and the aspect ratio is destroyed. I'll cross-post to that ticket...

As for Safari, no idea. I'll look into it.

[kini]

I'm going to rebase this on #11650 / 4.8.alpha1 .

Regarding Safari's behavior, I found  this, which links to some internal Apple bug tracker which I cannot access. Can we assume this is Safari's problem?

[kini]

Now that #11650 is in, here's a rebase on sage 4.8.alpha6.

How the above GIF looks after the patch:

On  the imgur page you can see how the GIF's transparency and non-white backgrounds interact. Compare with  the old behavior.

[kini]

Whoops, the "before" image is a bit different now in sage 4.8.alpha6 than it was when I posted 9. See the description for the updated GIF.

[jhpalmieri]

This looks good to me. By the way, where is this syntax for defining strings

s = ('abc' 'def')

documented?

[kini]

 Here is the relevant section of the Python docs. Actually,

s = 'abc' 'def'

works too, but the parentheses in the patch force a line continuation.

Thanks for the review!

[jdemeyer]

apply to $SAGE_ROOT/devel/sage

[jdemeyer]

Changed commit message.

[kini]

Wait, what? So now we're not putting the trac ticket number in the commit message? Hmm, I didn't realize that. Thanks for the heads up.

[jdemeyer]

Replying to kini:

Wait, what? So now we're not putting the trac ticket number in the commit message? Hmm, I didn't realize that. Thanks for the heads up.

Funny how in the beginning when I was release manager, I had to convince everybody to put ticket numbers in the commit message and quite a few didn't do it (or worse: put the wrong ticket number). Since sage-4.7, the ticket numbers are added automatically but it seems that now, more people than before are actually adding the ticket number.

[kini]

Of course, the best way would be to have people make commits to the tree, and then close the ticket with a reference to the commit, rather than making the commit upon release, when the ticket has been closed already. But for that I guess we need to implement the github-based sage --push/pull workflow Robert Bradshaw suggested, or something similar.

By the way, why not just check whether the ticket number is already in the commit message and not add it again if so?

[jdemeyer]

Replying to kini:

By the way, why not just check whether the ticket number is already in the commit message and not add it again if so?

When this was discussed on sage-devel (please don't ask me to find the thread again), somebody suggested that having a uniform format

Trac #54321: change foo to bar

could be useful for scripts. I would find a version of "hg blame" with ticket numbers quite interesting. If somebody would implement this, then the standard-format ticket numbers would certainly be useful.