Skip to content

feat(sokol): Phase 4 font + audio backend impl (#447, #448) - #108

Merged
apotema merged 2 commits into
mainfrom
feat/sokol-phase4-font-audio-agent
May 13, 2026
Merged

feat(sokol): Phase 4 font + audio backend impl (#447, #448)#108
apotema merged 2 commits into
mainfrom
feat/sokol-phase4-font-audio-agent

Conversation

@apotema

@apotema apotema commented May 13, 2026

Copy link
Copy Markdown
Contributor

Summary

First concrete backend to opt into the Phase 4 asset-streaming font + audio contracts. Validates the codegen marshal boundary end-to-end with a real backend, not just scaffolding.

Font (gfx side) — labelle-gfx#258 / labelle-engine#448

  • Vendored stb_truetype.h + stb_truetype_impl.c next to the existing stb_image* files. Public domain.
  • gfx.zig grows top-level FontAtlas, DecodedFont, Glyph, CodepointEntry, KernPair, CodepointRange, FontBakeParams, decodeFont, uploadFontAtlas, unloadFontAtlas.
  • decodeFont uses the pack API (stbtt_PackBegin / stbtt_PackFontRange per range / stbtt_PackEnd). Picked pack over BakeFontBitmap because:
    1. Honors multiple non-contiguous codepoint ranges.
    2. Skyline packing is denser than the strip pack.
    3. Supports oversampling (default 1x for now; future PR exposes via FontBakeParams).
  • Atlas is an 8-bit alpha bitmap uploaded as a sokol sg_image with R8 pixel format.
  • Kerning extraction walks the packed-codepoint set pairwise and emits only non-zero advances.

Audio (audio side) — labelle-engine#447

  • Vendored stb_vorbis.c (single-file impl) and dr_wav.h + dr_wav_impl.c (header+impl split). Public domain.
  • audio.zig grows top-level Sound, DecodedAudio, decodeAudio, uploadSound, unloadSound, alongside the existing path-based loadSound.
  • Sound is extern struct { slot_index: u32, generation: u32 } — generation-tagged so unload can detect stale handles whose slots have been recycled.
  • decodeAudio dispatches on file_type: wav -> dr_wav, ogg -> stb_vorbis. Both routes allocate through the caller's allocator per the Phase 4 ownership contract.
  • uploadSound plugs PCM into the existing audio_slots pool (converted to f32 since the sokol_audio callback mixes f32), bumps generation.
  • unloadSound validates generation match before tearing down. Eager-frees the buffer (Phase 4 callers churn loads at runtime; deferring like the legacy path would balloon residual memory).

Design decisions worth flagging

  • Legacy unloadSound(id: u32) renamed to unloadSoundById so the Phase 4 contract can take the bare unloadSound name. The engine's audio Backend wrapper hard-codes that name; Zig has no function overloading. The path-based loadSound(path) u32 API stays untouched per the constraint in the brief. The sokol example was updated to match the rename.
  • extern struct for the POD wire types (Glyph, CodepointEntry, KernPair, CodepointRange, Sound) — keeps the assembler's marshal boundary layout-stable. DecodedFont/DecodedAudio/FontAtlas stay as regular structs because they live behind the API boundary on the backend side and aren't memcpy'd.
  • stb_vorbis decl/impl split via stb_vorbis_decl.h@cImport-ing stb_vorbis.c directly would compile the implementation a second time into the Zig test binary and collide with the C-source-side TU on every stb_vorbis_* symbol. The hand-rolled header carries just the prototypes we call.
  • audio_mod gains link_libc = true plus the two C sources and the same emsdk sysroot guard gfx_mod already uses for wasm32-emscripten.

Test plan

  • zig build test (cross-compile-friendly) -> 8/8 passed (the existing audio_slots regression locks).
  • zig build test-host (new native step) -> 15/15 passed (8 slots + 4 audio decoder + 3 font decoder).
  • zig build in backends/sokol/example/ -> clean (validates the unloadSoundById rename didn't break the runtime example).
  • Top-level zig build test in labelle-assembler/ -> green (the existing codegen unit tests that snapshot the generated BackendAudio.unloadSound(s) text still match — the codegen targets the new name).

References

  • labelle-engine#447 (audio loader)
  • labelle-engine#448 (font loader)
  • labelle-gfx#258 (font traits on Backend(Impl))
  • labelle-engine#532 (asset streaming RFC)
  • labelle-assembler#107 (resource gating for the codegen wiring)

First concrete backend to opt into the Phase 4 asset-streaming font
and audio contracts (labelle-engine#447, #448; labelle-gfx#258;
labelle-assembler#107). Validates the codegen marshal boundary end-to-
end with a real backend, not just scaffolding.

Font (gfx side):
- Vendored stb_truetype.h (public-domain Sean Barrett single-header)
  alongside the existing stb_image.h, behind stb_truetype_impl.c
  mirroring the stb_image_impl.c TU split.
- New top-level `FontAtlas`, `DecodedFont`, `Glyph`, `CodepointEntry`,
  `KernPair`, `CodepointRange`, `FontBakeParams` on `gfx.zig`. POD
  records are `extern struct` so the assembler's writeFontBackendWiring
  field-by-field copy lands on a stable layout.
- `decodeFont` uses `stbtt_PackBegin` / `stbtt_PackFontRange` (one call
  per `CodepointRange`) / `stbtt_PackEnd`. Chose pack over
  BakeFontBitmap for skyline packing density + multi-range support;
  oversampling stays at 1x for now, exposable via FontBakeParams later.
- `uploadFontAtlas` uploads the alpha bitmap as a sokol `sg.Image` with
  R8 pixel format; `unloadFontAtlas` destroys the image.
- gfx_mod gains `addCSourceFile` for stb_truetype_impl.c and inherits
  the existing emsdk sysroot guard for wasm32-emscripten.

Audio (audio side):
- Vendored stb_vorbis.c (single-file impl) and dr_wav.h
  (header+_impl.c split). stb_vorbis decls reach Zig through a
  hand-rolled stb_vorbis_decl.h so `@cImport`-ing the .c TU directly
  doesn't collide with the C-source-side impl at link time.
- New top-level `Sound`, `DecodedAudio`, `decodeAudio`, `uploadSound`,
  `unloadSound` on `audio.zig`. `Sound` is `extern struct
  { slot_index, generation }` — generation-tagged so unload detects
  stale handles whose slots have been recycled by a subsequent upload.
- `decodeAudio` dispatches on `file_type`: "wav" → `dr_wav`
  (drwav_init_memory + drwav_read_pcm_frames_s16), "ogg" → `stb_vorbis`
  (open_memory + get_samples_short_interleaved). Both routes allocate
  through the caller's allocator per the Phase 4 ownership contract.
- `uploadSound` plugs the decoded PCM into the existing `audio_slots`
  pool (converted to f32 since the sokol_audio callback mixes in f32),
  bumps the per-slot generation, and hands back a `Sound` handle.
- `unloadSound` validates generation match before tearing down the
  slot — eager-frees the f32 buffer (Phase 4 callers churn loads at
  runtime; the legacy `unloadSoundById` defers free to `deinit` for
  audio-callback safety, which is fine when unloads are end-of-program).
- audio_mod gains `link_libc = true`, the two C sources, and the same
  emsdk sysroot guard gfx_mod uses for wasm32-emscripten.

Design decisions worth flagging:
- The legacy `unloadSound(id: u32)` was renamed to `unloadSoundById`
  so the Phase 4 contract can take the bare `unloadSound` name (the
  engine's audio Backend wrapper hard-codes that name). The path-based
  `loadSound(path) u32` API stays untouched per the constraint. The
  sokol example was updated to match the rename.
- `extern struct` for `Glyph`/`CodepointEntry`/`KernPair`/`CodepointRange`/
  `Sound` keeps the assembler's marshal boundary layout-stable across
  toolchain upgrades. `DecodedFont`/`DecodedAudio`/`FontAtlas` stay as
  regular structs because they live behind the API boundary on the
  backend side and aren't memcpy'd.
- stb_vorbis's single-file model forced a decl/impl split — see the
  `stb_vorbis_decl.h` rationale in the header itself.

Tests:
- 8 existing `audio_slots` tests stay green.
- 3 new gfx tests (decodeFont rejects empty/zero-atlas/garbage input).
- 4 new audio tests (decodeAudio rejects empty/unknown-format/garbage,
  Sound layout invariants for the wire shape).
- Existing `audio_compile_check` keeps compile-only behaviour for
  cross-compile; new `test-host` step adds run-side execution for
  native macOS/Linux dev loops.
- `zig build test` stays green (the cross-compile-friendly path);
  `zig build test-host` runs all 15 tests natively.

Refs: labelle-engine#447, labelle-engine#448, labelle-gfx#258,
labelle-engine#532, labelle-assembler#107
@cursor

cursor Bot commented May 13, 2026

Copy link
Copy Markdown

PR Summary

Medium Risk
Adds new C-based font/audio decoding and new backend-facing APIs plus build/test wiring changes, which can affect cross-compilation and runtime audio behavior (threading/unload semantics). Scope is contained to the sokol backend.

Overview
Implements the Sokol backend’s Phase 4 asset-loading surfaces by adding font baking support (compiling stb_truetype into gfx_mod) and audio decode/upload/unload support in audio.zig (WAV via dr_wav, OGG via stb_vorbis, with generation-tagged Sound handles).

Updates the build to link libc for audio, compile the new C sources, and extend the emscripten sysroot include-path workaround to these new dependencies; also adds gfx compile-checks and a separate test-host step to run pure CPU decoder unit tests.

Renames legacy unloadSound to unloadSoundById and updates the Sokol example to match, freeing up unloadSound(sound: Sound) for the Phase 4 contract.

Reviewed by Cursor Bugbot for commit bf2b4a6. Bugbot is set up for automated code reviews on this repo. Configure here.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request implements Phase 4 asset loading for the Sokol backend, introducing CPU-side decoding and GPU-side uploading for audio (WAV/OGG via dr_wav and stb_vorbis) and fonts (TTF/OTF via stb_truetype). The changes include a new generation-tagged handle system for sounds, skyline-packed font atlas generation, and updated build configurations for Emscripten and native testing. Review feedback identifies several critical issues: potential double-free errors in font decoding due to redundant manual frees alongside errdefer blocks, integer overflow vulnerabilities during buffer size calculations on 32-bit platforms, and the need for stricter validation of decoded frame counts to avoid uninitialized data. Additionally, a performance improvement was suggested for the O(N^2) kerning extraction logic.

Comment thread backends/sokol/src/gfx.zig Outdated
Comment on lines +1202 to +1204
allocator.free(glyphs);
allocator.free(codepoint_index);
return error.FontAtlasTooSmall;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

critical

The glyphs and codepoint_index slices are already protected by errdefer at lines 1180 and 1183. Manually freeing them here before returning an error will cause a double-free when the errdefer blocks execute.

            return error.FontAtlasTooSmall;

Comment thread backends/sokol/src/audio.zig Outdated
const channels: u8 = @intCast(wav.channels);
if (total_frames == 0 or channels == 0) return error.AudioDecodeFailed;

const total_samples = total_frames * channels;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

security-high high

The calculation of total_samples can overflow usize on 32-bit platforms if total_frames is large. This would lead to an undersized buffer allocation and a subsequent buffer overflow in drwav_read_pcm_frames_s16. Use std.math.mul to safely calculate the total number of samples.

    const total_samples = std.math.mul(usize, total_frames, channels) catch return error.AudioDecodeFailed;

Comment thread backends/sokol/src/audio.zig Outdated
Comment on lines +598 to +599
const got = drwav.drwav_read_pcm_frames_s16(&wav, total_frames, samples.ptr);
if (got == 0) return error.AudioDecodeFailed;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

drwav_read_pcm_frames_s16 returns the number of frames actually read. If this is less than total_frames, the samples buffer will contain uninitialized data at the end. It's safer to treat a partial read as an error in this context.

    const got = drwav.drwav_read_pcm_frames_s16(&wav, total_frames, samples.ptr);
    if (got < total_frames) return error.AudioDecodeFailed;

Comment thread backends/sokol/src/audio.zig Outdated
const total_frames: usize = @intCast(total_samples_c);
if (total_frames == 0) return error.AudioDecodeFailed;

const total_samples = total_frames * channels;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

security-high high

The calculation of total_samples can overflow usize on 32-bit platforms. Use std.math.mul to safely calculate the total number of samples to avoid an undersized buffer allocation.

    const total_samples = std.math.mul(usize, total_frames, channels) catch return error.AudioDecodeFailed;

samples.ptr,
@intCast(total_samples),
);
if (got <= 0) return error.AudioDecodeFailed;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

stb_vorbis_get_samples_short_interleaved returns the number of frames decoded. If this is less than total_frames, the buffer will be partially uninitialized. Ensure that the full stream was decoded.

    if (got <= 0 or @as(usize, @intCast(got)) < total_frames) return error.AudioDecodeFailed;

Comment thread backends/sokol/src/gfx.zig Outdated
// both the success and discard paths (mirroring `decodeImage`).
const atlas_w: usize = params.atlas_width;
const atlas_h: usize = params.atlas_height;
const bitmap = try allocator.alloc(u8, atlas_w * atlas_h);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

security-high high

The calculation atlas_w * atlas_h can overflow usize on 32-bit platforms if the requested dimensions are large. This would result in an undersized buffer allocation and a buffer overflow in stbtt_PackBegin. Use std.math.mul to safely calculate the size.

    const bitmap = try allocator.alloc(u8, std.math.mul(usize, atlas_w, atlas_h) catch return error.FontDecodeFailed);

Comment thread backends/sokol/src/gfx.zig Outdated
Comment on lines +1262 to +1276
for (codepoint_index) |a| {
for (codepoint_index) |b| {
const adv_units = stbtt.stbtt_GetCodepointKernAdvance(
&font_info,
@intCast(a.codepoint),
@intCast(b.codepoint),
);
if (adv_units == 0) continue;
try kern_list.append(allocator, .{
.first = a.codepoint,
.second = b.codepoint,
.advance = @as(f32, @floatFromInt(adv_units)) * scale,
});
}
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

This nested loop results in $O(N^2)$ calls to stbtt_GetCodepointKernAdvance, where $N$ is the number of glyphs. For large font bakes, this can be extremely slow as each call performs a binary search in the font's kerning table. Consider using stbtt_GetKerningTable to extract all pairs in one pass and then filtering them to improve performance.

Comment thread backends/sokol/src/gfx.zig
Comment thread backends/sokol/src/audio.zig Outdated
@apotema

apotema commented May 13, 2026

Copy link
Copy Markdown
Contributor Author

@copilot review

Five fixes from the #108 review pass:

1. decodeFont (gfx.zig): drop manual free(glyphs)/free(codepoint_index)
   in the stbtt_PackFontRange-failure branch — both slices already have
   `errdefer allocator.free(...)` at their alloc sites, so the manual
   frees were a double-free on the FontAtlasTooSmall path.

2. unloadSound (audio.zig): mirror the legacy `unloadSoundById` pattern.
   Previously freed `s.samples` eagerly from the main thread; the audio
   callback runs on a separate thread and may have captured the slot
   pointer just above our voice-deactivation loop, so the eager free
   was a use-after-free. Now mark the slot unloaded and let `deinit`
   free at shutdown (after `saudio.shutdown()` has joined the audio
   thread). The slot stays non-null so the buffer is not leaked.

3. usize-overflow guards on three multiplies that feed `allocator.alloc`:
   - gfx.zig: atlas_w * atlas_h          → error.FontAtlasTooLarge
   - audio.zig (wav): frames * channels  → error.AudioTooLarge
   - audio.zig (ogg): frames * channels  → error.AudioTooLarge
   On 32-bit / wasm32 these could wrap and produce an undersized buffer
   that the C decoder writes past. `std.math.mul` returns an error on
   overflow so the path is allocation-safe everywhere.

4. Short-read handling in the WAV and OGG decode paths:
   - WAV: reject `got < total_frames` (was only checking `got == 0`)
   - OGG: reject `got <= 0` AND `@as(usize, @intcast(got)) < total_frames`
   The trailing bytes would otherwise be uninitialised and get mixed
   into the output device. `errdefer` frees the partial buffer.

5. Replace the O(N²) `stbtt_GetCodepointKernAdvance` double loop in
   decodeFont with a single `stbtt_GetKerningTable` pass. The kerning
   table is indexed by glyph index, not codepoint, so we build a
   sorted `(glyph_index, codepoint)` map (O(N log N)) and do an
   O(log N) binary-search lookup per table entry. Total cost goes
   from O(N²) to O(N log N + K log N), where K is the font's stored
   pair count. Pairs that reference glyphs outside the baked set are
   dropped — behaviour-equivalent to the old loop for the baked
   codepoint set.

Verified: `zig build test-host` → 15/15 tests pass.
@apotema

apotema commented May 13, 2026

Copy link
Copy Markdown
Contributor Author

Pushed bf2b4a6 addressing all five review findings:

#1 (critical, double-free in decodeFont) — dropped the manual allocator.free(glyphs) / allocator.free(codepoint_index) in the stbtt_PackFontRange failure branch. The errdefers at the alloc sites handle it; the manual frees were stacking on top.

#2 (high, UAF race in unloadSound) — mirrored the legacy unloadSoundById pattern: deactivate voices, then markSoundUnloaded, do NOT free here. deinit frees at shutdown after saudio.shutdown() has joined the audio thread. The slot stays non-null so the buffer remains reachable (no leak). Note: this drops the eager-free behaviour the previous comment justified — runtime slot-pool growth from unloaded-but-unfreed slots is a known trade-off and matches legacy semantics. Worth a follow-up if streaming churn exhausts the 256-slot pool in practice.

#3 (high security, usize overflow on 3 multiplies) — wrapped with std.math.mul:

  • gfx.zig bitmap: error.FontAtlasTooLarge
  • audio.zig WAV / OGG: error.AudioTooLarge

#4 (high, silent partial reads in WAV/OGG)

  • WAV: if (got < total_frames) return error.AudioDecodeFailed; (was only got == 0)
  • OGG: if (got <= 0 or @as(usize, @intCast(got)) < total_frames) return error.AudioDecodeFailed;
  • errdefer already in place frees the partial buffer.

#5 (medium, O(N^2) kerning) — fixed cleanly. Single-pass stbtt_GetKerningTable + a sorted (glyph_index, codepoint) map with O(log N) binary-search lookup per table entry. Overall O(N log N + K log N) vs the previous O(N^2). Pairs referencing glyphs outside the baked set are dropped (behaviour-equivalent to the old loop on the baked set).

Verification: cd backends/sokol && zig build test-host -> 15/15 tests passed.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR implements the first “real” Phase 4 backend integration for font atlas baking (gfx) and audio decoding/upload (audio) in the sokol backend, including the necessary vendored C decoders and build-system wiring. It aims to validate the Phase 4 asset-streaming marshal boundary end-to-end with concrete decode/upload/unload APIs.

Changes:

  • Added Phase 4 font surface in gfx.zig (decodeFont, uploadFontAtlas, unloadFontAtlas) backed by stb_truetype packing APIs.
  • Added Phase 4 audio surface in audio.zig (decodeAudio, uploadSound, unloadSound) backed by dr_wav + stb_vorbis, and renamed legacy unload to unloadSoundById.
  • Updated sokol backend build wiring for new C sources and added a test-host step to run decoder unit tests natively.

Reviewed changes

Copilot reviewed 7 out of 10 changed files in this pull request and generated 5 comments.

Show a summary per file
File Description
backends/sokol/src/stb_vorbis_decl.h Adds a declaration-only header so Zig can @cImport stb_vorbis without compiling the implementation twice.
backends/sokol/src/stb_truetype.h Vendors stb_truetype for font baking/packing.
backends/sokol/src/stb_truetype_impl.c Adds a single TU to compile stb_truetype implementation.
backends/sokol/src/gfx.zig Introduces Phase 4 font decode/upload/unload API and unit tests.
backends/sokol/src/dr_wav_impl.c Adds a single TU to compile dr_wav implementation.
backends/sokol/src/audio.zig Introduces Phase 4 audio decode/upload/unload API, adds decoders, renames legacy unload, adds tests.
backends/sokol/example/main.zig Updates example to use unloadSoundById after the legacy API rename.
backends/sokol/build.zig Wires new C sources, enables libc for audio module, adds gfx compile-check and a native test-host run step.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +1198 to +1204
@intCast(r.first),
count,
&packed_chars[write_idx],
);
if (ok == 0) {
// Partial-pack failures usually mean "atlas too small";
// bubble it up as a decode error so the catalog reports
Comment on lines +1126 to +1133
// R8 alpha atlas — `sg.PixelFormat.R8` on the upload side. We
// allocate the bitmap from `allocator` so the caller frees it on
// both the success and discard paths (mirroring `decodeImage`).
const atlas_w: usize = params.atlas_width;
const atlas_h: usize = params.atlas_height;
// Guard against 32-bit (incl. wasm32) `usize` wraparound on the
// bitmap size multiply — a wrap would alloc an undersized buffer
// that the C packer happily writes past.
Comment on lines +590 to +605
const total_frames: usize = @intCast(wav.totalPCMFrameCount);
const channels: u8 = @intCast(wav.channels);
if (total_frames == 0 or channels == 0) return error.AudioDecodeFailed;

// Guard against 32-bit (incl. wasm32) `usize` wraparound on the
// frame × channel multiply — a wrap would alloc an undersized
// buffer that drwav happily writes past.
const total_samples = std.math.mul(usize, total_frames, channels) catch return error.AudioTooLarge;
const samples = try allocator.alloc(i16, total_samples);
errdefer allocator.free(samples);

const got = drwav.drwav_read_pcm_frames_s16(&wav, total_frames, samples.ptr);
// Treat short reads as failures: the trailing samples are
// uninitialised, so emitting the buffer would mix garbage into
// the output. `errdefer` above frees the partial buffer.
if (got < total_frames) return error.AudioDecodeFailed;
Comment on lines +624 to +646

const info = stbv.stb_vorbis_get_info(vorbis);
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;

// Guard against 32-bit (incl. wasm32) `usize` wraparound on the
// frame × channel multiply — a wrap would alloc an undersized
// buffer that stb_vorbis happily writes past.
const total_samples = std.math.mul(usize, total_frames, channels) catch return error.AudioTooLarge;
const samples = try allocator.alloc(i16, total_samples);
errdefer allocator.free(samples);

// `get_samples_short_interleaved` takes (channels, dest, dest_len_in_shorts)
// and returns the number of FRAMES decoded (or 0/negative on error).
const got = stbv.stb_vorbis_get_samples_short_interleaved(
vorbis,
info.channels,
samples.ptr,
Comment on lines +694 to +714
sound_generations[slot_idx] += 1;

return .{ .slot_index = slot_idx, .generation = sound_generations[slot_idx] };
}

/// Counterpart to `uploadSound`. Validates the generation tag so a
/// stale handle (one whose slot has been recycled) is a no-op
/// rather than tearing down the live sound that now lives there.
pub fn unloadSound(sound: Sound) void {
if (sound.slot_index == 0 or sound.slot_index >= slots.MAX_SOUNDS) return;
if (sound_generations[sound.slot_index] != sound.generation) return;

// Stop any voices playing this slot so the audio callback won't
// chase a freed pointer between the markUnloaded below and the
// shutdown-time free. Same ordering as the legacy
// `unloadSoundById` path (see this file, ~line 350).
for (&voices) |*voice| {
if (voice.active and voice.sound_id == sound.slot_index) {
voice.active = false;
}
}
@apotema

apotema commented May 13, 2026

Copy link
Copy Markdown
Contributor Author

All 5 inline findings were already addressed in bf2b4a6 (commit pushed before this review fired off, so the line numbers Copilot quotes don't match the current tree). Mapping each Copilot comment to the existing fix:

Copilot finding Fix in bf2b4a6
Double-free of glyphs / codepoint_index in decodeFont error path Manual allocator.free calls removed — errdefer handles it
atlas_w * atlas_h overflow on 32-bit Now std.math.mul(usize, atlas_w, atlas_h) catch return error.FontAtlasTooLarge (gfx.zig:1134)
drwav_read_pcm_frames_s16 partial read if (got < total_frames) return error.AudioDecodeFailed
stb_vorbis_get_samples_short_interleaved partial read Same got check, with the @intCast after a got <= 0 guard
unloadSound UAF race with audio callback Mirrors legacy unloadSoundById: deactivate voices, mark slot unloaded, defer the free to deinit after saudio.shutdown() joins the audio thread

These were the same 5 findings Gemini and Cursor flagged. CI re-running on bf2b4a6 now; merging once it goes green.

@apotema
apotema merged commit 561cce3 into main May 13, 2026
4 checks passed
@apotema
apotema deleted the feat/sokol-phase4-font-audio-agent branch May 13, 2026 19:17

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes and found 2 potential issues.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit bf2b4a6. Configure here.

break;
}
}
if (slot_idx == 0) return error.AudioSlotsExhausted;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Phase 4 and legacy slot allocation can collide

Medium Severity

uploadSound scans from index 1 for null slots but never updates next_sound_id. The legacy loadSound blindly writes to sounds[next_sound_id] without checking occupancy. If both APIs are used, uploadSound can claim a slot ahead of next_sound_id, and a subsequent loadSound call overwrites it — silently destroying the Phase 4 sound, leaking its f32 samples buffer, and leaving the Phase 4 Sound handle stale. Worse, loadSound doesn't bump sound_generations, so the orphaned Phase 4 handle still passes the generation check in unloadSound, which would then incorrectly mark the new legacy sound as unloaded.

Additional Locations (1)
Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit bf2b4a6. Configure here.

// it is not leaked. Slot reuse for fresh `uploadSound` calls is
// gated on `sounds[i] == null`, matching the legacy behaviour —
// unloaded slots are NOT reclaimed at runtime by design.
slots.markSoundUnloaded(&sounds, sound.slot_index);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unloaded Phase 4 slots are never recyclable

Medium Severity

unloadSound calls markSoundUnloaded, which keeps the slot non-null (just flagged). uploadSound only considers null slots for reuse. This means every Phase 4 upload+unload cycle permanently consumes one of the 255 usable slots (MAX_SOUNDS minus reserved index 0). Phase 4 callers that churn sound loads at runtime — the stated use case in the PR description — will hit error.AudioSlotsExhausted after 255 cycles. The PR description claims "eager-frees the buffer" but the implementation defers all cleanup to deinit.

Additional Locations (1)
Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit bf2b4a6. Configure here.

apotema added a commit that referenced this pull request May 13, 2026
…ecycling (closes #110) (#111)

* fix(sokol): audio slot allocation correctness — Phase 4 vs legacy + recycling

Two correctness gaps in the sokol Phase 4 audio impl (#108 follow-up):

1. Phase 4 / legacy collision: `loadSound` blindly wrote to
   `sounds[next_sound_id]`, `uploadSound` scanned for `null` from
   index 1 — the two had no shared notion of "free", so a mixed-API
   game could double-claim the same slot. Both paths now route through
   a new `allocateSoundSlot` helper in `audio_slots.zig` (single source
   of truth). Same fix applied to `loadMusic` via `allocateMusicSlot`.

2. Slots never recyclable: `markSoundUnloaded` keeps the slot non-null
   (deferred-free pattern from #10) but `uploadSound` only considered
   `null` slots, so 256 upload+unload cycles permanently exhausted the
   pool. `allocateSoundSlot` now also returns slots whose `unloaded`
   flag is set; the recycle site parks the old samples buffer on a
   module-level `pending_sound_frees: std.ArrayList([]const f32)` list
   that `deinit` drains after `saudio.shutdown()` has joined the audio
   thread — preserving the no-UAF guarantee that drove the original
   deferred-free choice.

Also bumps `sound_generations[idx]` on every claim (legacy + Phase 4),
so a stale `Sound` handle whose slot got recycled fails the generation
check in `unloadSound` and is a no-op instead of tearing down the new
occupant.

Test coverage:
  - `allocateSoundSlot` + `allocateMusicSlot` unit tests in
    `audio_slots.zig`: first-null behaviour, slot-0 reservation,
    recycling unloaded slots, returning null when every slot is live.
  - `audio.zig`: a 768-cycle upload+unload loop (3× MAX_SOUNDS) that
    used to error with `AudioSlotsExhausted` past cycle 255; a
    mixed-API test that asserts the shared allocator never assigns the
    same index to both paths; a generation-bump regression that holds
    a stale `Sound` handle across a recycle and verifies `unloadSound`
    fails-soft on it.

Side fix: `deinit` now guards `saudio.shutdown()` on `saudio.isvalid()`
so the test-host path (which flips `audio_initialized` without calling
`saudio.setup`) doesn't trip the sokol_audio assertion.

Verification: `zig build test-host` — 34/34 tests passed (was 15/15;
+19 tests across audio.zig and audio_slots.zig).

Closes #110. Follow-up to #108.

* fix(sokol): generation-check legacy unload paths + uploadSound channel guard

Three reviewer-flagged follow-ups for #111:

1. `unloadSoundById` / `unloadMusic` (Cursor Bugbot, Medium severity):
   pre-fix the legacy `u32` id was returned verbatim as the slot
   index. With slot recycling now enabled (`allocateSoundSlot`
   reclaims `unloaded` slots), a stale id held by game code could
   alias a recycled slot's new occupant and tear it down. Encode the
   slot's generation in the high 16 bits of the returned `u32`
   (`(gen & 0xFFFF) << 16 | (idx & 0xFFFF)`); both legacy unload
   paths decode and compare against `sound_generations` /
   `music_generations`, no-opping on mismatch. Read consumers
   (`playSound`, `setSoundVolume`, etc.) strip the generation but
   don't enforce it — passing a stale id there is harmless
   (addresses the recycled slot, same as v1 behaviour) and matches
   the safety story the Phase 4 `Sound`/`unloadSound` pair
   establishes. Adds a new `music_generations` array (paralleling
   `sound_generations`) and resets both in `deinit`. Chose Option A
   from the review (encode-in-u32) over Option B (side table)
   because it keeps the safety state localised to the returned
   handle and matches the Phase 4 `Sound` struct's design exactly.

2. `FontAtlas` extern struct (Copilot, cross-repo contract): mark
   `pub const FontAtlas` as `extern struct`. All four other POD
   types crossing the codegen marshal boundary (`Glyph`,
   `CodepointEntry`, `KernPair`, `CodepointRange`) already had this
   guarantee; `FontAtlas` participates in the same
   `writeFontBackendWiring` path. `sg.Image` is already extern, so
   the change is layout-equivalent in practice — it just makes the
   contract explicit.

3. `uploadSound` div-by-zero (Cursor Bugbot, Medium): the
   `@divTrunc(decoded.samples.len, channels)` math (channels=0 case)
   exists in this file via the slot pool / mixer stride. Add the
   same up-front guard raylib needs: `if (decoded.channels == 0)
   return error.AudioInvalidChannels;` before the slot allocator.
   `decodeAudio` already rejects zero-channel inputs, but
   `uploadSound` is a public API and a hand-constructed
   `DecodedAudio` could reach it.

Tests: +4 (`uploadSound rejects zero-channel`, `unloadSoundById
ignores stale legacy id after slot recycle`, `unloadMusic ignores
stale legacy id after slot recycle`, `legacy id encode/decode
roundtrip`). `zig build test-host` 34 → 38, still green.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants