Skip to content

fix(audio): count complete frames in JS Audio.write - #1683

Open
toprameen wants to merge 2 commits into
bendlang:mainfrom
toprameen:count-complete-audio-frames
Open

toprameen wants to merge 2 commits into
bendlang:mainfrom
toprameen:count-complete-audio-frames

Conversation

@toprameen

Copy link
Copy Markdown
Contributor

JS Audio.write counts each unmatched stereo sample as half a frame, so two separate one-sample writes can report one queued frame. Count complete frames per write, matching native behavior, and preserve whole-request rejection above 8192 samples.

Add a focused regression that prints 1 before the fix and 0 afterward through source/compiled CLIs and emitted Bun/Node. Local Audio fixtures and capacity checks pass; repository gate 54/54.

@toprameen
toprameen marked this pull request as ready for review October 17, 2026 19:56
@toprameen toprameen changed the title count complete frames in JS Audio.write fix(audio): count complete frames in JS Audio.write Oct 17, 2026
@Lorenzobattistela Lorenzobattistela self-assigned this Oct 19, 2026

@Lorenzobattistela Lorenzobattistela left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks! The fix reproduces: on main, JS prints 1 for two one-sample writes, and on this branch it prints 0, matching native. It also fixes a second case: on main, one odd sample followed by a full 8192-sample write is rejected in JS (queue 0.5 + 4096 > 4096), while native accepts it and reports 4096. All gates are green merged onto main 24a997f.

The JS device still doesn't match native, though. A few things before merging:

  1. The queue still drains in fractions of a frame. After filling the ring (4096 frames) and draining 0.3 of a frame by the clock, queued is 4095.7 and the write reports 4095. A caller that sizes its next write as 4096 - reported (the Sound.write loop in demos/app_slash_boss_3d does this) sends 1 frame, which is refused because 4095.7 + 1 > 4096. Native accepts it: its queue is written - read, both whole frame counts. A Node harness with a fake clock reproduces it on this branch.

  2. Because of (1), I'd fix the cause rather than round the input. Keep queued a whole number of frames, like native's counters: drain only whole frames by the clock, and carry the leftover time in at instead of dropping it into queued. Then Math.floor(n / 2) is the only rounding left, and the result no longer needs Math.floor(audio.queued).

  3. Please add a C twin for pause (tests/io/audio_whole_frames.c, returning the handle unchanged). Without it the native build fails with "no .c import: pause", so the gate drops the C lane and the whole-frames rule is never checked on native. With an identity pause the test prints 0 natively too. Then the "JS only" line in the header can go.

Smaller:

  • n > 8192 ? 0 changes nothing: for n > 8192, frames > 4096 already fails the room check. Dropping it removes the bare 8192, and the 4096 that's left should be one named constant next to audio.c's IO_RING.
  • pause works by writing the handle's private rate field. If audio_open.js ever renames it, pause does nothing, and the old n/2 bug still prints 0 (0.5 - ε + 0.5 rounds down), so the test would stop catching the regression. A short comment in audio_open.js naming the test that depends on rate would help.

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