Opened 9 years ago

Closed 8 years ago

#11313 closed defect (fixed)

Animated GIF plots should repaint bgcolor after each frame

Reported by: kini Owned by: jason, was
Priority: major Milestone: sage-5.0
Component: graphics Keywords: plot, animate, imagemagick
Cc: Merged in: sage-5.0.beta1
Authors: Keshav Kini Reviewers: John Palmieri
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Description (last modified by 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:

http://i.imgur.com/N8XRV.gif

After:

http://i.imgur.com/DpjS2.gif

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.

Attachments (1)

trac_11313-imagemagick-options.patch (982 bytes) - added by jdemeyer 8 years ago.
apply to $SAGE_ROOT/devel/sage

Download all attachments as: .zip

Change History (24)

comment:1 Changed 9 years ago by kini

  • Authors set to Keshav Kini
  • Status changed from new to needs_review

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

comment:2 Changed 9 years ago by kini

  • Description modified (diff)

comment:3 Changed 8 years ago by kcrisman

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

comment:4 Changed 8 years ago by 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'.

comment:5 Changed 8 years ago by kini

  • Description modified (diff)

comment:6 Changed 8 years ago by kini

  • Description modified (diff)

comment:7 Changed 8 years ago by kini

  • Description modified (diff)

Sorry about the spam...

comment:8 Changed 8 years ago by jhpalmieri

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

comment:9 follow-up: Changed 8 years ago by 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:

http://i.imgur.com/ppmZ8.gif

comment:10 Changed 8 years ago by 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.

comment:11 Changed 8 years ago by jhpalmieri

  • Status changed from needs_review to needs_work

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.

comment:12 Changed 8 years ago by 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.

comment:13 Changed 8 years ago by 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?

comment:14 Changed 8 years ago by kini

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

How the above GIF looks after the patch:

http://i.imgur.com/DpjS2.gif

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

comment:15 in reply to: ↑ 9 Changed 8 years ago by kini

  • Description modified (diff)
  • Status changed from needs_work to needs_review

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.

comment:16 follow-up: Changed 8 years ago by jhpalmieri

  • Milestone changed from sage-4.8 to sage-5.0
  • Reviewers set to John Palmieri
  • Status changed from needs_review to positive_review

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

s = ('abc' 'def')

documented?

comment:17 in reply to: ↑ 16 Changed 8 years ago by 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!

Changed 8 years ago by jdemeyer

apply to $SAGE_ROOT/devel/sage

comment:18 Changed 8 years ago by jdemeyer

Changed commit message.

comment:19 follow-up: Changed 8 years ago by 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.

comment:20 in reply to: ↑ 19 Changed 8 years ago by 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.

comment:21 follow-up: Changed 8 years ago by 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?

comment:22 in reply to: ↑ 21 Changed 8 years ago by 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.

comment:23 Changed 8 years ago by jdemeyer

  • Merged in set to sage-5.0.beta1
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.