Merge pull request #2 from fetlife/andrii/fix-review-findings

Fix waveform validation and rendering edge cases

Author: Antti

.gitignore

@@ -1,3 +1,4 @@
 build/
 target/
 audiowaveform-rs/
+.cargo-home/

crates/audiowaveform-cli/src/main.rs

@@ -424,7 +424,7 @@ fn run(cli: Cli) -> Result<(), String> {
                 split_channels: cli.split_channels,
                 amplitude_scale: match amplitude {
                     ParsedAmplitudeScale::Auto => Some(AmplitudeScale::Auto),
-                    ParsedAmplitudeScale::Fixed(_) => None,
+                    ParsedAmplitudeScale::Fixed(value) => Some(AmplitudeScale::Fixed(value)),
                 },
             },
         )?;
@@ -556,7 +556,7 @@ fn parse_amplitude_scale(value: &str) -> Result<ParsedAmplitudeScale, String> {
     let parsed = value
         .parse::<f64>()
         .map_err(|_| "Error: Invalid amplitude scale: must be a number".to_string())?;
-    if parsed < 0.0 {
+    if !parsed.is_finite() || parsed < 0.0 {
         Err("Error: Invalid amplitude scale: must be a positive number".to_string())
     } else {
         Ok(ParsedAmplitudeScale::Fixed(parsed))
@@ -666,8 +666,10 @@ fn resolve_raw_audio_config(cli: &Cli) -> Result<RawAudioConfig, String> {
     if channels <= 0 {
         return Err("Invalid number of input channels: must be greater than zero".to_string());
     }
+    let channels = u16::try_from(channels)
+        .map_err(|_| "Invalid number of input channels: maximum 65535".to_string())?;
 
-    RawAudioConfig::new(sample_rate as u32, channels as u16, sample_format).map_err(stringify_error)
+    RawAudioConfig::new(sample_rate as u32, channels, sample_format).map_err(stringify_error)
 }
 
 fn generate_waveform_from_input(

crates/audiowaveform-cli/tests/cli.rs

@@ -1,6 +1,7 @@
 mod support;
 
 use assert_cmd::Command;
+use audiowaveform::Waveform;
 use predicates::prelude::*;
 
 use self::support::{
@@ -55,6 +56,68 @@ fn rejects_invalid_enum_values_via_clap() {
         .stderr(predicate::str::contains("possible values"));
 }
 
+#[test]
+fn rejects_non_finite_numeric_values() {
+    let input = fixture_path("test_file_stereo_8bit_64spp_wav.dat");
+    let input = input.to_str().expect("utf8");
+
+    Command::cargo_bin("audiowaveform")
+        .expect("binary")
+        .args([
+            "-q",
+            "-i",
+            input,
+            "--output-format",
+            "png",
+            "-z",
+            "64",
+            "--start",
+            "inf",
+        ])
+        .assert()
+        .failure()
+        .stderr("Invalid start time: minimum 0\n");
+
+    Command::cargo_bin("audiowaveform")
+        .expect("binary")
+        .args([
+            "-q",
+            "-i",
+            input,
+            "--output-format",
+            "png",
+            "-z",
+            "64",
+            "--amplitude-scale",
+            "NaN",
+        ])
+        .assert()
+        .failure()
+        .stderr("Error: Invalid amplitude scale: must be a positive number\n");
+}
+
+#[test]
+fn rejects_raw_channel_counts_that_do_not_fit_the_library_type() {
+    Command::cargo_bin("audiowaveform")
+        .expect("binary")
+        .args([
+            "-q",
+            "--input-format",
+            "raw",
+            "--output-format",
+            "wav",
+            "--raw-samplerate",
+            "48000",
+            "--raw-channels",
+            "65537",
+            "--raw-format",
+            "s16le",
+        ])
+        .assert()
+        .failure()
+        .stderr("Invalid number of input channels: maximum 65535\n");
+}
+
 #[test]
 fn generates_dat_output_to_file_and_stdout() {
     let output = named_temp_file(".dat");
@@ -133,6 +196,40 @@ fn generates_json_and_text_outputs_to_stdout() {
         .stderr("Done\n");
 }
 
+#[test]
+fn applies_fixed_amplitude_scaling_to_waveform_data_output() {
+    let unscaled_output = named_temp_file(".json");
+    let scaled_output = named_temp_file(".json");
+
+    for (output, amplitude_scale) in [(&unscaled_output, "1.0"), (&scaled_output, "2.0")] {
+        Command::cargo_bin("audiowaveform")
+            .expect("binary")
+            .args([
+                "-q",
+                "-i",
+                fixture_path("test_file_stereo.wav").to_str().expect("utf8"),
+                "-o",
+                output.path().to_str().expect("utf8"),
+                "-z",
+                "64",
+                "--amplitude-scale",
+                amplitude_scale,
+            ])
+            .assert()
+            .success();
+    }
+
+    let unscaled = Waveform::load_from_path(unscaled_output.path(), None).expect("unscaled");
+    let scaled = Waveform::load_from_path(scaled_output.path(), None).expect("scaled");
+    let expected = unscaled
+        .interleaved_samples()
+        .iter()
+        .map(|value| (i32::from(*value) * 2).clamp(i32::from(i16::MIN), i32::from(i16::MAX)) as i16)
+        .collect::<Vec<_>>();
+
+    assert_eq!(scaled.interleaved_samples(), expected);
+}
+
 #[test]
 fn generates_png_output_to_file_and_stdout() {
     let output = named_temp_file(".png");

crates/audiowaveform/src/audio.rs

@@ -168,7 +168,13 @@ impl ScaleSpec {
                     ));
                 }
                 let frames = if let Some((start, end)) = time_range {
-                    if end < start {
+                    if !start.is_finite() || start < 0.0 {
+                        return Err(Error::invalid_argument(
+                            "start time",
+                            "Invalid start time: minimum 0",
+                        ));
+                    }
+                    if !end.is_finite() || end < start {
                         return Err(Error::invalid_argument(
                             "end time",
                             format!("Invalid end time, must be greater than {start}"),
@@ -739,6 +745,25 @@ mod tests {
             "Invalid end time, must be greater than 5"
         );
 
+        let error = ScaleSpec::FitWidth {
+            width_pixels: 400,
+            time_range: Some((f64::INFINITY, 10.0)),
+        }
+        .resolve(48_000, 96_000)
+        .expect_err("non-finite start time");
+        assert_eq!(error.to_string(), "Invalid start time: minimum 0");
+
+        let error = ScaleSpec::FitWidth {
+            width_pixels: 400,
+            time_range: Some((0.0, f64::INFINITY)),
+        }
+        .resolve(48_000, 96_000)
+        .expect_err("non-finite end time");
+        assert_eq!(
+            error.to_string(),
+            "Invalid end time, must be greater than 0"
+        );
+
         let error = ScaleSpec::FitWidth {
             width_pixels: 100_000,
             time_range: None,