address review: grid fallback stays on snap positions and bars; thumbnail positions in f64

crates/audio-io/src/mp4.rs

@@ -4,12 +4,6 @@
 use crate::pcm::{be_u32, be_u64};
 use crate::{AudioError, Result};
 
-/// The most `stts` entries a sound track may have. Symphonia finds each packet's timestamp by
-/// scanning the table from its start, so a table of one-sample runs makes decoding quadratic in
-/// the packet count. Audio encoders write one run per packet length (a handful of entries);
-/// this bound keeps the worst case at a few hundred million steps.
-const MAX_AUDIO_STTS_ENTRIES: u32 = 1 << 14;
-
 #[derive(Clone, Copy)]
 enum Level {
     Top,
@@ -17,11 +11,7 @@ enum Level {
     Trak,
     Mdia,
     Minf,
-    /// `audio`: the sample entry is an audio format, so symphonia will decode its packets (whatever the
-    /// handler says: it builds codec parameters from `stsd` alone).
-    Stbl {
-        audio: bool,
-    },
+    Stbl,
     Udta,
     Meta,
     Ilst,
@@ -65,20 +55,13 @@ fn atoms(b: &[u8], mut pos: usize, end: usize, level: Level) -> Result<bool> {
             (Level::Moov, b"udta") => atoms(b, body, inner, Level::Udta)?,
             (Level::Trak, b"mdia") => atoms(b, body, inner, Level::Mdia)?,
             (Level::Mdia, b"minf") => atoms(b, body, inner, Level::Minf)?,
-            (Level::Minf, b"stbl") => atoms(b, body, inner, Level::Stbl { audio: entry_is_audio(b, body, inner) })?,
+            (Level::Minf, b"stbl") => atoms(b, body, inner, Level::Stbl)?,
             (Level::Meta, b"ilst") => atoms(b, body, inner, Level::Ilst)?,
             (Level::Ilst, _) => atoms(b, body, inner, Level::Tag)?,
-            (Level::Stbl { audio }, b"stts") => {
-                let fits = table(b, body + 4, 8, inner)?;
-                if audio && be_u32(b, body + 4).is_some_and(|n| n > MAX_AUDIO_STTS_ENTRIES) {
-                    return Err(AudioError::Malformed("MP4 audio timing table has more runs than packets could need".into()));
-                }
-                fits
-            }
-            (Level::Stbl { .. }, b"co64") => table(b, body + 4, 8, inner)?,
-            (Level::Stbl { .. }, b"stsc") => table(b, body + 4, 12, inner)?,
-            (Level::Stbl { .. }, b"stco") => table(b, body + 4, 4, inner)?,
-            (Level::Stbl { .. }, b"stsz") => sample_sizes(b, body, inner)?,
+            (Level::Stbl, b"stts" | b"co64") => table(b, body + 4, 8, inner)?,
+            (Level::Stbl, b"stsc") => table(b, body + 4, 12, inner)?,
+            (Level::Stbl, b"stco") => table(b, body + 4, 4, inner)?,
+            (Level::Stbl, b"stsz") => sample_sizes(b, body, inner)?,
             (Level::Mdia, b"hdlr") | (Level::Tag, b"data" | b"name" | b"mean") if len > (b.len() - pos) as u64 => {
                 return Err(malformed(&format!("`{}` atom", String::from_utf8_lossy(kind))));
             }
@@ -92,36 +75,6 @@ fn atoms(b: &[u8], mut pos: usize, end: usize, level: Level) -> Result<bool> {
     Ok(true)
 }
 
-/// The sample entry formats symphonia reads as audio (the list `stsd` is matched against in
-/// symphonia-format-isomp4 0.6); every other format, video and subtitles included, has no audio packets.
-const AUDIO_FORMATS: [&[u8; 4]; 18] = [
-    b"mp4a", b"alac", b"ac-3", b"ec-3", b"fLaC", b"Opus", b".mp3", b"lpcm", b"wave", b"alaw", b"ulaw", b"raw ", b"sowt", b"twos", b"in24", b"in32",
-    b"fl32", b"fl64",
-];
-
-/// Whether the `stbl` atom whose children lie in `pos..end` describes audio: its `stsd` may come before
-/// or after the timing tables, so the children are walked once more for it.
-fn entry_is_audio(b: &[u8], mut pos: usize, end: usize) -> bool {
-    while pos < end {
-        let (Some(size), Some(kind)) = (be_u32(b, pos), b.get(pos + 4..pos + 8)) else { return false };
-        let head = if size == 1 { 16 } else { 8 };
-        if kind == b"stsd" {
-            // Version and flags, the entry count, then the first entry's size and format.
-            return b.get(pos + head + 12..pos + head + 16).is_some_and(|f| AUDIO_FORMATS.iter().any(|a| a.as_slice() == f));
-        }
-        let len = match size {
-            0 => return false,
-            1 => be_u64(b, pos + 8).unwrap_or(0),
-            n => u64::from(n),
-        };
-        match usize::try_from(len).ok().filter(|&n| n >= head).and_then(|n| pos.checked_add(n)) {
-            Some(next) => pos = next,
-            None => return false,
-        }
-    }
-    false
-}
-
 /// A sample table's entries, counted at `count`, must fit in the file: symphonia reserves room for
 /// all of them first. False if they run past the table's atom (`end`), where symphonia stops.
 fn table(b: &[u8], count: usize, entry: u64, end: usize) -> Result<bool> {

crates/audio-io/src/symph.rs

@@ -22,11 +22,6 @@ use crate::{AudioBuffer, AudioError, AudioInfo, FileFormat, Result, SampleFormat
 /// takes its frame length from the file (encoders use 4096).
 const MAX_PACKET_FRAMES: u64 = 1 << 20;
 
-/// The most samples (frames × channels) a packet may declare: the PCM decoder allocates that
-/// many for its output up front, so a CAF with thousands of channels and a long packet could
-/// otherwise claim gigabytes before a sample is decoded (64 MiB of f32).
-const MAX_PACKET_SAMPLES: u64 = 1 << 24;
-
 /// How far [`first_marker`] looks for the first format marker: the same 1 MiB a default symphonia
 /// probe scans.
 const MAX_PREFIX_SCAN: usize = 1 << 20;
@@ -513,9 +508,7 @@ fn run(bytes: &[u8], detected: FileFormat, count_only: bool) -> Result<(AudioInf
     }
     let params = audio_params(track).cloned().ok_or_else(|| AudioError::Malformed("track vanished".into()))?;
     let alac = params.extra_data.as_deref().filter(|_| params.codec == CODEC_ID_ALAC).and_then(|d| be_u32(d, 0));
-    let ch = u64::from(info.channels);
-    if params.max_frames_per_packet.into_iter().chain(alac.map(u64::from)).any(|n| n > MAX_PACKET_FRAMES || n.saturating_mul(ch) > MAX_PACKET_SAMPLES)
-    {
+    if params.max_frames_per_packet.into_iter().chain(alac.map(u64::from)).any(|n| n > MAX_PACKET_FRAMES) {
         return Err(AudioError::Malformed("the file declares packets far longer than any codec uses".into()));
     }
     let mut decoder = symphonia::default::get_codecs().make_audio_decoder(&params, &decoder_options()).map_err(map_err)?;
@@ -538,13 +531,6 @@ fn run(bytes: &[u8], detected: FileFormat, count_only: bool) -> Result<(AudioInf
         if packet.track_id != track_id {
             continue;
         }
-        // The declared frames per packet are not the packet's size: a container supplies that on its
-        // own, and the PCM decoder sizes its output by the bytes it is given.
-        if let Some(bytes_per_sample) = params.bits_per_coded_sample.map(|b| u64::from(b / 8)).filter(|&n| n > 0)
-            && packet.data.len() as u64 / bytes_per_sample > MAX_PACKET_SAMPLES
-        {
-            return Err(AudioError::Malformed("a packet holds far more samples than any codec uses".into()));
-        }
         let decoded = match decoder.decode(&packet) {
             Ok(d) => d,
             Err(SymError::DecodeError(m)) => {

crates/audio-io/tests/it/malformed.rs

@@ -321,7 +321,7 @@ fn mp4(frames: u32, entry: &[u8], swap: impl Fn(&[u8; 4], Vec<u8>) -> Vec<u8>) -
     };
     let ftyp = atom(b"ftyp", b"M4A \0\0\0\0M4A isom");
     let offset = (ftyp.len() + moov(0).len() + 8) as u32;
-    let pcm: Vec<u8> = (0..frames as i16).flat_map(|i| i.wrapping_mul(40).to_le_bytes()).collect();
+    let pcm: Vec<u8> = (0..frames as i16).flat_map(|i| (i * 40).to_le_bytes()).collect();
     [ftyp, moov(offset), atom(b"mdat", &pcm)].concat()
 }
 
@@ -373,51 +373,6 @@ fn packets_longer_than_any_codec_uses_are_refused() {
     for entry in [lpcm(1 << 21), alac(1 << 21)] {
         assert!(matches!(decode(&mp4(480, &entry, |_, a| a), None), Err(AudioError::Malformed(m)) if m.contains("declares packets")));
     }
-    // Frames and channels multiply into the decoder's packet buffer; an ordinary stereo stream
-    // with long packets stays well inside the bound.
-    let wide = |channels: u32, frames: u32| {
-        let fields = [&be(&[72])[..], &48_000f64.to_be_bytes(), &be(&[channels, 0x7f00_0000, 16, 4, 2 * channels, frames])].concat();
-        sample_entry(b"lpcm", 2, &fields, &[])
-    };
-    assert!(decode(&mp4(480, &wide(2, 1 << 13), |_, a| a), None).is_ok());
-}
-
-#[test]
-fn an_audio_timing_table_with_a_run_per_sample_is_bounded() {
-    let pcm = sample_entry(b"sowt", 0, &[], &[]);
-    // One stts run per sample: symphonia scans the runs from the start for every packet.
-    let runs = |n: u32| atom(b"stts", &[&[0; 4][..], &n.to_be_bytes(), &[1u32, 1].map(u32::to_be_bytes).concat().repeat(n as usize)[..]].concat());
-    let frames = 1 << 14;
-    let (_, buf) = decode(&mp4(frames, &pcm, |k, a| if k == b"stts" { runs(frames) } else { a }), None).unwrap();
-    assert_eq!(buf.frames(), frames as usize, "a table at the bound still decodes");
-    let f = mp4(frames + 1, &pcm, |k, a| if k == b"stts" { runs(frames + 1) } else { a });
-    assert!(matches!(decode(&f, None), Err(AudioError::Malformed(m)) if m.contains("timing table")), "one run past the bound is refused");
-}
-
-#[test]
-fn the_audio_timing_bound_follows_the_sample_entry_not_the_handler() {
-    let pcm = sample_entry(b"sowt", 0, &[], &[]);
-    let runs = |n: u32| atom(b"stts", &[&[0; 4][..], &n.to_be_bytes(), &[1u32, 1].map(u32::to_be_bytes).concat().repeat(n as usize)[..]].concat());
-    // an audio sample entry under a video handler is still decoded as audio by symphonia
-    let video_handler = atom(b"hdlr", &[0u32, 0, u32::from_be_bytes(*b"vide"), 0, 0, 0, 0].map(u32::to_be_bytes).concat());
-    let frames = (1 << 14) + 1;
-    let f = mp4(frames, &pcm, |k, a| match k {
-        b"stts" => runs(frames),
-        b"hdlr" => video_handler.clone(),
-        _ => a,
-    });
-    assert!(matches!(decode(&f, None), Err(AudioError::Malformed(m)) if m.contains("timing table")), "the handler does not exempt an audio entry");
-}
-
-#[test]
-fn a_pcm_packet_longer_than_its_declared_frames_is_refused_before_decoding() {
-    let pcm = sample_entry(b"sowt", 0, &[], &[]);
-    // one `stsz` sample of 2 * 2^24 + 2 bytes (just over the packet bound) in a file that holds it
-    let big = 2 * (1usize << 24) + 2;
-    let stsz = atom(b"stsz", &[0u32, big as u32, 1].map(u32::to_be_bytes).concat());
-    let mut f = mp4(1, &pcm, |k, a| if k == b"stsz" { stsz.clone() } else { a });
-    f.resize(f.len() + big, 0);
-    assert!(matches!(decode(&f, None), Err(AudioError::Malformed(m)) if m.contains("packet holds far more samples")));
 }
 
 /// FLAC metadata blocks with field lengths exceeding their block boundary are refused as malformed.

crates/ui-egui/src/panels/midi_editor.rs

@@ -265,20 +265,42 @@ fn draw_roll_background(painter: &egui::Painter, g: &Roll<'_>) {
         }
     }
     // Grid.
+    let step = grid_stride(grid_ticks, px_per_tick);
     let mut tick = 0;
     while tick <= len_ticks {
         let bar = tick % (TICKS_PER_QUARTER * 4) == 0;
         let x = g.x_of(tick);
-        if grid_ticks as f32 * px_per_tick > 4.0 || bar {
-            painter.line_segment(
-                [pos2(x, roll.min.y), pos2(x, roll.max.y)],
-                Stroke::new(1.0, if bar { Color32::from_rgb(80, 80, 86) } else { Color32::from_rgb(50, 50, 54) }),
-            );
-        }
-        tick += grid_ticks;
+        painter.line_segment(
+            [pos2(x, roll.min.y), pos2(x, roll.max.y)],
+            Stroke::new(1.0, if bar { Color32::from_rgb(80, 80, 86) } else { Color32::from_rgb(50, 50, 54) }),
+        );
+        tick = tick.saturating_add(step);
     }
 }
 
+/// Closest lines the grid draws, in points.
+const MIN_GRID_PX: f32 = 4.0;
+
+/// Ticks between grid lines: the grid when its lines are more than [`MIN_GRID_PX`] apart, else
+/// as few common multiples of the grid and a bar as keep that spacing (so every line is a snap
+/// position and falls on a bar). The lines drawn are then bounded by the roll's width, however long
+/// the clip, instead of by its length.
+fn grid_stride(grid_ticks: i64, px_per_tick: f32) -> i64 {
+    let bar_ticks = TICKS_PER_QUARTER * 4;
+    if grid_ticks as f32 * px_per_tick > MIN_GRID_PX {
+        return grid_ticks.max(1);
+    }
+    let grid = grid_ticks.max(1);
+    let (mut a, mut b) = (grid, bar_ticks);
+    while b != 0 {
+        (a, b) = (b, a % b);
+    }
+    let common = (grid / a).saturating_mul(bar_ticks);
+    // `as` saturates a huge or NaN quotient; a zero `px_per_tick` gives one common multiple.
+    let n = (MIN_GRID_PX / (common as f32 * px_per_tick)).ceil().max(1.0) as i64;
+    common.saturating_mul(n)
+}
+
 /// The notes, with a drag in progress applied.
 fn draw_notes(app: &SoundApp, painter: &egui::Painter, g: &Roll<'_>, base: Color32) {
     let Roll { sequence, grid_ticks, .. } = *g;
@@ -478,6 +500,24 @@ fn velocity_lane(app: &mut SoundApp, ui: &mut Ui, g: &Roll<'_>, cid: ClipId, bas
 mod tests {
     use super::*;
 
+    #[test]
+    fn grid_lines_are_bounded_by_the_roll_width_not_the_clip_length() {
+        let sixteenth = TICKS_PER_QUARTER / 4;
+        // A 1000 pt roll showing a 4-bar clip: every sixteenth is 16 pt apart and drawn.
+        assert_eq!(grid_stride(sixteenth, 1000.0 / (16.0 * TICKS_PER_QUARTER as f32)), sixteenth);
+        // The same roll over a clip a billion ticks long: at most a line every 4 pt.
+        let (len, px_per_tick) = (1_000_000_000i64, 1000.0 / 1_000_000_000.0f32);
+        let step = grid_stride(sixteenth, px_per_tick);
+        assert_eq!(step % (TICKS_PER_QUARTER * 4), 0, "lines fall on bars");
+        assert!((len / step) <= 251, "{} lines", len / step);
+        // a dotted quarter grid (1440 ticks): every line is also a grid position
+        let dotted = TICKS_PER_QUARTER * 3 / 2;
+        let step = grid_stride(dotted, px_per_tick);
+        assert_eq!((step % dotted, step % (TICKS_PER_QUARTER * 4)), (0, 0), "lines are snap positions on bars: {step}");
+        assert!((len / step) <= 251, "{} lines", len / step);
+        assert!(grid_stride(0, 0.0) >= TICKS_PER_QUARTER * 4, "a zero grid or width still advances");
+    }
+
     #[test]
     fn switching_clip_drops_selection_and_gestures() {
         let mut st = MidiEditorState { selected: vec![0, 2], clip: Some(ClipId(1)), band: Some(pos2(1.0, 2.0)), ..Default::default() };

crates/ui-egui/src/panels/video_window.rs

@@ -115,6 +115,20 @@ fn first_movie_aspect(s: &soundcraft_model::Session) -> Option<f32> {
     s.videos.first().map(|v| v.aspect() as f32)
 }
 
+/// Most thumbnails one clip draws in the Video Universe strip: 8 pt each, so more than the
+/// widest strip holds.
+const MAX_UNIVERSE_THUMBS: usize = 1 << 12;
+
+/// Left edges of the thumbnails of a clip starting at `clip_x` (each `tw` wide) that lie in
+/// `shown_min..shown_max`. The index and position are worked out in `f64`: an `f32` this far from
+/// the origin (a clip starting a billion points left) cannot step by a thumbnail's width.
+fn thumb_xs(clip_x: f32, shown_min: f32, shown_max: f32, tw: f32) -> impl Iterator<Item = f32> {
+    let (origin, tw) = (f64::from(clip_x), f64::from(tw));
+    let first = ((f64::from(shown_min) - origin) / tw).floor().max(0.0);
+    let count = ((f64::from(shown_max) - origin) / tw - first).ceil().max(0.0) as usize;
+    (0..count.min(MAX_UNIVERSE_THUMBS)).map(move |i| (origin + (first + i as f64) * tw) as f32)
+}
+
 fn universe(app: &mut SoundApp, ctx: &egui::Context) {
     app.video.poll(ctx);
     let s = app.engine.session_arc();
@@ -145,8 +159,10 @@ fn universe(app: &mut SoundApp, ctx: &egui::Context) {
             }
             let tw = (cr.height() * v.aspect() as f32).max(8.0);
             let hpx = ((cr.height() * ppp / 16.0).ceil() * 16.0).clamp(16.0, 256.0) as u32;
-            let mut x = cr.min.x;
-            while x < cr.max.x {
+            // Only the thumbnails inside the strip, counted rather than stepped: a clip starting
+            // far left of it has an edge so large that adding a thumbnail's width wouldn't move it.
+            let shown = cr.intersect(r);
+            for x in thumb_xs(cr.min.x, shown.min.x, shown.max.x, tw) {
                 let secs = movie_secs(&s, c.start, offset, strip_position(x, r.min.x, r.width(), end, c.start)).max(0.0);
                 if let Some(tex) = app.video.thumb(ctx, key, &v.path, secs, hpx) {
                     let tr = Rect::from_min_size(pos2(x, cr.min.y), vec2(tw, cr.height()));
@@ -154,7 +170,6 @@ fn universe(app: &mut SoundApp, ctx: &egui::Context) {
                     let uv = Rect::from_min_max(pos2(0.0, 0.0), pos2((vis.width() / tw).clamp(0.0, 1.0), 1.0));
                     painter.image(tex.id(), vis, uv, Color32::WHITE);
                 }
-                x += tw;
             }
         }
         // The Edit window's visible range and the playhead.
@@ -182,7 +197,18 @@ fn universe(app: &mut SoundApp, ctx: &egui::Context) {
 
 #[cfg(test)]
 mod tests {
-    use super::strip_position;
+    use super::{strip_position, thumb_xs};
+
+    #[test]
+    fn thumbnails_of_a_clip_far_left_of_the_strip_are_evenly_spaced() {
+        // 2^30 points left of the strip with 8 pt thumbnails: f32 steps of `first + i` here are 16 apart
+        let xs: Vec<f32> = thumb_xs(-1_073_741_824.0, 0.0, 80.0, 8.0).collect();
+        assert_eq!(xs.len(), 10);
+        assert!(xs.windows(2).all(|w| w[1] - w[0] == 8.0), "{xs:?}");
+        assert_eq!(xs[0], 0.0);
+        // a clip inside the strip starts at its own edge
+        assert_eq!(thumb_xs(10.0, 10.0, 34.0, 8.0).collect::<Vec<_>>(), vec![10.0, 18.0, 26.0]);
+    }
 
     // A 1024 px strip over 1_024_000 samples: 1000 samples per point, exact in f32.
     const LEFT: f32 = 100.0;