Skip to content

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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 7 commits into from
Apr 3, 2017
72 changes: 61 additions & 11 deletions lib/matplotlib/animation.py
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
import six
from six.moves import xrange, zip

import numpy as np
import os
import platform
import sys
Expand Down Expand Up @@ -62,9 +63,38 @@


def adjusted_figsize(w, h, dpi, n):
'''Compute figure size so that pixels are a multiple of n

Parameters
----------
w, h : float
Size in inches

dpi : float
The dpi

n : int
The target multiple

Returns
-------
wnew, hnew : float
The new figure size in inches.
'''

# this maybe simplified if / when we adopt consistent rounding for
# pixel size across the whole library
def correct_roundoff(x, dpi, n):
if int(x*dpi) % n != 0:
if int(np.nextafter(x, np.inf)*dpi) % n == 0:
x = np.nextafter(x, np.inf)
elif int(np.nextafter(x, -np.inf)*dpi) % n == 0:
x = np.nextafter(x, -np.inf)
return x

wnew = int(w * dpi / n) * n / dpi
hnew = int(h * dpi / n) * n / dpi
return wnew, hnew
return (correct_roundoff(wnew, dpi, n), correct_roundoff(hnew, dpi, n))


# A registry for available MovieWriter classes
Expand Down Expand Up @@ -278,8 +308,11 @@ def _adjust_frame_size(self):
verbose.report('figure size (inches) has been adjusted '
'from %s x %s to %s x %s' % (wo, ho, w, h),
level='helpful')
else:
w, h = self.fig.get_size_inches()
verbose.report('frame size in pixels is %s x %s' % self.frame_size,
level='debug')
return w, h

def setup(self, fig, outfile, dpi=None):
'''
Expand All @@ -301,7 +334,7 @@ def setup(self, fig, outfile, dpi=None):
if dpi is None:
dpi = self.fig.dpi
self.dpi = dpi
self._adjust_frame_size()
self._w, self._h = self._adjust_frame_size()

# Run here so that grab_frame() can write the data to a pipe. This
# eliminates the need for temp files.
Expand Down Expand Up @@ -337,6 +370,10 @@ def grab_frame(self, **savefig_kwargs):
verbose.report('MovieWriter.grab_frame: Grabbing frame.',
level='debug')
try:
# re-adjust the figure size in case it has been changed by the
# user. We must ensure that every frame is the same size or
# the movie will not save correctly.
self.fig.set_size_inches(self._w, self._h)
# Tell the figure to save its data to the sink, using the
# frame format and dpi.
self.fig.savefig(self._frame_sink(), format=self.frame_format,
Expand Down Expand Up @@ -386,16 +423,21 @@ def isAvailable(cls):
if not bin_path:
return False
try:
p = subprocess.Popen(bin_path,
shell=False,
stdout=subprocess.PIPE,
stderr=subprocess.PIPE,
creationflags=subprocess_creation_flags)
p.communicate()
return True
p = subprocess.Popen(
bin_path,
shell=False,
stdout=subprocess.PIPE,
stderr=subprocess.PIPE,
creationflags=subprocess_creation_flags)
return cls._handle_subprocess(p)
except OSError:
return False

@classmethod
Copy link
Member

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 its cls argument?

Copy link
Contributor Author

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 in cls in the future.

def _handle_subprocess(cls, process):
process.communicate()
return True


class FileMovieWriter(MovieWriter):
'''`MovieWriter` for writing to individual files and stitching at the end.
Expand Down Expand Up @@ -570,10 +612,18 @@ def output_args(self):

return args + ['-y', self.outfile]

@classmethod
def _handle_subprocess(cls, process):
_, err = process.communicate()
# Ubuntu 12.04 ships a broken ffmpeg binary which we shouldn't use
if 'Libav' in err.decode():
return False
return True


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

Frames are streamed directly to ffmpeg via a pipe and written in a single
Expand All @@ -594,7 +644,7 @@ def _args(self):

# Combine FFMpeg options with temp file-based writing
@writers.register('ffmpeg_file')
class FFMpegFileWriter(FileMovieWriter, FFMpegBase):
class FFMpegFileWriter(FFMpegBase, FileMovieWriter):
'''File-based ffmpeg writer.

Frames are written to temporary files on disk and then stitched
Expand Down
11 changes: 10 additions & 1 deletion lib/matplotlib/tests/test_animation.py
Original file line number Diff line number Diff line change
Expand Up @@ -145,6 +145,14 @@ def test_save_animation_smoketest(tmpdir, writer, extension):
ax.set_xlim(0, 10)
ax.set_ylim(-1, 1)

dpi = None
codec = None
if writer == 'ffmpeg':
# Issue #8253
fig.set_size_inches((10.85, 9.21))
dpi = 100.
codec = 'h264'

def init():
line.set_data([], [])
return line,
Expand All @@ -160,7 +168,8 @@ def animate(i):
with tmpdir.as_cwd():
anim = animation.FuncAnimation(fig, animate, init_func=init, frames=5)
try:
anim.save('movie.' + extension, fps=30, writer=writer, bitrate=500)
anim.save('movie.' + extension, fps=30, writer=writer, bitrate=500,
dpi=dpi, codec=codec)
except UnicodeDecodeError:
pytest.xfail("There can be errors in the numpy import stack, "
"see issues #1891 and #2679")
Expand Down