Merge pull request #52 from nyanpasu64/terminate-ffmpeg

Properly terminate FFmpeg output processes

ovgenpy/outputs.py

@@ -49,7 +49,7 @@ def register_output(config_t: Type[IOutputConfig]):
 
 # FFmpeg command line generation
 
-class _FFmpegCommand:
+class _FFmpegProcess:
     def __init__(self, templates: List[str], ovgen_cfg: 'Config'):
         self.templates = templates
         self.ovgen_cfg = ovgen_cfg
@@ -122,9 +122,19 @@ class PipeOutput(Output):
         return retval   # final value
 
     def __exit__(self, exc_type, exc_val, exc_tb):
+        self.close()
         if exc_type is not None:
+            e = None
             for popen in self._pipeline:
                 popen.terminate()
+                # https://stackoverflow.com/a/49038779/2683842
+                try:
+                    popen.wait(1)   # timeout=seconds
+                except subprocess.TimeoutExpired as e:
+                    popen.kill()
+
+            if e:
+                raise e
 
 
 # FFmpegOutput
@@ -135,8 +145,7 @@ class FFmpegOutputConfig(IOutputConfig):
     path: Optional[str]
     args: str = ''
 
-    # Do not use `-movflags faststart`, I get corrupted mp4 files (missing MOOV)
-    video_template: str = '-c:v libx264 -crf 18 -preset superfast'
+    video_template: str = '-c:v libx264 -crf 18 -preset superfast -movflags faststart'
     audio_template: str = '-c:a aac -b:a 384k'
 
 
@@ -147,7 +156,7 @@ class FFmpegOutput(PipeOutput):
     def __init__(self, ovgen_cfg: 'Config', cfg: FFmpegOutputConfig):
         super().__init__(ovgen_cfg, cfg)
 
-        ffmpeg = _FFmpegCommand([FFMPEG, '-y'], ovgen_cfg)
+        ffmpeg = _FFmpegProcess([FFMPEG, '-y'], ovgen_cfg)
         ffmpeg.add_output(cfg)
         ffmpeg.templates.append(cfg.args)
 
@@ -174,7 +183,7 @@ class FFplayOutput(PipeOutput):
     def __init__(self, ovgen_cfg: 'Config', cfg: FFplayOutputConfig):
         super().__init__(ovgen_cfg, cfg)
 
-        ffmpeg = _FFmpegCommand([FFMPEG], ovgen_cfg)
+        ffmpeg = _FFmpegProcess([FFMPEG], ovgen_cfg)
         ffmpeg.add_output(cfg)
         ffmpeg.templates.append('-f nut')
 
@@ -184,6 +193,8 @@ class FFplayOutput(PipeOutput):
         p2 = subprocess.Popen(ffplay, stdin=p1.stdout)
 
         p1.stdout.close()
+        # assert p2.stdin is None   # True unless Popen is being mocked (test_output).
+
         self.open(p1, p2)
 
 

ovgenpy/ovgenpy.py

@@ -232,8 +232,13 @@ class Ovgen:
                         for output in self.outputs:
                             output.write_frame(frame)
 
+            if self.raise_on_teardown:
+                raise self.raise_on_teardown
+
         if PRINT_TIMESTAMP:
             # noinspection PyUnboundLocalVariable
             dtime = time.perf_counter() - begin
             render_fps = (end_frame - begin_frame) / dtime
             print(f'FPS = {render_fps}')
+
+    raise_on_teardown: Exception = None

tests/conftest.py

@@ -10,11 +10,18 @@ if TYPE_CHECKING:
 
 @pytest.fixture
 def Popen(mocker: 'pytest_mock.MockFixture'):
+    real_Popen = subprocess.Popen
+
+    def popen_factory(*args, **kwargs):
+        popen = mocker.create_autospec(real_Popen)
+
+        popen.stdin = mocker.mock_open()(os.devnull, "wb")
+        popen.stdout = mocker.mock_open()(os.devnull, "rb")
+        assert popen.stdin != popen.stdout
+
+        popen.wait.return_value = 0
+        return popen
+
     Popen = mocker.patch.object(subprocess, 'Popen', autospec=True)
-    popen = Popen.return_value
-
-    popen.stdin = open(os.devnull, "wb")
-    popen.stdout = open(os.devnull, "rb")
-    popen.wait.return_value = 0
-
+    Popen.side_effect = popen_factory
     yield Popen

tests/test_output.py

@@ -39,9 +39,24 @@ def test_output():
     assert not Path('-').exists()
 
 
+# Ensure ovgen closes pipe to output upon completion.
+@pytest.mark.usefixtures('Popen')
+def test_close_output(Popen):
+    """ FFplayOutput unit test: Ensure ffmpeg and ffplay are terminated when Python
+    exceptions occur.
+    """
+
+    ffplay_cfg = FFplayOutputConfig()
+    output: FFplayOutput
+    with ffplay_cfg(CFG) as output:
+        pass
+
+    output._pipeline[0].stdin.close.assert_called()
+    for popen in output._pipeline:
+        popen.wait.assert_called()  # Does wait() need to be called?
+
+
 # Ensure ovgen terminates FFplay upon exceptions.
-
-
 @pytest.mark.usefixtures('Popen')
 def test_terminate_ffplay(Popen):
     """ FFplayOutput unit test: Ensure ffmpeg and ffplay are terminated when Python
@@ -61,8 +76,8 @@ def test_terminate_ffplay(Popen):
 
 @pytest.mark.usefixtures('Popen')
 def test_ovgen_terminate_ffplay(Popen, mocker: 'pytest_mock.MockFixture'):
-    """ Integration test: Ensure ffmpeg and ffplay are terminated when Python exceptions
-    occur. """
+    """ Integration test: Ensure ovgenpy calls terminate() on ffmpeg and ffplay when
+    Python exceptions occur. """
 
     cfg = default_config(
         channels=[ChannelConfig('tests/sine440.wav')],
@@ -83,6 +98,25 @@ def test_ovgen_terminate_ffplay(Popen, mocker: 'pytest_mock.MockFixture'):
         popen.terminate.assert_called()
 
 
+@pytest.mark.skip('Launches ffmpeg and ffplay processes, creating a ffplay window')
+def test_ovgen_terminate_works():
+    """ Ensure that ffmpeg/ffplay terminate quickly after Python exceptions, when
+    `popen.terminate()` is called. """
+
+    cfg = default_config(
+        channels=[ChannelConfig('tests/sine440.wav')],
+        master_audio='tests/sine440.wav',
+        outputs=[FFplayOutputConfig()],
+        end_time=0.5,   # Reduce test duration
+    )
+    ovgen = Ovgen(cfg, '.')
+    ovgen.raise_on_teardown = DummyException
+
+    with pytest.raises(DummyException):
+        # Raises `subprocess.TimeoutExpired` if popen.terminate() doesn't work.
+        ovgen.play()
+
+
 # TODO integration test without audio
 
 # TODO integration test on ???