API: forbid unsafe settings and keywords in AbstractMovieWriter.grab_frame

Several of the keyword arguments (dpi, bbox_inches) to savefig will change the
dimensions of the rendered figure or byte output (format).  Changing these
mid-stream in generating a movie will in a best case result in an animation that
jumps around and in a worst case silently generate a completely corrupted
movie.

The rcparam savefig.bbox can have the same effect.

The low level AbstractMovieWriter.grab_frame is now strict and will error if
any possibly unsafe conditions are met.

closes matplotlib#25608

Co-authored-by: Elliott Sales de Andrade <[email protected]>

Author: tacaswell

doc/api/next_api_changes/behavior/25361-TAC.rst

@@ -0,0 +1,18 @@
+Reject size related keyword arguments to MovieWriter *grab_frame* method
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+
+Although we pass `.Figure.savefig` keyword arguments through the
+`.AbstractMovieWriter.grab_frame` some of the arguments will result in invalid
+output if passed.  To be successfully stitched into a movie, each frame
+must be exactly the same size, thus *bbox_inches* and *dpi* are excluded.
+Additionally, the movie writers are opinionated about the format of each
+frame, so the *format* argument is also excluded.  Passing these
+arguments will now raise `TypeError` for all writers (it already did so for some
+arguments and some writers).  The *bbox_inches* argument is already ignored (with
+a warning) if passed to `.Animation.save`.
+
+
+Additionally, if :rc:`savefig.bbox` is set to ``'tight'``,
+`.AbstractMovieWriter.grab_frame` will now error.  Previously this rcParam
+would be temporarily overridden (with a warning) in `.Animation.save`, it is
+now additionally overridden in `.AbstractMovieWriter.saving`.

lib/matplotlib/animation.py

@@ -213,7 +213,13 @@ def grab_frame(self, **savefig_kwargs):
         Grab the image information from the figure and save as a movie frame.
 
         All keyword arguments in *savefig_kwargs* are passed on to the
-        `~.Figure.savefig` call that saves the figure.
+        `~.Figure.savefig` call that saves the figure.  However, several
+        keyword arguments that are supported by `~.Figure.savefig` may not be
+        passed as they are controlled by the MovieWriter:
+
+        - *dpi*, *bbox_inches*:  These may not be passed because each frame of the
+           animation much be exactly the same size in pixels.
+        - *format*: This is controlled by the MovieWriter.
         """
 
     @abc.abstractmethod
@@ -351,6 +357,7 @@ def finish(self):
 
     def grab_frame(self, **savefig_kwargs):
         # docstring inherited
+        _validate_grabframe_kwargs(savefig_kwargs)
         _log.debug('MovieWriter.grab_frame: Grabbing frame.')
         # Readjust the figure size in case it has been changed by the user.
         # All frames must have the same size to save the movie correctly.
@@ -457,6 +464,7 @@ def _base_temp_name(self):
     def grab_frame(self, **savefig_kwargs):
         # docstring inherited
         # Creates a filename for saving using basename and counter.
+        _validate_grabframe_kwargs(savefig_kwargs)
         path = Path(self._base_temp_name() % self._frame_counter)
         self._temp_paths.append(path)  # Record the filename for later use.
         self._frame_counter += 1  # Ensures each created name is unique.
@@ -491,6 +499,7 @@ def setup(self, fig, outfile, dpi=None):
         self._frames = []
 
     def grab_frame(self, **savefig_kwargs):
+        _validate_grabframe_kwargs(savefig_kwargs)
         buf = BytesIO()
         self.fig.savefig(
             buf, **{**savefig_kwargs, "format": "rgba", "dpi": self.dpi})
@@ -747,6 +756,7 @@ def setup(self, fig, outfile, dpi=None, frame_dir=None):
         self._clear_temp = False
 
     def grab_frame(self, **savefig_kwargs):
+        _validate_grabframe_kwargs(savefig_kwargs)
         if self.embed_frames:
             # Just stop processing if we hit the limit
             if self._hit_limit:
@@ -1776,3 +1786,16 @@ def _draw_frame(self, framedata):
                 a.set_animated(self._blit)
 
     save_count = _api.deprecate_privatize_attribute("3.7")
+
+
+def _validate_grabframe_kwargs(savefig_kwargs):
+    if mpl.rcParams['savefig.bbox'] == 'tight':
+        raise ValueError(
+            f"{mpl.rcParams['savefig.bbox']=} must not be 'tight' as it "
+            "may cause frame size to vary, which is inappropriate for animation."
+        )
+    for k in ('dpi', 'bbox_inches', 'format'):
+        if k in savefig_kwargs:
+            raise TypeError(
+                f"grab_frame got an unexpected keyword argument {k!r}"
+            )

lib/matplotlib/tests/test_animation.py

@@ -62,6 +62,8 @@ def setup(self, fig, outfile, dpi, *args):
         self._count = 0
 
     def grab_frame(self, **savefig_kwargs):
+        from matplotlib.animation import _validate_grabframe_kwargs
+        _validate_grabframe_kwargs(savefig_kwargs)
         self.savefig_kwargs = savefig_kwargs
         self._count += 1
 
@@ -193,6 +195,38 @@ def test_save_animation_smoketest(tmpdir, writer, frame_format, output, anim):
     del anim
 
 
+@pytest.mark.parametrize('writer, frame_format, output', gen_writers())
+def test_grabframe(tmpdir, writer, frame_format, output):
+    WriterClass = animation.writers[writer]
+
+    if frame_format is not None:
+        plt.rcParams["animation.frame_format"] = frame_format
+
+    fig, ax = plt.subplots()
+
+    dpi = None
+    codec = None
+    if writer == 'ffmpeg':
+        # Issue #8253
+        fig.set_size_inches((10.85, 9.21))
+        dpi = 100.
+        codec = 'h264'
+
+    test_writer = WriterClass()
+    # Use temporary directory for the file-based writers, which produce a file
+    # per frame with known names.
+    with tmpdir.as_cwd():
+        with test_writer.saving(fig, output, dpi):
+            # smoke test it works
+            test_writer.grab_frame()
+            for k in {'dpi', 'bbox_inches', 'format'}:
+                with pytest.raises(
+                        TypeError,
+                        match=f"grab_frame got an unexpected keyword argument {k!r}"
+                ):
+                    test_writer.grab_frame(**{k: object()})
+
+
 @pytest.mark.parametrize('writer', [
     pytest.param(
         'ffmpeg', marks=pytest.mark.skipif(