Rework usage of FFMPEG_VideoReader's pos and lastread; FFMPEG_AudioReader.close_proc() -> close()

CHANGELOG.md

@@ -14,14 +14,16 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 ### Added <!-- for new features -->
 
 ### Changed <!-- for changes in existing functionality -->
+- `FFMPEG_AudioReader.close_proc()` -> `FFMPEG_AudioReader.close()` for consistency with `FFMPEG_VideoReader` [#1220]
 - `ffmpeg_parse_infos()` and `VideoFileClip` now detect the actual duration of the decoded video instead of the duration stored in its metadata [#1063]
 
 ### Deprecated <!-- for soon-to-be removed features -->		
 
 ### Removed <!-- for now removed features -->
 
 ### Fixed <!-- for any bug fixes -->
-
+- `OSError: MoviePy error: failed to read the first frame of video file...` would occasionally occur for no reason [#1220]
+- Warnings are no longer being supressed by MoviePy [#1191]
 
 
 ## [v2.0.0.dev1](https://github.com/zulko/moviepy/tree/v2.0.0.dev1) (2020-06-04)

moviepy/audio/io/AudioFileClip.py

@@ -91,5 +91,5 @@ def coreader(self):
     def close(self):
         """ Close the internal reader. """
         if self.reader:
-            self.reader.close_proc()
+            self.reader.close()
             self.reader = None

moviepy/audio/io/readers.py

@@ -68,7 +68,7 @@ def __init__(
     def initialize(self, starttime=0):
         """ Opens the file, creates the pipe. """
 
-        self.close_proc()  # if any
+        self.close()  # if any
 
         if starttime != 0:
             offset = min(1, starttime)
@@ -160,14 +160,6 @@ def seek(self, pos):
         # last case standing: pos = current pos
         self.pos = pos
 
-    def close_proc(self):
-        if hasattr(self, "proc") and self.proc is not None:
-            self.proc.terminate()
-            for std in [self.proc.stdout, self.proc.stderr]:
-                std.close()
-            self.proc.wait()
-            self.proc = None
-
     def get_frame(self, tt):
         if isinstance(tt, np.ndarray):
             # lazy implementation, but should not cause problems in
@@ -255,6 +247,14 @@ def buffer_around(self, framenumber):
 
         self.buffer_startframe = new_bufferstart
 
+    def close(self):
+        if self.proc:
+            self.proc.terminate()
+            self.proc.stdout.close()
+            self.proc.stderr.close()
+            self.proc.wait()
+            self.proc = None
+
     def __del__(self):
         # If the garbage collector comes, make sure the subprocess is terminated.
-        self.close_proc()
+        self.close()

moviepy/video/io/ffmpeg_reader.py

@@ -5,7 +5,6 @@
 
 from __future__ import division
 
-import logging
 import os
 import re
 import subprocess as sp
@@ -16,8 +15,6 @@
 from moviepy.config import FFMPEG_BINARY  # ffmpeg, ffmpeg.exe, etc...
 from moviepy.tools import cvsecs
 
-logging.captureWarnings(True)
-
 
 class FFMPEG_VideoReader:
     def __init__(
@@ -70,13 +67,10 @@ def __init__(
         self.bufsize = bufsize
         self.initialize()
 
-        self.pos = 1
-        self.lastread = self.read_frame()
-
     def initialize(self, starttime=0):
         """Opens the file, creates the pipe. """
 
-        self.close()  # if any
+        self.close(delete_lastread=False)  # if any
 
         if starttime != 0:
             offset = min(1, starttime)
@@ -119,14 +113,18 @@ def initialize(self, starttime=0):
 
         if os.name == "nt":
             popen_params["creationflags"] = 0x08000000
-
         self.proc = sp.Popen(cmd, **popen_params)
 
+        # This will be incremented by the subsequent `read_frame`
+        self.pos = self.get_frame_number(starttime) - 1
+        self.lastread = self.read_frame()
+
     def skip_frames(self, n=1):
         """Reads and throws away n frames """
         w, h = self.size
         for i in range(n):
             self.proc.stdout.read(self.depth * w * h)
+
             # self.proc.stdout.flush()
         self.pos += n
 
@@ -135,8 +133,9 @@ def read_frame(self):
         nbytes = self.depth * w * h
 
         s = self.proc.stdout.read(nbytes)
-        if len(s) != nbytes:
+        self.pos += 1
 
+        if len(s) != nbytes:
             warnings.warn(
                 "Warning: in file %s, " % (self.filename)
                 + "%d bytes wanted but %d bytes read," % (nbytes, len(s))
@@ -145,18 +144,16 @@ def read_frame(self):
                 + "Using the last valid frame instead.",
                 UserWarning,
             )
-
             if not hasattr(self, "lastread"):
                 raise IOError(
                     (
                         "MoviePy error: failed to read the first frame of "
-                        "video file %s. That might mean that the file is "
+                        f"video file {self.filename}. That might mean that the file is "
                         "corrupted. That may also mean that you are using "
                         "a deprecated version of FFMPEG. On Ubuntu/Debian "
                         "for instance the version in the repos is deprecated. "
                         "Please update to a recent version from the website."
                     )
-                    % (self.filename)
                 )
 
             result = self.lastread
@@ -180,40 +177,41 @@ def get_frame(self, t):
         whenever possible, by moving between adjacent frames.
         """
 
-        # these definitely need to be rechecked sometime. Seems to work.
-
-        # I use that horrible '+0.00001' hack because sometimes due to numerical
-        # imprecisions a 3.0 can become a 2.99999999... which makes the int()
-        # go to the previous integer. This makes the fetching more robust in the
-        # case where you get the nth frame by writing get_frame(n/fps).
-
-        pos = int(self.fps * t + 0.00001) + 1
-
+        pos = self.get_frame_number(t)
         # Initialize proc if it is not open
         if not self.proc:
+            print(f"Proc not detected")
             self.initialize(t)
-            self.pos = pos
-            self.lastread = self.read_frame()
+            return self.lastread
 
         if pos == self.pos:
             return self.lastread
         elif (pos < self.pos) or (pos > self.pos + 100):
+            # We can't just skip forward to `pos` or it would take too long
             self.initialize(t)
-            self.pos = pos
+            return self.lastread
         else:
+            # If pos == self.pos + 1, this line has no effect
             self.skip_frames(pos - self.pos - 1)
-        result = self.read_frame()
-        self.pos = pos
-        return result
+            result = self.read_frame()
+            return result
+
+    def get_frame_number(self, t):
+        """Helper method to return the frame number at time ``t``"""
+        # I used this horrible '+0.00001' hack because sometimes due to numerical
+        # imprecisions a 3.0 can become a 2.99999999... which makes the int()
+        # go to the previous integer. This makes the fetching more robust when you
+        # are getting the nth frame by writing get_frame(n/fps).
+        return int(self.fps * t + 0.00001) + 1
 
-    def close(self):
+    def close(self, delete_lastread=True):
         if self.proc:
             self.proc.terminate()
             self.proc.stdout.close()
             self.proc.stderr.close()
             self.proc.wait()
             self.proc = None
-        if hasattr(self, "lastread"):
+        if delete_lastread and hasattr(self, "lastread"):
             del self.lastread
 
     def __del__(self):

tests/test_ffmpeg_reader.py

@@ -1,8 +1,9 @@
 # -*- coding: utf-8 -*-
 """FFmpeg reader tests meant to be run with pytest."""
 import pytest
+import numpy as np
 
-from moviepy.video.io.ffmpeg_reader import ffmpeg_parse_infos
+from moviepy.video.io.ffmpeg_reader import ffmpeg_parse_infos, FFMPEG_VideoReader
 
 
 def test_ffmpeg_parse_infos():
@@ -32,5 +33,125 @@ def test_ffmpeg_parse_infos_for_i926():
     assert d["audio_found"]
 
 
+def test_sequential_frame_pos():
+    """test_video.mp4 contains 5 frames at 1 fps.
+    Each frame is 1x1 pixels and the sequence is Red, Green, Blue, Black, White.
+    The rgb values are not pure due to compression."""
+    reader = FFMPEG_VideoReader("media/test_video.mp4")
+    assert reader.pos == 1
+
+    # Get first frame
+    frame_1 = reader.get_frame(0)
+    assert reader.pos == 1
+    assert np.array_equal(frame_1, [[[254, 0, 0]]])
+
+    # Get a specific sequential frame
+    frame_2 = reader.get_frame(1)
+    assert reader.pos == 2
+    assert np.array_equal(frame_2, [[[0, 255, 1]]])
+
+    # Get next frame. Note `.read_frame()` instead of `.get_frame()`
+    frame_3 = reader.read_frame()
+    assert reader.pos == 3
+    assert np.array_equal(frame_3, [[[0, 0, 255]]])
+
+    # Skip a frame
+    skip_frame = reader.get_frame(4)
+    assert reader.pos == 5
+    assert np.array_equal(skip_frame, [[[255, 255, 255]]])
+
+
+def test_unusual_order_frame_pos():
+    reader = FFMPEG_VideoReader("media/test_video.mp4")
+    assert reader.pos == 1
+
+    # Go straight to end
+    end_frame = reader.get_frame(4)
+    assert reader.pos == 5
+    assert np.array_equal(end_frame, [[[255, 255, 255]]])
+
+    # Repeat the previous frame
+    second_end_frame = reader.get_frame(4)
+    assert reader.pos == 5
+    assert np.array_equal(second_end_frame, [[[255, 255, 255]]])
+
+    # Go backwards
+    previous_frame = reader.get_frame(3)
+    assert reader.pos == 4
+    assert np.array_equal(previous_frame, [[[0, 0, 0]]])
+
+    # Go back to start
+    start_frame = reader.get_frame(0)
+    assert reader.pos == 1
+    assert np.array_equal(start_frame, [[[254, 0, 0]]])
+
+
+def test_large_skip_frame_pos():
+    reader = FFMPEG_VideoReader("media/big_buck_bunny_0_30.webm")
+    assert reader.fps == 24
+
+    # 10 sec * 24 fps = 240 frames
+    frame = reader.get_frame(240 // 24)
+    assert reader.pos == 241
+
+    frame2 = reader.get_frame(719 / 24)
+    assert reader.pos == 720
+
+    # Go backwards
+    backwards_frame = reader.get_frame(120 // 24)
+    assert reader.pos == 121
+
+
+def test_large_small_skip_equal():
+    """Get the 241st frame of the file in 4 different ways:
+    Reading every frame, Reading every 24th frame, Jumping straight there"""
+    sequential_reader = FFMPEG_VideoReader("media/big_buck_bunny_0_30.webm")
+    small_skip_reader = FFMPEG_VideoReader("media/big_buck_bunny_0_30.webm")
+    large_skip_reader = FFMPEG_VideoReader("media/big_buck_bunny_0_30.webm")
+    assert small_skip_reader.fps == large_skip_reader.fps == sequential_reader.fps == 24
+
+    # Read every frame sequentially
+    for t in np.arange(0, 10, 1 / 24):
+        sequential_reader.get_frame(t)
+    sequential_final_frame = sequential_reader.get_frame(10)
+
+    # Read in increments of 24 frames
+    for t in range(10):
+        small_skip_reader.get_frame(t)
+    small_skip_final_frame = small_skip_reader.get_frame(10)
+
+    # Jumps straight forward 240 frames. This is greater than 100 so it uses
+    # FFmpeg to reseek at the right position.
+    large_skip_final_frame = large_skip_reader.get_frame(10)
+
+    assert (
+        sequential_reader.pos == small_skip_reader.pos == large_skip_reader.pos == 241
+    )
+
+    # All frames have gone forward an equal amount, so should be equal
+    assert np.array_equal(sequential_final_frame, small_skip_final_frame)
+    assert np.array_equal(small_skip_final_frame, large_skip_final_frame)
+
+
+def test_seeking_beyond_file_end():
+    reader = FFMPEG_VideoReader("media/test_video.mp4")
+    frame_1 = reader.get_frame(0)
+
+    with pytest.warns(UserWarning, match="Using the last valid frame instead"):
+        end_of_file_frame = reader.get_frame(5)
+    assert np.array_equal(frame_1, end_of_file_frame)
+    assert reader.pos == 6
+
+    # Try again with a jump larger than 100 frames
+    # (which triggers different behaivour in `.get_frame()`
+    reader = FFMPEG_VideoReader("media/big_buck_bunny_0_30.webm")
+    frame_1 = reader.get_frame(0)
+
+    with pytest.warns(UserWarning, match="Using the last valid frame instead"):
+        end_of_file_frame = reader.get_frame(30)
+    assert np.array_equal(frame_1, end_of_file_frame)
+    assert reader.pos == 30 * 24 + 1
+
+
 if __name__ == "__main__":
     pytest.main()