Repository navigation
Updated the SignalSmith module to be much easier to work with. - #1164
Conversation
WalkthroughAdds three public APIs to SignalSmith: Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant SignalSmith
participant minimp3
participant AudioServer
participant WAVEncoder
Caller->>SignalSmith: change_tempo(path, tempo, pitch)
SignalSmith->>minimp3: Load MP3 file / decode frames
minimp3-->>SignalSmith: PCM frames
SignalSmith->>AudioServer: Query current mix/sample rate
AudioServer-->>SignalSmith: mix_rate
SignalSmith->>SignalSmith: Create playback & resample to mix_rate
SignalSmith->>SignalSmith: Deinterleave channels
SignalSmith->>SignalSmith: Time-stretch & pitch-shift processing
SignalSmith->>SignalSmith: Re-encode to 16-bit PCM
SignalSmith->>WAVEncoder: Build AudioStreamWAV from PCM data
WAVEncoder-->>SignalSmith: AudioStreamWAV
SignalSmith-->>Caller: Ref<AudioStreamWAV>
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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
🤖 Fix all issues with AI agents
In `@modules/signalsmith/signalsmith_module.cpp`:
- Around line 51-55: The CI fails due to inconsistent indentation and an extra
space: replace the space-based indentation with tabs for the
ClassDB::bind_method lines binding get_last_sample_rate, get_last_channels,
reset, process, and change_tempo, and remove the extra space after
"bind_method(" on the line binding get_last_channels so all five bind_method
calls use tab indentation and exact formatting as the surrounding file.
- Around line 213-223: Indentation in the mixing loop uses spaces instead of
tabs; update the while block that manipulates frames (the loop containing
frames.resize(old + block), int mixed = pb->mix(...), and frames.resize(old)) to
use tabs for all leading indentation so it matches project style and fixes the
CI lint error, ensuring each nested statement (including the if (mixed <= 0)
block and break;) is indented with a tab level consistent with surrounding code.
In `@modules/signalsmith/signalsmith_module.h`:
- Around line 63-68: The declarations for get_last_sample_rate,
get_last_channels, process, and change_tempo in signalsmith_module.h are
indented with spaces instead of the project's tabs causing clang-format CI
failure; open the header and replace the leading spaces on those lines with tabs
to match the file's existing indentation style (align the function declarations
for get_last_sample_rate(), get_last_channels(), PackedFloat32Array
process(...), and Ref<AudioStreamWAV> change_tempo(...)) so the file
consistently uses tabs.
🧹 Nitpick comments (2)
modules/signalsmith/signalsmith_module.cpp (1)
207-208: Local variables shadow member variables.
channelsandsample_rateshadow the class member variables of the same name. While the code works correctly because the local values are passed to the setters (lines 241-242), this pattern is confusing and error-prone for future maintenance.♻️ Proposed fix - use distinct local variable names
- const int channels = mp3->is_monophonic() ? 1 : 2; - const int sample_rate = AudioServer::get_singleton()->get_mix_rate(); + const int mp3_channels = mp3->is_monophonic() ? 1 : 2; + const int mix_rate = AudioServer::get_singleton()->get_mix_rate();Then update usages at lines 229, 233, 241-242, 268-269 accordingly.
doc/classes/SignalSmith.xml (1)
12-23: Documentation forchange_tempolooks good, but getter methods are missing.The documentation for
change_tempois clear and accurate. However, theget_last_sample_rate()andget_last_channels()methods that were added in the implementation are not documented here.📝 Add missing method documentation
<method name="get_last_channels"> <return type="int" /> <description> Returns the number of audio channels configured for the last processed audio. </description> </method> <method name="get_last_sample_rate"> <return type="int" /> <description> Returns the sample rate configured for the last processed audio. </description> </method>
fdfaf6f to
0b7ec5a
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@modules/signalsmith/signalsmith_module.cpp`:
- Around line 213-223: The loop currently treats only mixed <= 0 as
end-of-stream, leaving zeroed frames when pb->mix(...) returns a positive value
less than block; change the logic in the loop that calls pb->mix(frames.ptrw() +
old, 1.0f, block) so that if mixed < block you resize to old + mixed (not old +
block) and then break, preserving only the actual mixed frames; keep the
existing behavior for mixed <= 0 where you resize back to old and break.
- Around line 192-205: In change_tempo, validate the incoming p_tempo and
p_pitch immediately: check that both are finite (not NaN/inf) and positive
before proceeding, and if either is invalid return early with ERR_FAIL_COND_V
(returning the empty Ref<AudioStreamWAV> out) to avoid reusing stale values;
perform these checks at the top of SignalSmith::change_tempo (using
std::isfinite or Math::is_nan equivalents) and only call any playback setters
(e.g., on the AudioStreamPlaybackResampled instance) when the inputs pass
validation.
🧹 Nitpick comments (1)
doc/classes/SignalSmith.xml (1)
24-33: Add brief descriptions for the new getters. The empty descriptions make the API feel incomplete in docs.
| while (true) { | ||
| int old = frames.size(); | ||
| frames.resize(old + block); | ||
| int mixed = pb->mix(frames.ptrw() + old, 1.0f, block); | ||
|
|
||
| if (mixed <= 0) { | ||
| frames.resize(old); | ||
|
|
||
| break; | ||
| } | ||
| } |
There was a problem hiding this comment.
Handle partial mixes to avoid trailing silence. If pb->mix() returns a positive count smaller than block, the current code keeps the remaining zeroed frames, inflating length and adding silence. On Line 216, treat mixed < block as end-of-stream and resize to old + mixed.
🔧 Proposed fix
while (true) {
int old = frames.size();
frames.resize(old + block);
int mixed = pb->mix(frames.ptrw() + old, 1.0f, block);
if (mixed <= 0) {
frames.resize(old);
break;
}
+
+ if (mixed < block) {
+ frames.resize(old + mixed);
+ break;
+ }
}AudioStreamPlaybackResampled::mix return semantics at end-of-stream: can it return a positive value smaller than the requested frame count?
🤖 Prompt for AI Agents
In `@modules/signalsmith/signalsmith_module.cpp` around lines 213 - 223, The loop
currently treats only mixed <= 0 as end-of-stream, leaving zeroed frames when
pb->mix(...) returns a positive value less than block; change the logic in the
loop that calls pb->mix(frames.ptrw() + old, 1.0f, block) so that if mixed <
block you resize to old + mixed (not old + block) and then break, preserving
only the actual mixed frames; keep the existing behavior for mixed <= 0 where
you resize back to old and break.
There was a problem hiding this comment.
@mcdubhghlas can pb->mix actually return a partially full buffer here?
I feel like the majority of the time we should be seeing multiples of 1024, but in the off chance it doesnt line up, this could result in an audio buffer underflow with audio artifacts.
We may want to consider @coderabbitai 's suggested fix here.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for AudioStreamPlaybackResampled class definition and mix method
rg -n "class AudioStreamPlaybackResampled" --type=cpp -A 30
Length of output: 2022
🏁 Script executed:
#!/bin/bash
# Search for the mix method implementation in AudioStreamPlaybackResampled
rg -n "AudioStreamPlaybackResampled.*::mix" --type=cpp -A 20
Length of output: 4892
🏁 Script executed:
#!/bin/bash
# Also search for any virtual mix method that might be overridden
rg -n "virtual.*mix.*AudioFrame" --type=cpp -B 2 -A 10
Length of output: 12087
🏁 Script executed:
#!/bin/bash
# Get the full implementation of AudioStreamPlaybackResampled::mix
rg -n "int AudioStreamPlaybackResampled::mix" --type=cpp -A 60
Length of output: 4572
🏁 Script executed:
#!/bin/bash
# Also look at _mix_internal implementations which is called by mix
rg -n "int.*_mix_internal.*AudioFrame.*p_buffer.*p_frames" --type=cpp -A 30 | head -200
Length of output: 14268
@Arctis-Fireblight Yes, pb->mix() can definitely return a partially full buffer!
Based on the source code analysis, AudioStreamPlaybackResampled::mix is designed to return fewer frames than requested when it reaches the end of the stream. Here's what happens:
The method internally calls _mix_internal() and when that returns fewer frames than requested (lines 235-241 in servers/audio/audio_stream.cpp), it marks where the audio data ends. The final return value (line 248) is mixed_frames_total, which can be less than the requested p_frames when the stream ends.
Current issue: When mixed is between 1 and block-1 (i.e., 0 < mixed < 1024), the code keeps the full resized buffer including the zeroed frames at the end, causing exactly the audio artifact you're concerned about.
Recommendation: CodeRabbit's suggested fix is correct and should be applied. The loop should handle the partial buffer case:
if (mixed < block) {
frames.resize(old + mixed);
break;
}This ensures only the actual mixed frames are preserved, preventing trailing silence or buffer underflow artifacts in the processed audio output.
🧬 Code Graph Analysis Results
[modules/signalsmith/signalsmith_module.cpp:change_tempo function]
Ref SignalSmith::change_tempo(const String &path, float p_tempo, float p_pitch) {
Ref out;
Ref<AudioStreamMP3> mp3 = AudioStreamMP3::load_from_file(path);
ERR_FAIL_COND_V(mp3.is_null(), out);
Ref<AudioStreamPlayback> pb_base = mp3->instantiate_playback();
ERR_FAIL_COND_V(pb_base.is_null(), out);
AudioStreamPlaybackResampled *pb = Object::cast_to<AudioStreamPlaybackResampled>(pb_base.ptr());
ERR_FAIL_COND_V(pb == nullptr, out);
pb->start(0.0);
const int channels = mp3->is_monophonic() ? 1 : 2;
const int sample_rate = AudioServer::get_singleton()->get_mix_rate();
Vector<AudioFrame> frames;
const int block = 1024;
while (true) {
int old = frames.size();
frames.resize(old + block);
int mixed = pb->mix(frames.ptrw() + old, 1.0f, block);
if (mixed <= 0) {
frames.resize(old);
break;
}
}
ERR_FAIL_COND_V(frames.is_empty(), out);
// Convert AudioFrame to float PCM
PackedFloat32Array input;
input.resize(frames.size() * channels);
float *input_w = input.ptrw();
for (int i = 0; i < frames.size(); i++) {
if (channels == 2) {
input_w[i * 2 + 0] = frames[i].left;
input_w[i * 2 + 1] = frames[i].right;
} else {
input_w[i] = frames[i].left;
}
}
set_sample_rate(sample_rate);
set_channels(channels);
set_tempo(p_tempo);
set_pitch(p_pitch);
reset();
PackedFloat32Array processed = process(input);
ERR_FAIL_COND_V(processed.is_empty(), out);
// convert float pcm to pcm16
PackedByteArray pcm16;
pcm16.resize(processed.size() * 2);
uint8_t *pcm_w = pcm16.ptrw();
const float *proc_r = processed.ptr();
for (int i = 0; i < processed.size(); i++) {
float s = CLAMP(proc_r[i], -1.0f, 1.0f);
int16_t v = int16_t(s * 32767.0f);
pcm_w[i * 2 + 0] = uint8_t(v & 0xff);
pcm_w[i * 2 + 1] = uint8_t((v >> 8) & 0xff);
}
// Builds a streamable WAV
out.instantiate();
out->set_mix_rate(sample_rate);
out->set_stereo(channels == 2);
out->set_format(AudioStreamWAV::FORMAT_16_BITS);
out->set_data(pcm16);
return out;
}
There was a problem hiding this comment.
Thanks CodeRabbit!
@mcdubhghlas do you mind making the suggested fix and then rebasing off of the current dev branch?
This branch you are working off of seems to contain already merged commits.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
…RNED NITO A BEETLE BUT HOW WILL I GET TO WORK!?!?!?!one!?
0b7ec5a to
387f63a
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@doc/classes/SignalSmith.xml`:
- Around line 24-33: The getters get_last_channels and get_last_sample_rate lack
descriptions; add concise docstrings clarifying that "last" refers to the most
recently processed or configured audio stream values (e.g., the channel count
and sample rate of the last processed buffer/frame or last set input). Update
the <description> for method name="get_last_channels" to state it returns the
channel count of the most recently processed audio buffer/stream, and update the
<description> for method name="get_last_sample_rate" to state it returns the
sample rate (in Hz) of the most recently processed audio buffer/stream.
♻️ Duplicate comments (2)
modules/signalsmith/signalsmith_module.cpp (2)
192-205: Fail fast on invalid tempo/pitch to avoid stale state reuse.
Invalid values are silently ignored by setters, so a bad call can reuse previous settings.✅ Suggested guardrails
Ref<AudioStreamWAV> SignalSmith::change_tempo(const String &path, float p_tempo, float p_pitch) { Ref<AudioStreamWAV> out; + + ERR_FAIL_COND_V(!(p_tempo > 0.0f), out); + ERR_FAIL_COND_V(!(p_pitch > 0.0f), out); Ref<AudioStreamMP3> mp3 = AudioStreamMP3::load_from_file(path); ERR_FAIL_COND_V(mp3.is_null(), out);
213-221: Handle partial mixes to avoid trailing silence.
Ifpb->mix()returns fewer frames than requested, the extra zeroed frames remain; alsoold + mixedis unsafe ifmixedis negative.🔧 Safer mix loop
while (true) { int old = frames.size(); frames.resize(old + block); int mixed = pb->mix(frames.ptrw() + old, 1.0f, block); if (mixed <= 0) { - frames.resize(old + mixed); + frames.resize(old); break; } + + if (mixed < block) { + frames.resize(old + mixed); + break; + } }
| <method name="get_last_channels" qualifiers="const"> | ||
| <return type="int" /> | ||
| <description> | ||
| </description> | ||
| </method> | ||
| <method name="get_last_sample_rate" qualifiers="const"> | ||
| <return type="int" /> | ||
| <description> | ||
| </description> | ||
| </method> |
There was a problem hiding this comment.
Add short descriptions for the new getters.
Empty descriptions make it unclear what “last” refers to for users.
📝 Suggested doc update
<method name="get_last_channels" qualifiers="const">
<return type="int" />
<description>
+ Returns the channel count last set via [method set_channels] or [method change_tempo].
</description>
</method>
<method name="get_last_sample_rate" qualifiers="const">
<return type="int" />
<description>
+ Returns the sample rate last set via [method set_sample_rate] or [method change_tempo].
</description>
</method>🤖 Prompt for AI Agents
In `@doc/classes/SignalSmith.xml` around lines 24 - 33, The getters
get_last_channels and get_last_sample_rate lack descriptions; add concise
docstrings clarifying that "last" refers to the most recently processed or
configured audio stream values (e.g., the channel count and sample rate of the
last processed buffer/frame or last set input). Update the <description> for
method name="get_last_channels" to state it returns the channel count of the
most recently processed audio buffer/stream, and update the <description> for
method name="get_last_sample_rate" to state it returns the sample rate (in Hz)
of the most recently processed audio buffer/stream.
2 participants
Now there is just less nonsense to use this. You simply select the file you want to stream, apply the tempo (pitch, too, but that is optional) and then play it.
Pitch is defaulted to 1.0, meaning it does not change.
Here is a video of it working:
https://github.com/user-attachments/assets/b352491f-87e5-46ba-a8de-7eb477827021
and here is the project file
redot-signalsmith-test.zip
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.