fix: stabilize desktop audio playback

crates/nesemu-desktop/src/main.rs

@@ -3,23 +3,25 @@ use std::path::{Path, PathBuf};
 use std::rc::Rc;
 use std::sync::atomic::{AtomicU32, Ordering as AtomicOrdering};
 use std::sync::Arc;
-use std::time::Duration;
+use std::time::{Duration, Instant};
 
-use gtk::gio;
 use gtk::gdk;
+use gtk::gio;
 use gtk::glib;
 use gtk::prelude::*;
 use gtk4 as gtk;
 use nesemu::prelude::{EmulationState, HostConfig, RuntimeHostLoop};
 use nesemu::{
-    FRAME_HEIGHT, FRAME_RGBA_BYTES, FRAME_WIDTH, FrameClock, InputProvider, JoypadButton,
-    JoypadButtons, NesRuntime, RingBuffer, set_button_pressed,
+    set_button_pressed, FrameClock, InputProvider, JoypadButton, JoypadButtons, NesRuntime,
+    RingBuffer, VideoMode, VideoOutput, FRAME_HEIGHT, FRAME_RGBA_BYTES, FRAME_WIDTH,
 };
 
 const APP_ID: &str = "org.nesemu.desktop";
 const TITLE: &str = "NES Emulator";
 const SCALE: i32 = 3;
 const SAMPLE_RATE: u32 = 48_000;
+const AUDIO_RING_CAPACITY: usize = 1536;
+const AUDIO_CALLBACK_FRAMES: u32 = 256;
 
 fn main() {
     if std::env::var_os("GSK_RENDERER").is_none() {
@@ -28,9 +30,7 @@ fn main() {
         }
     }
 
-    let app = gtk::Application::builder()
-        .application_id(APP_ID)
-        .build();
+    let app = gtk::Application::builder().application_id(APP_ID).build();
 
     let initial_rom: Rc<RefCell<Option<PathBuf>>> =
         Rc::new(RefCell::new(std::env::args().nth(1).map(PathBuf::from)));
@@ -125,16 +125,17 @@ fn build_ui(app: &gtk::Application, initial_rom: Option<PathBuf>) {
 
     // --- State ---
     let desktop = Rc::new(RefCell::new(DesktopApp::new(Arc::clone(&volume))));
-    let frame_for_draw: Rc<RefCell<Vec<u8>>> =
-        Rc::new(RefCell::new(vec![0u8; FRAME_RGBA_BYTES]));
+    let frame_for_draw: Rc<RefCell<Vec<u8>>> = Rc::new(RefCell::new(vec![0u8; FRAME_RGBA_BYTES]));
+    let scheduler = Rc::new(RefCell::new(DesktopFrameScheduler::new()));
 
     // --- Draw function (pixel-perfect nearest-neighbor) ---
     {
         let frame_for_draw = Rc::clone(&frame_for_draw);
         drawing_area.set_draw_func(move |_da, cr, width, height| {
             let frame = frame_for_draw.borrow();
-            let stride =
-                cairo::Format::ARgb32.stride_for_width(FRAME_WIDTH as u32).unwrap();
+            let stride = cairo::Format::ARgb32
+                .stride_for_width(FRAME_WIDTH as u32)
+                .unwrap();
             let mut argb = vec![0u8; stride as usize * FRAME_HEIGHT];
             for y in 0..FRAME_HEIGHT {
                 for x in 0..FRAME_WIDTH {
@@ -223,6 +224,7 @@ fn build_ui(app: &gtk::Application, initial_rom: Option<PathBuf>) {
     // --- Open ROM handler ---
     let do_open_rom = {
         let desktop = Rc::clone(&desktop);
+        let scheduler = Rc::clone(&scheduler);
         let sync_ui = Rc::clone(&sync_ui);
         let window = window.clone();
         Rc::new(move || {
@@ -244,6 +246,7 @@ fn build_ui(app: &gtk::Application, initial_rom: Option<PathBuf>) {
             chooser.add_filter(&all_filter);
 
             let desktop = Rc::clone(&desktop);
+            let scheduler = Rc::clone(&scheduler);
             let sync_ui = Rc::clone(&sync_ui);
             chooser.connect_response(move |dialog, response| {
                 if response == gtk::ResponseType::Accept {
@@ -252,6 +255,7 @@ fn build_ui(app: &gtk::Application, initial_rom: Option<PathBuf>) {
                         if let Err(err) = app_state.load_rom_from_path(&path) {
                             eprintln!("Failed to load ROM '{}': {err}", path.display());
                         } else {
+                            scheduler.borrow_mut().reset_timing();
                             let name = rom_filename(&path);
                             sync_ui(&app_state, Some(&name));
                         }
@@ -273,20 +277,24 @@ fn build_ui(app: &gtk::Application, initial_rom: Option<PathBuf>) {
 
     {
         let desktop = Rc::clone(&desktop);
+        let scheduler = Rc::clone(&scheduler);
         let sync_ui = Rc::clone(&sync_ui);
         pause_button.connect_clicked(move |_| {
             let mut app_state = desktop.borrow_mut();
             app_state.toggle_pause();
+            scheduler.borrow_mut().reset_timing();
             sync_ui(&app_state, None);
         });
     }
 
     {
         let desktop = Rc::clone(&desktop);
+        let scheduler = Rc::clone(&scheduler);
         let sync_ui = Rc::clone(&sync_ui);
         reset_button.connect_clicked(move |_| {
             let mut app_state = desktop.borrow_mut();
             app_state.reset();
+            scheduler.borrow_mut().reset_timing();
             sync_ui(&app_state, None);
         });
     }
@@ -305,11 +313,13 @@ fn build_ui(app: &gtk::Application, initial_rom: Option<PathBuf>) {
     let action_pause = gio::SimpleAction::new("toggle-pause", None);
     {
         let desktop = Rc::clone(&desktop);
+        let scheduler = Rc::clone(&scheduler);
         let sync_ui = Rc::clone(&sync_ui);
         action_pause.connect_activate(move |_, _| {
             let mut app_state = desktop.borrow_mut();
             if app_state.is_loaded() {
                 app_state.toggle_pause();
+                scheduler.borrow_mut().reset_timing();
                 sync_ui(&app_state, None);
             }
         });
@@ -320,11 +330,13 @@ fn build_ui(app: &gtk::Application, initial_rom: Option<PathBuf>) {
     let action_reset = gio::SimpleAction::new("reset", None);
     {
         let desktop = Rc::clone(&desktop);
+        let scheduler = Rc::clone(&scheduler);
         let sync_ui = Rc::clone(&sync_ui);
         action_reset.connect_activate(move |_, _| {
             let mut app_state = desktop.borrow_mut();
             if app_state.is_loaded() {
                 app_state.reset();
+                scheduler.borrow_mut().reset_timing();
                 sync_ui(&app_state, None);
             }
         });
@@ -354,6 +366,7 @@ fn build_ui(app: &gtk::Application, initial_rom: Option<PathBuf>) {
     // --- Drag-and-drop ---
     {
         let desktop = Rc::clone(&desktop);
+        let scheduler = Rc::clone(&scheduler);
         let sync_ui = Rc::clone(&sync_ui);
         let drop_target = gtk::DropTarget::new(gio::File::static_type(), gdk::DragAction::COPY);
         drop_target.connect_drop(move |_, value, _, _| {
@@ -364,6 +377,7 @@ fn build_ui(app: &gtk::Application, initial_rom: Option<PathBuf>) {
                         eprintln!("Failed to load ROM '{}': {err}", path.display());
                         return false;
                     }
+                    scheduler.borrow_mut().reset_timing();
                     let name = rom_filename(&path);
                     sync_ui(&app_state, Some(&name));
                     return true;
@@ -376,23 +390,45 @@ fn build_ui(app: &gtk::Application, initial_rom: Option<PathBuf>) {
 
     // --- Game loop ---
     {
-        let desktop = Rc::clone(&desktop);
-        let drawing_area = drawing_area.clone();
-        let frame_for_draw = Rc::clone(&frame_for_draw);
-        glib::timeout_add_local(Duration::from_millis(16), move || {
+        schedule_game_loop(
+            Rc::clone(&desktop),
+            drawing_area.clone(),
+            Rc::clone(&frame_for_draw),
+            Rc::clone(&scheduler),
+        );
+    }
+
+    window.present();
+}
+
+fn schedule_game_loop(
+    desktop: Rc<RefCell<DesktopApp>>,
+    drawing_area: gtk::DrawingArea,
+    frame_for_draw: Rc<RefCell<Vec<u8>>>,
+    scheduler: Rc<RefCell<DesktopFrameScheduler>>,
+) {
+    let interval = desktop.borrow().frame_interval();
+    let delay = scheduler
+        .borrow_mut()
+        .delay_until_next_frame(Instant::now(), interval);
+
+    glib::timeout_add_local_once(delay, move || {
+        {
             let mut app_state = desktop.borrow_mut();
+            let now = Instant::now();
+            let interval = app_state.frame_interval();
+
+            scheduler.borrow_mut().mark_frame_complete(now, interval);
             app_state.tick();
 
             frame_for_draw
                 .borrow_mut()
                 .copy_from_slice(app_state.frame_rgba());
             drawing_area.queue_draw();
+        }
 
-            glib::ControlFlow::Continue
-        });
-    }
-
-    window.present();
+        schedule_game_loop(desktop, drawing_area, frame_for_draw, scheduler);
+    });
 }
 
 fn rom_filename(path: &Path) -> String {
@@ -445,7 +481,7 @@ struct CpalAudioSink {
 
 impl CpalAudioSink {
     fn new(volume: Arc<AtomicU32>) -> Self {
-        let ring = Arc::new(RingBuffer::new(4096));
+        let ring = Arc::new(RingBuffer::new(AUDIO_RING_CAPACITY));
         let ring_for_cb = Arc::clone(&ring);
         let vol_for_cb = Arc::clone(&volume);
         let stream = Self::try_build_stream(ring_for_cb, vol_for_cb);
@@ -456,10 +492,7 @@ impl CpalAudioSink {
         }
     }
 
-    fn try_build_stream(
-        ring: Arc<RingBuffer>,
-        volume: Arc<AtomicU32>,
-    ) -> Option<cpal::Stream> {
+    fn try_build_stream(ring: Arc<RingBuffer>, volume: Arc<AtomicU32>) -> Option<cpal::Stream> {
         use cpal::traits::{DeviceTrait, HostTrait, StreamTrait};
 
         let host = cpal::default_host();
@@ -471,11 +504,7 @@ impl CpalAudioSink {
             }
         };
 
-        let config = cpal::StreamConfig {
-            channels: 1,
-            sample_rate: cpal::SampleRate(SAMPLE_RATE),
-            buffer_size: cpal::BufferSize::Default,
-        };
+        let config = cpal_stream_config();
 
         let stream = match device.build_output_stream(
             &config,
@@ -523,6 +552,85 @@ impl nesemu::AudioOutput for CpalAudioSink {
     }
 }
 
+#[cfg(test)]
+fn audio_ring_latency_ms(capacity: usize, sample_rate: u32) -> f64 {
+    ((capacity.saturating_sub(1)) as f64 / sample_rate as f64) * 1000.0
+}
+
+#[cfg(test)]
+fn required_audio_ring_capacity(sample_rate: u32, mode: VideoMode) -> usize {
+    let samples_per_frame = (sample_rate as f64 / mode.frame_hz()).ceil() as usize;
+    samples_per_frame + AUDIO_CALLBACK_FRAMES as usize + 1
+}
+
+fn cpal_stream_config() -> cpal::StreamConfig {
+    cpal::StreamConfig {
+        channels: 1,
+        sample_rate: cpal::SampleRate(SAMPLE_RATE),
+        buffer_size: cpal::BufferSize::Fixed(AUDIO_CALLBACK_FRAMES),
+    }
+}
+
+struct DesktopFrameScheduler {
+    next_deadline: Option<Instant>,
+}
+
+impl DesktopFrameScheduler {
+    fn new() -> Self {
+        Self {
+            next_deadline: None,
+        }
+    }
+
+    fn reset_timing(&mut self) {
+        self.next_deadline = None;
+    }
+
+    fn delay_until_next_frame(&mut self, now: Instant, _interval: Duration) -> Duration {
+        match self.next_deadline {
+            None => {
+                self.next_deadline = Some(now);
+                Duration::ZERO
+            }
+            Some(deadline) if now < deadline => deadline - now,
+            Some(_) => Duration::ZERO,
+        }
+    }
+
+    fn mark_frame_complete(&mut self, now: Instant, interval: Duration) {
+        let mut next_deadline = self.next_deadline.unwrap_or(now) + interval;
+        while next_deadline <= now {
+            next_deadline += interval;
+        }
+        self.next_deadline = Some(next_deadline);
+    }
+}
+
+struct BufferedVideo {
+    frame_rgba: Vec<u8>,
+}
+
+impl BufferedVideo {
+    fn new() -> Self {
+        Self {
+            frame_rgba: vec![0; FRAME_RGBA_BYTES],
+        }
+    }
+
+    fn frame_rgba(&self) -> &[u8] {
+        &self.frame_rgba
+    }
+}
+
+impl VideoOutput for BufferedVideo {
+    fn present_rgba(&mut self, frame: &[u8], width: usize, height: usize) {
+        if width != FRAME_WIDTH || height != FRAME_HEIGHT || frame.len() != FRAME_RGBA_BYTES {
+            return;
+        }
+        self.frame_rgba.copy_from_slice(frame);
+    }
+}
+
 // ---------------------------------------------------------------------------
 // Application state
 // ---------------------------------------------------------------------------
@@ -531,7 +639,7 @@ struct DesktopApp {
     host: Option<RuntimeHostLoop<Box<dyn FrameClock>>>,
     input: InputState,
     audio: CpalAudioSink,
-    frame_rgba: Vec<u8>,
+    video: BufferedVideo,
     state: EmulationState,
 }
 
@@ -541,7 +649,7 @@ impl DesktopApp {
             host: None,
             input: InputState::default(),
             audio: CpalAudioSink::new(volume),
-            frame_rgba: vec![0; FRAME_RGBA_BYTES],
+            video: BufferedVideo::new(),
             state: EmulationState::Paused,
         }
     }
@@ -589,23 +697,137 @@ impl DesktopApp {
             return;
         };
 
-        let mut null_video = nesemu::NullVideo;
-        if let Err(err) = host.run_frame_unpaced(&mut self.input, &mut null_video, &mut self.audio)
-        {
-            eprintln!("Frame execution error: {err}");
-            self.state = EmulationState::Paused;
-            return;
+        match host.run_frame_unpaced(&mut self.input, &mut self.video, &mut self.audio) {
+            Ok(_) => {}
+            Err(err) => {
+                eprintln!("Frame execution error: {err}");
+                self.state = EmulationState::Paused;
+                return;
+            }
         }
-
-        self.frame_rgba
-            .copy_from_slice(&host.runtime().frame_rgba());
     }
 
     fn frame_rgba(&self) -> &[u8] {
-        &self.frame_rgba
+        self.video.frame_rgba()
+    }
+
+    fn frame_interval(&self) -> Duration {
+        self.host
+            .as_ref()
+            .map(|host| host.runtime().video_mode().frame_duration())
+            .unwrap_or_else(|| VideoMode::Ntsc.frame_duration())
     }
 
     fn input_mut(&mut self) -> &mut InputState {
         &mut self.input
     }
 }
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+    use nesemu::{VideoOutput, FRAME_HEIGHT, FRAME_WIDTH};
+    use std::time::Instant;
+
+    #[test]
+    fn frame_scheduler_waits_until_frame_deadline() {
+        let mut scheduler = DesktopFrameScheduler::new();
+        let start = Instant::now();
+        let interval = Duration::from_micros(16_639);
+
+        assert_eq!(
+            scheduler.delay_until_next_frame(start, interval),
+            Duration::ZERO
+        );
+        scheduler.mark_frame_complete(start, interval);
+        assert!(
+            scheduler.delay_until_next_frame(start + Duration::from_millis(1), interval)
+                > Duration::ZERO
+        );
+        assert_eq!(
+            scheduler.delay_until_next_frame(start + interval, interval),
+            Duration::ZERO
+        );
+    }
+
+    #[test]
+    fn buffered_video_captures_presented_frame() {
+        let mut video = BufferedVideo::new();
+        let mut frame = vec![0u8; FRAME_RGBA_BYTES];
+        frame[0] = 0x12;
+        frame[1] = 0x34;
+        frame[2] = 0x56;
+        frame[3] = 0x78;
+
+        video.present_rgba(&frame, FRAME_WIDTH, FRAME_HEIGHT);
+
+        assert_eq!(video.frame_rgba(), frame.as_slice());
+    }
+
+    #[test]
+    fn frame_scheduler_reset_restarts_from_immediate_tick() {
+        let mut scheduler = DesktopFrameScheduler::new();
+        let start = Instant::now();
+        let interval = Duration::from_micros(16_639);
+
+        assert_eq!(
+            scheduler.delay_until_next_frame(start, interval),
+            Duration::ZERO
+        );
+        scheduler.mark_frame_complete(start, interval);
+        assert!(scheduler.delay_until_next_frame(start, interval) > Duration::ZERO);
+
+        scheduler.reset_timing();
+        assert_eq!(
+            scheduler.delay_until_next_frame(start, interval),
+            Duration::ZERO
+        );
+    }
+
+    #[test]
+    fn frame_scheduler_reports_zero_delay_when_late() {
+        let mut scheduler = DesktopFrameScheduler::new();
+        let start = Instant::now();
+        let interval = Duration::from_micros(16_639);
+
+        assert_eq!(
+            scheduler.delay_until_next_frame(start, interval),
+            Duration::ZERO
+        );
+        scheduler.mark_frame_complete(start, interval);
+
+        assert_eq!(
+            scheduler.delay_until_next_frame(start + interval + Duration::from_millis(2), interval),
+            Duration::ZERO
+        );
+    }
+
+    #[test]
+    fn desktop_audio_ring_budget_stays_below_25ms() {
+        let latency_ms = audio_ring_latency_ms(AUDIO_RING_CAPACITY, SAMPLE_RATE);
+        let max_budget_ms = 40.0;
+        assert!(
+            latency_ms <= max_budget_ms,
+            "desktop audio ring latency budget too high: {latency_ms:.2}ms"
+        );
+    }
+
+    #[test]
+    fn desktop_audio_uses_fixed_low_latency_callback_size() {
+        let config = cpal_stream_config();
+        assert_eq!(
+            config.buffer_size,
+            cpal::BufferSize::Fixed(AUDIO_CALLBACK_FRAMES)
+        );
+    }
+
+    #[test]
+    fn desktop_audio_ring_has_frame_burst_headroom() {
+        let required = required_audio_ring_capacity(SAMPLE_RATE, VideoMode::Ntsc);
+        assert!(
+            AUDIO_RING_CAPACITY >= required,
+            "audio ring too small for frame burst: capacity={}, required={required}",
+            AUDIO_RING_CAPACITY,
+        );
+    }
+}