Repository navigation
sokol: collapse audio onto shared labelle-audio mixer (Phase 2 fan-out, f32) - #390
Conversation
Phase 2 audio fan-out, the sokol/f32 case. sokol drives audio through
sokol_audio.h's f32 stream callback, so this uses labelle-audio v0.3.0's
new f32 DeviceSink path: SokolSink declares sample_format = .f32 and the
shared Mixer(SokolSink) renders the mix straight into sokol_audio's
[*]f32 buffer in normalized [-1,1] interleaved samples (no conversion).
- New src/audio/sink.zig: the sokol_audio device + f32 stream callback,
satisfying the shared f32 DeviceSink contract (ensureStarted/stop/
framesMixed + sample_format = .f32). 2-channel stereo stream.
- New src/audio/decode.zig: KEPT the Phase 4 OGG/WAV CPU decoder
(decodeAudio + DecodedAudio/Sound) because the shared mixer decodes
WAV only and the assembler's writeAudioBackendWiring needs OGG.
- src/audio.zig: thin facade over Mixer(SokolSink); public API names +
signatures preserved verbatim (legacy load/play + Phase 4 catalog).
uploadSound routes decoded i16 PCM to Audio.loadSoundFromPcm.
- Deleted the duplicated mixer/slots/WAV-parser: audio_slots.zig +
audio/{state,system,legacy,phase4}.zig (~1000 LOC net removed). Their
slot/mixer/#298/#110/#111 logic now lives in labelle-audio.
- build.zig: wire labelle-audio v0.3.0 into the audio module; drop the
audio_slots host test (moved to labelle-audio).
- build.zig.zon: add labelle-audio v0.3.0 dep.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughThe sokol audio backend now forwards audio control and decoding through ChangesSokol Audio Backend Migration to labelle-audio
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request implements Phase 2 of the pluggable-backends RFC by offloading WAV decoding, PCM mixing, and slot management from the Sokol audio backend to the shared labelle-audio package. The local slot management and legacy audio code have been removed, and audio.zig has been refactored into a thin adapter. A new sink.zig implements the f32 DeviceSink contract, while decode.zig retains the CPU-based OGG/WAV decoder. The review feedback suggests improving safety in decode.zig by replacing direct @intCast calls with std.math.cast to handle malformed files gracefully, and removing a redundant saudio.isvalid() check in sink.zig to prevent potential resource leaks during shutdown.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| const total_frames: usize = @intCast(wav.totalPCMFrameCount); | ||
| const channels: u8 = @intCast(wav.channels); | ||
| if (total_frames == 0 or channels == 0) return error.AudioDecodeFailed; |
There was a problem hiding this comment.
Using @intCast directly on wav.totalPCMFrameCount and wav.channels can cause a panic in safety-checked builds if the values are out of bounds for usize or u8 (e.g., on 32-bit platforms or with malformed WAV files). Using std.math.cast is much safer as it returns null on overflow, allowing us to handle the error gracefully.
const total_frames = std.math.cast(usize, wav.totalPCMFrameCount) orelse return error.AudioTooLarge;
const channels = std.math.cast(u8, wav.channels) orelse return error.AudioDecodeFailed;
if (total_frames == 0 or channels == 0) return error.AudioDecodeFailed;
| const channels: u8 = @intCast(info.channels); | ||
| const sample_rate: u32 = @intCast(info.sample_rate); | ||
| if (channels == 0) return error.AudioDecodeFailed; | ||
|
|
||
| const total_samples_c = stbv.stb_vorbis_stream_length_in_samples(vorbis); | ||
| const total_frames: usize = @intCast(total_samples_c); | ||
| if (total_frames == 0) return error.AudioDecodeFailed; |
There was a problem hiding this comment.
Similar to decodeWav, using @intCast directly on info.channels, info.sample_rate, and total_samples_c can cause a panic if the values are out of bounds (e.g., on 32-bit platforms or with malformed OGG files). We should use std.math.cast to safely cast these values and handle any overflow gracefully.
const channels = std.math.cast(u8, info.channels) orelse return error.AudioDecodeFailed;
const sample_rate = std.math.cast(u32, info.sample_rate) orelse return error.AudioDecodeFailed;
if (channels == 0) return error.AudioDecodeFailed;
const total_samples_c = stbv.stb_vorbis_stream_length_in_samples(vorbis);
const total_frames = std.math.cast(usize, total_samples_c) orelse return error.AudioTooLarge;
if (total_frames == 0) return error.AudioDecodeFailed;
| if (device_started.load(.acquire)) { | ||
| if (saudio.isvalid()) saudio.shutdown(); | ||
| device_started.store(false, .release); |
There was a problem hiding this comment.
The saudio.isvalid() check before calling saudio.shutdown() is redundant because device_started is only set to true if saudio.setup was successfully called. Furthermore, if the device somehow becomes invalid after setup, skipping saudio.shutdown() would leak the setup state and leave sokol_audio in an inconsistent state (as saudio.shutdown is required to reset the internal setup_called flag). We should call saudio.shutdown() unconditionally if device_started is true.
if (device_started.load(.acquire)) {
saudio.shutdown();
device_started.store(false, .release);
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@backends/sokol/src/audio/decode.zig`:
- Around line 85-87: The narrowing casts in the audio decode path can trap
before the function returns an audio error, so guard the input sizes first and
only cast after validating bounds. In the WAV branch around
wav.totalPCMFrameCount and wav.channels, check for zero/overflow using the
original values before calling `@intCast`, and do the same in the OGG path for
data.len, total_samples_c, and total_samples so oversized inputs are rejected
gracefully instead of trapping. Keep the fix localized to decode.zig and
preserve the existing error.AudioDecodeFailed flow.
In `@backends/sokol/src/audio/sink.zig`:
- Around line 102-117: The audio sink setup path in `sink.zig` leaves `saudio`
in setup state when `saudio.isvalid()` fails, which can cause a later
`ensureStarted()` to attempt setup again on the same context. In the failed
branch of the `saudio.setup()` / `saudio.isvalid()` flow, add a
`saudio.shutdown()` before returning or exiting the branch so the Sokol audio
state is reset cleanly. Use the existing `saudio` setup/validation block and
keep the successful `device_started.store(true, .release)` path unchanged.
In `@backends/sokol/src/audio/tests.zig`:
- Around line 39-46: The test around audio.uploadSound currently relies on
explicit cleanup after the assertion, so a failure in testing.expect can skip
unloadSound and leak the uploaded slot into later tests. Add a defer immediately
after the successful uploadSound call in tests.zig so teardown always runs, and
keep the existing explicit unloadSound call only if you still want to verify
double-unload behavior in this test.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 3741094d-3f5c-4173-8c55-f1e5dccb7009
📒 Files selected for processing (11)
backends/sokol/build.zigbackends/sokol/build.zig.zonbackends/sokol/src/audio.zigbackends/sokol/src/audio/decode.zigbackends/sokol/src/audio/legacy.zigbackends/sokol/src/audio/phase4.zigbackends/sokol/src/audio/sink.zigbackends/sokol/src/audio/state.zigbackends/sokol/src/audio/system.zigbackends/sokol/src/audio/tests.zigbackends/sokol/src/audio_slots.zig
💤 Files with no reviewable changes (5)
- backends/sokol/src/audio/system.zig
- backends/sokol/src/audio_slots.zig
- backends/sokol/src/audio/phase4.zig
- backends/sokol/src/audio/state.zig
- backends/sokol/src/audio/legacy.zig
…wn (review) - decode.zig: std.math.cast (null on overflow) instead of @intcast for WAV/OGG frame-count/channels/sample-rate — a malformed file or 32-bit usize can't panic the cast now (Gemini + CodeRabbit). - sink.zig stop(): drop the redundant inner isvalid() guard — device_started is only set when setup succeeded, so saudio.shutdown (asserts setup_called, not isvalid) is safe + required unconditionally; fixed the misleading comment (CodeRabbit + Gemini). - tests.zig: defer the teardown so a failed assertion can't leak the slot into the next test (CodeRabbit).
1 participant
The final backend collapse of the Phase 2 audio pilot — and the f32 path of
labelle-audiov0.3.0 in action (sokol_audio's callback is f32).Changes (backends/sokol) — 1,628 → 626 LOC (~1,000 deleted)
audio/sink.zig—SokolSinksatisfies the f32DeviceSink(sample_format = .f32): wrapssokol_audio's f32 stream callback →MixCallbackF32. The mixer writes normalized [-1,1] f32 directly, no conversion.Mixer(SokolSink)replaces the home-grown f32 mixer + slot state + WAV parser. Deleted:audio_slots.zig(306),audio/state.zig(69),audio/legacy.zig(401), and the mixer half ofaudio/system.zig.labelle-audiov0.3.0.Why the OGG decoder stayed
The assembler's
writeAudioBackendWiringemits directBackendAudio.decodeAudio/uploadSoundcalls for every backend and feeds OGG too, but the shared mixer'sloadSoundFromMemoryis WAV-only. So sokol's dr_wav/stb_vorbis decoder is kept (nowaudio/decode.zig);uploadSoundroutes decoded i16 PCM into the mixer vialoadSoundFromPcm, and the externSoundlayout is preserved. No codegen changes.Verified
backends/sokol:zig build+zig build test-hostgreen (15/15). raylib/sdl stay as-is (delegate to full engines).Phase 2 status
bgfx ✅ wgpu ✅ sokol ✅ (this) — all hand-rolled audio collapsed onto one shared mixer. raylib/sdl independent (no duplication). f32 mixer released v0.3.0.
Summary by CodeRabbit
0and safer unload behavior).