Conversation
maciejmakowski2003
left a comment
maciejmakowski2003
left a comment
There was a problem hiding this comment.
worth to check performance on drums
There was a problem hiding this comment.
Pull request overview
This PR aims to ensure a stable 128-frame render quantum across platforms after the incoming audio graph refactor, by making iOS/Android audio callbacks pull the graph in fixed RENDER_QUANTUM_SIZE steps and handling mismatched system buffer sizes via buffering.
Changes:
- iOS: refactors
IOSAudioPlayerrender callback to always pull 128-frame quanta and carry over any “tail” frames across callbacks. - Android: configures Oboe to request
RENDER_QUANTUM_SIZEframes per data callback. - Convolver: removes the custom
processInputsoverride and adds a runtime warning when quantum sizing is unexpected.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/react-native-audio-api/ios/audioapi/ios/core/IOSAudioPlayer.mm | Implements fixed-quantum pulling and tail buffering for non-multiple-of-128 callback sizes. |
| packages/react-native-audio-api/ios/audioapi/ios/core/IOSAudioPlayer.h | Adds private buffering helpers/state to support fixed-quantum delivery. |
| packages/react-native-audio-api/common/cpp/audioapi/core/effects/ConvolverNode.h | Removes the processInputs override declaration. |
| packages/react-native-audio-api/common/cpp/audioapi/core/effects/ConvolverNode.cpp | Removes processInputs override implementation and adds a warning check in processNode. |
| packages/react-native-audio-api/android/src/main/cpp/audioapi/android/core/AudioPlayer.cpp | Requests 128 frames per Oboe data callback via setFramesPerDataCallback. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| #import <NativeAudioPlayer.h> | ||
| #else // when compiled as C++ | ||
| typedef struct objc_object NativeAudioPlayer; | ||
| typedef struct objc_object AudioBufferList; |
There was a problem hiding this comment.
AudioBufferList is a CoreAudio C struct, but in the non-ObjC branch it’s currently typedef’d to objc_object. That makes the deliverOutputBuffers(AudioBufferList*, ...) member have a different signature depending on whether __OBJC__ is defined (pure C++ TUs like AudioContext.cpp vs ObjC++), which is undefined behavior and can break linking/ABI. Prefer a consistent forward declaration (e.g., struct AudioBufferList;) in the C++ branch, or hide deliverOutputBuffers behind #ifdef __OBJC__ so the class definition is identical across translation units.
| typedef struct objc_object AudioBufferList; | |
| struct AudioBufferList; |
packages/react-native-audio-api/ios/audioapi/ios/core/IOSAudioPlayer.h
Outdated
Show resolved
Hide resolved
packages/react-native-audio-api/ios/audioapi/ios/core/IOSAudioPlayer.mm
Outdated
Show resolved
Hide resolved
| // if not running, set output to 0 | ||
| if (!isRunning_.load(std::memory_order_acquire)) { | ||
| for (int channel = 0; channel < channelCount_; ++channel) { | ||
| auto *outputChannel = static_cast<float *>(outputData->mBuffers[channel].mData); | ||
| std::memset(outputChannel, 0, static_cast<size_t>(numFrames) * sizeof(float)); | ||
| } | ||
| return; | ||
| } | ||
|
|
||
| int outPos = 0; | ||
| while (outPos < numFrames) { | ||
| const int need = numFrames - outPos; | ||
|
|
||
| if (pendingSavedCount_ > 0) { | ||
| const int fromPending = std::min(need, pendingSavedCount_); | ||
|
|
||
| // populate output with pendingSaved | ||
| for (int ch = 0; ch < channelCount_; ++ch) { | ||
| float *dst = static_cast<float *>(outputData->mBuffers[ch].mData) + outPos; | ||
| const float *src = pendingSaved[ch].begin(); | ||
| std::memcpy(dst, src, fromPending * sizeof(float)); | ||
|
|
||
| // move the remaining samples to the beginning of the pendingSaved buffer | ||
| const int remain = pendingSavedCount_ - fromPending; | ||
| if (remain > 0) { | ||
| float *buf = pendingSaved[ch].begin(); | ||
| std::memmove(buf, buf + fromPending, remain * sizeof(float)); | ||
| } | ||
| } | ||
|
|
||
| pendingSavedCount_ -= fromPending; | ||
| outPos += fromPending; | ||
| continue; | ||
| } | ||
|
|
||
| renderAudio_(audioBuffer_, RENDER_QUANTUM_SIZE); | ||
|
|
There was a problem hiding this comment.
deliverOutputBuffers checks isRunning_ only once at the top of the callback. If stop()/suspend() flips isRunning_ while this callback is still executing (and numFrames > 128, causing multiple renderAudio_ pulls), this will keep rendering audio for the remainder of the callback. Consider re-checking isRunning_ inside the while loop (e.g., right before each renderAudio_ call) and zero-filling the remaining output if it becomes false.
| if (processingBuffer->getSize() != RENDER_QUANTUM_SIZE) { | ||
| printf( | ||
| "[AUDIOAPI WARN] convolver requires 128 buffer size for each render quantum, otherwise quality of convolution is very poor\n"); | ||
| } |
There was a problem hiding this comment.
printf is used in processNode but this file doesn’t include <cstdio>/<stdio.h>, which can be a build break in C++. Also, emitting printf from the audio thread is not real-time safe and can cause glitches. If you need diagnostics here, prefer a debug-only assert or a one-time atomic flag that can be surfaced/logged off the audio thread; and if the intent is to validate quantum sizing, the check should likely be framesToProcess != RENDER_QUANTUM_SIZE rather than processingBuffer->getSize() (buffer capacity).
|
@maciejmakowski2003 tbh I do not see any performance downgrade |
…Player.mm Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Closes #
Introduced changes
Checklist