Handle floating point round-off error when converting to pixels for h264 animations #8253
Conversation
|
@efiring I believe you originally added the code I'm deleting. Would you mind taking a look? |
lib/matplotlib/animation.py
Outdated
| # 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 |
6b9dd12 to
e0c212b
Compare
|
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. |
lib/matplotlib/animation.py
Outdated
| 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) |
There was a problem hiding this comment.
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).
|
I agree with @efiring , we should handle this on our side by outputting the right sized-frame. |
e0c212b to
f09dd61
Compare
|
@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. |
lib/matplotlib/animation.py
Outdated
| @@ -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) | |||
There was a problem hiding this comment.
This rounds to the nearest inch which is not what we want.
There was a problem hiding this comment.
Ugh, good point!
Unfortunately putting the rounding in frame_size produces a corrupted movie:
|
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. |
Is there a straightforward way to do this as an argument to e.g. |
|
OK, third (fourth?) time's the charm? I think this one is correct. Now we use In other news, I now am very annoyed with floating point! |
|
It seems like the underlying problem here is that the agg backend (or whatever) converts figsize to pixels by doing |
lib/matplotlib/animation.py
Outdated
| @@ -337,6 +347,7 @@ def grab_frame(self, **savefig_kwargs): | |||
| verbose.report('MovieWriter.grab_frame: Grabbing frame.', | |||
| level='debug') | |||
| try: | |||
| self._adjust_frame_size() | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
thanks for the suggestion, makes the intent clearer
2341c61 to
3bdea02
Compare
…e of matplotlib adjusted it in the meantime
3bdea02 to
53865c5
Compare
|
|
||
| # Combine FFMpeg options with pipe-based writing | ||
| @writers.register('ffmpeg') | ||
| class FFMpegWriter(MovieWriter, FFMpegBase): |
lib/matplotlib/animation.py
Outdated
| stdout=subprocess.PIPE, | ||
| stderr=subprocess.PIPE, | ||
| creationflags=subprocess_creation_flags) | ||
| if not cls._handle_subprocess(p): |
There was a problem hiding this comment.
could this just bereturn cls._handle_subprocess(p)?
53865c5 to
926a59f
Compare
|
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 In principle in the future this code can be removed since Ubuntu brought back the real |
|
Hmm, it may soon be time to switch Travis to the trusty builds. If |
lib/matplotlib/animation.py
Outdated
| 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: |
|
+1 for not supporting more than the oldest, still Canonical-supported Ubuntu LTS (and that's only a lower bound :-)). |
|
@dopplershift yeah, moving travis to use a 14.04 image seems like the way to go come April :) |
| except OSError: | ||
| return False | ||
|
|
||
| @classmethod |
There was a problem hiding this comment.
Is there a reason for using @classmethod rather than @staticmethod here, even though the method doesn't use its cls argument?
There was a problem hiding this comment.
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.
|
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. |
a507300 to
5d4f5de
Compare
There was a problem hiding this comment.
Contingent on tests passing.
I added a commit rather than asking @ngoldbaum to make some minor changes.
|
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
|
backported to v2.0.x as cab1abd Sorry this missed 2.0.1 :( |
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 callsset_size_inchesafter matplotlib does) I needed another call to_adjust_frame_sizebefore actually writing each frame. This avoids the crazy corrupted video I saw in the original issue I opened about this.