Merge pull request #382 from corrscope/custom-render-path

CHANGELOG.md

@@ -1,5 +1,9 @@
 ## 0.7.1 (unreleased)
 
+### Major Changes
+
+- Change `-r/--render` command line flag to take an output path (#382)
+
 ### Changelog
 
 - Update NumPy so `poetry install` on Python 3.8+ won't build NumPy from source (#371)

README.md

@@ -83,8 +83,8 @@ poetry run corr (args)
 1. Play (requires ffmpeg):
     - `corrscope master.yaml -p/--play`
 
-1. Render and encode MP4 video (requires ffmpeg)
+1. Render and encode video (requires ffmpeg)
-    - `corrscope master.yaml -r/--render`
+    - `corrscope master.yaml -r/--render file.mp4` (other file extensions supported)
 
 ## Contributing
 

corrscope/cli.py

@@ -34,7 +34,7 @@ YAML_EXTS = [".yaml"]
 # Default extension when writing Config.
 YAML_NAME = YAML_EXTS[0]
 
-# Default output extension
+# Default output extension, only used in GUI and unit tests
 VIDEO_NAME = ".mp4"
 
 
@@ -85,14 +85,14 @@ CONTEXT_SETTINGS = dict(help_option_names=["-h", "--help"])
 # Override default .yaml settings (only if YAML file not supplied)
 # Incorrect [option] name order: https://github.com/pallets/click/issues/793
 @click.option('--audio', '-a', type=File, help=
-        'Config: Input path for master audio file')
+        'Input path for master audio file')
 # Disables GUI
 @click.option('--write', '-w', is_flag=True, help=
         "Write config YAML file to current directory (don't open GUI).")
 @click.option('--play', '-p', is_flag=True, help=
         "Preview (don't open GUI).")
-@click.option('--render', '-r', is_flag=True, help=
+@click.option('--render', '-r', type=OutFile, help=
-        "Render and encode MP4 video (don't open GUI).")
+        "Render and encode video to file (don't open GUI).")
 # Debugging
 @click.option('--profile', is_flag=True, help=
         'Debug: Write CProfiler snapshot')
@@ -105,7 +105,7 @@ def main(
     # gui
     write: bool,
     play: bool,
-    render: bool,
+    render: Optional[str],
     profile: bool,
 ):
     """Intelligent oscilloscope visualizer for .wav files.
@@ -227,7 +227,7 @@ def main(
             outputs.append(FFplayOutputConfig())
 
         if render:
-            video_path = _get_file_name(cfg_path, cfg, ext=VIDEO_NAME)
+            video_path = render
             outputs.append(cfg.get_ffmpeg_cfg(video_path))
 
         if outputs:

corrscope/corrscope.py

@@ -1,4 +1,5 @@
 # -*- coding: utf-8 -*-
+import os.path
 import time
 from contextlib import ExitStack, contextmanager
 from enum import unique
@@ -91,7 +92,7 @@ class Config(
     ffmpeg_cli: FFmpegOutputConfig = attr.ib(factory=lambda: FFmpegOutputConfig(None))
 
     def get_ffmpeg_cfg(self, video_path: str) -> FFmpegOutputConfig:
-        return attr.evolve(self.ffmpeg_cli, path=video_path)
+        return attr.evolve(self.ffmpeg_cli, path=os.path.abspath(video_path))
 
     benchmark_mode: BenchmarkMode = attr.ib(
         BenchmarkMode.NONE, converter=BenchmarkMode.by_name

corrscope/gui/__init__.py

@@ -166,9 +166,11 @@ class MainWindow(qw.QMainWindow, Ui_MainWindow):
     - Otherwise empty string.
         - self.get_save_filename() calls cli.get_file_stem().
 
-    CLI filename is the same,
+    CLI YAML filename is the same,
-    but defaults to "corrscope.{yaml, mp4}" instead of empty string.
+    but defaults to "corrscope.yaml" instead of empty string.
     - cli._get_file_name() calls cli.get_file_stem().
+
+    CLI video filename is explicitly specified by the user.
     """
 
     def __init__(self, cfg_or_path: Union[Config, Path]):

corrscope/outputs.py

@@ -22,6 +22,8 @@ class IOutputConfig(DumpableAttrs):
     cls: "ClassVar[Type[Output]]"
 
     def __call__(self, corr_cfg: "Config") -> "Output":
+        """Must be called in the .yaml file's directory.
+        This is used to properly resolve corr_cfg.master_audio."""
         return self.cls(corr_cfg, cfg=self)
 
 
@@ -217,6 +219,25 @@ class PipeOutput(Output):
 
 class FFmpegOutputConfig(IOutputConfig):
     # path=None writes to stdout.
+    #
+    # This parameter is not loaded from disk, but set when the user picks a render path
+    # on the GUI or calls the CLI with `--render out.mp4`.
+    #
+    # It must be an absolute path. How is this ensured?
+    #
+    # - Paths supplied from the GUI are always absolute.
+    # - Paths supplied from the CLI must be resolved before storing in
+    #   FFmpegOutputConfig.
+    #
+    # Why are relative paths not allowed? Currently, to resolve `corr_cfg.master_audio`
+    # relative to the config file, we change directories to the config dir, then call
+    # `abspath(corr_cfg.master_audio)`. As a result, if we called `corr dir/cfg.yaml -r
+    # out.mp4` and corrscope didn't resolve `out.mp4` before passing into
+    # FFmpegOutputConfig, it would mistakenly write to `dir/out.mp4`.
+    #
+    # In the future, relative paths could be allowed by not switching directories to the
+    # config dir, and finding another way to resolve `corr_cfg.master_audio` based on
+    # the config dir.
     path: Optional[str]
     args: str = ""
 

tests/test_cli.py

@@ -140,15 +140,19 @@ def test_load_yaml_another_dir(mocker, Popen):
     # Log execution of CorrScope().play()
     Wave = mocker.spy(corrscope.channel, "Wave")
 
-    # Issue: this test does not use cli.main() to compute output path.
+    # Same function as used in cli.py and gui/__init__.py.
-    # Possible solution: Call cli.main() via Click runner.
+    output = cfg.get_ffmpeg_cfg(mp4)
-    output = FFmpegOutputConfig(cli._get_file_name(None, cfg, cli.VIDEO_NAME))
+
     corr = CorrScope(cfg, Arguments(subdir, [output]))
     corr.play()
 
-    # Compute absolute paths
+    # The .wav path (specified in Config) should be resolved relative to the config
+    # file.
     wav_abs = abspath(f"{subdir}/{wav}")
-    mp4_abs = abspath(f"{subdir}/{mp4}")
+
+    # The output video path (specified in CLI --render) should be resolved relative to
+    # the shell's working directory.
+    mp4_abs = abspath(mp4)
 
     # Test `wave_path`
     args, kwargs = Wave.call_args