Repository: Zulko/moviepy

Issue:
'coreader' methods of 'VideoFileClip' and 'AudioFileClip' are buggy
These methods were created at #1332 and https://github.com/Zulko/moviepy/commit/fdc51d29b7ce2f3b2c4d0d907c1c62b5da168128, but IMHO I think that are a misunderstanding of how `copy.copy` and `copy.deepcopy` works in Python.

- `coreader` methods currently does a shallow copy (like `copy.copy`) but only taking into account `filename` parameter.
- `Clip.copy` method is working like `copy.deepcopy`, but even for `AudioClip` the `mask` and `audio` attributes are checked in the instance.

### Actual Behavior

The `coreader` functionality is not working. It's implemented as `return AudioFileClip(self.filename, self.buffersize)`, but `AudioFileClip` accepts `buffersize` as a kwarg argument, so it's not taking the `buffersize`. The same applies to `VideoFileClip`.

The documentation of `coreader` methods are very poor, what are they supposed to do? I think that should be added an example, if they are really implemented and are not bad `copy.copy`-like code.

#### Steps to Reproduce the Problem

```python
>>> from moviepy.editor import *
>>> clip = AudioFileClip('media/crunching.mp3', buffersize=1000)
>>> clip_2 = clip.coreader()
>>> clip.buffersize
1000
>>> clip_2.buffersize
200000
```

### Expected behaviour

I'm not sure, are a failed copy, I'm thinking in two possible cases:

- New instance with only filename as the same argument
- New instance with filename and buffersize as same arguments 
  
I suspect that are just weird copy code, and `copy` method should be used instead (ovewriting it in `AudioClip` to because a `deepcopy` in those are not needed).

### Specifications

  - Python Version: 3.8.5
  - Moviepy Version: master
  - Platform Name: Ubuntu
  - Platform Version: 20.04


Discussion:
tburrows13: I fixed the changelog, so feel free to merge when you like.
coveralls: 
[![Coverage Status](https://coveralls.io/builds/36369481/badge)](https://coveralls.io/builds/36369481)

Coverage decreased (-0.1%) to 66.04% when pulling **8fce2c8a29bad99202db1e8ec183295446ea5b07 on mondeja:copy-methods** into **727cbca9c765833ebb3bd2ec26f337eaa98a3484 on Zulko:master**.

mondeja: Blocked until #1453 merges.

Patch:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index ba00923..a3fa0f5 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -12,7 +12,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 ### Important Announcements
 
 ### Added <!-- for new features -->
-- `VideoFileClip.coreader()` that creates a new clip created from the same source file as the original (to complement the existing `AudioFileClip.coreader()`) [\#1332](https://github.com/Zulko/moviepy/pull/1332)
+- Support for `copy.copy(clip)` and `copy.deepcopy(clip)` with same behaviour as `clip.copy()` [\#1442](https://github.com/Zulko/moviepy/pull/1442)
 - `audio.fx.multiply_stereo_volume` to control volume by audio channels [\#1424](https://github.com/Zulko/moviepy/pull/1424)
 
 ### Changed <!-- for changes in existing functionality -->
@@ -24,6 +24,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 - `moviepy.video.fx.all` and `moviepy.audio.fx.all`. Use the fx method directly from the clip instance or import the fx function from `moviepy.video.fx` and `moviepy.audio.fx`. [\#1105](https://github.com/Zulko/moviepy/pull/1105)
 
 ### Removed <!-- for now removed features -->
+- `VideoFileClip.coreader` and `AudioFileClip.coreader` methods removed. Use `VideoFileClip.copy` and `AudioFileClip.copy` instead [\#1442](https://github.com/Zulko/moviepy/pull/1442)
 - `audio.fx.audio_loop` removed. Use `video.fx.loop` instead for all types of clip [\#1451](https://github.com/Zulko/moviepy/pull/1451)
 
 ### Fixed <!-- for any bug fixes -->
diff --git a/docs/getting_started/efficient_moviepy.rst b/docs/getting_started/efficient_moviepy.rst
index 5642ee0..3342501 100644
--- a/docs/getting_started/efficient_moviepy.rst
+++ b/docs/getting_started/efficient_moviepy.rst
@@ -32,7 +32,6 @@ However, if you close a clip too early, methods on the clip (and any clips deriv
 So, the rules of thumb are:
 
     * Call ``close()`` on any clip that you **construct** once you have finished using it and have also finished using any clip that was derived from it.
-    * Also close any clips you create through ``AudioFileClip.coreader()``.
     * Even if you close a ``CompositeVideoClip`` instance, you still need to close the clips it was created from.
     * Otherwise, if you have a clip that was created by deriving it from from another clip (e.g. by calling ``with_mask()``), then generally you shouldn't close it. Closing the original clip will also close the copy.
 
diff --git a/examples/example_with_sound.py b/examples/example_with_sound.py
index 35e5569..ee1298d 100644
--- a/examples/example_with_sound.py
+++ b/examples/example_with_sound.py
@@ -31,10 +31,7 @@ mask = color_split(
 mask_clip = ImageClip(mask, is_mask=True)
 
 clip_left = (
-    main_clip.coreader()
-    .subclip(0, duration)
-    .crop(x1=60, x2=60 + 2 * W / 3)
-    .with_mask(mask_clip)
+    main_clip.subclip(0, duration).crop(x1=60, x2=60 + 2 * W / 3).with_mask(mask_clip)
 )
 
 
@@ -47,8 +44,7 @@ mask = color_split(
 mask_clip = ImageClip(mask, is_mask=True)
 
 clip_right = (
-    main_clip.coreader()
-    .subclip(21, 21 + duration)
+    main_clip.subclip(21, 21 + duration)
     .crop(x1=70, x2=70 + 2 * W / 3)
     .with_mask(mask_clip)
 )
diff --git a/examples/painting_effect.py b/examples/painting_effect.py
index 90d260c..989aa98 100644
--- a/examples/painting_effect.py
+++ b/examples/painting_effect.py
@@ -7,10 +7,8 @@ from moviepy import *
 charade = VideoFileClip("../../videos/charade.mp4")
 tfreeze = convert_to_seconds(19.21)  # Time of the freeze, 19'21
 
-# when using several subclips of a same clip, it can be faster
-# to create 'coreaders' of the clip (=other entrance points).
-clip_before = charade.coreader().subclip(tfreeze - 2, tfreeze)
-clip_after = charade.coreader().subclip(tfreeze, tfreeze + 2)
+clip_before = charade.subclip(tfreeze - 2, tfreeze)
+clip_after = charade.subclip(tfreeze, tfreeze + 2)
 
 
 # THE FRAME TO FREEZE
diff --git a/moviepy/Clip.py b/moviepy/Clip.py
index a7f58a8..89a9758 100644
--- a/moviepy/Clip.py
+++ b/moviepy/Clip.py
@@ -4,7 +4,7 @@ all the methods that are common to the two subclasses of Clip, VideoClip
 and AudioClip.
 """
 
-from copy import copy
+import copy as _copy
 
 import numpy as np
 import proglog
@@ -60,23 +60,10 @@ class Clip:
         self.memoized_frame = None
 
     def copy(self):
-        """Shallow copy of the clip.
-
-        Returns a shallow copy of the clip whose mask and audio will
-        be shallow copies of the clip's mask and audio if they exist.
-
-        This method is intensively used to produce new clips every time
-        there is an outplace transformation of the clip (clip.resize,
-        clip.subclip, etc.)
         """
-
-        new_clip = copy(self)
-        if hasattr(self, "audio"):
-            new_clip.audio = copy(self.audio)
-        if hasattr(self, "mask"):
-            new_clip.mask = copy(self.mask)
-
-        return new_clip
+        Allows the usage of ``.copy()`` in clips as chained methods invocation.
+        """
+        return _copy.copy(self)
 
     @convert_parameter_to_seconds(["t"])
     def get_frame(self, t):
diff --git a/moviepy/audio/AudioClip.py b/moviepy/audio/AudioClip.py
index 0aeaace..9600ce7 100644
--- a/moviepy/audio/AudioClip.py
+++ b/moviepy/audio/AudioClip.py
@@ -29,6 +29,10 @@ class AudioClip(Clip):
       for a sound, it is just a float. What 'makes' the sound are
       the variations of that float in the time.
 
+    duration
+      Duration of the clip (in seconds). Some clips are infinite, in
+      this case their duration will be ``None``.
+
     nchannels
       Number of channels (one or two for mono or stereo).
 
diff --git a/moviepy/audio/io/AudioFileClip.py b/moviepy/audio/io/AudioFileClip.py
index 8b6d424..b16659e 100644
--- a/moviepy/audio/io/AudioFileClip.py
+++ b/moviepy/audio/io/AudioFileClip.py
@@ -45,23 +45,11 @@ class AudioFileClip(AudioClip):
     of these instances, you must call close() afterwards, or the subresources
     will not be cleaned up until the process ends.
 
-    If copies are made, and close() is called on one, it may cause methods on
-    the other copies to fail.
-
-    However, coreaders must be closed separately.
-
     Examples
     ----------
 
     >>> snd = AudioFileClip("song.wav")
     >>> snd.close()
-    >>> snd = AudioFileClip("song.mp3", fps = 44100)
-    >>> second_reader = snd.coreader()
-    >>> second_reader.close()
-    >>> snd.close()
-    >>> with AudioFileClip(mySoundArray, fps=44100) as snd:  # from a numeric array
-    >>>     pass  # Close is implicitly performed by context manager.
-
     """
 
     @convert_path_to_string("filename")
@@ -88,12 +76,6 @@ class AudioFileClip(AudioClip):
         self.make_frame = lambda t: self.reader.get_frame(t)
         self.nchannels = self.reader.nchannels
 
-    def coreader(self):
-        """Returns a copy of the AudioFileClip, i.e. a new entrance point
-        to the audio file. Use copy when you have different clips
-        watching the audio file at different times."""
-        return AudioFileClip(self.filename, self.buffersize)
-
     def close(self):
         """ Close the internal reader. """
         if self.reader:
diff --git a/moviepy/video/VideoClip.py b/moviepy/video/VideoClip.py
index 66d909f..b1ba574 100644
--- a/moviepy/video/VideoClip.py
+++ b/moviepy/video/VideoClip.py
@@ -7,6 +7,7 @@ main subclasses:
 import os
 import subprocess as sp
 import tempfile
+import copy as _copy
 
 import numpy as np
 import proglog
@@ -116,6 +117,33 @@ class VideoClip(Clip):
     def aspect_ratio(self):
         return self.w / float(self.h)
 
+    def __copy__(self):
+        """Mixed copy of the clip.
+
+        Returns a shallow copy of the clip whose mask and audio will
+        be shallow copies of the clip's mask and audio if they exist.
+
+        This method is intensively used to produce new clips every time
+        there is an outplace transformation of the clip (clip.resize,
+        clip.subclip, etc.)
+
+        Acts like a deepcopy except for the fact that readers and other
+        possible unpickleables objects are not copied.
+        """
+        cls = self.__class__
+        new_clip = cls.__new__(cls)
+        for attr in self.__dict__:
+            value = getattr(self, attr)
+            if attr in ("mask", "audio"):
+                value = _copy.copy(value)
+            setattr(new_clip, attr, value)
+
+        new_clip.audio = _copy.copy(self.audio)
+        new_clip.mask = _copy.copy(self.mask)
+        return new_clip
+
+    copy = __copy__
+
     # ===============================================================
     # EXPORT OPERATIONS
 
diff --git a/moviepy/video/fx/blink.py b/moviepy/video/fx/blink.py
index 2d3a367..c19801f 100644
--- a/moviepy/video/fx/blink.py
+++ b/moviepy/video/fx/blink.py
@@ -1,13 +1,10 @@
-import copy
-
-
 def blink(clip, duration_on, duration_off):
     """
     Makes the clip blink. At each blink it will be displayed ``duration_on``
     seconds and disappear ``duration_off`` seconds. Will only work in
     composite clips.
     """
-    new_clip = copy.copy(clip)
+    new_clip = clip.copy()
     if new_clip.mask is None:
         new_clip = new_clip.with_mask()
     duration = duration_on + duration_off
diff --git a/moviepy/video/io/VideoFileClip.py b/moviepy/video/io/VideoFileClip.py
index f17cb23..9c8f7ae 100644
--- a/moviepy/video/io/VideoFileClip.py
+++ b/moviepy/video/io/VideoFileClip.py
@@ -147,11 +147,16 @@ class VideoFileClip(VideoClip):
                 nbytes=audio_nbytes,
             )
 
-    def coreader(self):
-        """Returns a copy of the VideoFileClip, i.e. a new entrance point
-        to the video file. Use copy when you have different clips
-        watching the video file at different times."""
-        return VideoFileClip(self.filename, audio_buffersize=self.audio.buffersize)
+    def __deepcopy__(self, memo):
+        """AudioFileClip can't be deeply copied because the locked Thread
+        of ``proc`` isn't pickleable. Without this override, calls to
+        ``copy.deepcopy(clip)`` will raise next ``TypeError``:
+
+        ```
+        TypeError: cannot pickle '_thread.lock' object
+        ```
+        """
+        return self.__copy__()
 
     def close(self):
         """ Close the internal reader. """


Test patch:
diff --git a/tests/test_AudioClips.py b/tests/test_AudioClips.py
index 3e11551..0509efd 100644
--- a/tests/test_AudioClips.py
+++ b/tests/test_AudioClips.py
@@ -16,13 +16,6 @@ from moviepy.utils import close_all_clips
 from tests.test_helper import TMP_DIR
 
 
-def test_audio_coreader():
-    sound = AudioFileClip("media/crunching.mp3")
-    sound = sound.subclip(1, 4)
-    sound2 = AudioFileClip("media/crunching.mp3")
-    sound2.write_audiofile(os.path.join(TMP_DIR, "coreader.mp3"))
-
-
 def test_audioclip():
     make_frame = lambda t: [np.sin(440 * 2 * np.pi * t)]
     audio = AudioClip(make_frame, duration=2, fps=22050)
diff --git a/tests/test_Clip.py b/tests/test_Clip.py
index 37cd6d4..fba15db 100644
--- a/tests/test_Clip.py
+++ b/tests/test_Clip.py
@@ -1,5 +1,8 @@
+import copy
+
 import pytest
 
+from moviepy.Clip import Clip
 from moviepy.video.VideoClip import BitmapClip
 
 
@@ -16,5 +19,39 @@ def test_clip_equality():
     assert clip != different_clip
 
 
+@pytest.mark.parametrize(
+    "copy_func",
+    (
+        lambda clip: clip.copy(),
+        lambda clip: copy.copy(clip),
+        lambda clip: copy.deepcopy(clip),
+    ),
+    ids=(
+        "clip.copy()",
+        "copy.copy(clip)",
+        "copy.deepcopy(clip)",
+    ),
+)
+def test_clip_copy(copy_func):
+    """Clip must be copied with `.copy()` method, `copy.copy()` and
+    `copy.deepcopy()` (same behaviour).
+    """
+    clip = Clip()
+    other_clip = Clip()
+
+    # shallow copy of clip
+    for attr in clip.__dict__:
+        setattr(clip, attr, "foo")
+
+    copied_clip = copy_func(clip)
+
+    # assert copied attributes
+    for attr in copied_clip.__dict__:
+        assert getattr(copied_clip, attr) == getattr(clip, attr)
+
+        # other instances are not edited
+        assert getattr(copied_clip, attr) != getattr(other_clip, attr)
+
+
 if __name__ == "__main__":
     pytest.main()
diff --git a/tests/test_VideoClip.py b/tests/test_VideoClip.py
index 964d1df..6567835 100644
--- a/tests/test_VideoClip.py
+++ b/tests/test_VideoClip.py
@@ -1,3 +1,4 @@
+import copy
 import os
 
 import numpy as np
@@ -9,7 +10,7 @@ from moviepy.utils import close_all_clips
 from moviepy.video.compositing.CompositeVideoClip import CompositeVideoClip
 from moviepy.video.fx.speedx import speedx
 from moviepy.video.io.VideoFileClip import VideoFileClip
-from moviepy.video.VideoClip import ColorClip, BitmapClip
+from moviepy.video.VideoClip import ColorClip, BitmapClip, VideoClip
 
 from tests.test_helper import TMP_DIR
 
@@ -298,5 +299,71 @@ def test_setfps_withchangeduration():
     close_all_clips(locals())
 
 
+def test_copied_videoclip_write_videofile():
+    """Check if a copied ``VideoClip`` instance can render a file which has
+    the same features as the copied clip when opening with ``VideoFileClip``.
+    """
+    clip = BitmapClip([["RRR", "GGG", "BBB"]], fps=1)
+    copied_clip = clip.copy()
+
+    output_filepath = os.path.join(TMP_DIR, "copied_videoclip_from_bitmap.webm")
+    copied_clip.write_videofile(output_filepath)
+    copied_clip_from_file = VideoFileClip(output_filepath)
+
+    assert list(copied_clip.size) == list(copied_clip_from_file.size)
+    assert copied_clip.duration == copied_clip_from_file.duration
+
+
+@pytest.mark.parametrize(
+    "copy_func",
+    (
+        lambda clip: clip.copy(),
+        lambda clip: copy.copy(clip),
+        lambda clip: copy.deepcopy(clip),
+    ),
+    ids=("clip.copy()", "copy.copy(clip)", "copy.deepcopy(clip)"),
+)
+def test_videoclip_copy(copy_func):
+    """It must be possible to do a mixed copy of VideoClip using ``clip.copy()``,
+    ``copy.copy(clip)`` and ``copy.deepcopy(clip)``.
+    """
+    clip = VideoClip()
+    other_clip = VideoClip()
+
+    for attr in clip.__dict__:
+        # mask and audio are shallow copies that should be initialized
+        if attr in ("mask", "audio"):
+            if attr == "mask":
+                nested_object = BitmapClip([["R"]], duration=0.01)
+            else:
+                nested_object = AudioClip(
+                    lambda t: [np.sin(880 * 2 * np.pi * t)], duration=0.01, fps=44100
+                )
+            setattr(clip, attr, nested_object)
+        else:
+            setattr(clip, attr, "foo")
+
+    copied_clip = copy_func(clip)
+
+    # VideoClip attributes are copied
+    for attr in copied_clip.__dict__:
+        value = getattr(copied_clip, attr)
+        assert value == getattr(clip, attr)
+
+        # other instances are not edited
+        assert value != getattr(other_clip, attr)
+
+        # shallow copies of mask and audio
+        if attr in ("mask", "audio"):
+            for nested_attr in value.__dict__:
+                assert getattr(value, nested_attr) == getattr(
+                    getattr(clip, attr), nested_attr
+                )
+
+    # nested objects of instances copies are not edited
+    assert other_clip.mask is None
+    assert other_clip.audio is None
+
+
 if __name__ == "__main__":
     pytest.main()
diff --git a/tests/test_VideoFileClip.py b/tests/test_VideoFileClip.py
index 6ce2d18..48814d7 100644
--- a/tests/test_VideoFileClip.py
+++ b/tests/test_VideoFileClip.py
@@ -1,5 +1,5 @@
 """Video file clip tests meant to be run with pytest."""
-
+import copy
 import os
 
 import pytest
@@ -45,22 +45,43 @@ def test_ffmpeg_resizing():
         video.close()
 
 
-def test_shallow_copy():
-    """Call a function which uses @outplace
-    and verify that making a shallow copy and deleting it
-    does not corrupt the original clip."""
-    video_file = "media/big_buck_bunny_0_30.webm"
-    video = VideoFileClip(video_file)
-    video_copy = video.with_start(1)
-    del video_copy
-    # The clip object buffers 200000 frames, around 5 seconds ahead.
-    # When recentering the buffer, if the new buffer is more than 1000000 frames,
-    # around 25s ahead of the end of the current buffer, the reader will
-    # reinitialize and fix self.proc.
-    # Thus to trigger the bug, you have to look for a frame between ~5 and ~30
-    # seconds away. These numbers might vary for different reasons and it would
-    # be nice to have a test which was robust to changes in default buffer size, etc.
-    video.audio.make_frame(15)
+def test_copied_videofileclip_write_videofile():
+    """Check that a copied ``VideoFileClip`` can be renderizable using
+    ``write_videofile``, opened from that render and the new video shares
+    the same data that the original clip.
+    """
+    input_video_filepath = "media/big_buck_bunny_432_433.webm"
+    output_video_filepath = os.path.join(TMP_DIR, "copied_videofileclip.mp4")
+
+    clip = VideoFileClip(input_video_filepath).subclip(0, 1)
+    copied_clip = clip.copy()
+
+    copied_clip.write_videofile(output_video_filepath)
+
+    assert os.path.exists(output_video_filepath)
+    copied_clip_from_file = VideoFileClip(output_video_filepath)
+
+    assert copied_clip.fps == copied_clip_from_file.fps
+    assert list(copied_clip.size) == copied_clip_from_file.size
+    assert isinstance(copied_clip.reader, type(copied_clip_from_file.reader))
+
+
+def test_videofileclip_safe_deepcopy(monkeypatch):
+    """Attempts to do a deepcopy of a VideoFileClip will do a mixed copy,
+    being redirected to ``__copy__`` method of ``VideoClip``, see the
+    documentation of ``VideoFileClip.__deepcopy__`` for more information
+    about this.
+    """
+    clip = VideoFileClip("media/chaplin.mp4")
+
+    # patch __copy__ in the clip
+    def fake__copy__():
+        return "foo"
+
+    monkeypatch.setattr(clip, "__copy__", fake__copy__)
+
+    # this should not raise any exception (see `VideoFileClip.__deepcopy__`)
+    assert copy.deepcopy(clip) == "foo"
 
 
 if __name__ == "__main__":
diff --git a/tests/test_issues.py b/tests/test_issues.py
index 35d097c..5216822 100644
--- a/tests/test_issues.py
+++ b/tests/test_issues.py
@@ -7,7 +7,6 @@ from moviepy.video.VideoClip import ColorClip, ImageClip, VideoClip
 from moviepy.video.compositing.CompositeVideoClip import CompositeVideoClip
 from moviepy.video.compositing.concatenate import concatenate_videoclips
 from moviepy.video.compositing.transitions import crossfadein, crossfadeout
-from moviepy.video.fx.blink import blink
 from moviepy.video.fx.resize import resize
 from moviepy.audio.io.AudioFileClip import AudioFileClip
 from moviepy.video.io.VideoFileClip import VideoFileClip
@@ -348,14 +347,6 @@ def test_issue_417():
     # final.with_duration(7).write_videofile("test.mp4", fps=30)
 
 
-def test_issue_467():
-    cad = "media/python_logo.png"
-    clip = ImageClip(cad)
-
-    # caused an error, NameError: global name 'copy' is not defined
-    clip = clip.fx(blink, duration_on=1, duration_off=1)
-
-
 def test_issue_470():
     audio_clip = AudioFileClip("media/crunching.mp3")