From 5d20c892dedce7bc7486acbd72fbd35da69e413e Mon Sep 17 00:00:00 2001 From: skal Date: Wed, 20 May 2026 22:44:44 +0200 Subject: fix: code review cleanup — bugs, dead code, factorization (-167 lines) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Bugs: - B1: fix dead tempo debug (prev_tempo captured after assignment) - B2: fix ReloadAssetsFromFile leak for disk-loaded assets; simplify DropAsset - B3: fix get_free_pool_slot leak (unregister synth + free data on reuse) - B4: volatile -> std::atomic with acquire/release in miniaudio_backend, synth - B5: fix unaligned reads in scene_loader (memcpy-based read_f32/read_u32) - B6: fix shader module + BGL + pipeline layout leaks in gpu.cc, pipeline_builder Dead code: - D1: remove unused particle_defs.h - D3: remove create_post_process_pipeline_simple (zero callers) - D4: remove empty gpu_draw() - D5: remove write-only Hybrid3D::initialized_ - D6: remove legacy pending buffer path in audio.cc Factorization: - F1: Effect::run_fullscreen_pass() replaces boilerplate in 5 effects - F2: particle_common.wgsl snippet, #include in 3 WGSL shaders - F3: gpu_create_shader_module() helper, used in 3 call sites - F5: get_world_aabb() shared between bvh.cc and physics.cc - F6: samples_to_seconds() replaces 6 inline expressions - F7: gpu_create_linear/nearest_sampler use SamplerCache; add nearest() preset 37/37 tests passing. handoff(Claude): code review batch — all items verified, no regressions. --- src/audio/audio.cc | 59 ++++------------------------------ src/audio/backend/miniaudio_backend.cc | 20 ++++++------ src/audio/backend/miniaudio_backend.h | 8 +++-- src/audio/ring_buffer.h | 5 +++ src/audio/synth.cc | 51 ++++++++++++++++------------- src/audio/tracker.cc | 10 +++++- 6 files changed, 64 insertions(+), 89 deletions(-) (limited to 'src/audio') diff --git a/src/audio/audio.cc b/src/audio/audio.cc index 3b98452..91dd05b 100644 --- a/src/audio/audio.cc +++ b/src/audio/audio.cc @@ -28,12 +28,6 @@ static void clip_samples(float* buf, int count) { // Global ring buffer for audio streaming static AudioRingBuffer g_ring_buffer; -// Pending write buffer for partially written samples -// Maximum size: one chunk (533 frames @ 60fps = 1066 samples stereo) -#define MAX_PENDING_SAMPLES 2048 -static float g_pending_buffer[MAX_PENDING_SAMPLES]; -static int g_pending_samples = 0; // How many samples are waiting to be written - // Global backend pointer for audio abstraction static AudioBackend* g_audio_backend = nullptr; static MiniaudioBackend g_default_backend; @@ -56,9 +50,6 @@ void audio_init() { // In production code, use AudioEngine::init() which manages initialization // order. - // Clear pending buffer - g_pending_samples = 0; - // Use default backend if none set if (g_audio_backend == nullptr) { g_audio_backend = &g_default_backend; @@ -74,8 +65,7 @@ float audio_get_required_prefill_time() { bool audio_is_prefilled() { const int buffered = g_ring_buffer.available_read(); - const float buffered_time = - (float)buffered / (RING_BUFFER_SAMPLE_RATE * RING_BUFFER_CHANNELS); + const float buffered_time = samples_to_seconds(buffered); const float required = audio_get_required_prefill_time(); return buffered_time >= (required - 0.001f); // 1ms tolerance } @@ -89,9 +79,7 @@ void audio_start() { #if !defined(STRIP_ALL) if (!audio_is_prefilled()) { const int buffered = g_ring_buffer.available_read(); - const float buffered_ms = (float)buffered / - (RING_BUFFER_SAMPLE_RATE * RING_BUFFER_CHANNELS) * - 1000.0f; + const float buffered_ms = samples_to_seconds(buffered) * 1000.0f; printf("WARNING: Audio buffer not pre-filled (%.1fms < %.1fms)\n", buffered_ms, audio_get_required_prefill_time() * 1000.0f); } @@ -116,40 +104,9 @@ void audio_render_ahead(float music_time, float dt, float target_fill) { // Keep rendering small chunks until buffer is full enough while (true) { - // First, try to flush any pending samples from previous partial writes - if (g_pending_samples > 0) { - const int written = - g_ring_buffer.write(g_pending_buffer, g_pending_samples); - - if (written > 0) { - // Some or all samples were written - // Move remaining samples to front of buffer - const int remaining = g_pending_samples - written; - if (remaining > 0) { - for (int i = 0; i < remaining; ++i) { - g_pending_buffer[i] = g_pending_buffer[written + i]; - } - } - g_pending_samples = remaining; - - // Notify backend (for testing/tracking) -#if !defined(STRIP_ALL) - if (g_audio_backend != nullptr) { - g_audio_backend->on_frames_rendered(written / RING_BUFFER_CHANNELS); - } -#endif - } - - // If still have pending samples, buffer is full - wait for consumption - if (g_pending_samples > 0) - break; - } - // Check current buffer state const int buffered_samples = g_ring_buffer.available_read(); - const float buffered_time = - (float)buffered_samples / - (RING_BUFFER_SAMPLE_RATE * RING_BUFFER_CHANNELS); + const float buffered_time = samples_to_seconds(buffered_samples); // Stop if buffer is full enough if (buffered_time >= target_lookahead) @@ -238,20 +195,16 @@ float audio_get_playback_time() { (float)(elapsed * RING_BUFFER_SAMPLE_RATE * RING_BUFFER_CHANNELS); const float total_samples = (float)last_callback_samples + interpolated_samples; - return total_samples / (RING_BUFFER_SAMPLE_RATE * RING_BUFFER_CHANNELS); + return samples_to_seconds((int64_t)total_samples); } } // Fallback: coarse ring buffer time (before first callback or no backend) - const int64_t total_samples = g_ring_buffer.get_total_read(); - return (float)total_samples / - (RING_BUFFER_SAMPLE_RATE * RING_BUFFER_CHANNELS); + return samples_to_seconds(g_ring_buffer.get_total_read()); } float audio_get_render_time() { - const int64_t total_samples = g_ring_buffer.get_total_written(); - return (float)total_samples / - (RING_BUFFER_SAMPLE_RATE * RING_BUFFER_CHANNELS); + return samples_to_seconds(g_ring_buffer.get_total_written()); } float audio_get_realtime_peak() { diff --git a/src/audio/backend/miniaudio_backend.cc b/src/audio/backend/miniaudio_backend.cc index 312d36e..8871d10 100644 --- a/src/audio/backend/miniaudio_backend.cc +++ b/src/audio/backend/miniaudio_backend.cc @@ -12,11 +12,11 @@ // Real-time peak measured at actual playback time // Updated in audio_callback when samples are read from ring buffer -volatile float MiniaudioBackend::realtime_peak_ = 0.0f; +std::atomic MiniaudioBackend::realtime_peak_{0.0f}; // Smooth playback time interpolation -volatile double MiniaudioBackend::last_callback_time_ = 0.0; -volatile int64_t MiniaudioBackend::last_callback_samples_ = 0; +std::atomic MiniaudioBackend::last_callback_time_{0.0}; +std::atomic MiniaudioBackend::last_callback_samples_{0}; // Static callback for miniaudio (C API requirement) void MiniaudioBackend::audio_callback(ma_device* pDevice, void* pOutput, @@ -148,8 +148,10 @@ void MiniaudioBackend::audio_callback(ma_device* pDevice, void* pOutput, // Update smooth playback time tracking (absolute time, no epoch needed) struct timespec ts; clock_gettime(CLOCK_MONOTONIC, &ts); - last_callback_time_ = ts.tv_sec + ts.tv_nsec / 1e9; - last_callback_samples_ = ring_buffer->get_total_read(); + last_callback_time_.store(ts.tv_sec + ts.tv_nsec / 1e9, + std::memory_order_release); + last_callback_samples_.store(ring_buffer->get_total_read(), + std::memory_order_release); #if defined(DEBUG_LOG_RING_BUFFER) if (actually_read < samples_to_read) { @@ -280,11 +282,11 @@ void MiniaudioBackend::shutdown() { } float MiniaudioBackend::get_realtime_peak() { - return realtime_peak_; + return realtime_peak_.load(std::memory_order_acquire); } void MiniaudioBackend::get_callback_state(double* out_time, - int64_t* out_samples) { - *out_time = last_callback_time_; - *out_samples = last_callback_samples_; + int64_t* out_samples) { + *out_time = last_callback_time_.load(std::memory_order_acquire); + *out_samples = last_callback_samples_.load(std::memory_order_acquire); } diff --git a/src/audio/backend/miniaudio_backend.h b/src/audio/backend/miniaudio_backend.h index 953a0c0..01fa790 100644 --- a/src/audio/backend/miniaudio_backend.h +++ b/src/audio/backend/miniaudio_backend.h @@ -6,6 +6,8 @@ #include "../audio_backend.h" #include "miniaudio.h" +#include +#include // Production audio backend using miniaudio library // Manages real hardware audio device and playback @@ -31,11 +33,11 @@ class MiniaudioBackend : public AudioBackend { // Real-time peak measured at actual playback time (not pre-buffer) // Updated in audio_callback when samples are read from ring buffer - static volatile float realtime_peak_; + static std::atomic realtime_peak_; // Smooth playback time interpolation (updated in callback) - static volatile double last_callback_time_; // Absolute CLOCK_MONOTONIC time - static volatile int64_t last_callback_samples_; + static std::atomic last_callback_time_; // Absolute CLOCK_MONOTONIC time + static std::atomic last_callback_samples_; // Static callback required by miniaudio C API static void audio_callback(ma_device* pDevice, void* pOutput, diff --git a/src/audio/ring_buffer.h b/src/audio/ring_buffer.h index 1a21542..ba8cb49 100644 --- a/src/audio/ring_buffer.h +++ b/src/audio/ring_buffer.h @@ -19,6 +19,11 @@ RING_BUFFER_CHANNELS) / \ 1000) +// Convert sample count to seconds (accounting for stereo channels) +inline float samples_to_seconds(int64_t samples) { + return (float)samples / (RING_BUFFER_SAMPLE_RATE * RING_BUFFER_CHANNELS); +} + class AudioRingBuffer { public: AudioRingBuffer(); diff --git a/src/audio/synth.cc b/src/audio/synth.cc index 0161385..d584c62 100644 --- a/src/audio/synth.cc +++ b/src/audio/synth.cc @@ -5,7 +5,9 @@ #include "synth.h" #include "audio/dct.h" #include "audio/ola.h" +#include "audio/ring_buffer.h" #include "util/debug.h" +#include #include #include // For memset @@ -32,18 +34,18 @@ struct Voice { int start_sample_offset; // Samples to wait before producing audio output - const volatile float* active_spectral_data; + const float* active_spectral_data; }; static struct { Spectrogram spectrograms[MAX_SPECTROGRAMS]; - const volatile float* active_spectrogram_data[MAX_SPECTROGRAMS]; + std::atomic active_spectrogram_data[MAX_SPECTROGRAMS]; bool spectrogram_registered[MAX_SPECTROGRAMS]; } g_synth_data; static Voice g_voices[MAX_VOICES]; -static volatile float g_current_output_peak = - 0.0f; // Global peak for visualization +static std::atomic g_current_output_peak{ + 0.0f}; // Global peak for visualization static float g_tempo_scale = 1.0f; // Playback speed multiplier #if !defined(STRIP_ALL) @@ -53,7 +55,7 @@ static float g_elapsed_time_sec = 0.0f; // Tracks elapsed time for event hooks void synth_init() { memset(&g_synth_data, 0, sizeof(g_synth_data)); memset(g_voices, 0, sizeof(g_voices)); - g_current_output_peak = 0.0f; + g_current_output_peak.store(0.0f, std::memory_order_relaxed); #if !defined(STRIP_ALL) g_elapsed_time_sec = 0.0f; #endif /* !defined(STRIP_ALL) */ @@ -108,7 +110,8 @@ int synth_register_spectrogram(const Spectrogram* spec) { for (int i = 0; i < MAX_SPECTROGRAMS; ++i) { if (!g_synth_data.spectrogram_registered[i]) { g_synth_data.spectrograms[i] = *spec; - g_synth_data.active_spectrogram_data[i] = spec->spectral_data_a; + g_synth_data.active_spectrogram_data[i].store( + spec->spectral_data_a, std::memory_order_release); g_synth_data.spectrogram_registered[i] = true; return i; } @@ -128,8 +131,9 @@ float* synth_begin_update(int spectrogram_id) { return nullptr; } - const volatile float* active_ptr = - g_synth_data.active_spectrogram_data[spectrogram_id]; + const float* active_ptr = + g_synth_data.active_spectrogram_data[spectrogram_id].load( + std::memory_order_acquire); if (active_ptr == g_synth_data.spectrograms[spectrogram_id].spectral_data_a) { return (float*)(g_synth_data.spectrograms[spectrogram_id].spectral_data_b); @@ -144,18 +148,17 @@ void synth_commit_update(int spectrogram_id) { return; } - const volatile float* old_active_ptr = - g_synth_data.active_spectrogram_data[spectrogram_id]; + const float* old_active_ptr = + g_synth_data.active_spectrogram_data[spectrogram_id].load( + std::memory_order_acquire); const float* new_active_ptr = (old_active_ptr == g_synth_data.spectrograms[spectrogram_id].spectral_data_a) ? g_synth_data.spectrograms[spectrogram_id].spectral_data_b : g_synth_data.spectrograms[spectrogram_id].spectral_data_a; - // Atomic swap using GCC/Clang builtins for thread safety - __atomic_store_n( - (const float**)&g_synth_data.active_spectrogram_data[spectrogram_id], - new_active_ptr, __ATOMIC_RELEASE); + g_synth_data.active_spectrogram_data[spectrogram_id].store( + new_active_ptr, std::memory_order_release); } void synth_trigger_voice(int spectrogram_id, float volume, float pan, @@ -215,7 +218,8 @@ void synth_trigger_voice(int spectrogram_id, float volume, float pan, v.fractional_pos = 0.0f; v.start_sample_offset = start_offset_samples; v.active_spectral_data = - g_synth_data.active_spectrogram_data[spectrogram_id]; + g_synth_data.active_spectrogram_data[spectrogram_id].load( + std::memory_order_acquire); #if !defined(STRIP_ALL) // Notify backend of voice trigger event (for testing/tracking) @@ -233,7 +237,7 @@ void synth_trigger_voice(int spectrogram_id, float volume, float pan, void synth_render(float* output_buffer, int num_frames) { // Faster decay for more responsive visuals - g_current_output_peak *= 0.90f; + float peak = g_current_output_peak.load(std::memory_order_relaxed) * 0.90f; for (int i = 0; i < num_frames; ++i) { float left_sample = 0.0f; @@ -258,7 +262,8 @@ void synth_render(float* output_buffer, int num_frames) { // Fetch the latest active spectrogram pointer for this voice v.active_spectral_data = - g_synth_data.active_spectrogram_data[v.spectrogram_id]; + g_synth_data.active_spectrogram_data[v.spectrogram_id].load( + std::memory_order_acquire); const float* spectral_frame = (const float*)v.active_spectral_data + (v.current_spectral_frame * DCT_SIZE); @@ -286,14 +291,14 @@ void synth_render(float* output_buffer, int num_frames) { output_buffer[i * 2 + 1] = right_sample; // Update the peak with the new max (attack) - g_current_output_peak = fmaxf( - g_current_output_peak, fmaxf(fabsf(left_sample), fabsf(right_sample))); + peak = fmaxf(peak, fmaxf(fabsf(left_sample), fabsf(right_sample))); } + g_current_output_peak.store(peak, std::memory_order_release); + #if !defined(STRIP_ALL) - // Update elapsed time for event tracking (32000 Hz sample rate) - const float sample_rate = 32000.0f; - g_elapsed_time_sec += (float)num_frames / sample_rate; + // Update elapsed time for event tracking + g_elapsed_time_sec += (float)num_frames / RING_BUFFER_SAMPLE_RATE; #endif /* !defined(STRIP_ALL) */ } @@ -308,5 +313,5 @@ int synth_get_active_voice_count() { } float synth_get_output_peak() { - return g_current_output_peak; + return g_current_output_peak.load(std::memory_order_acquire); } diff --git a/src/audio/tracker.cc b/src/audio/tracker.cc index 95b8022..a511a62 100644 --- a/src/audio/tracker.cc +++ b/src/audio/tracker.cc @@ -255,9 +255,17 @@ static int get_free_pool_slot() { } // If all slots are active, reuse the oldest one (round-robin) - // This automatically handles cleanup of old patterns + // Clean up the old slot before reuse const int slot = g_next_pool_slot; g_next_pool_slot = (g_next_pool_slot + 1) % MAX_SPECTROGRAMS; + + if (g_spec_pool[slot].synth_id >= 0) { + synth_unregister_spectrogram(g_spec_pool[slot].synth_id); + } + delete[] g_spec_pool[slot].data; + g_spec_pool[slot].data = nullptr; + g_spec_pool[slot].synth_id = -1; + g_spec_pool[slot].active = false; return slot; } -- cgit v1.2.3