Repository: nathanbabcock/ffmpeg-sidecar (language: rust)

Issue:
Prompt to overwrite file hangs infinitely waiting for stdin
If the `-y` or `-n` is not set and the user attempts to overwrite an image, the CLI will prompt the user for input:

```txt
File 'test.webp' already exists. Overwrite? [y/N]
```

However this will be invisible when used from within `ffmpeg-sidecar`, and nothing will be parsed in the logs since there's no newline at the end of the stderr buffer while it's prompting the user for input.

I think the solution is to start the CLI with `-nostdin` by default, if not otherwise specified — but need to double-check that this isn't a breaking change for packages which rely on sending a quit command (`q`) over stdin to gracefully end the ffmpeg process.

The workaround is to use `-y`, `-n`, `-nostdin`, or just avoid overwriting without those flags set.

Found by @jtof-dev


Discussion / hints:

Fix patch:
diff --git a/examples/h265_transcode.rs b/examples/h265_transcode.rs
index 49a322d..5163cb5 100644
--- a/examples/h265_transcode.rs
+++ b/examples/h265_transcode.rs
@@ -56,7 +56,7 @@ fn main() {
     // `for_each` blocks through the end of the iterator,
     // so we run it in another thread.
     transformed_frames.for_each(|f| {
-      stdin.write(&f.data).ok();
+      stdin.write_all(&f.data).ok();
     });
   });
 
diff --git a/src/command.rs b/src/command.rs
index 7e9b20b..46f68a2 100644
--- a/src/command.rs
+++ b/src/command.rs
@@ -214,16 +214,15 @@ impl FfmpegCommand {
   ///
   /// Possible values depend on codec:
   ///   * 0-51 for h264 (default is 23), see [ffmpeg encoding guide for h264
-  /// for more details](https://trac.ffmpeg.org/wiki/Encode/H.264#crf)
+  ///     for more details](https://trac.ffmpeg.org/wiki/Encode/H.264#crf)
   ///   * 0-51 for h265 (default is 28), see [ffmpeg encoding guide for h265
-  /// for more details](https://trac.ffmpeg.org/wiki/Encode/H.265#ConstantRateFactorCRF)
+  ///     for more details](https://trac.ffmpeg.org/wiki/Encode/H.265#ConstantRateFactorCRF)
   ///   * 0-63 for vp9  (no default, 31 is recommended for 1080p HD video),
-  /// see [ffmpeg encoding guide for vp9 for more details](https://trac.ffmpeg.org/wiki/Encode/VP9#constrainedq)
+  ///     see [ffmpeg encoding guide for vp9 for more details](https://trac.ffmpeg.org/wiki/Encode/VP9#constrainedq)
   ///   * 0-63 for av1(libaom-av1) (no default), see [ffmpeg encoding guide
-  /// for libaom for more details](https://trac.ffmpeg.org/wiki/Encode/AV1#ConstantQuality)
+  ///     for libaom for more details](https://trac.ffmpeg.org/wiki/Encode/AV1#ConstantQuality)
   ///   * 0-63 for av1(libsvtav1) (default is 30), see [ffmpeg encoding guide
-  /// for svt-av1 for mode details](https://trac.ffmpeg.org/wiki/Encode/AV1#CRF)
-  ///
+  ///     for svt-av1 for mode details](https://trac.ffmpeg.org/wiki/Encode/AV1#CRF)
   pub fn crf(&mut self, crf: u32) -> &mut Self {
     self.arg("-crf:v");
     self.arg(crf.to_string());
@@ -345,18 +344,18 @@ impl FfmpegCommand {
   /// - `none`: Do not use any hardware acceleration (the default).
   /// - `auto`: Automatically select the hardware acceleration method.
   /// - `vdpau`: Use VDPAU (Video Decode and Presentation API for Unix) hardware
-  /// acceleration.
+  ///   acceleration.
   /// - `dxva2`: Use DXVA2 (DirectX Video Acceleration) hardware acceleration.
   /// - `d3d11va`: Use D3D11VA (DirectX Video Acceleration) hardware
   ///   acceleration.
   /// - `vaapi`: Use VAAPI (Video Acceleration API) hardware acceleration.
   /// - `qsv`: Use the Intel QuickSync Video acceleration for video transcoding.
   ///   - Unlike most other values, this option does not enable accelerated
-  /// decoding (that is used automatically whenever a qsv decoder is selected),
-  /// but accelerated transcoding, without copying the frames into the system
-  /// memory.
+  ///     decoding (that is used automatically whenever a qsv decoder is selected),
+  ///     but accelerated transcoding, without copying the frames into the system
+  ///     memory.
   ///   - For it to work, both the decoder and the encoder must support QSV
-  /// acceleration and no filters must be used.
+  ///     acceleration and no filters must be used.
   ///
   /// This option has no effect if the selected hwaccel is not available or not
   /// supported by the chosen decoder.
@@ -606,15 +605,28 @@ impl FfmpegCommand {
     self.inner.get_args()
   }
 
+  /// Appends `-n` (no overwrite) to the args list if needed.
+  /// The interactive "Would you like to overwrite?" prompt is problematic,
+  /// since it won't be parsed by the log parser and the process will appear
+  /// to hang indefinitely without any indication of what's happening.
+  pub fn prevent_overwrite_prompt(&mut self) -> &mut Self {
+    let is_overwrite_arg = |arg| arg == "-y" || arg == "-n" || arg == "-nostdin";
+    if !self.get_args().any(is_overwrite_arg) {
+      self.no_overwrite();
+    }
+    self
+  }
+
   /// Spawn the ffmpeg command as a child process, wrapping it in a
   /// `FfmpegChild` interface.
   ///
-  /// Please note that if the result is not used with [wait()](FfmpegChild::wait)
+  /// Please note that if the result is not used with [`wait()`](FfmpegChild::wait)
   /// the process is not cleaned up correctly resulting in a zombie process
   /// until your main thread exits.
   ///
   /// Identical to `spawn` in [`std::process::Command`].
   pub fn spawn(&mut self) -> io::Result<FfmpegChild> {
+    self.prevent_overwrite_prompt();
     self.inner.spawn().map(FfmpegChild::from_inner)
   }
 
@@ -659,6 +671,7 @@ impl FfmpegCommand {
     // Configure `FfmpegCommand`
     let mut ffmpeg_command = Self { inner };
     ffmpeg_command.set_expected_loglevel();
+    // todo: ffmpeg_command.no_overwrite();
     ffmpeg_command
   }
 


Test patch:
diff --git a/src/test.rs b/src/test.rs
index c660688..c92790d 100644
--- a/src/test.rs
+++ b/src/test.rs
@@ -1,3 +1,5 @@
+use std::{sync::mpsc, thread, time::Duration};
+
 use crate::{
   command::{ffmpeg_is_installed, FfmpegCommand},
   event::FfmpegEvent,
@@ -314,3 +316,135 @@ fn test_filter_complex() {
     .count();
   assert!(num_frames == 5);
 }
+
+/// Should not hang prompting for user input on overwrite
+/// https://github.com/nathanbabcock/ffmpeg-sidecar/issues/35
+#[test]
+fn test_overwrite_fallback() -> anyhow::Result<()> {
+  let output_path = "output/test_overwrite_fallback.jpg";
+  let timeout_ms = 1000;
+
+  let write_file_with_timeout = || {
+    let mut command = FfmpegCommand::new();
+    command.testsrc().frames(1).output(output_path);
+    spawn_with_timeout(&mut command, timeout_ms)
+  };
+
+  write_file_with_timeout()?;
+  let time1 = std::fs::metadata(output_path)?.modified()?;
+
+  write_file_with_timeout()?;
+  let time2 = std::fs::metadata(output_path)?.modified()?;
+
+  assert_eq!(time1, time2);
+
+  Ok(())
+}
+
+#[test]
+fn test_overwrite_nostdin() -> anyhow::Result<()> {
+  let output_path = "output/test_overwrite_nostdin.jpg";
+
+  let write_file = || -> anyhow::Result<_> {
+    FfmpegCommand::new()
+      .arg("-nostdin")
+      .testsrc()
+      .frames(1)
+      .output(output_path)
+      .spawn()?
+      .wait()
+      .map_err(Into::into)
+  };
+
+  write_file()?;
+  let time1 = std::fs::metadata(output_path)?.modified()?;
+
+  write_file()?;
+  let time2 = std::fs::metadata(output_path)?.modified()?;
+
+  assert_eq!(time1, time2);
+
+  Ok(())
+}
+
+#[test]
+fn test_overwrite() -> anyhow::Result<()> {
+  let output_path = "output/test_overwrite.jpg";
+
+  let write_file = || -> anyhow::Result<_> {
+    FfmpegCommand::new()
+      .overwrite()
+      .testsrc()
+      .frames(1)
+      .output(output_path)
+      .spawn()?
+      .wait()
+      .map_err(Into::into)
+  };
+
+  write_file()?;
+  let time1 = std::fs::metadata(output_path)?.modified()?;
+
+  write_file()?;
+  let time2 = std::fs::metadata(output_path)?.modified()?;
+
+  assert_ne!(time1, time2);
+
+  Ok(())
+}
+
+#[test]
+fn test_no_overwrite() -> anyhow::Result<()> {
+  let output_path = "output/test_no_overwrite.jpg"; // same file, ok if it exists
+
+  let write_file = || -> anyhow::Result<_> {
+    FfmpegCommand::new()
+      .no_overwrite()
+      .testsrc()
+      .frames(1)
+      .output(output_path)
+      .spawn()?
+      .wait()
+      .map_err(Into::into)
+  };
+
+  write_file()?;
+  let time1 = std::fs::metadata(output_path)?.modified()?;
+
+  write_file()?;
+  let time2 = std::fs::metadata(output_path)?.modified()?;
+
+  assert_eq!(time1, time2);
+
+  Ok(())
+}
+
+/// Returns `Err` if the timeout thread finishes before the FFmpeg process
+fn spawn_with_timeout(command: &mut FfmpegCommand, timeout: u64) -> anyhow::Result<()> {
+  let (sender, receiver) = mpsc::channel();
+
+  // Thread 1: Waits for 1000ms and sends a message
+  let timeout_sender = sender.clone();
+  thread::spawn(move || {
+    thread::sleep(Duration::from_millis(timeout));
+    timeout_sender.send("timeout").ok();
+  });
+
+  // Thread 2: Consumes the FFmpeg events and sends a message
+  let mut ffmpeg_child = command.spawn()?;
+  let iter = ffmpeg_child.iter()?;
+  thread::spawn(move || {
+    iter.for_each(|_| {});
+    // Note: `.wait()` would not work here, because it closes `stdin` automatically
+    sender.send("ffmpeg").ok();
+  });
+
+  // Race the two threads
+  let finished_first = receiver.recv()?;
+  ffmpeg_child.kill()?;
+  match finished_first {
+    "timeout" => anyhow::bail!("Timeout thread expired before FFmpeg"),
+    "ffmpeg" => Ok(()),
+    _ => anyhow::bail!("Unknown message received"),
+  }
+}