Show animation

[vbraun]

Bandaid until we can implement a proper fix at #17783

CC: @novoselt @gagern

Component: graphics

Author: Martin von Gagern

Branch/Commit: 9a48ae0

Reviewer: Volker Braun

Issue created by migration from https://trac.sagemath.org/ticket/18176

[vbraun]

Author: Volker Braun

[vbraun]

Branch: u/vbraun/show_animation

[vbraun]

New commits:

39263a4

Remove optional arguments to show since its all broken

[vbraun]

Commit: 39263a4

[kcrisman]

comment:4

Blocker?

Anyway, were the optional arguments previously broken, had no one tested with the optional tag, or was this a recent change? Andrey's email suggests recent...

[vbraun]

comment:5

The code is only tested with --optional=imagemagick, so in other words it was not tested.

[vbraun]

comment:6

And I don't have a particular preference for whether to make it a blocker or not. If somebody cares about animations then they can always review this ticket before sage-6.6 is out (i.e. very soon)

[kcrisman]

comment:8

Given that Andrey may not have had a chance to respond about this yet, I'm moving it to blocker until either I get a chance to review it (though all seems straightforward) or he can say it's not that high. a works, just not a.show. Does it seems like Martin's changes or the display changes are more likely to have caused this?

[novoselt]

comment:9

I don't know when it got broken - I was thinking I should check how animations work in SageMathCell with my recent changes and discovered they don't work at all in Sage.

The real problem is that they were not even tested.

And my concern with changes on this ticket is that it will break a.show(delay=10) in user code.

But if it is up to me to decide, this is not a blocker.

[vbraun]

comment:10

I tried to preserve the delay parameter, but then I don't even know what its supposed to mean. Sometimes its the numerator of some fraction (with a separate delay_denominator. Sometimes its centiseconds, because thats super-convenient right. Never mind why you would ever pass a fraction as separate numerator/denominator in Sage. IMHO it should be the reciprocal anyways, fps = frames per second. That is much more common when talking about animation speeds.

[gagern]

comment:11

What exactly do you mean by “its all broken”? It would be kind of nice if the problem report were to report the problem, before suggesting a fix… In any case, I'm on 6.6.beta1 here, using the Sage notebook, and with imagemagick 6.9.0.3 installed on my system. And animations with delay and iterations do work for me. To be more specific,

sage: a = animate([sin(x + float(k)) for k in srange(0,2*pi,0.3)], xmin=0, xmax=2*pi, figsize=[2,1])
sage: a.show(delay=10, iterations=2) # shows at higher frame rate then stops
sage: a.show(delay=500)              # shows at lower frame rate

So what exactly is broken?

[gagern]

comment:12

Replying to @vbraun:

I tried to preserve the delay parameter, but then I don't even know what its supposed to mean.

The doc says correctly:

• delay between frames (measured in hundredths of a second, default value 20)
• (default: 20) delay in hundredths of a second between frames

This is used throughout the public Animation class. You might argue that it's a suboptimal choice, but it's what the GIF spec uses, what most animated GIF editors use, and what Sage has been using so far.

Sometimes its the numerator of some fraction (with a separate delay_denominator. Sometimes its centiseconds, because thats super-convenient right. Never mind why you would ever pass a fraction as separate numerator/denominator in Sage.

That's in the APngAssembler I wrote, and it's there that way because I consider the APngAssembler a low-level interface, and the low level spec of APNG uses that representation for rational numbers. I saw no reason to complicate things further by introducing my own translation layer, and I didn't consider delay_denominator harmful enough to fix it at 100 instead of leaving the caller an option to override this.

IMHO it should be the reciprocal anyways, fps = frames per second. That is much more common when talking about animation speeds.

For movies, yes. For animated GIFs, in my experience no. I wouldn't mind having fps as an additional argument, so users can choose between specifying that or delay. But I'd not kill off the delay argument without proper deprecation. And don't you dare call this spacebar heating!

[gagern]

comment:13

I notice that delay and iterations only have limited support from ffmpeg. The docs say as much. What does sage.misc.sage_ostools.have_program("convert") report for you?

[vbraun]

comment:14

Replying to @gagern:

it's what the GIF spec uses, what most animated GIF editors use

By the same line of argument I suggest we replace seconds everywhere with jiffies, since this is what the kernel developers use. If you go to the movies, whats the frame delay in centiseconds? How many frames per second? ;-)

What is broken:

• testing (0% coverage unless you specifically set --optional)
• save doesn't respect delay, iterations (show goes through save)

[vbraun]

comment:15

And I do have imagemagick installed, the tests are just not run by default (See also #13540)

[gagern]

comment:16

When was the last time you had a Sage-generated movie play at a cinema in your parts? On a more serious note, I believe that delay makes a lot of sense if one speaks of a number associated to individual frames, but less so if it's the same for all of them. Which is a reason why delay makes more sense in a GIF environment than an MPEG one.

show does go through save only since your commit 7747261. It was calling gif before that. In dc0067d for #7298 I had written a code path which routed show through save without breaking such stuff. Unfortunately, #7298 won't merge since it conflicts with your changes, and I haven't yet found the time to rewrite it appropriately.

Test coverage is tricky. Since we don't bundle imagemagick, we can't rely on convert so we can't create GIF. The same holds for ffmpeg and the various video formats. And even if we had these tools enabled, how do we automatically verify that the file looks as it should? I guess we could disassemble the file to some textual representation of all its settings, but that would require considerable amounts of code. Or we could set up something like Selenium based tests. Although it's hard to get timing right with those. But I'm against punting a useful feature just because we can't auto-test it as well as we'd like.

[vbraun]

comment:17

Test coverage should be easy since PIL(low) can be used to write animated GIF, we just don't use it.

In any case, not every optional argument for every potential image file format needs to be exposed via show()

[vbraun]

comment:18

Animations will be totally broken in Sage-6.6 since apparently that is preferable to not supporting some optional keyword argument. Propose to close this as wontfix until we can rewrite the animations so that no keyword is left behind.

[kcrisman]

comment:19

Animations will be totally broken in Sage-6.6 since apparently that is preferable to not supporting some optional keyword argument. Propose to close this as wontfix until we can rewrite the animations so that no keyword is left behind.

I don't have any objection (well, not enough to break animations) to this temp fix, I just don't have time to properly do a review. As I say,

all seems straightforward

so please let's make animations still work. Or at least, animation show method, I guess without show it still works, correct?

[kcrisman]

comment:20

Animations will be totally broken in Sage-6.6 since apparently that is preferable to not supporting some optional keyword argument. Propose to close this as wontfix until we can rewrite the animations so that no keyword is left behind.

I don't have any objection (well, not enough to break animations) to this temp fix, I just don't have time to properly do a review. As I say,

Actually, I will just make time.

[kcrisman]

comment:22

Okay, this is weird. I get it working in the command line now, but

**********************************************************************
File "src/sage/plot/animate.py", line 34, in sage.plot.animate
Failed example:
    a         # optional -- ImageMagick
Exception raised:
      File "<doctest sage.plot.animate[0]>", line 1, in <module>
        a         # optional -- ImageMagick
    NameError: name 'a' is not defined
**********************************************************************

when running tests. In fact, I get this error while doctesting whether or not I apply this! Command

$ ./sage -tp 4 --optional=imagemagick src/sage/plot/

[vbraun]

comment:23

I know, the tests with enabling optional stuff are generally broken.

[kcrisman]

comment:24

Oh, duh! I have to do all or sage as well - http://www.sagemath.org/doc/developer/doctesting.html#run-optional-tests. Sorry, my bad.

[kcrisman]

comment:25

Give me another hour - I have to run an time-constrained errand right now but will return to this shortly.

[gagern]

Changed branch from u/vbraun/show_animation to u/gagern/t/17783/animationSaveKwds

[gagern]

Changed commit from 39263a4 to 14dd281

[gagern]

Changed author from Volker Braun to Martin von Gagern

[gagern]

comment:27

Here is a branch which preserves the functionality of keyword arguments to show by passing them on through save to gif. It even has doctests to ensure that the delay and iteration count actually end up in the created file. And I fixed one other doctest failure introduced by 7747261. Now things work for me in the Sage Notebook, and sage -bt --long --optional=all src/sage/plot/animate.py is happy as well. Please review.

---

New commits:

e56fbea	Trac #18176: Pass keywords from show through save to gif resp. ffmpeg
14dd281	Don't use file name argument to show method

[vbraun]

Reviewer: Volker Braun

[vbraun]

Changed branch from u/gagern/t/17783/animationSaveKwds to 14dd281

[kcrisman]

Changed commit from 14dd281 to none

[kcrisman]

comment:30

Wow, you pick up your kid, and the world keeps rotating ;-) Nice!!!

[vbraun]

comment:31

[plotting ] /mnt/disk/home/buildslave-sage/slave/sage_git/build/local/lib/python2.7/site-packages/sage/plot/animate.py:docstring of sage.plot.animate.Animation.save:49: WARNING: Block quote ends without a blank line; unexpected unindent.
Error building the documentation.
Traceback (most recent call last):
  File "/mnt/disk/home/buildslave-sage/slave/sage_git/build/src/doc/common/builder.py", line 1626, in <module>
    getattr(get_builder(name), type)()
  File "/mnt/disk/home/buildslave-sage/slave/sage_git/build/src/doc/common/builder.py", line 292, in _wrapper
    getattr(get_builder(document), 'inventory')(*args, **kwds)
  File "/mnt/disk/home/buildslave-sage/slave/sage_git/build/src/doc/common/builder.py", line 503, in _wrapper
    x.get(99999)
  File "/mnt/disk/home/buildslave-sage/slave/sage_git/build/local/lib/python/multiprocessing/pool.py", line 558, in get
    raise self._value
OSError: [plotting ] /mnt/disk/home/buildslave-sage/slave/sage_git/build/local/lib/python2.7/site-packages/sage/plot/animate.py:docstring of sage.plot.animate.Animation.save:49: WARNING: Block quote ends without a blank line; unexpected unindent.

[gagern]

comment:32

Replying to @vbraun:

Test coverage should be easy since PIL(low) can be used to write animated GIF, we just don't use it.

It can? A quick search of the web returned several posts where people talked about animated GIF read support, or the absence of write support, or asked about write support and got no answer. If you are certain about this, and perhaps even know an address with some documentation, could you open a ticket for this?

In any case, not every optional argument for every potential image file format needs to be exposed via show()

We had that discussion in #7298 comment:50 and following, and I still disagree. Let's continue this aspect in this thread on the sage-devel list, so we can get more opinons on this.

[gagern]

Changed branch from 14dd281 to u/gagern/14dd2818961df94bafdc2e2d1f2b640d3a68d5ae

[gagern]

Changed branch from u/gagern/14dd2818961df94bafdc2e2d1f2b640d3a68d5ae to u/gagern/t/17783/animationSaveKwds

[gagern]

Commit: 9a48ae0

[gagern]

comment:35

Just forgot to use r""" for the docstring once I started using escape sequences in string literals within that docstring.

---

New commits:

e56fbea	Trac #18176: Pass keywords from show through save to gif resp. ffmpeg
14dd281	Don't use file name argument to show method
9a48ae0	Use raw mode for docstring

[vbraun]

Changed branch from u/gagern/t/17783/animationSaveKwds to 9a48ae0