fix(android/ios): cache AudioBuffer copy instead of recopying on every setBuffer - #1281
Closed
WentTheFox wants to merge 2 commits into
Closed
WentTheFox wants to merge 2 commits into
WentTheFox wants to merge 2 commits into
Conversation
…y setBuffer AudioBufferSourceNodeHostObject::setBuffer() deep-copied the entire AudioBuffer on every `.buffer = x` reassignment, even when reassigning the exact same underlying buffer. That's exactly what seeking is. A live AudioBufferSourceNode can't be repositioned, so the standard pattern is to stop/disconnect it and create a fresh source node wrapping the already-decoded buffer at a new offset. Every seek therefore triggered a full, unnecessary copy of the whole track's PCM data. On a 4-minute stereo buffer this is a real full-size copy (~85MB) per seek. The old copy isn't freed until its replacement's scheduled audio event actually runs, so repeated rapid seeking accumulates full-size buffers faster than the audio thread can free the previous ones. The growing native heap eventually crashes the app via a Hermes GC OOM once it can no longer find room to grow the JS heap. This caches the defensive copy on the JS-visible AudioBufferHostObject and reuses it across repeated reassignments of the same buffer. The cache is invalidated only when the buffer's data could actually have been mutated: copyToChannel, or a live getChannelData() view having escaped to JS, since JS could write through that view at any later time. This keeps the exact copy-on-first-touch semantics the original code needed for pitch-correction and mutation safety, while making the "same buffer, many source nodes" pattern free after the first copy instead of paying for a fresh copy on every single reassignment. The caching logic lives in a new, standalone ImmutableBufferCache utility rather than directly in AudioBufferHostObject, because HostObjects/*.cpp is excluded from this project's C++ test suite (see common/cpp/test/CMakeLists.txt) and a plain utility class can be unit tested without a jsi::Runtime. Also documents this in the best-practices guide. The existing "reuse the same AudioBuffer across nodes" guidance was already correct, but silently expensive before this fix; it is now actually cheap as advertised. Verified: - The library's own C++ test suite still passes in full, plus 4 new unit tests for ImmutableBufferCache covering reuse, distinct-copy identity, and both invalidation paths (420/420 total). - A minimal repro app (creating a fresh AudioBufferSourceNode/GainNode pair wrapping the same AudioBuffer every ~150ms, simulating rapid seeking) went from ~71MB leaked per seek (visibly crashing within ~60 iterations) to no measurable per-seek growth across three consecutive 60-iteration runs, measured via `adb shell dumpsys meminfo` before/after/+30s-settled. - A real app using this pattern for playback seeking held native heap flat (Android, Samsung Galaxy S24+) across ~200 rapid seeks that previously crashed within a similar span. Fixes software-mansion#1263
WentTheFox
marked this pull request as ready for review
September 10, 2026 11:20
WPT non-regression comparisonERROR — the comparison did not produce a report; the test run itself likely failed. Workflow run · this comment is updated on every push. |
Member
|
I changed your approach in #1283, left your commit so after merge you will still have contribution. You can look in mentioned PR's description for more detailed explanation, but tldr it will be easier to iterate this way and I also adressed something called |
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
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.
Closes #1263
None.
Introduced changes
AudioBufferSourceNodeHostObject::setBuffer()deep-copied the entireAudioBufferon every.buffer = xreassignment, even when reassigning the exact same underlying buffer. That's exactly what seeking is. A liveAudioBufferSourceNodecan't be repositioned, so the standard pattern is to stop/disconnect it and create a fresh source node wrapping the already-decoded buffer at a new offset. Every seek therefore triggered a full, unnecessary copy of the whole track's PCM data.On a 4-minute stereo buffer this is a real full-size copy (~85MB) per seek. The old copy isn't freed until its replacement's scheduled audio event actually runs, so repeated rapid seeking accumulates full-size buffers faster than the audio thread can free the previous ones. The growing native heap eventually crashes the app via a Hermes GC OOM once it can no longer find room to grow the JS heap.
This caches the defensive copy on the JS-visible
AudioBufferHostObjectand reuses it across repeated reassignments of the same buffer. The cache is invalidated only when the buffer's data could actually have been mutated:copyToChannel, or a livegetChannelData()view having escaped to JS, since JS could write through that view at any later time. This keeps the exact copy-on-first-touch semantics the original code needed for pitch-correction and mutation safety, while making the "same buffer, many source nodes" pattern free after the first copy instead of paying for a fresh copy on every single reassignment.The caching logic lives in a new, standalone
ImmutableBufferCacheutility rather than directly inAudioBufferHostObject, becauseHostObjects/*.cppis excluded from this project's C++ test suite (seecommon/cpp/test/CMakeLists.txt), and a plain utility class can be unit tested without ajsi::Runtime.Also updates the best-practices guide. The existing "reuse the same
AudioBufferacross nodes" advice was already correct, but silently expensive before this fix; it is now actually cheap as advertised.by @mdydek - implemented "acquire buffer content" logic in the
AudioBufferSourceNode. It blocks and invalidates all the js views of the buffer that were remembered before callingstarton the node, thus making changes after start noop in the context of the nodeVerification
ImmutableBufferCachecovering reuse, distinct-copy identity, and both invalidation paths (420/420 total).AudioBufferSourceNode/GainNodepair wrapping the sameAudioBufferevery ~150ms to simulate rapid seeking, went from ~71MB leaked per seek (crashing within ~60 iterations) to no measurable per-seek growth across three consecutive 60-seek runs, measured withadb shell dumpsys meminfobefore/after/+30s-settled.Checklist
AudioBufferSourceNode.web.tsdelegates directly to the browser's native implementation, which never had this bugAudioBuffer/setBufferare JSI HostObjects, not TurboModule methods, so nothing in the generated spec changed🤖 Generated with Claude Code
https://claude.ai/code/session_014PuKaaBmaMbrAtgs1a4xTC