Fix 'freeze' FX not freezing at the end and add proper tests (#1461)

* Fixed 'freeze' FX not freezing at the end with 't=end' argument

* Improve CHANGELOG entry

* Fix tests for 'freeze' FX and handle inconsistency of arguments

* Improve comments

* Add more test combinations

* Improve tests ids

CHANGELOG.md

@@ -33,6 +33,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 - Fixed positioning error generating frames in `CompositeVideoClip` [\#1420](https://github.com/Zulko/moviepy/pull/1420)
 - Changed deprecated `tostring` method by `tobytes` in `video.io.gif_writers::write_gif` [\#1429](https://github.com/Zulko/moviepy/pull/1429)
 - Fixed calling `audio_normalize` on a clip with no sound causing `ZeroDivisionError` [\#1401](https://github.com/Zulko/moviepy/pull/1401)
+- Fixed `freeze` FX was freezing at time minus 1 second as the end [\#1461](https://github.com/Zulko/moviepy/pull/1461)
 
 
 ## [v2.0.0.dev2](https://github.com/zulko/moviepy/tree/v2.0.0.dev2) (2020-10-05)

moviepy/video/fx/freeze.py

@@ -7,17 +7,21 @@ def freeze(clip, t=0, freeze_duration=None, total_duration=None, padding_end=0):
     """Momentarily freeze the clip at time t.
 
     Set `t='end'` to freeze the clip at the end (actually it will freeze on the
-    frame at time clip.duration - padding_end seconds).
-    With ``duration``you can specify the duration of the freeze.
+    frame at time clip.duration - padding_end seconds - 1 / clip_fps).
+    With ``duration`` you can specify the duration of the freeze.
     With ``total_duration`` you can specify the total duration of
     the clip and the freeze (i.e. the duration of the freeze is
-    automatically calculated). One of them must be provided.
+    automatically computed). One of them must be provided.
     """
 
     if t == "end":
-        t = clip.duration - padding_end - 1
+        t = clip.duration - padding_end - 1 / clip.fps
 
     if freeze_duration is None:
+        if total_duration is None:
+            raise ValueError(
+                "You must provide either 'freeze_duration' or 'total_duration'"
+            )
         freeze_duration = total_duration - clip.duration
 
     before = [clip.subclip(0, t)] if (t != 0) else []

tests/test_fx.py

@@ -202,24 +202,127 @@ def test_fadeout():
     close_all_clips(locals())
 
 
-def test_freeze():
-    clip = BitmapClip([["R"], ["G"], ["B"]], fps=1)  # 3 separate frames
+@pytest.mark.parametrize(
+    (
+        "t",
+        "freeze_duration",
+        "total_duration",
+        "padding_end",
+        "output_frames",
+    ),
+    (
+        # at start, 1 second (default t == 0)
+        (
+            None,
+            1,
+            None,
+            None,
+            ["R", "R", "G", "B"],
+        ),
+        # at start, 1 second (explicit t)
+        (
+            0,
+            1,
+            None,
+            None,
+            ["R", "R", "G", "B"],
+        ),
+        # at end, 1 second
+        (
+            "end",
+            1,
+            None,
+            None,
+            ["R", "G", "B", "B"],
+        ),
+        # at end 1 second, padding end 1 second
+        (
+            "end",
+            1,
+            None,
+            1,
+            ["R", "G", "G", "B"],
+        ),
+        # at 2nd frame, 1 second
+        (
+            1,  # second 0 is frame 1, second 1 is frame 2...
+            1,
+            None,
+            None,
+            ["R", "G", "G", "B"],
+        ),
+        # at 2nd frame, 2 seconds
+        (
+            1,
+            2,
+            None,
+            None,
+            ["R", "G", "G", "G", "B"],
+        ),
+        # `freeze_duration`, `total_duration` are None
+        (1, None, None, None, ValueError),
+        # `total_duration` 5 at start (2 seconds)
+        (None, None, 5, None, ["R", "R", "R", "G", "B"]),
+        # total duration 5 at end
+        ("end", None, 5, None, ["R", "G", "B", "B", "B"]),
+        # total duration 5 padding end
+        ("end", None, 5, 1, ["R", "G", "G", "G", "B"]),
+    ),
+    ids=[
+        "at start, 1 second (default t == 0)",
+        "at start, 1 second (explicit t)",
+        "at end, 1 second",
+        "at end 1 second, padding end 1 second",
+        "at 2nd frame, 1 second",
+        "at 2nd frame, 2 seconds",
+        "`freeze_duration`, `total_duration` are None",
+        "`total_duration` 5 at start (2 seconds)",
+        "`total_duration` 5 at end",
+        "`total_duration` 5 padding end",
+    ],
+)
+def test_freeze(t, freeze_duration, total_duration, padding_end, output_frames):
+    input_frames = ["R", "G", "B"]
+    clip_duration = len(input_frames)
 
-    clip1 = freeze(clip, t=1, freeze_duration=1)
-    target1 = BitmapClip([["R"], ["G"], ["G"], ["B"]], fps=1)
-    assert clip1 == target1
+    # create BitmapClip with predefined set of colors, during 1 second each one
+    clip = BitmapClip([list(color) for color in input_frames], fps=1).with_duration(
+        clip_duration
+    )
 
-    clip2 = freeze(clip, t="end", freeze_duration=1)
-    target2 = BitmapClip([["R"], ["G"], ["B"], ["B"]], fps=1)
-    assert clip2 == target2
+    # build kwargs passed to `freeze`
+    possible_kwargs = {
+        "t": t,
+        "freeze_duration": freeze_duration,
+        "total_duration": total_duration,
+        "padding_end": padding_end,
+    }
+    kwargs = {
+        kw_name: kw_value
+        for kw_name, kw_value in possible_kwargs.items()
+        if kw_value is not None
+    }
 
-    clip3 = freeze(clip, t=1, total_duration=4)
-    target3 = BitmapClip([["R"], ["G"], ["G"], ["B"]], fps=1)
-    assert clip3 == target3
+    # freeze clip
+    if hasattr(output_frames, "__traceback__"):
+        with pytest.raises(output_frames):
+            freeze(clip, **kwargs)
+        return
+    else:
+        freezed_clip = freeze(clip, **kwargs)
+
+    # assert new duration
+    expected_freeze_duration = (
+        freeze_duration
+        if freeze_duration is not None
+        else total_duration - clip_duration
+    )
+    assert freezed_clip.duration == clip_duration + expected_freeze_duration
 
-    clip4 = freeze(clip, t="end", total_duration=4, padding_end=1)
-    target4 = BitmapClip([["R"], ["G"], ["G"], ["B"]], fps=1)
-    assert clip4 == target4
+    # assert colors are the expected
+    for i, color in enumerate(freezed_clip.iter_frames()):
+        expected_color = list(BitmapClip.DEFAULT_COLOR_DICT[output_frames[i]])
+        assert list(color[0][0]) == expected_color
 
 
 def test_freeze_region():
@@ -235,8 +338,6 @@ def test_freeze_region():
     target2 = BitmapClip([["BBB", "DDD"], ["BBR", "DDD"], ["BBC", "DDD"]], fps=1)
     assert clip2 == target2
 
-    pass
-
 
 def test_gamma_corr():
     pass