fix(audio): implement Marmalade sound callback lifecycle - #2
Conversation
WalkthroughThe audio backend changes from music and IMA ADPCM handling to callback-generated PCM and WAV chunk playback. Channel completion uses atomic state and loop metadata, revised channel APIs expose callback types and loop indices, and runtime device operations invoke ChangesAudio backend migration
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant s3eDeviceYield
participant audio_pump
participant SDL_mixer
participant ChannelCallbacks
s3eDeviceYield->>audio_pump: process audio state
SDL_mixer->>audio_pump: report completed channel
audio_pump->>ChannelCallbacks: invoke end or stop callback
audio_pump->>SDL_mixer: replay generated PCM for loop
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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 `@src/s3e_audio.c`:
- Around line 330-339: The audio generation ABI in the struct used by the
surrounding generation flow assumes 32-bit pointers because info.target and
info.orig_start are uint32_t while the layout is asserted at 32 bytes. Confirm
and enforce the supported ABI with a compile-time sizeof(void*) guard, or change
both address fields and the associated size assertion to pointer-sized types
while preserving the callback’s expected layout.
- Around line 940-945: Update the playback flow around render_generator_audio so
a return value of 0 preserves and plays the caller-supplied data as raw PCM,
rather than treating it as failure; retain failure handling for negative results
and generated-audio handling for positive results. Ensure s3eSoundChannelPlay
continues supporting channels without a registered S3E_CHANNEL_GEN_AUDIO
callback.
In `@src/s3e_runtime.c`:
- Around line 168-170: Update s3eDeviceYield around audio_pump() and
wait_with_timers() so audio is serviced throughout the wait rather than only
before it. Integrate audio_pump() into the wait loop or bound its sleeps by
upcoming audio-completion deadlines, ensuring service_finished_sound() runs
promptly while preserving timer dispatch and wait behavior.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 3543f341-4006-4505-9808-0b59f9c1f149
📒 Files selected for processing (3)
include/s3e_host_internal.hsrc/s3e_audio.csrc/s3e_runtime.c
| struct s3e_sound_gen_audio_info info = { | ||
| .channel = channel, | ||
| .target = (uint32_t)(uintptr_t)(pcm + written), | ||
| .num_samples = requested, | ||
| .mix = 0, | ||
| .orig_start = (uint32_t)(uintptr_t)data, | ||
| .orig_num_samples = samples, | ||
| .reps_remaining = 0, | ||
| .end_sample = 0, | ||
| }; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Look for enforced 32-bit target in the build config.
fd -e mk -e cmake -e txt -e ninja | xargs rg -nP -- '-m32|EM_386|i686|x86|POINTER_SIZE|sizeof\(void' 2>/dev/null
rg -nP 'CMAKE_SIZEOF_VOID_P|add_compile_options\(.*-m32' -g 'CMakeLists.txt'Repository: Producdevity/cod-boz-port
Length of output: 163
🏁 Script executed:
#!/bin/bash
set -u
echo "== files =="
git ls-files | sed -n '1,200p'
echo "== locate s3e_audio.c =="
fd -a 's3e_audio\.c$' . || true
echo "== search relevant symbols =="
rg -n "_Static_assert|s3e_sound_gen_audio_info|sound_gen_audio|target|orig_start|pcm\+" -S .Repository: Producdevity/cod-boz-port
Length of output: 7064
Confirm this audio generation ABI is intended for 32-bit pointers.
info.target and info.orig_start are allocated as uint32_t and the struct is asserted to be 32 bytes, so this ABI is currently pointer-sized only where sizeof(void*) == 4. If this can run with 64-bit pointers, the casts truncate addresses and the generator callback can write to a malformed PCM target. Add a static guard on sizeof(void*) for the supported ABI, or widen these fields to a pointer-sized type.
🤖 Prompt for 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.
In `@src/s3e_audio.c` around lines 330 - 339, The audio generation ABI in the
struct used by the surrounding generation flow assumes 32-bit pointers because
info.target and info.orig_start are uint32_t while the layout is asserted at 32
bytes. Confirm and enforce the supported ABI with a compile-time sizeof(void*)
guard, or change both address fields and the associated size assertion to
pointer-sized types while preserving the callback’s expected layout.
Source: Linters/SAST tools
| uint32_t pcm_samples = size; | ||
| int16_t *decoded = decode_ima_adpcm_mono(data, (size_t)size * sizeof(int16_t), &pcm_samples); | ||
| const int16_t *pcm = decoded ? decoded : data; | ||
| int16_t *decoded = NULL; | ||
| if (render_generator_audio(channel, data, size, &decoded, &pcm_samples) <= 0) { | ||
| fprintf(stderr, "[audio] sound generator failed on channel %d\n", channel); | ||
| return 1; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check whether callers register a GenAudio callback before playing.
rg -nP '\bs3eSoundChannel(Register|Play)\s*\(' -g '!src/s3e_audio.c' -C2Repository: Producdevity/cod-boz-port
Length of output: 163
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== files =="
git ls-files | sed -n '1,200p'
echo "== locate s3e_audio and related symbols =="
fd -a 's3e_audio\.c$|s3e' . | sed -n '1,200p'
rg -n "render_generator_audio|s3eSoundChannelPlay|s3eSoundChannelRegister|GenAudio|GEN_AUDIO|callbacks|s3e_audio" . --glob '!*.png' --glob '!*.jpg' 2>/dev/null | sed -n '1,240p'
echo "== target file outline/section =="
wc -l src/s3e_audio.c
sed -n '860,1005p' src/s3e_audio.cRepository: Producdevity/cod-boz-port
Length of output: 11392
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== render_generator_audio implementation =="
sed -n '260,330p' src/s3e_audio.c
echo "== sound slot construction / clear path =="
sed -n '80,115p' src/s3e_audio.c
sed -n '640,720p' src/s3e_audio.c
echo "== current diff around changed symbol =="
git diff -- src/s3e_audio.c | sed -n '1,180p'
echo "== deterministic control-flow probe from source text =="
python3 - <<'PY'
from pathlib import Path
p = Path('src/s3e_audio.c')
s = p.read_text()
checks = {
"render_returns_0_for_null_function": "return 0;" in s.split("static int render_generator_audio", splitnum=2)[1].split("}", 1)[0],
"play_rejects_generators_returning_zero_or_less": "{\n init_sound_slots();\n if (!audio_open() || !data || size == 0 || size > UINT32_MAX / 2u) {\n return 1;\n }\n audio_pump();\n\n if (channel < 0) {\n channel = s3eSoundGetFreeChannel();\n }\n if (!sound_channel_valid(channel)) {\n return 1;\n }\n\n struct sound_slot *slot = &g_sound_slots[channel];\n uint32_t pcm_samples = size;\n int16_t *decoded = NULL;\n if (render_generator_audio(channel, data, size, &decoded, &pcm_samples) <= 0) {\n fprintf(stderr, \"[audio] sound generator failed on channel %d\\n\", channel);\n return 1;\n }" in s,
"play_has_raw_decoder_after_generator_return_0_path": "decode_samples" in s.split("if (render_generator_audio(channel, data, size, &decoded, &pcm_samples) <= 0)", splitnum=2)[1].split("\n uint32_t wav_size", 1)[0],
}
for k,v in checks.items():
print(f"{k}={v}")
PYRepository: Producdevity/cod-boz-port
Length of output: 5983
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== render_generator_audio implementation =="
sed -n '288,360p' src/s3e_audio.c
echo "== play_audio_buffer wrapper that decodes raw data =="
sed -n '415,490p' src/s3e_audio.c
echo "== s3eSoundChannelPlay implementation including source_data branch =="
sed -n '924,975p' src/s3e_audio.c
echo "== deterministic callbacks/functions and raw-play path presence =="
python3 - <<'PY'
from pathlib import Path
s = Path('src/s3e_audio.c').read_text()
rg = "static int32_t invoke_callback"
first_return_0_idx = s.index("return 0;", s.index("static int render_generator_audio"), s.index("static int decode_samples"))
print("render_generator_audio_return_0_for_missing_callback_before_decode=", first_return_0_idx > s.index("static int render_generator_audio") and first_return_0_idx < s.index("static int decode_samples"))
print("play_has_render_success_guard=", "if (render_generator_audio(channel, data, size, &decoded, &pcm_samples) <= 0)" in s)
print("play_raw_data_branch_after_render_success=", "if (slot->source_data && slot->source_data == data)" in s)
print("play_raw_data_branch_after_raw_fail=", "if (!slot->chunk && !raw_playback_disabled) {" in s)
print("play_raw_data_branch_uses_play_audio_buffer=", "play_audio_buffer((const void *)slot->source_data, slot->source_samples, (uint32_t)slot->reps_remaining);" in s[s.index("play_audio_buffer"):s.index("play_audio_buffer")+500])
for k in ["gen_audio_unregister_uses_callbacks_clear", "sound_register_clears_missing_callbacks", "play_initial_clears_source_data"]:
print(k, k in s)
PY
echo "== diff summary =="
git diff --stat -- src/s3e_audio.c
git diff -- src/s3e_audio.c | sed -n '80,140p'Repository: Producdevity/cod-boz-port
Length of output: 7013
Restore direct-PCM playback when no GEN_AUDIO callback is registered.
render_generator_audio returns 0 when the generator callback is unset, and s3eSoundChannelPlay treats any <= 0 result as failure while discarding the caller-supplied data. Preserve a raw-PCM fallback so channels without s3eSoundChannelRegister(..., S3E_CHANNEL_GEN_AUDIO, ...) can still play.
🤖 Prompt for 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.
In `@src/s3e_audio.c` around lines 940 - 945, Update the playback flow around
render_generator_audio so a return value of 0 preserves and plays the
caller-supplied data as raw PCM, rather than treating it as failure; retain
failure handling for negative results and generated-audio handling for positive
results. Ensure s3eSoundChannelPlay continues supporting channels without a
registered S3E_CHANNEL_GEN_AUDIO callback.
| uint64_t s3eDeviceYield(int32_t ms) { | ||
| input_pump(); | ||
| audio_pump(); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -eu
rg -n -C 12 '\bwait_with_timers\s*\(' src include
rg -n -C 4 '\baudio_pump\s*\(' src includeRepository: Producdevity/cod-boz-port
Length of output: 5042
🏁 Script executed:
# Check the full audio_pump implementation and related structures
sed -n '452,470p' src/s3e_audio.c
# Check what service_finished_sound does
rg -n -A 10 'service_finished_sound' src/s3e_audio.c
# Check if sleep_ms does anything with audio
rg -n -B 2 -A 6 'sleep_ms' src/s3e_runtime.cRepository: Producdevity/cod-boz-port
Length of output: 2010
🏁 Script executed:
# Find sleep_ms implementation to see if it does anything special
rg -n -B 2 -A 8 'sleep_ms\(' src/s3e_runtime.c | head -40
# Also check if there's any other place where audio_pump might be called
rg -n 'audio_pump' src/Repository: Producdevity/cod-boz-port
Length of output: 1160
🏁 Script executed:
# Find the sleep_ms implementation - it's likely in a platform-specific file
rg -n 'void sleep_ms' src/
# Check header files too
rg -n 'sleep_ms' include/Repository: Producdevity/cod-boz-port
Length of output: 270
🏁 Script executed:
# Check the sleep_ms implementation
sed -n '45,60p' src/s3e_host.c
# Also look at the context around it
sed -n '40,70p' src/s3e_host.cRepository: Producdevity/cod-boz-port
Length of output: 1337
Service audio during the wait, not only before it.
audio_pump() runs before wait_with_timers(). Since that wait loop does not pump audio—it only dispatches timers and sleeps—sounds completing during the wait will not reach service_finished_sound() until the next runtime call, delaying loop rescheduling and callbacks and potentially causing audible gaps. Integrate audio pumping into the wait loop or otherwise bound waits by audio completion deadlines.
🧰 Tools
🪛 Clang (14.0.6)
[warning] 168-168: parameter name 'ms' is too short, expected at least 3 characters
(readability-identifier-length)
🪛 Cppcheck (2.21.0)
[style] 168-168: The function 's3eDeviceYield' should have static linkage since it is not used outside of its translation unit.
(staticFunction)
🤖 Prompt for 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.
In `@src/s3e_runtime.c` around lines 168 - 170, Update s3eDeviceYield around
audio_pump() and wait_with_timers() so audio is serviced throughout the wait
rather than only before it. Integrate audio_pump() into the wait loop or bound
its sleeps by upcoming audio-completion deadlines, ensuring
service_finished_sound() runs promptly while preserving timer dispatch and wait
behavior.
1 participant
Summary by CodeRabbit
New Features
Bug Fixes