fix(audio-studio/ios): make AAC trims honour the requested rate and depth - #461
Merged
Conversation
added 5 commits
August 21, 2026 15:06
…epth The AAC path wrote source-format buffers to a target-rate writer, violating AVAudioFile's format-match contract and mis-timing the output. Probed at a 44.1kHz source: one second came back as 2.0000s at 22.05kHz and 0.9187s at 48kHz. It now converts against the writer's resolved format — the same treatment the WAV path received in #450 — with the same ratio-sized output buffer, single-shot input, downmix on channel reduction, and errors instead of silent skips. The writer is scoped so the read-back sees the finished file. Fixing the rate exposed a pre-existing bitrate defect: the encoder rejects combinations its profile cannot serve — measured, 96kbps and up fail at 22.05kHz mono with error 560226676, and no formula predicts the ceiling (my first guess of 8 bits/sample/channel was wrong; the real limit there is ~3). So the writer is opened with the requested bitrate and reopened without one when the encoder refuses, honouring the request where possible instead of failing a trim the platform can serve. Bit depth: an omitted bitDepth forced 16 during any rate or channel change, contrary to the contract that the input format is preserved, and a bitDepth-only request took the fast path and was ignored. The default is now the input's own depth, depth changes leave the fast path, and no result hardcodes 16. Validated on the iOS simulator, per the hard rule: - 1000ms trim to AAC@22050: durationMs 1114 (AAC priming), rate 22050 — previously ~2000ms - 1000ms trim to WAV bitDepth 32: durationMs 1000, bitDepth 32 — previously ignored - plain single/keepRanges trims unchanged at 5000ms/4000ms Adds testTrimAudioWith(options) to the agentic bridge so trim changes can be validated on device with arbitrary options.
…olving them Three review blockers, all reproduced on device before fixing. The AAC path checked only the writer's resolved format, so a rate the platform cannot serve came back as something else: an AAC writer turns 1Hz into 8kHz and that conversion succeeds. The requested format is now checked first, matching the WAV path. On device, sampleRate: 1 rejects where it previously would have returned 8kHz audio. An explicitly requested bitrate the encoder refuses was silently replaced with its default — the result carries no effective bitrate and a debug log is not caller-visible. An explicit request now errors; only the library's own 128000 default gives way, which makes the encoder default intentional rather than a swallowed failure. On device: explicit 128k at 22.05kHz rejects, the same trim without a bitrate succeeds at 22050. Bit depth clamped the input before comparing it, so a 24-bit source asked for 16-bit compared equal, took the fast path, kept 24 bits and reported 16. The raw depth now drives the comparison while a separate value supplies the writable default, so unexpressible depths still default sensibly without hiding a real conversion. Device validation (iOS simulator, fresh install): - aac@1Hz: REJECTED (was: 8kHz output) - aac@22050 with explicit 128k: REJECTED; without: 1115ms @ 22050 - wav bitDepth 32: 1000ms, depth 32 - plain single/keepRanges: 5000ms/4000ms, unchanged
…substitute The requested-format check proved a PCM conversion existed, not that the encoder would use the rate asked for. Probed: the AAC writer resolves 1Hz and 7999Hz to 8000Hz, and 384000Hz to 192000Hz — all of which passed the PCM check and would have returned audio at a rate the caller never requested. The writer's resolved rate is now compared against the request and a substitution refused. On device: aac@1 and aac@384000 reject, while aac@8000/22050/48000 succeed at exactly the rate requested. Changelog corrected — it still described bitrate fallback as the behaviour when an explicit request now errors.
Round-2 review found three defects, each reproduced with an AVFoundation probe. The fast path claimed to preserve the input and was the one place not doing it. It wrote `inputFormat.settings`, where inputFormat is processingFormat — float32 for every PCM WAV. Probe: a genuine 16-bit source produced a 32-bit float output while the result reported bitDepth 16. Now writes fileFormat.settings. The [16, 32] writable-depth allowlist was wrong. Probed each depth against AVAudioFile: 8, 16, 24 and 32 all round-trip at the requested depth. The allowlist silently downconverted 8- and 24-bit sources to 16 with nothing requested, contradicting the documented preserve-input contract. The AAC conversion loop discarded the converter status and accepted zero output frames. A 44.1kHz-to-8kHz conversion of 1-4 input frames returns .endOfStream with no error and zero frames; the loop wrote that buffer and counted the input frames, so the reported duration described audio the file does not contain. Now checks the status, skips empty buffers, counts written frames, and refuses to promote a file that received none. Two more found while fixing those, unrelated to #451: - `?? 16 / 8` parsed as `?? (16 / 8)`, so with the bit-depth key present bytesPerSample was 16 rather than 2 and every byte offset in the preview loop came out eight times too large. - In the AAC block, totalFrames started at zero and grew as work completed while cumulativeFrames was never incremented there at all — progress was a stale outer value divided by a moving total. The denominator is now precomputed and input consumed is tracked separately from frames written. Coverage: AudioProcessor.swift cannot join the SwiftPM test target (it pulls in the Expo module graph), which is why every one of these shipped untested. The format decisions are extracted into TrimFormatResolution and used by AudioProcessor, with 8 tests over the depth and fast-path rules. 91 iOS tests pass, up from 83.
… one converter helper Round-3 review found two defects. The first is one I should have caught: the production target did not compile. `writtenFrames` was declared inside the AAC autoreleasepool closure and read by the empty-output guard after it — `cannot find 'writtenFrames' in scope`. The declaration is now outside the closure. I reported "91 iOS tests pass" as evidence last round, and that was worthless here: Package.swift does not list AudioProcessor.swift (it pulls in the Expo module graph), so the suite never compiled the file I changed. Verified this time by building the app target: BUILD SUCCEEDED with the file compiled for real. The WAV re-encode path still promoted empty output as success. It had the same zero-frame defect fixed on the AAC path last round — discarded converter status, wrote zero-frame buffers, promoted unconditionally. The reviewer's probe produced a promoted 4096-byte WAV with length 0 from a 44.1kHz-to-8kHz conversion of 1 to 4 frames. Rather than fix it twice, both loops now call one `convertOneBuffer` helper that owns the supply-once callback, the error check, the status check and the frame count. The duplication is what let the two paths drift in the first place — one got the guard, the other did not — which was also the reviewer's nit. Also required a `pod install`: the podspec glob covers TrimFormatResolution.swift but the existing pod project predated it, so the build reported "Build input file cannot be found" until the project was regenerated. 91 iOS tests pass, and the app target builds.
Collaborator
Author
Device validation (iOS Simulator, iPhone 17 Pro Max)Built and installed the app target from this branch, then exercised trim end to end. Recorded a source file: Trimmed it via The check that matters, run against the actual files with Both outputs. Genuine 16-bit integer WAVs. For contrast, what the pre-fix fast path produced — an AVFoundation probe writing A 32-bit float file reported as 16-bit. That is the defect, and it is gone. Build verification
91 SwiftPM tests also pass, covering the extracted Note: the installed bundle predates |
added 6 commits
August 21, 2026 23:16
This branch adds ios/TrimFormatResolution.swift, and the podspec glob publishes it, so the package now ships 439 files rather than 438. The manifest check merged in #469 caught exactly that — which is the case it was written for. Regenerated: one line added, nothing else changed.
…ts tail Review found multi-range trims produce different audio depending on how the selection is split. Both re-encode loops built a fresh AVAudioConverter per range, and convertOneBuffer signalled .endOfStream after each buffer, so the resampler's filter state and fractional sample position were discarded between ranges. Measured at 44.1 to 48kHz, converting 4410 frames: as one range 4800 frames as 100 adjacent 44-frame ranges 47 frames Two changes. The converter is created once, before the loop, in both the WAV and AAC paths. And convertOneBuffer now reports .noDataNow rather than .endOfStream when its single buffer is consumed, which keeps the stream open instead of finalizing the resampler. .endOfStream was the larger half of the problem: hoisting alone does nothing while every call still ends the stream. .noDataNow leaves samples buffered when the ranges run out, so both loops now drain the converter afterwards. That recovers the tail: as one range 4800 as 100 ranges, after the fix 4789 ideal 4800 0.2% short across a hundred splices rather than 99%. The first version of the drain compiled under `yarn test:ios` and failed the real build with three "cannot find in scope" errors, because it sat outside the autoreleasepool holding outputFile and the converter. That suite does not compile AudioProcessor.swift, which is why the app target is the check that counts here. 91 SwiftPM tests pass and the app target builds with AudioProcessor.swift compiled.
…ized rate Three findings from review, the first a direct consequence of my last commit. Switching to .noDataNow let AVAudioConverter fill the destination entirely from output it had already queued, returning without ever invoking the callback. The input buffer went unconsumed while the loop advanced to the next range and dropped it. Probed on the simulator: ranges [4410, 44] produced 4800 frames against 4848 expected, and [4410, 44 x 10] produced 4943 against 5279. convertOneBuffer now converts until the callback has actually taken the input, writing each output buffer as it appears — one input can yield several, so both call sites stopped assuming the destination holds all of it. The AAC writer echoes the requested sample rate while finalization can substitute a different one. Probed: 8001 to 8000, 22051 to 22050, 44099 and 44101 to 44100, 48001 to 48000. The work file is now reopened and its rate checked before promotion, so the result cannot report a rate the file does not have. The drain broke on zero frames before checking status, so a .error with no NSError set looked like a clean end of stream and truncated output was promoted as success. Status is checked first now. A failed buffer allocation there also ended the flush silently; that is an error rather than a break, since the alternative is silent truncation. testEveryProbedDepthIsWritable compared the constant to itself while claiming to have verified against AVAudioFile. It now writes and reopens a WAV at each of 8, 16, 24 and 32 bits and asserts the depth survives. 91 SwiftPM tests pass and the app target builds with AudioProcessor.swift compiled. On the simulator, trimAudio produces 5.000s and 4.000s outputs, both Int16, the second being the multi-range case these fixes are about.
Review: the pre-promotion validation used try?, so a work file that could not be reopened skipped the rate check entirely and was promoted anyway — an unreadable output reported as success, which is worse than the substituted-rate case the check was added for. Reopening now throws with the underlying reason. Also merged the two `### Fixed` sections that had accumulated under [Unreleased]. All entries kept; the section order is now Fixed, Added, Changed. The reviewer's probe confirms the convert loop from the last commit behaves: ranges [4410, 44] needed eight queued-output writes before the converter took the second buffer, and finalized correctly. 91 SwiftPM tests pass, the app target builds, and on the simulator trimAudio still produces a 5.000s Int16 output.
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #451.
Reproduced, fixed, validated on device
The fixes
Rate: the AAC path wrote source-format buffers to a target-rate writer. Now converts against the writer's resolved format — the same treatment the WAV path received in #450: ratio-sized output buffer, single-shot input, downmix on channel reduction, throw instead of skip, writer scoped before read-back.
Bitrate: fixing the rate exposed a pre-existing defect — the encoder rejects bitrates its profile can't serve at the target rate. Measured: 96kbps+ fails at 22.05kHz mono (error 560226676); no formula predicts the ceiling — my first guess of 8 bits/sample/channel was wrong, the real limit there is ~3. So the writer opens with the requested bitrate and reopens without one when refused, since omitting the key always succeeds.
Depth: omitted
bitDepthforced 16 during any conversion, contrary to the preserve-input contract; a depth-only request took the fast path and was ignored. Default is now the input's own depth, depth changes leave the fast path, no result hardcodes 16.Infrastructure
Adds
testTrimAudioWith(options)to the agentic bridge — the existingtestTrimAudiois fixed-options, so trim changes couldn't be validated on device with the failing configurations.Suites
iOS 83 passed, typecheck clean, app BUILD SUCCEEDED (fresh install;
lastUpdateTimechecked).