Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork7.9k
Handle floating point round-off error when converting to pixels for h264 animations#8253
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
https:
6b9dd12 toe0c212bCompareI 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
The fix might be to setforward=False here. Some of the backends will feed-back andmostly 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 tof09dd61Compare@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This rounds to the nearest inch which is not what we want.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Ugh, good point!
Unfortunately putting the rounding inframe_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! |
njsmith commentedMar 11, 2017
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Why do we need this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
What if the animation callback function callsset_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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
thanks for the suggestion, makes the intent clearer
2341c61 to3bdea02Compare…e of matplotlib adjusted it in the meantime
3bdea02 to53865c5Compare| # Combine FFMpeg options with pipe-based writing | ||
| @writers.register('ffmpeg') | ||
| class FFMpegWriter(MovieWriter, FFMpegBase): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
why change the mro?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
nevemind, I see why ❤️ MI.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
could this just bereturn cls._handle_subprocess(p)?
53865c5 to926a59fCompareOK, 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 |
QuLogic commentedMar 13, 2017 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Hmm, it may soon be time to switch Travis to the trusty builds. If |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Generally 👍 on the approach. (There almost seems as much effort here to fix tests on 12.04 as there is fixing the actual bug.)
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: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Notelif?
+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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Is there a reason for using@classmethod rather than@staticmethod here, even though the method doesn't use itscls argument?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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 incls 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 to5d4f5deCompareThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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 animationsConflicts:lib/matplotlib/tests/test_animation.py - conflicts in tests
backported to v2.0.x ascab1abd Sorry this missed 2.0.1 :( |
Uh oh!
There was an error while loading.Please reload this page.
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.