Handle floating point round-off error when converting to pixels for h264 animations #8253

ngoldbaum · 2017-03-10T00:52:15Z

Rather than handling the rescaling in matplotlib, tell ffmpeg to do the rescaling for us. This avoids issues described in #8250 where the rescaling is susceptible to round-off error. It also allows us to delete some code.

EDIT:

Now let's try rounding the output of adjusted_figsize. In addition I found that to get this to work properly with yt (and I guess in general any other library that calls set_size_inches after matplotlib does) I needed another call to _adjust_frame_size before actually writing each frame. This avoids the crazy corrupted video I saw in the original issue I opened about this.

ngoldbaum · 2017-03-10T01:06:10Z

@efiring I believe you originally added the code I'm deleting. Would you mind taking a look?

QuLogic · 2017-03-10T01:06:37Z

lib/matplotlib/animation.py

+            # The h264 codec requires that frames passed to it must have a
+            # a width and height that are even numbers of pixels. This tells
+            # ffmpeg to rescale the frames to the nearest even number of pixels
+            # see http://stackoverflow.com/a/20848224/1382869


efiring · 2017-03-10T06:23:48Z

I don't think this is a good approach. If I understand correctly, what happens with this approach is that each image is generated by mpl (using Agg) with, for example, 501x301 pixels, and then ffmpeg does a massive interpolation to a 500x300 grid. This undoes whatever pixel-snapping and other operations mpl/agg did that involved knowing what the pixel grid would be. I think it fundamentally degrades the image, and I don't see any benefit to it. In the approach I used, the figure size is tweaked so that the desired pixel grid is produced from the start, and never modified. If there is a problem with the floating-point math, I suspect that can be corrected in some more direct way, but I haven't looked at your example in #8250.

tacaswell · 2017-03-10T17:43:11Z

lib/matplotlib/animation.py

-            wo, ho = self.fig.get_size_inches()
-            w, h = adjusted_figsize(wo, ho, self.dpi, 2)
-            if not (wo, ho) == (w, h):
-                self.fig.set_size_inches(w, h, forward=True)


The fix might be to set forward=False here. Some of the backends will feed-back and mostly go to the size you want (which is another set of things we need to fix, but).

tacaswell · 2017-03-10T17:43:41Z

I agree with @efiring , we should handle this on our side by outputting the right sized-frame.

ngoldbaum · 2017-03-10T21:25:00Z

@efiring @tacaswell ok, I've rewritten this pull request in what is hopefully a more copacetic way. Let me know if you have additional comments.

tacaswell · 2017-03-10T22:31:08Z

lib/matplotlib/animation.py

@@ -62,8 +63,8 @@


 def adjusted_figsize(w, h, dpi, n):
-    wnew = int(w * dpi / n) * n / dpi
-    hnew = int(h * dpi / n) * n / dpi
+    wnew = np.round(int(w * dpi / n) * n / dpi)


This rounds to the nearest inch which is not what we want.

Ugh, good point!

Unfortunately putting the rounding in frame_size produces a corrupted movie:

http://imgur.com/a/CjftT

tacaswell · 2017-03-10T22:34:49Z

If we are having trouble getting this to round trip nicely (and this is a perineal request, the ability to specify the pixel size, not the inch size of the png), then we should fall back to just trimming a single row / column off the image before handing it to ffmpeg.

ngoldbaum · 2017-03-10T23:05:57Z

fall back to just trimming a single row / column off the image before handing it to ffmpeg

Is there a straightforward way to do this as an argument to e.g. savefig? I think I could do it using pillow but not easily using pure matplotlib.

ngoldbaum · 2017-03-10T23:26:26Z

OK, third (fourth?) time's the charm? I think this one is correct. Now we use np.nextafter to correct the results of adjusted_figsize if there's any floating point jitter.

In other news, I now am very annoyed with floating point!

njsmith · 2017-03-11T00:16:25Z

It seems like the underlying problem here is that the agg backend (or whatever) converts figsize to pixels by doing int(width * dpi), and the floating point error + int() truncation introduce some nasty jitter. Rather than trying to model this jitter and correct for it like the PR does right now, maybe it would make more sense to fix the backend code so it computes pixels as round(width * dpi)? This should produce more accurate and predictable results in general, as well as avoiding the super-tricky and fragile code here...

tacaswell · 2017-03-12T19:21:16Z

lib/matplotlib/animation.py

@@ -337,6 +347,7 @@ def grab_frame(self, **savefig_kwargs):
        verbose.report('MovieWriter.grab_frame: Grabbing frame.',
                       level='debug')
        try:
+            self._adjust_frame_size()


Why do we need this?

What if the animation callback function calls set_size_inches? This happens indirectly with any animation using a yt plot when the figure gets updated. yt assumes it can control the figure size, which is a reasonable assumption everywhere else in matplotlib, except for here when it's trying to work around this h264 issue.

This only partially prevents the problem if the callbacks always set the same size. Probably should cache the size (and dpi) in setup and then re-set them here.

thanks for the suggestion, makes the intent clearer

… error

…e of matplotlib adjusted it in the meantime

tacaswell · 2017-03-13T17:47:56Z

lib/matplotlib/animation.py


 # Combine FFMpeg options with pipe-based writing
 @writers.register('ffmpeg')
-class FFMpegWriter(MovieWriter, FFMpegBase):


why change the mro?

nevemind, I see why ❤️ MI.

tacaswell · 2017-03-13T17:50:57Z

lib/matplotlib/animation.py

+                stdout=subprocess.PIPE,
+                stderr=subprocess.PIPE,
+                creationflags=subprocess_creation_flags)
+            if not cls._handle_subprocess(p):


could this just bereturn cls._handle_subprocess(p)?

ngoldbaum · 2017-03-13T19:10:16Z

OK, I think the test I added is finally passing now.

I had to add some extra code to fix an issue that the travis builds on Ubuntu 12.04 ran into. On that version of Ubuntu they shipped an ffmpeg compatibility binary that wraps avconv. This ffmpeg is not fully compatible with the real ffmpeg (for the purposes of this pull request, it doesn't work with the h264 codec), which caused the test I added to fail. The fix for that is to update the logic that searches for ffmpeg to ignore an ffmpeg that is really a wrapper for avconv.

In principle in the future this code can be removed since Ubuntu brought back the real ffmpeg in 16.04 and Ubuntu 12.04 is going EOL very soon.

QuLogic · 2017-03-13T21:22:33Z

Hmm, it may soon be time to switch Travis to the trusty builds. If master is not going to come out for another month at least, then maybe that time is now.

dopplershift

Generally 👍 on the approach. (There almost seems as much effort here to fix tests on 12.04 as there is fixing the actual bug.)

dopplershift · 2017-03-15T17:54:25Z

lib/matplotlib/animation.py

+    if int(x*dpi) % n != 0:
+        if int(np.nextafter(x, np.inf)*dpi) % n == 0:
+            x = np.nextafter(x, np.inf)
+        if int(np.nextafter(x, -np.inf)*dpi) % n == 0:


Not elif?

anntzer · 2017-03-15T17:57:51Z

+1 for not supporting more than the oldest, still Canonical-supported Ubuntu LTS (and that's only a lower bound :-)).

ngoldbaum · 2017-03-15T18:14:47Z

@dopplershift yeah, moving travis to use a 14.04 image seems like the way to go come April :)

efiring · 2017-03-26T23:36:00Z

lib/matplotlib/animation.py

        except OSError:
            return False

+    @classmethod


Is there a reason for using @classmethod rather than @staticmethod here, even though the method doesn't use its cls argument?

No particular reason I think I was under the false impression that can't override a staticmethod in a subclass. In any case I think it makes sense to leave it as is just in case anyone needs access to the data in cls in the future.

efiring · 2017-03-26T23:41:09Z

As a reminder, we still have the open question as to whether consistently rounding rather than truncating (see #8265) would be a good approach instead of, or in addition to, the floating point adjustment done here.

tacaswell

Contingent on tests passing.

I added a commit rather than asking @ngoldbaum to make some minor changes.

ngoldbaum · 2017-04-03T19:20:39Z

Thanks Tom!

Handle floating point round-off error when converting to pixels for h264 animations Conflicts: lib/matplotlib/tests/test_animation.py - conflicts in tests

tacaswell · 2017-05-03T03:15:53Z

backported to v2.0.x as cab1abd

Sorry this missed 2.0.1 :(

QuLogic reviewed Mar 10, 2017

View reviewed changes

ngoldbaum force-pushed the simplify-figsize-adjust branch 2 times, most recently from 6b9dd12 to e0c212b Compare March 10, 2017 01:09

tacaswell reviewed Mar 10, 2017

View reviewed changes

tacaswell added this to the 2.0.1 (next bug fix release) milestone Mar 10, 2017

tacaswell added the topic: animation label Mar 10, 2017

ngoldbaum force-pushed the simplify-figsize-adjust branch from e0c212b to f09dd61 Compare March 10, 2017 21:22

tacaswell reviewed Mar 10, 2017

View reviewed changes

tacaswell mentioned this pull request Mar 11, 2017

FIX: round instead of truncate Agg canvas size #8265

Draft

ngoldbaum changed the title ~~Let ffmpeg handle image rescaling for h264~~ Handle floating point round-off error when converting to pixels for h264 animations Mar 12, 2017

tacaswell reviewed Mar 12, 2017

View reviewed changes

ngoldbaum force-pushed the simplify-figsize-adjust branch from 2341c61 to 3bdea02 Compare March 13, 2017 16:50

Nathan Goldbaum added 4 commits March 13, 2017 12:00

Round the output of adjusted_figsize to avoid floating point roundoff…

d8c6b56

… error

Adjust the frame size before writing to disk in case a library outsid…

feec74d

…e of matplotlib adjusted it in the meantime

use nextafter to correct floating point round-off error

30d5b61

add test for movie writing floating point roundoff issues

08eb434

ngoldbaum force-pushed the simplify-figsize-adjust branch from 3bdea02 to 53865c5 Compare March 13, 2017 17:01

tacaswell reviewed Mar 13, 2017

View reviewed changes

don't attempt to use libav ffmpeg shim on ubuntu 12.04

926a59f

ngoldbaum force-pushed the simplify-figsize-adjust branch from 53865c5 to 926a59f Compare March 13, 2017 18:20

dopplershift reviewed Mar 15, 2017

View reviewed changes

make test case a little clearer

f287ac6

tacaswell added the Release critical For bugs that make the library unusable (segfaults, incorrect plots, etc) and major regressions. label Mar 21, 2017

efiring reviewed Mar 26, 2017

View reviewed changes

DOC/MNT : add docs + make helper function private

5d4f5de

tacaswell force-pushed the simplify-figsize-adjust branch from a507300 to 5d4f5de Compare April 3, 2017 19:18

tacaswell approved these changes Apr 3, 2017

View reviewed changes

efiring merged commit 44fcb87 into matplotlib:master Apr 3, 2017

QuLogic modified the milestones: 2.0.2 (next bug fix release), 2.0.1 (next bug fix release) May 3, 2017

tacaswell modified the milestones: 2.0.3 (next bug fix release), 2.0.2 (critical bug fixes from 2.0.1) May 7, 2017

anntzer mentioned this pull request Jun 11, 2017

Fix 'animation' unable to detect AVConv. #8743

Merged

tacaswell mentioned this pull request Apr 4, 2018

image_stack_to_movie bug in srv1 NSLS-II-CSX/csxtools#62

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Handle floating point round-off error when converting to pixels for h264 animations #8253

Handle floating point round-off error when converting to pixels for h264 animations #8253

ngoldbaum commented Mar 10, 2017 •

edited

Loading

ngoldbaum commented Mar 10, 2017

QuLogic Mar 10, 2017

efiring commented Mar 10, 2017

tacaswell Mar 10, 2017

tacaswell commented Mar 10, 2017

ngoldbaum commented Mar 10, 2017

tacaswell Mar 10, 2017

ngoldbaum Mar 10, 2017

tacaswell commented Mar 10, 2017

ngoldbaum commented Mar 10, 2017

ngoldbaum commented Mar 10, 2017

njsmith commented Mar 11, 2017

tacaswell Mar 12, 2017

ngoldbaum Mar 12, 2017

tacaswell Mar 12, 2017

ngoldbaum Mar 12, 2017

tacaswell Mar 13, 2017

tacaswell Mar 13, 2017

tacaswell Mar 13, 2017

ngoldbaum commented Mar 13, 2017

QuLogic commented Mar 13, 2017 •

edited

Loading

dopplershift left a comment

dopplershift Mar 15, 2017

anntzer commented Mar 15, 2017

ngoldbaum commented Mar 15, 2017

efiring Mar 26, 2017

ngoldbaum Mar 28, 2017

efiring commented Mar 26, 2017

tacaswell left a comment

ngoldbaum commented Apr 3, 2017

tacaswell commented May 3, 2017

Handle floating point round-off error when converting to pixels for h264 animations #8253

Handle floating point round-off error when converting to pixels for h264 animations #8253

Conversation

ngoldbaum commented Mar 10, 2017 • edited Loading

ngoldbaum commented Mar 10, 2017

Choose a reason for hiding this comment

efiring commented Mar 10, 2017

Choose a reason for hiding this comment

tacaswell commented Mar 10, 2017

ngoldbaum commented Mar 10, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tacaswell commented Mar 10, 2017

ngoldbaum commented Mar 10, 2017

ngoldbaum commented Mar 10, 2017

njsmith commented Mar 11, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ngoldbaum commented Mar 13, 2017

QuLogic commented Mar 13, 2017 • edited Loading

dopplershift left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anntzer commented Mar 15, 2017

ngoldbaum commented Mar 15, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

efiring commented Mar 26, 2017

tacaswell left a comment

Choose a reason for hiding this comment

ngoldbaum commented Apr 3, 2017

tacaswell commented May 3, 2017

ngoldbaum commented Mar 10, 2017 •

edited

Loading

QuLogic commented Mar 13, 2017 •

edited

Loading