module: added Signalsmith time-stretching module. #1150
Conversation
WalkthroughIntroduces a new SignalSmith audio processing module that wraps the Signalsmith Stretch time-stretching library, accompanied by comprehensive third-party audio processing libraries (Signalsmith Linear for FFT/STFT/linear algebra and Signalsmith Stretch for pitch/time manipulation). Includes module registration, build configuration, documentation, and web/command-line utilities for the third-party libraries. Changes
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Comment |
|
@coderabbitia review |
|
@Arctis-Fireblight: I'll review the changes in this PR. ✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI Agents
In @modules/signalsmith/SCsub:
- Line 9: Remove the nonexistent unused include path string
"#modules/signalsmith/signalsmith-linear/include" from the SCsub include list;
update the includes so only the valid
"#modules/signalsmith/signalsmith-stretch/include" remains and verify there are
no other references to signalsmith-linear (e.g., in any SConscript or build
instantiation code) to avoid dead entries.
- Around line 11-14: The CCFLAGS array currently disables warnings globally
which hides issues in our own code; instead, remove the global "-Wno-shadow" and
"-Wno-class-memaccess" entries from CCFLAGS and wrap only the external library
header include (the include that pulls in signalsmith-stretch around where the
file includes that header) with scoped suppression using #pragma GCC diagnostic
push, the appropriate #pragma GCC diagnostic ignored for -Wshadow and
-Wclass-memaccess, then #pragma GCC diagnostic pop after the include; ensure the
scoped pragmas surround the specific include of the signalsmith-stretch headers
so the module’s own compilation still reports warnings.
In @modules/signalsmith/signalsmith_module.cpp:
- Around line 98-122: SignalSmith::process currently computes input_frames via
integer division which silently drops trailing samples when total_samples %
channels != 0; add a validation after computing total_samples that checks if
(total_samples % channels != 0) and handle it explicitly (for example, log an
error/warning via your logger or return an empty PackedFloat32Array) instead of
continuing; reference the symbols total_samples, channels, input_frames and
perform the check before computing input_frames so the function either rejects
the malformed input or documents/truncates it deterministically.
In @modules/signalsmith/signalsmith-stretch:
- Line 1: The repository is missing proper submodule configuration for
signalsmith-stretch and signalsmith-linear; create a top-level .gitmodules file
with entries for path = modules/signalsmith/signalsmith-stretch and path =
modules/signalsmith/signalsmith-linear and their correct upstream URLs (use the
official Signalsmith repo URLs), add and commit .gitmodules, then run git
submodule init && git submodule update --recursive --remote (or git submodule
update --init --recursive) to populate the directories, ensure each submodule
has a URL in .git/config after init, and update CI to clone with git clone
--recurse-submodules so CI fetches these submodules during builds.
🧹 Nitpick comments (5)
doc/classes/SignalSmith.xml (2)
62-64: Clarify the tempo-to-output relationship in the documentation.The description states "This value influences the ratio between input and output buffer sizes" but doesn't clearly explain how. Consider adding: "A tempo of 2.0 produces approximately half as many output samples as input samples, speeding up playback by 2x."
9-28: Consider enhancing the example to better demonstrate time-stretching.The current example generates a simple sine wave, which doesn't showcase the module's ability to handle complex audio. Consider adding a note like: "For real-world usage, input would typically come from AudioStreamSample.get_data() or a recorded audio buffer."
modules/signalsmith/signalsmith_module.h (2)
45-46: Document that changing sample_rate or channels resets internal state.Both
set_sample_rateandset_channelscallstretch.presetDefault(), which resets the internal processing state. This should be mentioned in the documentation (SignalSmith.xml) to warn users that changing these parameters mid-stream will cause audio discontinuities.
56-62: Consider adding getter methods for current configuration.The API only provides setters but no getters for the current sample_rate, channels, pitch, or tempo values. This makes it difficult for users to query the current state or validate their configuration.
Proposed additions to the public API
void set_sample_rate(int p_rate); void set_channels(int p_channels); void set_pitch(float p_pitch); void set_tempo(float p_tempo); + int get_sample_rate() const; + int get_channels() const; + float get_pitch() const; + float get_tempo() const; void reset();modules/signalsmith/signalsmith_module.cpp (1)
49-56: Remove redundant validation in constructor.Lines 50-51 check member variables that are already initialized to valid values in the header (
channels = 2,sample_rate = 44100). These checks will always evaluate to false and can be removed.🔎 Proposed fix
SignalSmith::SignalSmith() { - channels = (channels < 1) ? 1 : channels; - sample_rate = (sample_rate < 1) ? 44100 : sample_rate; tempo = 1.0f; // Correct current Signalsmith API stretch.presetDefault(channels, sample_rate); }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
doc/classes/SignalSmith.xmlmodules/signalsmith/SCsubmodules/signalsmith/config.pymodules/signalsmith/register_types.cppmodules/signalsmith/register_types.hmodules/signalsmith/signalsmith-linearmodules/signalsmith/signalsmith-stretchmodules/signalsmith/signalsmith_module.cppmodules/signalsmith/signalsmith_module.h
🧰 Additional context used
🧬 Code graph analysis (3)
modules/signalsmith/register_types.h (1)
modules/signalsmith/register_types.cpp (4)
initialize_signalsmith_module(37-43)initialize_signalsmith_module(37-37)uninitialize_signalsmith_module(45-49)uninitialize_signalsmith_module(45-45)
modules/signalsmith/signalsmith_module.h (1)
modules/signalsmith/signalsmith_module.cpp (16)
SignalSmith(49-56)SignalSmith(58-58)_bind_methods(40-47)_bind_methods(40-40)set_sample_rate(60-67)set_sample_rate(60-60)set_channels(69-76)set_channels(69-69)set_pitch(78-84)set_pitch(78-78)set_tempo(86-92)set_tempo(86-86)reset(94-96)reset(94-94)process(98-174)process(98-98)
modules/signalsmith/signalsmith_module.cpp (1)
modules/signalsmith/signalsmith_module.h (1)
SignalSmith(40-63)
🪛 GitHub Actions: 🔗 GHA
modules/signalsmith/SCsub
[error] 1-1: ruff: Undefined name Import (F821) in SCsub
[error] 3-3: ruff: Undefined name env (F821) in SCsub
[error] 23-23: ruff: Undefined name env (F821) in SCsub
doc/classes/SignalSmith.xml
[error] 1-1: file-format hook modified the file. Run formatting tool to review and commit changes.
🪛 Ruff (0.14.10)
modules/signalsmith/config.py
1-1: Unused function argument: env
(ARG001)
1-1: Unused function argument: platform
(ARG001)
🔇 Additional comments (18)
modules/signalsmith/signalsmith-linear (1)
1-1: No changes to review — Themodules/signalsmith/signalsmith-lineardirectory is empty and no.gitmodulesconfiguration exists. Either this PR is incomplete or the review target is incorrect. Clarify what files should actually be in this directory before proceeding.Likely an incorrect or invalid review comment.
modules/signalsmith/config.py (2)
1-2: LGTM! Standard cross-platform module configuration.The unconditional
Truereturn correctly indicates that this module can be built on all platforms. The unused parameters are required by the build system's interface and cannot be removed.
4-5: LGTM! No special configuration needed.The no-op implementation correctly indicates that this module requires no special build environment configuration. The unused
envparameter is required by the build system's interface.modules/signalsmith/register_types.h (1)
1-39: LGTM! Standard module registration header.The lifecycle function declarations follow the standard Redot module pattern and match the implementation in register_types.cpp.
modules/signalsmith/register_types.cpp (1)
37-49: LGTM! Standard module lifecycle implementation.The initialization correctly registers the SignalSmith class at the SCENE level, and the empty uninitialization is appropriate since RefCounted objects handle their own cleanup.
doc/classes/SignalSmith.xml (1)
1-82: Run the formatting tool to fix the file-format issue.The pipeline indicates that the file-format hook modified this file. Please run the project's formatting tool and commit the changes.
modules/signalsmith/SCsub (1)
1-25: SCsub files use SCons syntax and should not be linted as Python.The pipeline failures are false positives.
Import("env")andenvare standard SCons constructs found in 183+ SCsub files throughout the codebase (editor/, core/, modules/, drivers/, servers/, scene/, platform/). Theenv.*methods (likeadd_source_files(),Prepend(),Depends()) are SCons API calls, not Python. Ruff should be configured to exclude.scsubsubfiles from analysis.modules/signalsmith/signalsmith_module.h (2)
44-47: No action required. The pitch initialization relies on the Signalsmith Stretch library's default transpose factor of 1.0 (no pitch change), which is the correct behavior. The constructor intentionally does not callsetTransposeFactor; pitch is only modified when explicitly set via theset_pitch()method. This design is appropriate and consistent with the library's API.
37-37: The<random>include is necessary and should not be removed.The code directly uses
std::mt19937as a template parameter inSignalsmithStretch<float, std::mt19937>on line 44, which requires the<random>header. The signalsmith-stretch.h header does not provide this definition, so the include cannot be removed without breaking compilation.Likely an incorrect or invalid review comment.
modules/signalsmith/signalsmith_module.cpp (9)
40-47: LGTM!Method bindings are correctly implemented and all public API methods are properly exposed to the scripting system.
60-67: LGTM!Input validation and stretch reinitialization logic are correct.
69-76: LGTM!Input validation and stretch reinitialization logic are correct.
78-84: LGTM!The NaN-safe validation using
!(p_pitch > 0.0f)correctly rejects zero, negative values, and NaN.
86-92: LGTM!The NaN-safe validation is consistent with
set_pitchand correctly implemented.
94-96: LGTM!Reset delegation is correctly implemented.
162-174: LGTM!The interleaving logic correctly reconstructs the output array from per-channel buffers.
125-156: The heap allocations in the deinterleave and output buffer setup are acceptable for this offline/batch audio processing use case. This function processes discrete audio chunks (viaPackedFloat32Array), not streaming audio, so temporary vector allocations per call are both appropriate and expected.
159-159: The code already implements comprehensive input validation before callingstretch.process(). Thechannels,input_frames, andoutput_framesare all validated at lines 101–122, the stretch instance is properly configured with matching channel count and sample rate (lines 55, 66, 75), and separate input and output buffers are allocated (lines 125–156). This preventive validation approach is the correct pattern for the SignalSmith C++ library, which asserts on invalid conditions rather than throwing exceptions. No additional error handling is needed.Likely an incorrect or invalid review comment.
modules/signalsmith/SCsub
Outdated
| CPPPATH=[ | ||
| "#modules/signalsmith", | ||
| "#modules/signalsmith/signalsmith-stretch/include", | ||
| "#modules/signalsmith/signalsmith-linear/include", |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if signalsmith-linear is referenced anywhere in the module
echo "=== Searching for signalsmith-linear references ==="
rg -n "signalsmith.*linear" modules/signalsmith/
echo -e "\n=== Searching for any linear references ==="
rg -n "linear" modules/signalsmith/ | head -20
echo -e "\n=== Checking SCsub file context ==="
cat -n modules/signalsmith/SCsub | head -30
echo -e "\n=== Searching for signalsmith-stretch references ==="
rg -n "signalsmith.*stretch" modules/signalsmith/
echo -e "\n=== Listing files in signalsmith-linear directory ==="
find modules/signalsmith/ -type d -name "*linear*" -o -name "*stretch*" 2>/dev/nullRepository: Redot-Engine/redot-engine
Length of output: 1522
🏁 Script executed:
#!/bin/bash
# Check if signalsmith-stretch headers depend on signalsmith-linear
echo "=== Checking signalsmith-stretch headers for linear includes ==="
find modules/signalsmith/signalsmith-stretch -name "*.h" -o -name "*.hpp" | xargs rg -n "linear" 2>/dev/null || echo "No linear references found"
echo -e "\n=== All includes in signalsmith_module.h ==="
grep -n "#include" modules/signalsmith/signalsmith_module.h
echo -e "\n=== Checking what's in signalsmith-linear/include ==="
ls -la modules/signalsmith/signalsmith-linear/include/
echo -e "\n=== Checking all cpp/h files in signalsmith module ==="
find modules/signalsmith -maxdepth 1 -name "*.h" -o -name "*.cpp" | xargs grep -l "#include.*linear"Repository: Redot-Engine/redot-engine
Length of output: 50497
🏁 Script executed:
#!/bin/bash
# Check what's actually in signalsmith-linear directory
echo "=== Contents of signalsmith-linear directory ==="
find modules/signalsmith/signalsmith-linear -type f | head -20
echo -e "\n=== Directory structure ==="
find modules/signalsmith -type d | sortRepository: Redot-Engine/redot-engine
Length of output: 245
Remove the unused include path for signalsmith-linear.
The signalsmith-linear/include directory referenced in SCsub:9 does not exist and is not used anywhere in the module. Only signalsmith-stretch is included and instantiated in the source code. Remove this unused include path from the build configuration.
🤖 Prompt for AI Agents
In @modules/signalsmith/SCsub at line 9, Remove the nonexistent unused include
path string "#modules/signalsmith/signalsmith-linear/include" from the SCsub
include list; update the includes so only the valid
"#modules/signalsmith/signalsmith-stretch/include" remains and verify there are
no other references to signalsmith-linear (e.g., in any SConscript or build
instantiation code) to avoid dead entries.
modules/signalsmith/SCsub
Outdated
| CCFLAGS=[ | ||
| "-Wno-shadow", | ||
| "-Wno-class-memaccess", | ||
| ], |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, explore the signalsmith module structure
echo "=== Signalsmith module structure ==="
find modules/signalsmith -type f | head -20
echo -e "\n=== SCsub file content ==="
cat -n modules/signalsmith/SCsub
echo -e "\n=== Key headers and source files ==="
fd -e h -e hpp -e cpp -e c . modules/signalsmith | head -20Repository: Redot-Engine/redot-engine
Length of output: 1201
🏁 Script executed:
#!/bin/bash
# Check if the module code itself has shadowing or memaccess patterns
echo "=== Looking for variable shadowing patterns ==="
rg -A5 -B5 'for\s*\([^)]*\$|{[^}]*\$' modules/signalsmith --type cpp --type c
echo -e "\n=== Checking for direct memory operations that might trigger class-memaccess ==="
rg 'memcpy|memmove|memset|std::copy|std::fill' modules/signalsmith --type cpp --type c -A2 -B2Repository: Redot-Engine/redot-engine
Length of output: 331
🏁 Script executed:
#!/bin/bash
echo "=== register_types.cpp ==="
wc -l modules/signalsmith/register_types.cpp
cat -n modules/signalsmith/register_types.cpp
echo -e "\n=== signalsmith_module.cpp ==="
wc -l modules/signalsmith/signalsmith_module.cpp
head -100 -n modules/signalsmith/signalsmith_module.cpp | cat -nRepository: Redot-Engine/redot-engine
Length of output: 3468
🏁 Script executed:
#!/bin/bash
# Check external library headers for potential issues
echo "=== Checking external library headers ==="
find modules/signalsmith/signalsmith-stretch/include -type f -name "*.h" -o -name "*.hpp" | head -10
find modules/signalsmith/signalsmith-linear/include -type f -name "*.h" -o -name "*.hpp" | head -10
echo -e "\n=== Sample of external headers ==="
find modules/signalsmith -path "*/include/*.h" -o -path "*/include/*.hpp" | head -5 | xargs wc -lRepository: Redot-Engine/redot-engine
Length of output: 330
🏁 Script executed:
#!/bin/bash
echo "=== signalsmith_module.cpp (full file) ==="
cat -n modules/signalsmith/signalsmith_module.cppRepository: Redot-Engine/redot-engine
Length of output: 6966
🏁 Script executed:
#!/bin/bash
# Also check the header file to understand the class structure
echo "=== signalsmith_module.h ==="
cat -n modules/signalsmith/signalsmith_module.hRepository: Redot-Engine/redot-engine
Length of output: 3610
Consider using pragma directives to suppress warnings only for external library headers instead of globally.
The module code itself is clean and doesn't contain variable shadowing or unsafe memory operations. However, the warnings are likely triggered by the external signalsmith-stretch library headers included at line 38 of the header. Rather than suppressing warnings globally via CCFLAGS (which disables checks for the entire module), use #pragma GCC diagnostic directives around the external library includes to limit the suppression scope. This preserves warning detection for the module's own code while allowing compilation of headers that have these patterns.
🤖 Prompt for AI Agents
In @modules/signalsmith/SCsub around lines 11 - 14, The CCFLAGS array currently
disables warnings globally which hides issues in our own code; instead, remove
the global "-Wno-shadow" and "-Wno-class-memaccess" entries from CCFLAGS and
wrap only the external library header include (the include that pulls in
signalsmith-stretch around where the file includes that header) with scoped
suppression using #pragma GCC diagnostic push, the appropriate #pragma GCC
diagnostic ignored for -Wshadow and -Wclass-memaccess, then #pragma GCC
diagnostic pop after the include; ensure the scoped pragmas surround the
specific include of the signalsmith-stretch headers so the module’s own
compilation still reports warnings.
| @@ -0,0 +1 @@ | |||
| Subproject commit 2133f41a8f2766d2ade2e665fcf04f9ff7fd74a7 | |||
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check .gitmodules configuration
if [ -f .gitmodules ]; then
echo "=== .gitmodules content ===" && cat .gitmodules && echo ""
else
echo "No .gitmodules file found" && echo ""
fi
# Search for any documentation mentioning submodules or git setup
echo "=== Searching for submodule/git setup documentation ===" && \
fd -type f \( -name "README*" -o -name "SETUP*" -o -name "CONTRIBUTING*" -o -name "*.md" \) | \
head -20 | xargs grep -l "submodule\|git clone\|--recurse" 2>/dev/null || echo "No git setup docs found"
# Check for CI/CD configuration
echo "=== Checking CI/CD configuration files ===" && \
fd -type f \( -name "*.yml" -o -name "*.yaml" -o -name ".github" \) | grep -E "workflow|github|gitlab|ci" | head -10 && \
fd -type f -path "*/.github/workflows*" -o -name ".gitlab-ci.yml" -o -name ".travis.yml" -o -name "Jenkinsfile" 2>/dev/null | head -5
# Check if the Signalsmith submodule URL can be inferred from git config
echo "=== Checking git configuration ===" && \
git config --file .gitmodules --get-regexp signalsmith 2>/dev/null || echo "No git submodule config found"
# List all submodules if any exist
echo "=== All submodules ===" && \
git config --file .gitmodules --get-regexp path 2>/dev/null || echo "No submodules configured"Repository: Redot-Engine/redot-engine
Length of output: 690
🏁 Script executed:
# Check the actual file and its location
echo "=== Checking modules/signalsmith directory ===" && \
ls -la modules/signalsmith/ 2>/dev/null || echo "Directory not found"
# Show the actual content of the file mentioned in the review
echo "" && echo "=== Content of modules/signalsmith/signalsmith-stretch ===" && \
cat modules/signalsmith/signalsmith-stretch 2>/dev/null || echo "File not found"
# Check if .gitmodules exists anywhere
echo "" && echo "=== Searching for .gitmodules in entire repo ===" && \
find . -name ".gitmodules" -type f 2>/dev/null
# Check git status for any untracked or modified submodule files
echo "" && echo "=== Git status for modules directory ===" && \
git status modules/signalsmith/ 2>/dev/null || echo "Not tracked by git"
# Check if this is a gitlink (special git object type)
echo "" && echo "=== Checking git ls-files for signalsmith ===" && \
git ls-files --stage modules/signalsmith/ 2>/dev/null || echo "Not in git index"Repository: Redot-Engine/redot-engine
Length of output: 1739
🏁 Script executed:
# Check git config for submodule URLs
echo "=== Git submodule URLs in .git/config ===" && \
git config --file .git/config --get-regexp "submodule.*url" || echo "No submodule URLs found"
# Check if .gitmodules was ever in the repo
echo "" && echo "=== Searching git history for .gitmodules ===" && \
git log --all --full-history -- .gitmodules 2>/dev/null | head -20 || echo "No .gitmodules in history"
# Check submodule status
echo "" && echo "=== Git submodule status ===" && \
git submodule status 2>/dev/null || echo "Submodule command failed"
# Check if submodules are initialized
echo "" && echo "=== Checking if submodule directories are empty ===" && \
ls -la modules/signalsmith/signalsmith-stretch/ && \
ls -la modules/signalsmith/signalsmith-linear/ || echo "Directories are empty (not initialized)"
# Look for any .git references in submodule directories
echo "" && echo "=== Checking for .git in submodules ===" && \
[ -f modules/signalsmith/signalsmith-stretch/.git ] && echo "signalsmith-stretch has .git file" || echo "signalsmith-stretch missing .git"Repository: Redot-Engine/redot-engine
Length of output: 581
Critical: Submodule configuration is incomplete and will prevent cloning and building.
The submodules signalsmith-stretch and signalsmith-linear are registered in git (mode 160000) but their configuration is broken:
- Missing .gitmodules file: Required to define submodule repository URLs and paths. Without it,
git clone --recurse-submoduleswill fail. - No URLs in .git/config: Submodule URLs are not configured, making initialization impossible.
- Empty submodule directories: Directories exist but are empty (no .git file or content), indicating submodules were never initialized.
- git submodule status fails: The submodule system cannot operate without proper configuration.
Required fixes:
- Create
.gitmodulesfile with entries for both submodules, specifying repository URLs and paths. - Ensure each entry points to the correct upstream repository (e.g., official Signalsmith repositories).
- Initialize and fetch the submodules to populate the directories.
- Verify CI/CD configuration includes
git clone --recurse-submodulesto prevent build failures.
This blocks merge and must be resolved before integration.
🤖 Prompt for AI Agents
In @modules/signalsmith/signalsmith-stretch at line 1, The repository is missing
proper submodule configuration for signalsmith-stretch and signalsmith-linear;
create a top-level .gitmodules file with entries for path =
modules/signalsmith/signalsmith-stretch and path =
modules/signalsmith/signalsmith-linear and their correct upstream URLs (use the
official Signalsmith repo URLs), add and commit .gitmodules, then run git
submodule init && git submodule update --recursive --remote (or git submodule
update --init --recursive) to populate the directories, ensure each submodule
has a URL in .git/config after init, and update CI to clone with git clone
--recurse-submodules so CI fetches these submodules during builds.
mcdubhghlas
force-pushed
the
SoundTouch
branch
6 times, most recently
from
5f54f89 to
e0a0b80
Compare
|
@coderabbitia review |
|
@Arctis-Fireblight: I'll review the changes in this PR. ✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 7
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
🤖 Fix all issues with AI Agents
In @modules/signalsmith/signalsmith_module.cpp:
- Around line 49-56: The constructor SignalSmith::SignalSmith reads
uninitialized members (channels, sample_rate); initialize them before use by
setting default values either in the member declarations or via the constructor
initializer list (e.g., set channels and sample_rate to sensible defaults) and
then call stretch.presetDefault(channels, sample_rate); ensure tempo is also
initialized in the initializer list or declarations to avoid UB.
In @thirdparty/signalsmith-linear/fft.h:
- Around line 897-908: The split-real fft implementation (fft) passes tmpTime as
a const Sample* to complexFft.fft but only zeroes
tmpTime[0..tmpTime.size()/2-1], leaving the second half uninitialized;
initialize the entire tmpTime buffer (iterate i < tmpTime.size() or otherwise
set all elements to zero) before calling complexFft.fft so the complex FFT
receives a fully-initialized input and does not corrupt the imaginary output
(tmpFreq/tmpFreqI).
In @thirdparty/signalsmith-linear/linear.h:
- Around line 77-90: The assignment operator macro
LINEAR_SPLIT_POINTER_ASSIGNMENT_OP defines Value & operator OP(Other &&v) which
calls std::complex<V>::operator OP(std::forward<Other>(v)) but then incorrectly
copies _real and _imag from the input v; change those reads to take the updated
base-class values (e.g. use this->real() and this->imag() or
std::complex<V>::real()/imag()) so compound operators and scalar inputs reflect
the new state after std::complex<V>::operator OP(...) updates the object.
- Around line 1052-1062: The explicit template specializations for
reserve<float> and reserve<double> inside the class are non-standard; move those
specializations out to namespace scope or, simpler, eliminate specializations
and implement the behavior in the primary template using compile-time branching
(e.g. if constexpr (std::is_same_v<V,float>) { cached.reserveFloats(size); }
else if constexpr (std::is_same_v<V,double>) { cached.reserveDoubles(size); } ).
Update the primary template reserve(size_t) (and include <type_traits>) so it
calls cached.reserveFloats/reserveDoubles for float/double and does nothing
otherwise, avoiding the need to access private members from namespace-scope
specializations.
In @thirdparty/signalsmith-linear/platform/fft-accelerate.h:
- Around line 211-238: Add a move constructor to the Pow2FFT<double>
specialization to mirror the float specialization: implement Pow2FFT(Pow2FFT&&
other) noexcept that transfers ownership of fftSetup, hasSetup, _size, log2 and
moves splitReal and splitImag from other, then resets other's
fftSetup/hasSetup/_size so the destructor won't double-destroy; ensure the copy
constructor remains deleted and consider marking the move as noexcept.
- Around line 166-168: The code computes log2 via std::log2(size) then passes it
to vDSP_create_fftsetup and stores it in log2 without rounding, which can
mis-handle floating-point precision; update the calculation to round the result
(e.g., use std::round or std::lround on std::log2(size)) before assigning to
log2 and before calling vDSP_create_fftsetup so the value is an exact integer as
done in Pow2FFT<float>::resize(), ensuring fftSetup and hasSetup get the correct
FFT exponent.
- Around line 294-298: In Pow2RealFFT<double>::resize(), the computation of log2
currently uses std::log2(size) directly; change it to compute log2 as
std::round(std::log2(size)) to match the complex FFT versions and avoid
fractional exponents, then pass that rounded value into vDSP_create_fftsetupD
and assign fftSetup and hasSetup as before (affecting splitImag, log2, fftSetup,
vDSP_create_fftsetupD, hasSetup in the resize() implementation).
In @thirdparty/signalsmith-linear/platform/linear-accelerate.h:
- Around line 452-456: The overload for fillDiv has the wrong parameter type (it
accepts expression::Sub but is named fillDiv); change the parameter type to
expression::Div<A, expression::ConstantExpr<V>> so the function becomes
fillDiv(RealPointer<double> pointer, expression::Div<A,
expression::ConstantExpr<V>> expr, size_t size) and keep the body that builds
recipB from expr.b.value and calls fillMul(pointer, expression::makeMul(expr.a,
recipB), size).
- Around line 68-75: The explicit member-template specializations reserve<float>
and reserve<double> are declared inside the class body which is ill-formed
pre-C++20; move those specializations to namespace scope and keep only the
primary template declaration inside the class. Concretely: remove the in-class
template<> void reserve<float>(size_t) and template<> void
reserve<double>(size_t) definitions, leave the generic reserve<T>(size_t)
declaration in the class, then add namespace-scope specializations like
template<> void ClassName::reserve<float>(size_t size) {
cached.reserveFloats(size*4); } and template<> void
ClassName::reserve<double>(size_t size) { cached.reserveDoubles(size*4); }
(replace ClassName with the actual class name).
In @thirdparty/signalsmith-linear/stft.h:
- Around line 443-453: The COMPAT_SPELLING aliases for step/steps are
inconsistent; ensure the canonical (name) uses UK spelling and the alt uses US
spelling: change the duplicate/wrong entries so you call
COMPAT_SPELLING(analyseStep, analyzeStep) and COMPAT_SPELLING(analyseSteps,
analyzeSteps) (keep the existing COMPAT_SPELLING(analyse, analyze) and the
synthesise/synthesize pairs as-is) so the US-named wrappers invoke the UK-named
implementations.
In @thirdparty/signalsmith-linear/tests/CMakeLists.txt:
- Line 33: The tests/CMakeLists currently calls add_subdirectory(".."
${CMAKE_CURRENT_BINARY_DIR}/signalsmith-linear), which inverts normal CMake
structure and can cause circular/dependency and ordering issues; remove that
add_subdirectory call from the tests CMakeLists and instead update the parent
CMakeLists (the one in thirdparty/signalsmith-linear) to include
add_subdirectory(tests) so the top-level project controls inclusion of the tests
directory and preserves conventional build ordering.
- Around line 14-21: The FetchContent_Declare block for signalsmith_dsp
currently sets SOURCE_DIR which forces the dependency into the source tree;
remove the SOURCE_DIR argument from the signalsmith_dsp FetchContent_Declare
invocation (leave GIT_REPOSITORY, GIT_TAG and GIT_SHALLOW as-is) and keep the
FetchContent_MakeAvailable(signalsmith_dsp) call so CMake manages the dependency
in the build directory instead of polluting others/dsp.
- Around line 24-31: The FetchContent_Declare block for signalsmith_plot
currently sets SOURCE_DIR which places the fetched repo into the source tree;
remove the SOURCE_DIR argument from the FetchContent_Declare call for
signalsmith_plot so CMake manages the downloaded content in its build folder,
leaving the GIT_REPOSITORY, GIT_TAG, GIT_SHALLOW lines intact and keep the
FetchContent_MakeAvailable(signalsmith_plot) call as-is.
In @thirdparty/signalsmith-linear/tests/main.cpp:
- Around line 1-10: The test file uses std::strcmp (in main.cpp where
comparisons occur at the checks around lines 43, 57, 59, 61) but does not
include <cstring>, which is non-portable; add an explicit #include <cstring>
near the other includes in thirdparty/signalsmith-linear/tests/main.cpp so
std::strcmp is properly declared and no longer depends on transitive includes.
In @thirdparty/signalsmith-linear/tests/Makefile:
- Around line 14-20: The Makefile target out/tests hardcodes macOS-only compiler
flags in the g++ invocation (-framework Accelerate,
-DSIGNALSMITH_USE_ACCELERATE, -DACCELERATE_NEW_LAPACK) and an Apple-specific
workaround macro (SIGNALSMITH_IGNORE_BROKEN_APPLECLANG); update the Makefile to
detect the platform (e.g., uname/DARWIN check) and conditionally set the g++
flags and defines for macOS while providing alternative flags for Linux/Windows
or fallback to invoking the CMake build, or else document/rename the file to
indicate macOS-only (e.g., Makefile.darwin) and add a comment explaining the
SIGNALSMITH_IGNORE_BROKEN_APPLECLANG workaround so users know why it exists.
- Around line 1-12: Add a .PHONY declaration for the non-file targets to avoid
conflicts: insert a line like ".PHONY: main dev benchmark benchmark-fast" near
the top of the Makefile so that the targets main, dev, benchmark and
benchmark-fast are treated as phony commands rather than filenames.
In @thirdparty/signalsmith-linear/tests/others/signalsmith-fft.h:
- Around line 406-408: sizeMaximum currently calls FFT<V>::sizeMinimum(size/2)*2
which returns the next valid size above rather than below; change the call to
FFT<V>::sizeMaximum(size/2)*2 so sizeMaximum returns the largest valid FFT size
<= requested size (i.e., replace the use of sizeMinimum with sizeMaximum in the
static sizeMaximum(size_t) method).
In @thirdparty/signalsmith-linear/tests/stopwatch.h:
- Around line 9-23: The Windows-specific block uses a non-standard macro
`WINDOWS` and is marked untested; update the preprocessor guard to use standard
macros (e.g., replace `#ifdef WINDOWS` with `#if defined(_WIN32) ||
defined(_WIN64) || defined(_MSC_VER)`), ensure the Stopwatch class (symbols:
Stopwatch, now(), toSeconds()) compiles by including correct headers and types,
and add a comment or test hook indicating the code path has been validated (or
call out to a fallback POSIX implementation) so the untested note is removed and
the Windows path is reliably detected and compiled.
In @thirdparty/signalsmith-linear/tests/test-ffts.h:
- Around line 1076-1092: In runSplit, remove the redundant second call to
fft.resize(n) (the duplicate of the earlier call) so the FFT buffer is only
resized once before querying fft.steps(); keep the initial fft.resize(n) that
populates the buffer and delete the later fft.resize(n) immediately before
size_t fftSteps = fft.steps() to avoid unnecessary work.
In @thirdparty/signalsmith-linear/tests/test-stfts.h:
- Around line 381-385: The loop currently assigns both pV and uV to
packed.spectrum(0)[b], so only the packed spectrum is modified; change the
second reference so uV binds to unpacked.spectrum(0)[b] instead, ensuring pV and
uV are set to the same random complex values to modify both packed and unpacked
spectra for the synthesis consistency test (look for variables/expressions named
packed, unpacked, spectrum(0), pV and uV).
- Around line 361-370: The test loop mistakenly reads uV from packed.spectrum
instead of from unpacked, so pV and uV are identical and the comparison never
fails; change the assignment of uV to read from unpacked.spectrum(0)[b]
(matching how pV is taken from packed.spectrum(0)[b]) so the comparison inside
the for loop correctly detects mismatches between packed and unpacked spectra.
In @thirdparty/signalsmith-stretch/CMakeLists.txt:
- Around line 6-15: The CMakeLists currently uses
FetchContent_Declare/FetchContent_MakeAvailable to fetch signalsmith-linear
(GIT_TAG 0.2.6) even though the library is vendored; replace the
FetchContent_Declare()/FetchContent_MakeAvailable block with
add_subdirectory(../signalsmith-linear) so the local thirdparty copy is used,
and ensure target_link_libraries(signalsmith-stretch INTERFACE
signalsmith-linear) remains unchanged; if you choose to keep FetchContent
instead, update the GIT_TAG to 0.2.8.
In @thirdparty/signalsmith-stretch/cmd/CMakeLists.txt:
- Around line 10-17: The CMake FetchContent block for the 'inputs' target is
downloading inputs.zip from an external URL without integrity or opt-in
controls; update the FetchContent_Declare call for symbol inputs to include
URL_HASH with the expected SHA256 digest (requires CMake 3.24+), or wrap the
FetchContent_Declare/FetchContent_MakeAvailable sequence behind a user option
like FETCH_STRETCH_INPUTS (default OFF) so downloads are explicit, or
alternatively commit and reference the inputs.zip (or its unpacked contents) in
the repo and point SOURCE_DIR to it to remove the network dependency entirely;
ensure the chosen approach updates any related logic that currently assumes the
asset is always fetched.
In @thirdparty/signalsmith-stretch/cmd/main-dev.cpp:
- Around line 223-229: The conditional ordering around diff2 is inverted so the
stricter check diff2 < 1e-10 is never reached; update the branch in main-dev.cpp
to test the smallest threshold first (check diff2 < 1e-10 before diff2 < 1e-6)
so values satisfying the tighter tolerance print Console::Green, otherwise fall
back to Console::Yellow or Console::Red; locate the block that uses variable
diff2 and Console::Yellow/Green/Red and reorder the if/else conditions
accordingly.
In @thirdparty/signalsmith-stretch/signalsmith-stretch.h:
- Around line 471-472: Fix the typo in the comment inside the condition that
checks inputSamples < seekLength in signalsmith-stretch.h: change the comment
text from "to short for this - zero the output just to be polite" to "too short
for this - zero the output just to be polite" so the comment correctly uses
"too" in the block guarded by the if statement.
In @thirdparty/signalsmith-stretch/web/demo/index.html:
- Around line 286-293: Guard against stretch being undefined in the upload
handler: before calling stretch.stop() and before using stretch later in the
handler, check that stretch is truthy (e.g., if (stretch) ...) so the handler
safely handles cases where initial audio load failed; update the
$('#upload-file').onchange async callback that calls stretch.stop(), await
handleFile(...), sets controlValues.active and calls controlsChanged() to only
invoke stretch methods/properties when stretch is defined, while still awaiting
handleFile and preserving existing error handling.
- Line 83: Replace the invalid CSS unit in the playback slider rule: find the
declaration "margin: 0 1rex;" in the index.html stylesheet and change the unit
from "rex" to the valid "rem" so it becomes "margin: 0 1rem;". Ensure no other
occurrences of "rex" remain in the CSS.
- Line 310: The code assigns a non-existent event handler
playbackPosition.onmousecancel; replace that with a real event such as
playbackPosition.onmouseleave (if you want the handler when the mouse leaves the
element) or playbackPosition.onpointercancel (if you want pointer-cancel
semantics), e.g. set playbackPosition.onmouseup = playbackPosition.onmouseleave
= e => { ... } or playbackPosition.onmouseup = playbackPosition.onpointercancel
= e => { ... } so the handler will actually be invoked.
In @thirdparty/signalsmith-stretch/web/emscripten/compile.sh:
- Line 1: Add an explicit shebang as the first line of the compile.sh script to
declare the shell interpreter (e.g., use /usr/bin/env to locate bash), so the
script runs portably and predictably across environments; update the file named
compile.sh to prepend the shebang line before the existing comment and ensure
file is executable if needed.
In @thirdparty/signalsmith-stretch/web/emscripten/main.cpp:
- Around line 68-76: seek, process, and flush call stretch.* with the global
buffersIn/buffersOut but never validate that setBuffers initialized them; add
checks at the start of EMSCRIPTEN_KEEPALIVE seek, process, and flush to verify
buffersIn and/or buffersOut are non-empty (or a boolean "buffersSet" flag set by
setBuffers), and if not, return early or signal an error (e.g., return a
negative error code or call an error/logging function) instead of invoking
stretch.seek/process/flush with empty vectors to prevent undefined behavior.
In @thirdparty/signalsmith-stretch/web/index.html:
- Around line 111-115: The click handler for suggestion nodes blindly calls
fn().then(...) which throws if fn() returns a non-Promise (e.g.,
stretch.start()/stretch.stop()); update the node.onclick assigned in the loop so
it captures the result of fn(), then either wrap it with
Promise.resolve(result).then(console.log) or detect if result has a .then
function and call .then only in that case, otherwise just console.log the
result; refer to the existing fn closure and node.onclick handler and the
'.suggestion-code' class to locate where to change this behavior.
In @thirdparty/signalsmith-stretch/web/Makefile:
- Around line 1-8: Add a .PHONY declaration listing the non-file development
targets so Make won't confuse them with files: add a line like `.PHONY: jsdoc
server watch` (and include any other dev-only targets such as `clean`, `dev`,
etc. if present) near the top of the Makefile, but do NOT mark file-producing
targets `release/SignalsmithStretch.mjs` or `release/SignalsmithStretch.js` as
phony.
In @thirdparty/signalsmith-stretch/web/web-wrapper.js:
- Around line 238-241: The code calls configure() without the instance prefix,
which will throw ReferenceError when the method is a class member; update the
call in the block that compares outputList[0].length to this.channels to invoke
this.configure() instead, ensuring you reference the class method (configure) on
the current instance and keep the existing logic that updates this.channels from
outputList[0]?.length || 0.
🟠 Major comments (16)
thirdparty/signalsmith-linear/tests/CMakeLists.txt-33-33 (1)
33-33: Reconsider the build structure: parent should include tests, not vice versa.Adding the parent directory (
"..") as a subdirectory from within the tests directory is unconventional and can lead to:
- Circular dependency problems
- Build order confusion
- Maintenance difficulties
Recommended approach: The parent
CMakeLists.txt(inthirdparty/signalsmith-linear/) should useadd_subdirectory(tests)to include this test configuration, rather than having tests reach up and include the parent. This follows standard CMake project organization patterns.thirdparty/signalsmith-linear/tests/CMakeLists.txt-14-21 (1)
14-21: Remove SOURCE_DIR to avoid polluting the source tree.Specifying
SOURCE_DIRinFetchContent_Declareplaces downloaded dependencies directly into the source tree (underothers/dsp), which:
- Pollutes the source directory with external content
- Can interfere with version control
- Prevents CMake from properly managing the dependency cache
- May cause issues during clean builds
Remove the
SOURCE_DIRparameter and let FetchContent manage dependencies in the build directory.🔎 Proposed fix
FetchContent_Declare( signalsmith_dsp GIT_REPOSITORY https://github.com/Signalsmith-Audio/dsp.git GIT_TAG 5c7d1f3eb375b4862b682d310ccfbaafb6a4477e # main GIT_SHALLOW OFF - SOURCE_DIR "${CMAKE_CURRENT_SOURCE_DIR}/others/dsp" )thirdparty/signalsmith-linear/tests/CMakeLists.txt-24-31 (1)
24-31: Remove SOURCE_DIR to avoid polluting the source tree.The same issue as with
signalsmith_dsp: specifyingSOURCE_DIRplaces the downloadedsignalsmith_plotdependency in the source tree (underplot/), which pollutes the source directory and prevents proper dependency management by CMake.🔎 Proposed fix
FetchContent_Declare( signalsmith_plot GIT_REPOSITORY https://github.com/Signalsmith-Audio/plot.git GIT_TAG 1.0.0 GIT_SHALLOW ON - SOURCE_DIR "${CMAKE_CURRENT_SOURCE_DIR}/plot" )thirdparty/signalsmith-stretch/web/emscripten/compile.sh-1-1 (1)
1-1: Add a shebang line.Shell scripts should explicitly declare their interpreter for portability and clarity.
🔎 Proposed fix
+#!/bin/bash # compile.sh main.cpp out.js ModuleNamethirdparty/signalsmith-linear/tests/stopwatch.h-9-23 (1)
9-23: Non-standard Windows detection macro and untested code.The
WINDOWSmacro is non-standard and must be defined externally. Standard Windows detection uses_WIN32,_WIN64, or_MSC_VER. Additionally, the comment "completely untested!" indicates this code path hasn't been validated.🔎 Suggested fix
-#ifdef WINDOWS // completely untested! +#if defined(_WIN32) || defined(_WIN64) # include <windows.h> class Stopwatch { using Time = __int64; using Duration = Time; inline Time now() { LARGE_INTEGER result; QueryPerformanceCounter(&result); return result.QuadPart; } static double toSeconds(Duration t) { LARGE_INTEGER freq; QueryPerformanceFrequency(&freq); + // QueryPerformanceFrequency should never fail on modern Windows return t/double(freq); }thirdparty/signalsmith-stretch/cmd/CMakeLists.txt-10-17 (1)
10-17: Security and reliability: External dependency without integrity verification.The build downloads
inputs.zipfrom an external URL without checksum validation. This introduces both security and reliability risks:
- Security: If the external server is compromised, malicious content could be injected into the build.
- Reliability: Build requires internet access and breaks if the URL becomes unavailable.
- Supply chain: No verification that downloaded content matches expected files.
🔎 Recommended fixes
Option 1: Add URL hash verification (requires CMake 3.24+):
FetchContent_Declare( inputs URL https://signalsmith-audio.co.uk/code/stretch/inputs.zip + URL_HASH SHA256=<expected_hash_here> SOURCE_DIR "${CMAKE_CURRENT_SOURCE_DIR}/inputs" )Option 2: Bundle inputs.zip in the repository to eliminate external dependency.
Option 3: Make inputs optional for users who don't need the examples:
option(FETCH_STRETCH_INPUTS "Download example inputs" OFF) if(FETCH_STRETCH_INPUTS) # FetchContent block here endif()Committable suggestion skipped: line range outside the PR's diff.
thirdparty/signalsmith-linear/tests/Makefile-14-20 (1)
14-20: Makefile is hardcoded for macOS only.The direct
g++build path uses macOS-specific flags (-framework Accelerate, Apple-specific defines) without platform detection, making it unusable on Linux or Windows. Consider either:
- Adding platform detection logic to support multiple platforms
- Documenting that this Makefile is macOS-only (rename to
Makefile.darwinor add clear comments)- Relying solely on the CMake build path for cross-platform support
The
SIGNALSMITH_IGNORE_BROKEN_APPLECLANGflag also suggests compiler compatibility workarounds that should be documented.thirdparty/signalsmith-stretch/web/Makefile-1-8 (1)
1-8: Add .PHONY declarations for development targets.The bundle generation logic is correct. However, add
.PHONYdeclarations for targets that don't represent actual files to prevent conflicts and improve Make's behavior.Note: The
release/SignalsmithStretch.mjsandrelease/SignalsmithStretch.jstargets are file targets, so they should NOT be phony, but later targets likejsdoc,server, andwatchshould be.thirdparty/signalsmith-linear/tests/Makefile-1-12 (1)
1-12: Add .PHONY declarations for non-file targets.The targets
main,dev,benchmark, andbenchmark-fastare command targets, not files. Add.PHONYdeclarations to prevent conflicts if files with these names exist and to improve Make's performance.🔎 Proposed fix
+.PHONY: main dev benchmark benchmark-fast + main: out/tests @cd out && ./teststhirdparty/signalsmith-stretch/CMakeLists.txt-6-15 (1)
6-15: Use add_subdirectory() for already-vendored signalsmith-linear instead of FetchContent.The signalsmith-linear library is already vendored at
thirdparty/signalsmith-linear/, making the FetchContent download redundant. Replace the FetchContent block (lines 6-13) withadd_subdirectory(../signalsmith-linear)to use the vendored version, which eliminates unnecessary internet access during builds and ensures consistency with the repository's dependency structure.Additionally, update the vendored version to 0.2.8 (latest), as the current pin to 0.2.6 is outdated. If using add_subdirectory, you'll use the locally vendored version directly; if keeping FetchContent, update GIT_TAG to 0.2.8.
thirdparty/signalsmith-linear/tests/others/signalsmith-fft.h-406-408 (1)
406-408: Bug:sizeMaximumincorrectly callssizeMinimuminstead ofsizeMaximum.This will return the next valid size above the requested size rather than below it, which is the opposite of the intended behavior.
Proposed fix
static size_t sizeMaximum(size_t size) { - return FFT<V>::sizeMinimum(size/2)*2; + return FFT<V>::sizeMaximum(size/2)*2; }thirdparty/signalsmith-linear/platform/fft-accelerate.h-211-238 (1)
211-238: Missing move constructor forPow2FFT<double>.The
Pow2FFT<float>specialization includes a move constructor (lines 84-88), butPow2FFT<double>only has the copy constructor deleted. This inconsistency prevents movingPow2FFT<double>instances.Proposed fix: add move constructor
~Pow2FFT() { if (hasSetup) vDSP_destroy_fftsetupD(fftSetup); } + // Allow move, but not copy + Pow2FFT(const Pow2FFT &other) = delete; + Pow2FFT(Pow2FFT &&other) : _size(other._size), hasSetup(other.hasSetup), fftSetup(other.fftSetup), log2(other.log2), splitReal(std::move(other.splitReal)), splitImag(std::move(other.splitImag)) { + other.hasSetup = false; + }thirdparty/signalsmith-linear/platform/linear-accelerate.h-68-75 (1)
68-75: Explicit template specialization inside class may not compile on all compilers.Explicit specializations of member function templates must be declared at namespace scope, not inside the class body. This is ill-formed prior to C++20 and may fail to compile on older compilers like GCC < 14 or Clang < 18.
Suggested refactor: move specializations outside the class
template<> struct LinearImpl<true> : public LinearImplBase<true> { // ... template<class V> void reserve(size_t) {} - - template<> - void reserve<float>(size_t size) { - cached.reserveFloats(size*4); - } - template<> - void reserve<double>(size_t size) { - cached.reserveDoubles(size*4); - } // ... }; +template<> +inline void LinearImpl<true>::reserve<float>(size_t size) { + cached.reserveFloats(size*4); +} +template<> +inline void LinearImpl<true>::reserve<double>(size_t size) { + cached.reserveDoubles(size*4); +}Committable suggestion skipped: line range outside the PR's diff.
thirdparty/signalsmith-linear/linear.h-77-90 (1)
77-90: Assignment operators read from input value instead of updated base class.After
std::complex<V>::operator OP(std::forward<Other>(v))updates the base class, lines 81-82 read fromvinstead of from the updated base class state. This breaks for compound operations and scalar inputs.🔎 Proposed fix
#define LINEAR_SPLIT_POINTER_ASSIGNMENT_OP(OP) \ template<class Other> \ Value & operator OP(Other &&v) { \ std::complex<V>::operator OP(std::forward<Other>(v)); \ - _real = v.real(); \ - _imag = v.imag(); \ + _real = std::complex<V>::real(); \ + _imag = std::complex<V>::imag(); \ return *this; \ }thirdparty/signalsmith-linear/linear.h-1052-1062 (1)
1052-1062: Explicit template specialization inside class definition is non-standard.The explicit template specializations
template<> void reserve<float>andtemplate<> void reserve<double>are defined inside the class body. This is a compiler extension (MSVC, some GCC versions accept it) but is not standard C++. For portability, specializations should be at namespace scope.🔎 Proposed fix - move specializations to namespace scope
template<bool useLinear> struct LinearImpl : public LinearImplBase<useLinear> { LinearImpl() : LinearImplBase<useLinear>(this), cached(*this) {} using LinearImplBase<useLinear>::fill; // ... fill overload ... template<typename V> void reserve(size_t) {} - // Makes sure we don't allocate - template<> - void reserve<float>(size_t size) { - cached.reserveFloats(size); - } - template<> - void reserve<double>(size_t size) { - cached.reserveDoubles(size); - } private: CachedResults<LinearImpl, 32> cached; }; + +// Explicit specializations at namespace scope +template<> +template<> +inline void LinearImpl<true>::reserve<float>(size_t size) { + cached.reserveFloats(size); +} + +template<> +template<> +inline void LinearImpl<true>::reserve<double>(size_t size) { + cached.reserveDoubles(size); +} + +template<> +template<> +inline void LinearImpl<false>::reserve<float>(size_t size) { + cached.reserveFloats(size); +} + +template<> +template<> +inline void LinearImpl<false>::reserve<double>(size_t size) { + cached.reserveDoubles(size); +}Note: The
cachedmember access will need adjustment as it's private. An alternative is to useif constexpror tag dispatch within the primary template.Committable suggestion skipped: line range outside the PR's diff.
thirdparty/signalsmith-linear/stft.h-443-453 (1)
443-453: COMPAT_SPELLING macro has incorrect alias definitions.The macro is designed to create alternative spellings (US vs UK English), but several aliases are incorrect:
- Line 449:
analyseStepis aliased to itself instead of toanalyzeStep- Line 450:
analyseStepsis aliased toanalyzeStepsbut the base method isanalyseSteps()The pattern should map
analyze*(US) toanalyse*(UK), but the current definitions are inconsistent.🔎 Proposed fix
COMPAT_SPELLING(analyse, analyze); - COMPAT_SPELLING(analyseStep, analyseStep); - COMPAT_SPELLING(analyseSteps, analyzeSteps); + COMPAT_SPELLING(analyseStep, analyzeStep); + COMPAT_SPELLING(analyseSteps, analyzeSteps); COMPAT_SPELLING(synthesise, synthesize); COMPAT_SPELLING(synthesiseStep, synthesizeStep); COMPAT_SPELLING(synthesiseSteps, synthesizeSteps);
🟡 Minor comments (11)
thirdparty/signalsmith-stretch/web/demo/index.html-286-293 (1)
286-293: Potential null reference on upload ifstretchis uninitialized.If the initial audio load fails,
stretchmay be undefined when the user clicks upload. Callingstretch.stop()unconditionally could throw.🔎 Proposed fix
$('#upload-file').onchange = async e => { - stretch.stop(); + stretch?.stop(); await handleFile($('#upload-file').files[0]).catch(e => alert(e.message));thirdparty/signalsmith-stretch/web/index.html-111-115 (1)
111-115: Calling.then()on non-Promise return values will throw.The suggestion snippets include calls like
stretch.start()andstretch.stop()which may not return Promises. Calling.then()onundefinedthrows a TypeError.🔎 Proposed fix
$$('#suggestions pre').forEach(node => { let fn = new Function('return ' + node.textContent); node.classList.add('suggestion-code'); - node.onclick = e => fn().then(console.log); + node.onclick = e => Promise.resolve(fn()).then(console.log); });thirdparty/signalsmith-stretch/web/demo/index.html-83-83 (1)
83-83: CSS typo:1rexshould be1rem.The unit
rexis not valid CSS; browsers will ignore this property. This affects the horizontal margin on the playback slider.🔎 Proposed fix
#playback { grid-area: playback; height: 100%; position: relative; padding: 0; - margin: 0 1rex; + margin: 0 1rem; accent-color: currentcolor; }thirdparty/signalsmith-stretch/web/demo/index.html-310-310 (1)
310-310:onmousecancelis not a valid DOM event.The
mousecancelevent does not exist in the DOM specification. The handler will never be invoked. Consider usingonmouseleaveoronpointercanceldepending on the intended behavior.🔎 Proposed fix
- playbackPosition.onmouseup = playbackPosition.onmousecancel = e => { + playbackPosition.onmouseup = playbackPosition.onmouseleave = e => { playbackHeld = false; };thirdparty/signalsmith-linear/tests/main.cpp-1-10 (1)
1-10: Missing<cstring>header forstd::strcmp.The code uses
std::strcmpat lines 43, 57, 59, and 61, but<cstring>is not explicitly included. While it may be transitively included through other headers, relying on this is non-portable.🔎 Proposed fix
#include <iostream> #define LOG_EXPR(expr) std::cout << #expr << " = " << (expr) << std::endl; #include "./test-linear.h" #include "./test-ffts.h" #include "./test-stfts.h" #include <cstdlib> +#include <cstring> #include "./stop-denormals.h"thirdparty/signalsmith-linear/CMakeLists.txt-22-27 (1)
22-27: Use the correct find_package syntax for Intel IPP and ensure the CMake package path is discoverable.Intel oneAPI ships a standard CMake config (
IPPConfig.cmake), but the currentfind_package(IPP REQUIRED)uses incorrect syntax and lacks necessary path configuration. The correct approach isfind_package(ipp CONFIG REQUIRED)(lowercaseipp) and requires either setting theIPPROOTenvironment variable or adding the oneAPI CMake package directory toCMAKE_PREFIX_PATH(e.g.,<oneapi>/ipp/<version>/lib/cmake/ipp). Additionally, ensure the target names match what the package exports; the linked targetsIPP::ippcore,IPP::ipps,IPP::ippimay differ depending on the IPP version.thirdparty/signalsmith-stretch/cmd/main-dev.cpp-223-229 (1)
223-229: Color threshold conditions are ordered incorrectly.The stricter condition (
diff2 < 1e-10) will never be reached because any value satisfying it also satisfiesdiff2 < 1e-6, which is checked first. A very small difference (good result) would show Yellow instead of Green.🔎 Proposed fix
- if (diff2 < 1e-6) { - std::cout << Console::Yellow; - } else if (diff2 < 1e-10) { + if (diff2 < 1e-10) { std::cout << Console::Green; + } else if (diff2 < 1e-6) { + std::cout << Console::Yellow; } else { std::cout << Console::Red; }thirdparty/signalsmith-stretch/signalsmith-stretch.h-471-472 (1)
471-472: Typo: "to short" should be "too short".Proposed fix
if (inputSamples < seekLength) { - // to short for this - zero the output just to be polite + // too short for this - zero the output just to be politethirdparty/signalsmith-linear/platform/fft-accelerate.h-166-168 (1)
166-168: Missingstd::roundwhen computinglog2may cause incorrect FFT size.Unlike
Pow2FFT<float>::resize()at line 100, this uses direct assignment without rounding. For exact powers of 2,std::log2should return an integer, but floating-point precision issues could cause truncation errors.Proposed fix for consistency
- log2 = std::log2(size); + log2 = std::round(std::log2(size));thirdparty/signalsmith-linear/platform/fft-accelerate.h-294-298 (1)
294-298: Missingstd::roundinPow2RealFFT<double>::resize().Same issue as
Pow2RealFFT<float>- thelog2computation should usestd::roundfor consistency with the complex FFT versions.Proposed fix
- log2 = std::log2(size); + log2 = std::round(std::log2(size));thirdparty/signalsmith-linear/tests/test-ffts.h-1076-1092 (1)
1076-1092: Duplicatefft.resize(n)call inrunSplit.The FFT is resized at line 1082 and again at line 1091. The second call is redundant.
🔎 Proposed fix
void runSplit(size_t n, signalsmith::plot::Line2D &line, double benchmarkSeconds) { time.resize(n); freqR.resize(n/2); freqI.resize(n/2); time2.resize(n); - fft.resize(n); - std::default_random_engine engine(123456); std::uniform_real_distribution<Sample> dist{-1, 1}; for (auto &v : time) v = dist(engine); for (auto &v : freqR) v = dist(engine); for (auto &v : freqI) v = dist(engine); for (auto &v : time2) v = dist(engine); fft.resize(n);
🧹 Nitpick comments (35)
thirdparty/signalsmith-linear/tests/CMakeLists.txt (1)
5-9: Remove commented-out code.The commented-out alternative on line 8 should be removed to keep the build configuration clean and maintainable.
🔎 Proposed cleanup
# MSVC needs an extra flag to handle this many templates if (MSVC) add_compile_options(/bigobj) - #set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} /bigobj") endif()thirdparty/signalsmith-linear/README.md (1)
93-95: Add language identifier to the fenced code block.The code block should specify
bashorshfor proper syntax highlighting.🔎 Proposed fix
-``` +```bash g++ -framework Accelerate -DSIGNALSMITH_USE_ACCELERATE</details> </blockquote></details> <details> <summary>thirdparty/signalsmith-stretch/README.md (1)</summary><blockquote> `72-72`: **Replace hard tab with spaces.** The line contains a hard tab character. For consistency with markdown formatting standards, use spaces instead. <details> <summary>🔎 Proposed fix</summary> ```diff - return inputFreq*2; // up one octave + return inputFreq*2; // up one octavethirdparty/signalsmith-stretch/web/emscripten/compile.sh (3)
3-3: Declare and assign separately to avoid masking return values.Command substitution in assignments can mask non-zero exit codes, making errors harder to detect.
🔎 Proposed fix
-export SCRIPT_DIR=`dirname "$0"` +SCRIPT_DIR=$(dirname "$0") +export SCRIPT_DIR
19-20: Add error handling for popd.The popd command could fail silently, leaving the script in an unexpected directory.
🔎 Proposed fix
pushd "${EMSDK_DIR}" && git pull && ./emsdk install latest && ./emsdk activate latest - popd + popd || exit 1
34-34: Quote command substitution to prevent word splitting.Unquoted command substitution can cause issues if the path contains spaces or special characters.
🔎 Proposed fix
-mkdir -p $(dirname $OUTPUT_JS) +mkdir -p "$(dirname "$OUTPUT_JS")"thirdparty/signalsmith-linear/tests/stopwatch.h (1)
88-91: Consider numerical stability in variance calculation.The variance calculation uses the algebraically equivalent but numerically less stable formula
E[X²] - E[X]². For very small variances or large means, this can suffer from catastrophic cancellation. While thestd::max(0.0, ...)clamping prevents negative results, consider using Welford's online algorithm for better numerical stability in production timing code.thirdparty/signalsmith-linear/tests/stop-denormals.h (2)
7-9: Document magic number for MXCSR flags.The value
0x8040represents MXCSR bits for Flush-to-Zero (FTZ, bit 15 = 0x8000) and Denormals-Are-Zero (DAZ, bit 6 = 0x0040). Consider adding a comment or named constants for clarity.🔎 Suggested documentation
StopDenormals() : controlStatusRegister(_mm_getcsr()) { - _mm_setcsr(controlStatusRegister|0x8040); // Flush-to-Zero and Denormals-Are-Zero + // MXCSR bits: 0x8000 (FTZ - Flush-to-Zero), 0x0040 (DAZ - Denormals-Are-Zero) + _mm_setcsr(controlStatusRegister | 0x8040); }
18-23: Minor: Redundant assignment in ARM implementation.Line 21 has a redundant assignment:
status = asmStatus = asmStatus|0x01000000U;. The middle assignment toasmStatusis immediately overwritten. Consider clarifying the intent.🔎 Possible clarification
StopDenormals() { uintptr_t asmStatus; asm volatile("mrs %0, fpcr" : "=r"(asmStatus)); - status = asmStatus = asmStatus|0x01000000U; // Flush to Zero + asmStatus |= 0x01000000U; // Flush to Zero (bit 24) + status = asmStatus; asm volatile("msr fpcr, %0" : : "ri"(asmStatus)); }thirdparty/signalsmith-linear/tests/Makefile (1)
22-40: Add .PHONY declarations and consider removing hardcoded Xcode generator.
- The CMake targets (
cmake,cmake-benchmark,cmake-benchmark-fast,clean) should be declared as.PHONY.- The hardcoded
-G Xcodegenerator limits cross-platform usage. Consider allowing CMake to use the default generator for better portability.🔎 Proposed fixes
+.PHONY: cmake cmake-benchmark cmake-benchmark-fast clean + out-cmake/tests: CMakeLists.txt *.cpp *.h ../*.h ../platform/*.h - cmake -B build -G Xcode + cmake -B build cmake --build build --config Release mkdir -p out-cmake cp build/Release/tests out-cmakethirdparty/signalsmith-stretch/cmd/main.cpp (1)
1-3: Remove unused debug macro.The
LOG_EXPRmacro is defined but never used in this file. Consider removing it to keep the code clean.🔎 Proposed fix
-// helper for debugging -#include <iostream> -#define LOG_EXPR(expr) std::cout << #expr << " = " << (expr) << "\n"; - #include "signalsmith-stretch/signalsmith-stretch.h"thirdparty/signalsmith-stretch/web/Makefile (1)
10-23: Add .PHONY and consider a clean target.
- Declare
jsdoc,server, andwatchas.PHONYtargets.- Consider adding a
cleantarget to remove generated files inrelease/andemscripten/for consistency with other Makefiles in the project.🔎 Proposed additions
+.PHONY: jsdoc server watch clean + ## Development helpers jsdoc: release/SignalsmithStretch.js release/SignalsmithStretch.mjs npx jsdoc release --verbose server: python3 -m http.server watch: # pip3 install watchdog watchmedo shell-command --patterns='*.js;Makefile' --command='make jsdoc;echo "rebuilt"' --drop + +clean: + rm -rf release/SignalsmithStretch.js release/SignalsmithStretch.mjs emscripten/main.js out/thirdparty/signalsmith-stretch/cmd/Makefile (1)
1-45: Add.PHONYdeclarations for non-file targets.The targets
all,examples,dev,clean, andcmakedon't produce files with those names. Declaring them as.PHONYprevents issues if files with those names are ever created and improves Make's efficiency.🔎 Proposed fix
+.PHONY: all examples dev clean cmake + all: out/stretch out/stretch: main.cpp ../signalsmith-stretch.h util/*.h util/*.hxxthirdparty/signalsmith-linear/CMakeLists.txt (1)
7-10:add_compile_optionsapplies globally, not to this target.Using
add_compile_options(/bigobj)affects all targets built in the current directory scope, not just consumers of thesignalsmith-linearINTERFACE library. If only consumers need this flag, consider usingtarget_compile_optionsinstead.🔎 Proposed fix
# MSVC needs an extra flag to handle this many templates if (MSVC) - add_compile_options(/bigobj) - #set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} /bigobj") + target_compile_options(signalsmith-linear INTERFACE /bigobj) endif()thirdparty/signalsmith-stretch/cmd/main-dev.cpp (1)
138-138: RedundantexactLengthcheck.This code is inside the
elsebranch whereexactLengthis alreadyfalse(line 128 checksif (exactLength)), so this ternary will always evaluate to the second branch.- int tailSamples = exactLength ? stretch.outputLatency() : (stretch.outputLatency() + stretch.inputLatency()); // if we don't need exact length, add a bit more output to catch any wobbles past the end + int tailSamples = stretch.outputLatency() + stretch.inputLatency(); // add a bit more output to catch any wobbles past the endthirdparty/signalsmith-stretch/web/demo/Scope.mjs (1)
1-1: This appears to be minified/bundled code.This file is a single-line minified bundle containing embedded WASM (base64), HTML, and JavaScript. The static analysis warnings (global reassignment, redeclarations, use-before-declaration) are artifacts of the minification process and not actual bugs.
Consider adding a comment at the top indicating this is generated code, or include the source files and build process in the repository for maintainability.
thirdparty/signalsmith-stretch/web/web-wrapper.js (1)
346-346: Global variable assignment without declaration.
SignalsmithStretchis assigned withoutvar,let, orconst, creating an implicit global. While this is intentional for the UMD module pattern, consider using an explicit declaration for clarity and to avoid strict mode issues.-SignalsmithStretch = ((Module, audioNodeKey) => { +var SignalsmithStretch = ((Module, audioNodeKey) => {thirdparty/signalsmith-linear/platform/fft-ipp.h (2)
378-388: Pointer-to-integer cast for alignment calculation.The C-style cast
size_t(m_state->workBuffer)on line 379 converts a pointer to an integer. While this works on most platforms, usereinterpret_castoruintptr_tfor clearer intent and better portability.- size_t work = size_t(m_state->workBuffer); + size_t work = reinterpret_cast<uintptr_t>(m_state->workBuffer);The same issue exists at line 484 for the
doublespecialization.
269-317: Consider removing commented-out code.These commented-out function implementations (complexMulConj and split-complex variants) appear to be copy-pasted from the Accelerate backend (references to
vDSP_zvmul,DSPSplitComplex). If they're not planned for implementation, removing them would improve maintainability.thirdparty/signalsmith-linear/platform/fft-pffft.h (2)
38-62: Potential issue whensizeis 0.Line 48 calls
pffft_aligned_malloc(sizeof(float)*size*2)withsize=0, which allocates 0 bytes. The behavior is implementation-defined - it may returnnullptror a unique non-null pointer. Later code (e.g., line 87'smemcpy) may dereference this pointer. The destructor at line 27 callsresize(0), which would trigger this path.Consider adding an early return when
size == 0:void resize(size_t size) { _size = size; fallback = nullptr; if (hasSetup) pffft_destroy_setup(fftSetup); if (work) pffft_aligned_free(work); work = nullptr; if (tmpAligned) pffft_aligned_free(tmpAligned); tmpAligned = nullptr; + + if (size == 0) { + hasSetup = false; + return; + } // We use this for split-real, even if there's no PFFFT setup tmpAligned = (float *)pffft_aligned_malloc(sizeof(float)*size*2);
143-168: Same zero-size allocation issue inPow2RealFFT::resize().Line 153 has the same potential issue with
pffft_aligned_malloc(sizeof(float)*size*2)whensize=0. Apply the same early-return guard as suggested forPow2FFT::resize().thirdparty/signalsmith-linear/tests/others/signalsmith-fft.h (1)
204-206: Variable shadowing:factorredefined inside loop.The variable
factorat line 205 shadows the outerfactorvariable from line 196. While this compiles, it can be confusing and may lead to maintenance issues.Suggested fix
for (size_t f = 0; f < factor; ++f) { complex sum = working[0]; for (size_t i = 1; i < factor; ++i) { V phase = 2*M_PI*f*i/factor; - complex factor = {cos(phase), -sin(phase)}; - sum += perf::complexMul<inverse>(working[i], factor); + complex twist = {cos(phase), -sin(phase)}; + sum += perf::complexMul<inverse>(working[i], twist); } data[f*stride] = sum; }thirdparty/signalsmith-linear/platform/linear-accelerate.h (1)
43-55: Global mutable state is not thread-safe.
_basicFillWarningCounterand the static counter insidebasicFillWarning()are accessed without synchronization. IfLinearImplis used from multiple threads, this could cause data races. Consider usingstd::atomicif thread safety is needed, or document that these warnings are not thread-safe.thirdparty/signalsmith-stretch/signalsmith-stretch.h (1)
344-350: Variable shadowing: localchannelBandsshadows member variable.The local variable
channelBandsshadows the class memberchannelBands. This works correctly becausebandsForChannel()is called, but it's confusing and could lead to bugs during maintenance.Suggested fix: rename local variable
for (int c = 0; c < channels; ++c) { - auto channelBands = bandsForChannel(c); + auto chBands = bandsForChannel(c); auto *spectrumBands = stft.spectrum(c); for (int b = 0; b < bands; ++b) { - channelBands[b].prevInput = spectrumBands[b]; + chBands[b].prevInput = spectrumBands[b]; } }Note: This shadowing also occurs at lines 367, 387, 459, 645, 677, 699, 741, 793, 823, 976, and 1030.
thirdparty/signalsmith-linear/platform/fft-accelerate.h (1)
300-313: Using float literal0.5ffor double-precision computation.Lines 301 and 310 use
0.5f(float literal) when performing double-precision operations. While this works due to implicit conversion, using0.5would be more appropriate.Proposed fix
void fft(const double* input, Complex* output) { - double mul = 0.5f; + double mul = 0.5; vDSP_vsmulD(input, 2, &mul, splitReal.data(), 1, _size/2); // ... } void fft(const double *inR, double *outR, double *outI) { DSPDoubleSplitComplex outputSplit{outR, outI}; - double mul = 0.5f; + double mul = 0.5; // ... }thirdparty/signalsmith-linear/fft.h (1)
318-321: Unusedtmpvector inPow2FFT.The
tmpmember vector is resized inresize()but never used in any of the FFT methods. This appears to be dead code or a remnant from refactoring.Proposed fix: remove unused member
- void resize(size_t size) { - simpleFFT.resize(size); - tmp.resize(size); - } + void resize(size_t size) { + simpleFFT.resize(size); + } // ... private: - std::vector<Complex> tmp; SimpleFFT<Sample> simpleFFT; };thirdparty/signalsmith-linear/platform/fft-pffft-double.h (2)
38-62: Potential issue:tmpAlignedallocated even whensizeis 0.When
resize(0)is called (e.g., from destructor), line 48 allocatessizeof(double)*0*2 = 0bytes. Whilepffftd_aligned_malloc(0)behavior may be implementation-defined, it's better to guard against this.Proposed fix
void resize(size_t size) { _size = size; fallback = nullptr; if (hasSetup) pffftd_destroy_setup(fftSetup); if (work) pffftd_aligned_free(work); work = nullptr; if (tmpAligned) pffftd_aligned_free(tmpAligned); tmpAligned = nullptr; + if (!size) { + hasSetup = false; + return; + } + // We use this for split-real, even if there's no PFFFT setup tmpAligned = (double *)pffftd_aligned_malloc(sizeof(double)*size*2); if (size < 16) {
143-168: Same zero-size allocation issue inPow2RealFFT<double>::resize().Line 153 allocates
tmpAlignedbefore checking ifsizeis valid. Should guard againstsize == 0.Proposed fix
void resize(size_t size) { _size = size; fallback = nullptr; if (hasSetup) pffftd_destroy_setup(fftSetup); if (work) pffftd_aligned_free(work); work = nullptr; if (tmpAligned) pffftd_aligned_free(tmpAligned); tmpAligned = nullptr; + if (!size) { + hasSetup = false; + return; + } + // We use this for split-real, even if there's no PFFFT setup tmpAligned = (double *)pffftd_aligned_malloc(sizeof(double)*size*2);thirdparty/signalsmith-linear/tests/test-runner.h (1)
247-271: Minor: Trailing semicolon after function definition.Line 271 has an extra semicolon after the closing brace of
runBenchmark. This is harmless but inconsistent with the rest of the code.Proposed fix
return rounds/seconds; -}; +}thirdparty/signalsmith-linear/tests/test-linear.h (2)
7-10: Consider adding bounds checking for debug builds.The
getAssignablehelper directly returns a reference to array element without bounds validation. While this is acceptable for production performance, consider addingassertfor debug builds.🔎 Optional enhancement
template<class ArrayLike> auto getAssignable(ArrayLike &arr, size_t i) -> decltype(arr[i]) & { + // In debug builds, you might want bounds checking if arr supports .size() return arr[i]; }
269-274: Destructor performs I/O that could potentially fail.The destructor writes SVG files, which could fail. While acceptable for test utilities, consider wrapping in try-catch to prevent exceptions from escaping the destructor.
thirdparty/signalsmith-linear/tests/test-ffts.h (2)
182-182: Duplicate include of./others/signalsmith-fft.h.The header
./others/signalsmith-fft.his included twice (lines 182 and 410), which is redundant. While include guards prevent issues, removing the duplicate improves clarity.🔎 Proposed fix
Remove the duplicate include at line 410:
-#include "./others/signalsmith-fft.h" template<class Sample> struct SignalsmithFFTRealWrapper {Also applies to: 410-410
1189-1196: Consider using bit manipulation for power-of-2 check.The floating-point calculation for
isPow2may have precision issues for larger sizes. A more reliable approach uses bit manipulation.🔎 More robust power-of-2 check
template<class Sample, bool modified> void testRealFft(int size) { - bool isPow2 = std::round(std::exp2(std::round(std::log2(size)))) == size; + bool isPow2 = (size > 0) && ((size & (size - 1)) == 0); bool comparePow2 = !modified && isPow2;thirdparty/signalsmith-linear/linear.h (2)
503-514: Minor inefficiency inTemporary::reserve- potential double allocation.The method may allocate twice if the first allocation doesn't meet alignment requirements. Consider pre-calculating the required size including alignment padding.
🔎 Alternative approach
void reserve(size_t size) { if (buffer) delete[] buffer; - buffer = new V[size]; - alignedBuffer = nextAligned(buffer); - if (alignedBuffer != buffer) { - delete[] buffer; - buffer = new V[size + extraAlignmentItems]; - alignedBuffer = nextAligned(buffer); - } + // Always allocate with extra space for alignment + buffer = new V[size + extraAlignmentItems]; + alignedBuffer = nextAligned(buffer); start = alignedBuffer; - end = alignedBuffer + size; + end = alignedBuffer + size; // usable size is still `size` }
1067-1067: Unnecessary semicolon after namespace closing braces.The semicolon after
}}is not required and is a minor style inconsistency.-}}; // namespace +}} // namespace
| SignalSmith::SignalSmith() { | ||
| channels = (channels < 1) ? 1 : channels; | ||
| sample_rate = (sample_rate < 1) ? 44100 : sample_rate; | ||
| tempo = 1.0f; | ||
|
|
||
| // Correct current Signalsmith API | ||
| stretch.presetDefault(channels, sample_rate); | ||
| } |
There was a problem hiding this comment.
Reading uninitialized member variables in constructor.
Lines 50-51 read channels and sample_rate before they are initialized. This is undefined behavior. The ternary expressions compare uninitialized values, which may contain garbage.
🔎 Proposed fix
SignalSmith::SignalSmith() {
- channels = (channels < 1) ? 1 : channels;
- sample_rate = (sample_rate < 1) ? 44100 : sample_rate;
+ channels = 1;
+ sample_rate = 44100;
tempo = 1.0f;
// Correct current Signalsmith API
stretch.presetDefault(channels, sample_rate);
}Alternatively, initialize these with default values in the header's member declaration.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| SignalSmith::SignalSmith() { | |
| channels = (channels < 1) ? 1 : channels; | |
| sample_rate = (sample_rate < 1) ? 44100 : sample_rate; | |
| tempo = 1.0f; | |
| // Correct current Signalsmith API | |
| stretch.presetDefault(channels, sample_rate); | |
| } | |
| SignalSmith::SignalSmith() { | |
| channels = 1; | |
| sample_rate = 44100; | |
| tempo = 1.0f; | |
| // Correct current Signalsmith API | |
| stretch.presetDefault(channels, sample_rate); | |
| } |
🤖 Prompt for AI Agents
In @modules/signalsmith/signalsmith_module.cpp around lines 49 - 56, The
constructor SignalSmith::SignalSmith reads uninitialized members (channels,
sample_rate); initialize them before use by setting default values either in the
member declarations or via the constructor initializer list (e.g., set channels
and sample_rate to sensible defaults) and then call
stretch.presetDefault(channels, sample_rate); ensure tempo is also initialized
in the initializer list or declarations to avoid UB.
There was a problem hiding this comment.
They're defined in the header.
Although, it is technically redundant. Hm
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check the header file for member variable declarations and their initializers
cat modules/signalsmith/signalsmith_module.h
Length of output: 3068
@mcdubhghlas, you're absolutely right! I can confirm that in the header file (signalsmith_module.h), the member variables are initialized with default values:
int sample_rate = 44100;
int channels = 2;
float tempo = 1.0f;These in-class member initializers are applied before the constructor body executes, so there's no undefined behavior. The original review comment was based on seeing only the .cpp file without the header context.
That said, the logic in the constructor body is now redundant since the members already have valid defaults:
channels = (channels < 1) ? 1 : channels;will keepchannelsas 2sample_rate = (sample_rate < 1) ? 44100 : sample_rate;will keepsample_rateas 44100tempo = 1.0f;reassigns the same value
This redundancy is harmless, though you could simplify the constructor to just:
SignalSmith::SignalSmith() {
stretch.presetDefault(channels, sample_rate);
}But the current code is perfectly safe and valid.
|
Alright, thanks. I'll go over this tomorrow unless I get free time tonight. |
|
That's it. Everything else is thirdparty stuff I don't think we should touch. |
I dont think we need to be messing with third party code unless it is actually broken or otherwise needs modification. |
Arctis-Fireblight
left a comment
Arctis-Fireblight
left a comment
There was a problem hiding this comment.
Overall the module changes look good. But reviewing the Signalsmith code is giving me a headache... Though I am not going to nit-pick third party dependencies.
But I just want to make sure that we dont introduce a dependency on CMake here, and would prefer those various Cmake files get removed.
Outside of that, I think this just needs some testing and it should be good for merge outside of that.
mcdubhghlas
force-pushed
the
SoundTouch
branch
from
9b087db to
45873d5
Compare
mcdubhghlas
force-pushed
the
SoundTouch
branch
from
45873d5 to
0120056
Compare
There was a problem hiding this comment.
Actionable comments posted: 14
🤖 Fix all issues with AI agents
In @modules/signalsmith/signalsmith_module.cpp:
- Around line 55-71: SignalSmith::set_sample_rate and SignalSmith::set_channels
call stretch.presetDefault(channels, sample_rate) but do not clear the internal
processing buffers; after each presetDefault call add a call to stretch.reset()
so the stretch instance is reinitialized (i.e., update both
SignalSmith::set_sample_rate and SignalSmith::set_channels to call
stretch.reset() immediately after stretch.presetDefault(...)).
In @thirdparty/signalsmith-linear/platform/fft-accelerate.h:
- Line 166: Line computes log2 = std::log2(size) without rounding which is
inconsistent with the earlier usage and can introduce floating-point errors;
change this to compute log2 using std::round(std::log2(size)) so the result is
an integer-aligned value (match the form used at line ~100), i.e., replace the
direct std::log2 call for the variable named log2 with
std::round(std::log2(size)) wherever this non-rounded computation appears in
fft-accelerate.h to ensure consistent behavior for non-power-of-2 sizes.
- Line 295: The log2 calculation currently assigns std::log2(size) directly to
log2; for the double-size variant this must be rounded. Replace the assignment
with a rounded value, e.g. set log2 = std::round(std::log2(size)) (or cast the
rounded result to the expected integer type), referring to the existing log2
variable and the size parameter in fft-accelerate.h so the computed log2 is
correctly rounded for the double variant.
In @thirdparty/signalsmith-linear/platform/fft-pffft.h:
- Around line 46-48: tmpAligned is being allocated via pffft_aligned_malloc
before validating size, which can call malloc(0) when size == 0; move the
allocation of tmpAligned (the pffft_aligned_malloc(sizeof(float)*size*2) call)
to after the existing check for size < 16 (or add an explicit size == 0 branch)
so you only allocate when size > 0 (or handle the zero-size case explicitly
without calling pffft_aligned_malloc). Ensure any early-return or error path
still frees tmpAligned only when it was actually allocated.
In @thirdparty/signalsmith-linear/stft.h:
- Around line 448-450: Replace the incorrect compatibility mapping that maps
analyseStep to itself: change the COMPAT_SPELLING invocation currently written
as COMPAT_SPELLING(analyseStep, analyseStep) so the second argument is the
American spelling COMPAT_SPELLING(analyseStep, analyzeStep); leave the
surrounding COMPAT_SPELLING lines (e.g., COMPAT_SPELLING(analyse, analyze) and
COMPAT_SPELLING(analyseSteps, analyzeSteps)) unchanged and consider reporting
the typo upstream to Signalsmith maintainers.
In @thirdparty/signalsmith-linear/tests/stop-denormals.h:
- Around line 18-23: The ARM NEON constructor for StopDenormals currently
overwrites the saved FPCR with the modified value; change it to save the
original FPCR into the member 'status' first, then compute a modified copy (set
0x01000000U) to write back with "msr fpcr"; ensure the destructor restores the
value from 'status' (the original FPCR) rather than the modified value — i.e.,
in StopDenormals(), do status = asmStatus; then asmStatus |= 0x01000000U; asm
volatile("msr fpcr, %0" : : "ri"(asmStatus)); and keep the destructor restoring
'status'.
- Around line 3-13: Add the missing SSE intrinsics header so _mm_getcsr() and
_mm_setcsr() are declared: include <xmmintrin.h> immediately after the existing
#pragma once in the file containing the StopDenormals class; this will ensure
the uses of _mm_getcsr and _mm_setcsr in the StopDenormals
constructor/destructor compile when __SSE__ or _M_X64 is defined.
In @thirdparty/signalsmith-linear/tests/stopwatch.h:
- Around line 9-23: The Windows branch in the Stopwatch class is incorrect:
replace the nonstandard macro check "WINDOWS" with the proper compiler macro(s)
(e.g. _WIN32 or _WIN64) so the branch is selected on Windows, and fix the type
access in Stopwatch::toSeconds by dividing by double(freq.QuadPart)
(LARGE_INTEGER is a union/struct so you must use QuadPart) to avoid the type
error; update references in the class methods (now() and toSeconds())
accordingly and run tests to confirm Windows support is actually required.
In @thirdparty/signalsmith-linear/tests/test-linear.h:
- Around line 1-10: The header "test-linear.h" is missing an include guard which
can lead to redefinition errors when included multiple times; add a header guard
(either a traditional #ifndef/#define/#endif pair) or simply insert #pragma once
at the very top of the file before any includes so that definitions like the
template function getAssignable and the included headers are protected from
multiple inclusion.
In @thirdparty/signalsmith-linear/tests/test-stfts.h:
- Around line 361-370: The test loop incorrectly compares the packed spectrum to
itself: change the second read so uV is taken from unpacked.spectrum(0)[b]
instead of packed.spectrum(0)[b]; specifically update the assignment for uV
(currently using packed.spectrum) to use unpacked.spectrum(0)[b] so the
comparison between pV and uV is meaningful, and consider reporting this fix
upstream to the Signalsmith maintainers since this is third-party test code.
In @thirdparty/signalsmith-stretch/web/emscripten/compile.sh:
- Line 1: Add a POSIX shebang to the top of the compile.sh script so it always
runs under a known shell interpreter; update the compile.sh file (which expects
arguments like main.cpp out.js ModuleName) to begin with a line specifying
/bin/sh or /usr/bin/env sh so the script executes portably across environments.
- Line 34: The mkdir invocation uses unquoted command substitution which can
cause word-splitting if OUTPUT_JS contains spaces; update the line that
currently reads mkdir -p $(dirname $OUTPUT_JS) to quote both the command
substitution and the variable inside it so that dirname receives the full
OUTPUT_JS value and mkdir sees a single argument (i.e., use quoted dirname and
quoted OUTPUT_JS) to prevent incorrect directory creation or failures.
- Around line 15-21: The script can continue in the wrong directory if
pushd/popd fail; update the block that clones and enters ${EMSDK_DIR} so
directory operations are checked: after pushd "${EMSDK_DIR}" use "&&" chaining
or explicit checks (e.g., pushd "${EMSDK_DIR}" || { echo 'pushd failed'; exit 1;
}) before running git pull/install/activate, and ensure popd is followed by a
similar check (popd || { echo 'popd failed'; exit 1; }) or use a trap to restore
the original directory on failure; reference the existing pushd/popd usage
around the EMSDK_DIR clone block and ensure the script exits on any directory
operation failure.
In @thirdparty/signalsmith-stretch/web/web-wrapper.js:
- Around line 238-241: The code calls configure() without the instance
reference, causing a ReferenceError because configure is a class method; change
the call to this.configure() where outputList[0].length is compared to
this.channels (the block that updates this.channels based on outputList) so the
instance method is invoked correctly and respects the class context.
🧹 Nitpick comments (19)
thirdparty/signalsmith-linear/README.md (1)
93-95: Minor: Add language identifier to code fence.The shell command block on line 93 is missing a language tag for syntax highlighting. Consider updating:
-``` +```bash g++ -framework Accelerate -DSIGNALSMITH_USE_ACCELERATEThis is a minor markdown hygiene improvement and optional for third-party code. </blockquote></details> <details> <summary>thirdparty/signalsmith-linear/tests/Makefile (1)</summary><blockquote> `1-40`: **Third-party test Makefile - consider upstream improvements.** This Makefile is part of the upstream Signalsmith Linear library. While it functions correctly, there are a few optional improvements that could be contributed upstream: 1. **Missing .PHONY declarations**: Targets like `main`, `dev`, `benchmark`, `cmake`, and `clean` should be declared as `.PHONY` since they don't represent actual files. This prevents conflicts if files with these names exist. 2. **Platform-specific configuration**: The build configuration on lines 16-19 is hardcoded for macOS (Accelerate framework). This limits portability to other platforms, though it may be intentional for this test suite. Given that this is third-party code and the PR context indicates such code should not be modified unless necessary, these are noted as optional improvements rather than required changes. The Makefile functions correctly for its intended purpose. <details> <summary>Optional: Add phony declarations</summary> ```diff +.PHONY: main dev benchmark benchmark-fast cmake cmake-benchmark cmake-benchmark-fast clean + main: out/tests @cd out && ./tests
thirdparty/signalsmith-stretch/cmd/Makefile (1)
1-45: Third-party command-line tool Makefile - consider upstream improvements.This Makefile is part of the upstream Signalsmith Stretch library. Similar to the Linear tests Makefile, there are optional improvements that could be contributed upstream:
Missing .PHONY declarations: Targets like
all,examples,dev,clean, andcmakeshould be declared as.PHONYto prevent conflicts with files of the same name.Platform-specific configuration: The build configuration on lines 5-7 is hardcoded for macOS with the Accelerate framework, limiting portability.
Since this is third-party code from upstream, these are noted as optional improvements. The Makefile functions correctly for building and testing the Signalsmith Stretch command-line tool.
Optional: Add phony declarations
+.PHONY: all examples dev clean cmake + all: out/stretch
thirdparty/signalsmith-stretch/web/Makefile (1)
1-23: Third-party web build Makefile - consider upstream improvements.This Makefile is part of the upstream Signalsmith Stretch library's web/Emscripten build system. The build pipeline correctly produces browser-compatible JavaScript modules.
Optional improvement: Development helper targets (
jsdoc,server,watch) should be declared as.PHONYsince they don't produce files matching their target names.Since this is third-party code from upstream, this is noted as an optional improvement. The Makefile functions correctly for building the web/WASM artifacts.
Optional: Add phony declarations for helper targets
+.PHONY: jsdoc server watch + release/SignalsmithStretch.mjs: release/SignalsmithStretch.js
modules/signalsmith/signalsmith_module.cpp (3)
40-47: Consider exposing getters for sample_rate, channels, and tempo.The API exposes setters but no getters for the configuration properties. Consider adding getter methods and property bindings for completeness.
116-121: Unreachable negative output_frames check.The check
if (output_frames < 0)on line 119 is unreachable because:
input_framesis guaranteed positive (checked on line 112)tfis guaranteed positive (either tempo > 0 or 1.0f)std::lroundof a positive value returns a non-negative resultThe check is harmless but unnecessary.
♻️ Consider simplifying
const float tf = (tempo > 0.0f) ? tempo : 1.0f; - int output_frames = (int)std::lround((double)input_frames / (double)tf); - - if (output_frames < 0) { - output_frames = 0; - } + int output_frames = static_cast<int>(std::lround(static_cast<double>(input_frames) / static_cast<double>(tf)));
123-155: Per-call allocations may impact real-time audio performance.Each call to
process()allocates new vectors for deinterleaving (in_ch,out_ch,in_ptrs,out_ptrs). For real-time audio processing, consider caching these buffers as class members and resizing only when needed.
thirdparty/signalsmith-stretch/web/web-wrapper.js (1)
247-252: Unused variablechannelBufferin loop.The
channelBuffervariable is declared but never used. The buffer is filled with zeros regardless of the input. If the intent was to zero-fill, the variable declaration is unnecessary; if input was meant to be processed, this may be a logic error.Suggested cleanup
outputList[0].forEach((_, c) => { - let channelBuffer = inputs[c%inputs.length]; let buffer = new Float32Array(memory, this.buffersIn[c], outputBlockSize); buffer.fill(0); });
thirdparty/signalsmith-linear/platform/fft-ipp.h (3)
377-388: Pointer-to-integer cast for alignment may not be portable.Casting
m_state->workBuffer(a pointer) tosize_tfor alignment calculations may have issues on platforms where pointer size differs fromsize_tsize. Consider usinguintptr_tfor guaranteed correctness.Suggested fix
+#include <cstdint> // ... - size_t work = size_t(m_state->workBuffer); + uintptr_t work = reinterpret_cast<uintptr_t>(m_state->workBuffer); size_t aligned = (work + (complexAlign - 1)) & ~(complexAlign - 1);
14-18: Anonymous namespace in header may cause ODR violations.The
checkStatusfunction andStatetemplate are defined in an anonymous namespace within a header file. If this header is included in multiple translation units, each TU gets its own copy, which could lead to subtle ODR issues or code bloat. Consider usinginlineor moving to a named namespace.
269-317: Commented-out code references vDSP (Apple Accelerate).This appears to be copy-paste from the Accelerate backend. Consider removing the dead code for clarity, or if intentionally left for future reference, add a note explaining why.
thirdparty/signalsmith-linear/platform/fft-pffft.h (2)
29-36: Missing move assignment operator.The class has a deleted copy constructor and a move constructor, but no move assignment operator. This makes the class non-assignable after construction, which may limit its usability in containers or reassignment scenarios.
83-96: Alignment check usessize_tcast - preferuintptr_t.Casting pointers to
size_tfor alignment checking (lines 85, 90) is not guaranteed to be correct on all platforms. Usereinterpret_cast<uintptr_t>for portable pointer-to-integer conversion.Suggested fix
- if (size_t(input)&0x0F) { + if (reinterpret_cast<uintptr_t>(input) & 0x0F) {
thirdparty/signalsmith-linear/platform/fft-accelerate.h (2)
300-310: Using float literal0.5fwith double precision data.Lines 301 and 310 use
0.5f(float literal) when operating ondoubledata. While this works due to implicit conversion, using0.5would be more consistent and avoid any potential precision loss.Suggested fix
- double mul = 0.5f; + double mul = 0.5;
211-270: Missing move constructor forPow2FFT<double>.The float specializations have move constructors, but
Pow2FFT<double>andPow2RealFFT<double>do not. This inconsistency may cause issues if these types are used in move-heavy contexts.
thirdparty/signalsmith-stretch/signalsmith-stretch.h (2)
471-472: Typo in comment: "to short" should be "too short".Minor documentation fix.
Suggested fix
- // to short for this - zero the output just to be polite + // too short for this - zero the output just to be polite
205-207: Integer truncation inoutputSeekLengthreturn value.The method returns
intbut computesinputLatency() + playbackRate*outputLatency()whereplaybackRateisSample(float/double). The implicit conversion from floating-point to int truncates; consider using explicit rounding or returning a floating-point type.Suggested fix
int outputSeekLength(Sample playbackRate) const { - return inputLatency() + playbackRate*outputLatency(); + return int(std::round(inputLatency() + playbackRate*outputLatency())); }
thirdparty/signalsmith-linear/tests/test-runner.h (1)
271-271: Extraneous semicolon after function definition.The semicolon after the closing brace of
runBenchmarkis unnecessary (though harmless).Suggested fix
return rounds/seconds; -}; +}
thirdparty/signalsmith-linear/tests/test-ffts.h (1)
182-183: Duplicate header includes.The files
./others/signalsmith-fft.hand./others/dsp/fft.hare included twice each (lines 182/410 and 205/434). While harmless due to include guards, these duplicates could be consolidated.Since this is third-party test code, this is purely informational.
Also applies to: 410-411
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
thirdparty/signalsmith-stretch/web/demo/loop.mp3is excluded by!**/*.mp3
📒 Files selected for processing (51)
doc/classes/SignalSmith.xmlmodules/signalsmith/SCsubmodules/signalsmith/config.pymodules/signalsmith/register_types.cppmodules/signalsmith/register_types.hmodules/signalsmith/signalsmith_module.cppmodules/signalsmith/signalsmith_module.hthirdparty/signalsmith-linear/.gitignorethirdparty/signalsmith-linear/LICENSE.txtthirdparty/signalsmith-linear/README.mdthirdparty/signalsmith-linear/SUPPORT.txtthirdparty/signalsmith-linear/fft.hthirdparty/signalsmith-linear/include/signalsmith-linear/fft.hthirdparty/signalsmith-linear/include/signalsmith-linear/linear.hthirdparty/signalsmith-linear/include/signalsmith-linear/stft.hthirdparty/signalsmith-linear/linear.hthirdparty/signalsmith-linear/platform/fft-accelerate.hthirdparty/signalsmith-linear/platform/fft-ipp.hthirdparty/signalsmith-linear/platform/fft-pffft-double.hthirdparty/signalsmith-linear/platform/fft-pffft.hthirdparty/signalsmith-linear/platform/linear-accelerate.hthirdparty/signalsmith-linear/stft.hthirdparty/signalsmith-linear/tests/CMakeLists.txtthirdparty/signalsmith-linear/tests/Makefilethirdparty/signalsmith-linear/tests/main.cppthirdparty/signalsmith-linear/tests/others/signalsmith-fft.hthirdparty/signalsmith-linear/tests/stop-denormals.hthirdparty/signalsmith-linear/tests/stopwatch.hthirdparty/signalsmith-linear/tests/test-ffts.hthirdparty/signalsmith-linear/tests/test-linear.hthirdparty/signalsmith-linear/tests/test-runner.hthirdparty/signalsmith-linear/tests/test-stfts.hthirdparty/signalsmith-stretch/.gitignorethirdparty/signalsmith-stretch/LICENSE.txtthirdparty/signalsmith-stretch/README.mdthirdparty/signalsmith-stretch/SUPPORT.txtthirdparty/signalsmith-stretch/cmd/CMakeLists.txtthirdparty/signalsmith-stretch/cmd/Makefilethirdparty/signalsmith-stretch/cmd/main-dev.cppthirdparty/signalsmith-stretch/cmd/main.cppthirdparty/signalsmith-stretch/include/signalsmith-stretch/signalsmith-stretch.hthirdparty/signalsmith-stretch/signalsmith-stretch.hthirdparty/signalsmith-stretch/web/Makefilethirdparty/signalsmith-stretch/web/demo/Scope.mjsthirdparty/signalsmith-stretch/web/demo/index.htmlthirdparty/signalsmith-stretch/web/emscripten/compile.shthirdparty/signalsmith-stretch/web/emscripten/main.cppthirdparty/signalsmith-stretch/web/emscripten/main.jsthirdparty/signalsmith-stretch/web/emscripten/pre.jsthirdparty/signalsmith-stretch/web/index.htmlthirdparty/signalsmith-stretch/web/web-wrapper.js
✅ Files skipped from review due to trivial changes (1)
- thirdparty/signalsmith-stretch/LICENSE.txt
🚧 Files skipped from review as they are similar to previous changes (15)
- thirdparty/signalsmith-stretch/cmd/CMakeLists.txt
- thirdparty/signalsmith-linear/SUPPORT.txt
- thirdparty/signalsmith-stretch/SUPPORT.txt
- thirdparty/signalsmith-linear/tests/main.cpp
- thirdparty/signalsmith-linear/include/signalsmith-linear/stft.h
- thirdparty/signalsmith-linear/LICENSE.txt
- thirdparty/signalsmith-stretch/cmd/main.cpp
- modules/signalsmith/register_types.h
- modules/signalsmith/register_types.cpp
- thirdparty/signalsmith-stretch/web/emscripten/pre.js
- thirdparty/signalsmith-stretch/web/demo/index.html
- thirdparty/signalsmith-stretch/include/signalsmith-stretch/signalsmith-stretch.h
- thirdparty/signalsmith-linear/include/signalsmith-linear/linear.h
- doc/classes/SignalSmith.xml
- thirdparty/signalsmith-linear/linear.h
🧰 Additional context used
🧬 Code graph analysis (12)
thirdparty/signalsmith-stretch/cmd/main-dev.cpp (2)
thirdparty/signalsmith-stretch/signalsmith-stretch.h (1)
stretch(14-98)thirdparty/signalsmith-stretch/cmd/main.cpp (1)
args(12-12)
thirdparty/signalsmith-stretch/web/demo/Scope.mjs (1)
thirdparty/signalsmith-stretch/web/emscripten/main.cpp (2)
process(71-73)process(71-71)
modules/signalsmith/signalsmith_module.h (1)
modules/signalsmith/signalsmith_module.cpp (16)
SignalSmith(49-51)SignalSmith(53-53)_bind_methods(40-47)_bind_methods(40-40)set_sample_rate(55-62)set_sample_rate(55-55)set_channels(64-71)set_channels(64-64)set_pitch(73-79)set_pitch(73-73)set_tempo(81-87)set_tempo(81-81)reset(89-91)reset(89-89)process(93-173)process(93-93)
thirdparty/signalsmith-linear/platform/fft-pffft.h (1)
thirdparty/signalsmith-linear/platform/fft-pffft-double.h (5)
signalsmith(15-220)linear(15-218)resize(38-117)resize(143-168)fftInner(98-110)
thirdparty/signalsmith-linear/tests/test-runner.h (1)
thirdparty/signalsmith-linear/linear.h (12)
complex(38-40)complex(814-816)signalsmith(20-406)linear(20-41)linear(581-581)real(65-68)- `` (416-418)
- `` (488-490)
- `` (492-494)
- `` (833-835)
- `` (836-838)
imag(72-75)
thirdparty/signalsmith-linear/platform/linear-accelerate.h (3)
thirdparty/signalsmith-linear/fft.h (2)
signalsmith(16-86)linear(16-73)thirdparty/signalsmith-linear/linear.h (14)
signalsmith(20-406)linear(20-41)linear(581-581)complex(38-40)complex(814-816)- `` (416-418)
- `` (488-490)
- `` (492-494)
- `` (833-835)
- `` (836-838)
size_t(768-768)size_t(1053-1053)size(1056-1058)size(1060-1062)thirdparty/signalsmith-linear/platform/fft-ipp.h (2)
double(426-530)double(468-530)
thirdparty/signalsmith-linear/tests/others/signalsmith-fft.h (1)
thirdparty/signalsmith-linear/tests/test-linear.h (1)
std(18-20)
modules/signalsmith/signalsmith_module.cpp (2)
modules/signalsmith/signalsmith_module.h (1)
SignalSmith(40-63)thirdparty/signalsmith-stretch/signalsmith-stretch.h (2)
stretch(14-98)c(185-464)
thirdparty/signalsmith-linear/fft.h (4)
thirdparty/signalsmith-linear/platform/fft-accelerate.h (8)
signalsmith(8-61)linear(8-38)_impl(10-31)resize(90-103)resize(156-169)resize(224-238)resize(285-298)ifft(323-340)thirdparty/signalsmith-linear/platform/fft-pffft-double.h (12)
signalsmith(15-220)linear(15-218)resize(38-117)resize(143-168)fft(64-67)fft(68-71)fft(170-173)fft(174-181)ifft(73-76)ifft(77-80)ifft(183-186)ifft(187-194)thirdparty/signalsmith-linear/platform/fft-pffft.h (11)
signalsmith(15-220)linear(15-218)resize(38-117)resize(143-168)fft(64-67)fft(68-71)fft(170-173)fft(174-181)ifft(73-76)ifft(77-80)ifft(183-186)thirdparty/signalsmith-linear/platform/fft-ipp.h (21)
_impl(263-318)resize(330-332)resize(372-389)resize(435-437)resize(477-494)fft(334-339)fft(341-344)fft(391-394)fft(396-403)fft(439-444)fft(446-449)fft(496-499)fft(501-508)ifft(346-351)ifft(353-356)ifft(405-408)ifft(410-416)ifft(451-456)ifft(458-461)ifft(510-513)ifft(515-521)
thirdparty/signalsmith-linear/tests/test-ffts.h (1)
thirdparty/signalsmith-linear/tests/stopwatch.h (2)
Stopwatch(11-60)Stopwatch(26-56)
thirdparty/signalsmith-linear/platform/fft-pffft-double.h (1)
thirdparty/signalsmith-linear/platform/fft-pffft.h (4)
signalsmith(15-220)linear(15-218)resize(38-117)resize(143-168)
thirdparty/signalsmith-linear/stft.h (3)
thirdparty/signalsmith-linear/fft.h (14)
signalsmith(16-86)linear(16-73)fft(120-127)fft(138-147)fft(304-306)fft(307-309)fft(424-634)fft(884-896)fft(897-908)fft(1003-1007)fft(1008-1113)fft(1114-1118)fft(1119-1173)if(233-235)thirdparty/signalsmith-linear/linear.h (12)
signalsmith(20-406)linear(20-41)linear(581-581)complex(38-40)complex(814-816)size_t(768-768)size_t(1053-1053)operator(54-56)operator(97-99)operator(791-793)size(1056-1058)size(1060-1062)thirdparty/signalsmith-linear/platform/fft-ipp.h (7)
fft(334-339)fft(341-344)fft(391-394)fft(396-403)fft(439-444)double(426-530)double(468-530)
🪛 Biome (2.1.2)
thirdparty/signalsmith-stretch/web/emscripten/main.js
[error] 9-9: A global variable should not be reassigned.
Assigning to a global variable can override essential functionality.
(lint/suspicious/noGlobalAssign)
thirdparty/signalsmith-stretch/web/demo/Scope.mjs
[error] 1-1: A global variable should not be reassigned.
Assigning to a global variable can override essential functionality.
(lint/suspicious/noGlobalAssign)
[error] 1-1: A global variable should not be reassigned.
Assigning to a global variable can override essential functionality.
(lint/suspicious/noGlobalAssign)
[error] 1-1: Shouldn't redeclare 'Q'. Consider to delete it or rename it.
'Q' is defined here:
(lint/suspicious/noRedeclare)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: Shouldn't redeclare 'A'. Consider to delete it or rename it.
'A' is defined here:
(lint/suspicious/noRedeclare)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: Shouldn't redeclare 'c'. Consider to delete it or rename it.
'c' is defined here:
(lint/suspicious/noRedeclare)
[error] 1-1: Shouldn't redeclare 'g'. Consider to delete it or rename it.
'g' is defined here:
(lint/suspicious/noRedeclare)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: Shouldn't redeclare 'g'. Consider to delete it or rename it.
'g' is defined here:
(lint/suspicious/noRedeclare)
[error] 1-1: Shouldn't redeclare 'A'. Consider to delete it or rename it.
'A' is defined here:
(lint/suspicious/noRedeclare)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
🪛 checkmake (0.2.2)
thirdparty/signalsmith-stretch/web/Makefile
[warning] 1-1: Missing required phony target "all"
(minphony)
[warning] 1-1: Missing required phony target "clean"
(minphony)
[warning] 1-1: Missing required phony target "test"
(minphony)
thirdparty/signalsmith-linear/tests/Makefile
[warning] 1-1: Missing required phony target "all"
(minphony)
[warning] 1-1: Missing required phony target "clean"
(minphony)
[warning] 1-1: Missing required phony target "test"
(minphony)
thirdparty/signalsmith-stretch/cmd/Makefile
[warning] 1-1: Missing required phony target "all"
(minphony)
[warning] 1-1: Missing required phony target "clean"
(minphony)
[warning] 1-1: Missing required phony target "test"
(minphony)
[warning] 1-1: Target "all" should be declared PHONY.
(phonydeclared)
[warning] 12-12: Target body for "examples" exceeds allowed length of 5 (14).
(maxbodylength)
🪛 Clang (14.0.6)
thirdparty/signalsmith-linear/tests/stop-denormals.h
[error] 4-4: unknown type name 'class'
(clang-diagnostic-error)
[error] 4-4: expected ';' after top level declarator
(clang-diagnostic-error)
🪛 LanguageTool
thirdparty/signalsmith-stretch/README.md
[style] ~94-~94: Consider removing “of” to be more concise
Context: ...rocessing time. ### Split computation All of the .preset???() and .configure() metho...
(ALL_OF_THE)
[style] ~144-~144: Consider an alternative verb to strengthen your wording.
Context: ...s though, so please get in touch if you have any problems. Enabling -ffast-math (...
(IF_YOU_HAVE_THIS_PROBLEM)
[typographical] ~166-~166: This sign-off should end with a comma when followed by your name.
Context: .../colinmarc/signalsmith-stretch-rs). ## Thanks We'd like to particularly thank the fo...
(EN_SIGN_OFF_WITHOUT_COMMA)
🪛 markdownlint-cli2 (0.18.1)
thirdparty/signalsmith-linear/README.md
93-93: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
thirdparty/signalsmith-stretch/README.md
72-72: Hard tabs
Column: 1
(MD010, no-hard-tabs)
🪛 Ruff (0.14.10)
modules/signalsmith/config.py
1-1: Unused function argument: env
(ARG001)
1-1: Unused function argument: platform
(ARG001)
🪛 Shellcheck (0.11.0)
thirdparty/signalsmith-stretch/web/emscripten/compile.sh
[error] 1-1: Tips depend on target shell and yours is unknown. Add a shebang or a 'shell' directive.
(SC2148)
[warning] 3-3: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 20-20: Use 'popd ... || exit' or 'popd ... || return' in case popd fails.
(SC2164)
[warning] 34-34: Quote this to prevent word splitting.
(SC2046)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (13)
- GitHub Check: 🐧 Linux / Template w/ Mono, debug (target=template_debug)
- GitHub Check: 🐧 Linux / Template w/ Mono, release (target=template_release)
- GitHub Check: 🤖 Android / Template arm32 (target=template_release, arch=arm32)
- GitHub Check: 🏁 Windows / Template w/ GCC (target=template_release, use_mingw=yes)
- GitHub Check: 🏁 Windows / Template (target=template_release)
- GitHub Check: 🐧 Linux / Editor w/ Mono (target=editor)
- GitHub Check: 🐧 Linux / Editor with ThreadSanitizer (target=editor, dev_build=yes, use_tsan=yes, use_llvm=yes, linker=lld)
- GitHub Check: 🌐 Web / Template w/o threads (target=template_release, threads=no)
- GitHub Check: 🍎 macOS / Editor (target=editor)
- GitHub Check: 🤖 Android / Template arm64 (target=template_release, arch=arm64)
- GitHub Check: 🏁 Windows / Editor (target=editor)
- GitHub Check: 🌐 Web / Template w/ threads (target=template_release, threads=yes)
- GitHub Check: 🍎 macOS / Template (target=template_release)
🔇 Additional comments (34)
thirdparty/signalsmith-linear/.gitignore (1)
1-3: LGTM!The
.gitignorefile appropriately excludes macOS system files (.DS_Store), test artifacts, and build outputs (out/), which are standard patterns for third-party library integration.thirdparty/signalsmith-stretch/web/demo/Scope.mjs (1)
1-1: Third-party generated code - treat as binary artifact.This is a minified Emscripten-compiled WebAssembly wrapper with embedded web UI. As third-party generated code, it should not be manually modified or reformatted. The static analysis warnings (variable redeclarations, use-before-declaration, global reassignments) are false positives typical of minified JavaScript and can be safely ignored.
If updates are needed in the future, they should be made to the source files and regenerated through the build process.
thirdparty/signalsmith-linear/tests/stop-denormals.h (2)
4-4: Static analysis errors are false positives.The Clang errors about "unknown type name 'class'" and "expected ';' after top level declarator" are false positives. The code is valid C++; the static analysis tool is likely running in C mode or with incorrect compiler flags.
29-34: Fallback implementation is reasonable.The no-op fallback for unsupported architectures maintains portability while appropriately warning developers in C++23+ builds. This is a sensible approach for third-party library code.
thirdparty/signalsmith-linear/tests/stopwatch.h (5)
24-37: LGTM: Chrono implementation is correct.The non-Windows implementation properly uses
std::chronowith appropriate clock selection. Thestd::conditionalensures a steady clock is used, preferringhigh_resolution_clockwhen available and steady, otherwise falling back tosteady_clock.
39-43: Verify intended thread-safety guarantees.The class uses
std::atomic<Time>forlapStart(with a comment about memory barriers), but other members (lapBest,lapTotal,lapTotal2,lapCount) are not atomic or protected by locks. Iflap()is called concurrently from multiple threads, these non-atomic updates (lines 74-77) will cause data races.Since this is a test utility, single-threaded usage is likely intended. However, the atomic usage suggests some thread-safety consideration. Please verify:
- Is concurrent access to
lap()expected?- If yes, the non-atomic members need synchronization
- If no, consider documenting the single-threaded usage requirement
45-60: LGTM: Constructor implementations are correct.The overhead compensation in the constructor is a clever approach to account for measurement overhead. The copy constructor properly handles the non-copyable
std::atomicmember by explicitly loading and storing the value.
62-81: LGTM: Timing logic is correct.The lap-timing implementation follows a standard pattern:
start()initializes,startLap()marks the beginning, andlap()measures elapsed time while automatically preparing for the next lap. The logic for tracking best, total, and total squared values is correct for statistical calculations.
82-100: LGTM: Statistical calculations are mathematically correct.All statistical methods are implemented correctly:
- Variance uses the standard formula E[X²] - E[X]²
- Proper use of
std::max(0.0, ...)prevents negative values from overhead compensation- The
optimistic()method provides a sensible lower-bound estimate using mean minus standard deviationsthirdparty/signalsmith-stretch/.gitignore (1)
1-2: Standard gitignore patterns.Both entries are appropriate:
.DS_Storefor macOS metadata andout/for build artifacts.thirdparty/signalsmith-stretch/README.md (1)
1-156: Comprehensive and well-structured documentation.The README provides clear explanations, practical code examples, and thorough coverage of configuration, processing, pitch/time manipulation, latency, and formant compensation. The organization flows logically from basic usage through advanced topics. Well done.
thirdparty/signalsmith-linear/README.md (1)
1-99: Clear and well-organized documentation.The README effectively explains FFT/STFT wrappers, expression templates, chunked computation, and build options. The structure and examples are easy to follow.
thirdparty/signalsmith-stretch/web/emscripten/main.cpp (1)
1-77: LGTM! Emscripten integration follows standard patterns.The use of global state and extern "C" wrappers is appropriate for WebAssembly bindings. The EMSCRIPTEN_KEEPALIVE attributes ensure the API functions are preserved during compilation.
thirdparty/signalsmith-linear/tests/others/signalsmith-fft.h (1)
1-512: Third-party FFT implementation - no critical issues identified.This is a third-party FFT framework in the test utilities directory. The implementation follows standard FFT patterns with proper namespace isolation, template-based design, and comprehensive operator support. No critical issues identified in this integration.
thirdparty/signalsmith-linear/platform/linear-accelerate.h (1)
1-575: LGTM! Apple Accelerate framework integration is properly implemented.The platform-specific implementation correctly uses vDSP and vForce functions for optimized DSP operations. The expression template system with macro-generated overloads follows appropriate patterns for this type of optimization layer.
thirdparty/signalsmith-linear/include/signalsmith-linear/fft.h (1)
1-1: LGTM! Standard header forwarding pattern.The relative include path properly forwards to the main FFT implementation.
modules/signalsmith/config.py (1)
1-6: LGTM! Standard module build configuration.This minimal build configuration correctly follows the Redot module system conventions. The
can_buildandconfigurefunctions are required hook signatures for the build system, so the static analysis warnings about unused arguments are false positives and can be safely ignored.modules/signalsmith/SCsub (1)
1-33: LGTM! Well-structured module build script.The SCons build configuration correctly:
- Clones the environment to avoid polluting the global build settings
- Adds the thirdparty include path for the Signalsmith libraries
- Detects compiler variants properly (with clang exclusion to avoid false positives)
- Suppresses warnings appropriately for third-party code
The warning suppressions are well-justified for integrating external libraries that may not follow the project's strict warning policies.
thirdparty/signalsmith-stretch/web/emscripten/main.js (1)
1-19: Third-party Emscripten-generated code - no changes recommended.This is minified Emscripten-generated WebAssembly bootstrap code. The static analysis warning about global variable reassignment on line 9 (
globalThis = global) is a standard Node.js polyfill pattern and is intentional behavior for cross-environment compatibility.As noted in PR comments, third-party code should not be modified unless broken.
thirdparty/signalsmith-stretch/web/index.html (1)
1-118: Third-party demo file - no changes recommended.This is a third-party demo HTML file for testing the Signalsmith Stretch Web Audio integration. The
new Function()usage on line 112 is intentional for the demo's interactive code execution feature. As noted in PR comments, third-party code should not be modified unless broken.thirdparty/signalsmith-stretch/cmd/main-dev.cpp (1)
1-234: Third-party development/debugging tool - no changes recommended.This is a third-party command-line development tool for profiling and benchmarking the Signalsmith Stretch library. It includes conditional profiling hooks, memory tracking, and reference comparison functionality. As noted in PR comments, third-party code should not be modified unless broken.
modules/signalsmith/signalsmith_module.cpp (1)
157-158: LGTM - stretch.process call is correctly structured.The call correctly passes per-channel buffer pointers with frame counts.
thirdparty/signalsmith-linear/tests/CMakeLists.txt (1)
1-36: Third-party test CMake configuration - no changes recommended.This is a third-party CMake configuration for the Signalsmith Linear test suite. It follows standard CMake patterns with FetchContent for external dependencies. As noted in PR comments, third-party code should not be modified unless broken.
modules/signalsmith/signalsmith_module.h (1)
40-63: LGTM - Clean header design following Godot conventions.The class correctly wraps SignalsmithStretch with appropriate Godot bindings. The design decision to not cache the pitch value (calling
stretch.setTransposeFactordirectly inset_pitch) is valid since the underlying stretch object maintains this state.Consider whether you need to expose getter methods for
sample_rate,channels, andtempofor script-side introspection.thirdparty/signalsmith-stretch/web/web-wrapper.js (1)
1-436: Third-party AudioWorklet wrapper looks structurally sound.The overall implementation follows proper AudioWorklet patterns with appropriate message passing, Wasm module lifecycle management, and CommonJS/AMD export paths. The processing loop correctly handles both live input and time-sequenced audio modes.
thirdparty/signalsmith-linear/platform/fft-ipp.h (1)
1-532: IPP FFT wrapper implementation is functionally complete.The template specializations for
Pow2FFTandPow2RealFFTproperly manage IPP resources with RAII patterns, handle both complex and split-complex paths, and include appropriate error checking viacheckStatus.thirdparty/signalsmith-linear/platform/fft-pffft.h (1)
1-221: PFFFT wrapper implementation is well-structured.The implementation correctly handles the PFFFT library requirements including alignment, fallback to
SimpleFFTfor small sizes, and proper resource management. The move semantics prevent double-free issues.thirdparty/signalsmith-linear/platform/fft-accelerate.h (1)
1-340: Accelerate FFT wrapper implementation is comprehensive.The implementation properly wraps vDSP functions for both float and double, handles split-complex formats correctly, and manages FFT setup resources with RAII.
thirdparty/signalsmith-linear/tests/test-linear.h (1)
1-725: Comprehensive test harness for linear operations.The test infrastructure provides thorough coverage of real, complex, and split-complex operations with reference implementations for validation. The benchmarking and plotting capabilities are well-integrated.
thirdparty/signalsmith-stretch/signalsmith-stretch.h (1)
1-1060: Sophisticated time-stretching implementation with proper phase vocoder techniques.The SignalsmithStretch class implements a comprehensive phase vocoder with:
- Proper latency management and split computation support
- Formant preservation with optional compensation
- Peak detection for frequency mapping
- Silence detection for efficiency
- Clean RAII resource management
The algorithm is well-structured and the public API is clear.
thirdparty/signalsmith-linear/tests/test-runner.h (1)
1-362: Well-designed test runner infrastructure.The
RunDataclass provides proper alignment handling, seeded randomization for reproducibility, and comprehensive distance computation. TheRunPlotclass cleanly integrates benchmarking with visualization. RAII patterns are correctly applied throughout.thirdparty/signalsmith-linear/fft.h (1)
1-1380: Third-party FFT library integration looks good.The Signalsmith Linear FFT library is well-structured with:
- Proper include guards and namespace organization
- Defensive compile-time check for known Apple Clang 16.0.0 bugs (lines 8-10)
- Move-only semantics preventing accidental copies of FFT state
- Platform-specific backend overrides at end of file allowing optimized implementations
No issues identified with this third-party code.
thirdparty/signalsmith-linear/platform/fft-pffft-double.h (1)
1-221: PFFFT double-precision backend implementation is correct.The specializations properly handle:
- Resource lifecycle with cleanup in
resize()before new allocations- Move semantics that null out source pointers to prevent double-free (lines 33-35, 138-140)
- 32-byte alignment requirements for double-precision PFFFT transforms
- Fallback to SimpleFFT for sizes < 16 (complex) and < 32 (real)
The TODO comment on line 155 is a minor optimization note, not a defect.
thirdparty/signalsmith-linear/tests/test-ffts.h (1)
1-1316: Comprehensive FFT test harness.The test file provides thorough coverage of:
- All FFT wrapper variants (Simple, Pow2, Split, Modified)
- Both interleaved complex and split-complex data paths
- Float and double precision
- Correctness verification against reference DFT implementation
- Benchmark infrastructure for performance analysis
The Apple Clang 16.0.0 workaround (lines 539-542, 702-705, 1290-1293) is consistent with the main library's defensive check.
| void SignalSmith::set_sample_rate(int p_rate) { | ||
| if (p_rate < 1) { | ||
| return; | ||
| } | ||
|
|
||
| sample_rate = p_rate; | ||
| stretch.presetDefault(channels, sample_rate); | ||
| } | ||
|
|
||
| void SignalSmith::set_channels(int p_channels) { | ||
| if (p_channels < 1) { | ||
| return; | ||
| } | ||
|
|
||
| channels = p_channels; | ||
| stretch.presetDefault(channels, sample_rate); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
signalsmith-stretch presetDefault internal state reset
💡 Result:
presetDefault only configures block/interval and sample-rate; it does not clear the internal processing buffers. Call reset() to clear internal state/buffers after (or before) changing presets. [1][2]
Sources:
[1] signalsmith-stretch README (presetDefault / configure / reset).
[2] Signalsmith Audio project page (preset methods and "To clear the internal buffers: stretch.reset()").
Add stretch.reset() after calling presetDefault() in both methods.
presetDefault() only configures block/interval and sample-rate—it does not clear internal processing buffers. Call reset() after changing the preset to clear the internal state and prevent stale data from affecting subsequent audio processing.
void SignalSmith::set_sample_rate(int p_rate) {
if (p_rate < 1) {
return;
}
sample_rate = p_rate;
stretch.presetDefault(channels, sample_rate);
stretch.reset();
}
void SignalSmith::set_channels(int p_channels) {
if (p_channels < 1) {
return;
}
channels = p_channels;
stretch.presetDefault(channels, sample_rate);
stretch.reset();
}
🤖 Prompt for AI Agents
In @modules/signalsmith/signalsmith_module.cpp around lines 55 - 71,
SignalSmith::set_sample_rate and SignalSmith::set_channels call
stretch.presetDefault(channels, sample_rate) but do not clear the internal
processing buffers; after each presetDefault call add a call to stretch.reset()
so the stretch instance is reinitialized (i.e., update both
SignalSmith::set_sample_rate and SignalSmith::set_channels to call
stretch.reset() immediately after stretch.presetDefault(...)).
|
|
||
| splitReal.resize(size); | ||
| splitImag.resize(size); | ||
| log2 = std::log2(size); |
There was a problem hiding this comment.
Missing std::round() on log2 calculation.
Line 166 uses log2 = std::log2(size) without rounding, unlike line 100 which uses std::round(std::log2(size)). This inconsistency could cause issues with non-power-of-2 sizes due to floating-point precision.
Suggested fix
- log2 = std::log2(size);
+ log2 = std::round(std::log2(size));📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| log2 = std::log2(size); | |
| log2 = std::round(std::log2(size)); |
🤖 Prompt for AI Agents
In @thirdparty/signalsmith-linear/platform/fft-accelerate.h at line 166, Line
computes log2 = std::log2(size) without rounding which is inconsistent with the
earlier usage and can introduce floating-point errors; change this to compute
log2 using std::round(std::log2(size)) so the result is an integer-aligned value
(match the form used at line ~100), i.e., replace the direct std::log2 call for
the variable named log2 with std::round(std::log2(size)) wherever this
non-rounded computation appears in fft-accelerate.h to ensure consistent
behavior for non-power-of-2 sizes.
|
|
||
| splitReal.resize(size); | ||
| splitImag.resize(size); | ||
| log2 = std::log2(size); |
There was a problem hiding this comment.
Same issue: Missing std::round() on log2 calculation for double variant.
Suggested fix
- log2 = std::log2(size);
+ log2 = std::round(std::log2(size));📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| log2 = std::log2(size); | |
| log2 = std::round(std::log2(size)); |
🤖 Prompt for AI Agents
In @thirdparty/signalsmith-linear/platform/fft-accelerate.h at line 295, The
log2 calculation currently assigns std::log2(size) directly to log2; for the
double-size variant this must be rounded. Replace the assignment with a rounded
value, e.g. set log2 = std::round(std::log2(size)) (or cast the rounded result
to the expected integer type), referring to the existing log2 variable and the
size parameter in fft-accelerate.h so the computed log2 is correctly rounded for
the double variant.
|
|
||
| // We use this for split-real, even if there's no PFFFT setup | ||
| tmpAligned = (float *)pffft_aligned_malloc(sizeof(float)*size*2); |
There was a problem hiding this comment.
Memory allocation occurs before size validation.
tmpAligned is allocated unconditionally at line 48 before checking if size < 16. When size == 0, this allocates sizeof(float)*0*2 = 0 bytes. While pffft_aligned_malloc(0) may return a valid pointer on some platforms, behavior is implementation-defined. Consider moving the allocation after the size check or handling size 0 explicitly.
🤖 Prompt for AI Agents
In @thirdparty/signalsmith-linear/platform/fft-pffft.h around lines 46 - 48,
tmpAligned is being allocated via pffft_aligned_malloc before validating size,
which can call malloc(0) when size == 0; move the allocation of tmpAligned (the
pffft_aligned_malloc(sizeof(float)*size*2) call) to after the existing check for
size < 16 (or add an explicit size == 0 branch) so you only allocate when size >
0 (or handle the zero-size case explicitly without calling
pffft_aligned_malloc). Ensure any early-return or error path still frees
tmpAligned only when it was actually allocated.
| COMPAT_SPELLING(analyse, analyze); | ||
| COMPAT_SPELLING(analyseStep, analyseStep); | ||
| COMPAT_SPELLING(analyseSteps, analyzeSteps); |
There was a problem hiding this comment.
Typo in compatibility spelling macro.
Line 449 maps analyseStep to itself instead of analyzeStep:
COMPAT_SPELLING(analyseStep, analyseStep); // should be: analyzeStepThis creates a redundant wrapper instead of the intended American English alias. Since this is third-party code, you may want to report this upstream to the Signalsmith library maintainers rather than modifying the vendored copy.
🤖 Prompt for AI Agents
In @thirdparty/signalsmith-linear/stft.h around lines 448 - 450, Replace the
incorrect compatibility mapping that maps analyseStep to itself: change the
COMPAT_SPELLING invocation currently written as COMPAT_SPELLING(analyseStep,
analyseStep) so the second argument is the American spelling
COMPAT_SPELLING(analyseStep, analyzeStep); leave the surrounding COMPAT_SPELLING
lines (e.g., COMPAT_SPELLING(analyse, analyze) and COMPAT_SPELLING(analyseSteps,
analyzeSteps)) unchanged and consider reporting the typo upstream to Signalsmith
maintainers.
| for (size_t b = 1; b < packed.bands(); ++b) { | ||
| auto pV = packed.spectrum(0)[b]; | ||
| auto uV = packed.spectrum(0)[b]; | ||
| if (pV != uV) { | ||
| LOG_EXPR(b); | ||
| LOG_EXPR(pV); | ||
| LOG_EXPR(uV); | ||
| abort(); | ||
| } | ||
| } |
There was a problem hiding this comment.
Bug in third-party test code: comparing packed spectrum with itself.
Line 363 reads from packed.spectrum(0)[b] instead of unpacked.spectrum(0)[b], making this comparison always pass regardless of actual values:
auto pV = packed.spectrum(0)[b];
auto uV = packed.spectrum(0)[b]; // Bug: should be unpacked.spectrum(0)[b]This is third-party test code, so modifications should be approached carefully. Consider reporting this upstream to the Signalsmith library maintainers.
🤖 Prompt for AI Agents
In @thirdparty/signalsmith-linear/tests/test-stfts.h around lines 361 - 370, The
test loop incorrectly compares the packed spectrum to itself: change the second
read so uV is taken from unpacked.spectrum(0)[b] instead of
packed.spectrum(0)[b]; specifically update the assignment for uV (currently
using packed.spectrum) to use unpacked.spectrum(0)[b] so the comparison between
pV and uV is meaningful, and consider reporting this fix upstream to the
Signalsmith maintainers since this is third-party test code.
| @@ -0,0 +1,59 @@ | |||
| # compile.sh main.cpp out.js ModuleName | |||
There was a problem hiding this comment.
Add shebang line.
The script is missing a shebang, which can cause portability issues and unexpected behavior when the script is executed in different environments.
🔧 Proposed fix
+#!/bin/bash
# compile.sh main.cpp out.js ModuleName📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # compile.sh main.cpp out.js ModuleName | |
| #!/bin/bash | |
| # compile.sh main.cpp out.js ModuleName |
🧰 Tools
🪛 Shellcheck (0.11.0)
[error] 1-1: Tips depend on target shell and yours is unknown. Add a shebang or a 'shell' directive.
(SC2148)
🤖 Prompt for AI Agents
In @thirdparty/signalsmith-stretch/web/emscripten/compile.sh at line 1, Add a
POSIX shebang to the top of the compile.sh script so it always runs under a
known shell interpreter; update the compile.sh file (which expects arguments
like main.cpp out.js ModuleName) to begin with a line specifying /bin/sh or
/usr/bin/env sh so the script executes portably across environments.
| if ! test -d "${EMSDK_DIR}" | ||
| then | ||
| echo "SDK not found - cloning from Github" | ||
| git clone https://github.com/emscripten-core/emsdk.git "${EMSDK_DIR}" | ||
| pushd "${EMSDK_DIR}" && git pull && ./emsdk install latest && ./emsdk activate latest | ||
| popd | ||
| fi |
There was a problem hiding this comment.
Add error handling for directory operations.
If popd fails on Line 20, the script will continue executing in the wrong directory, potentially causing the subsequent commands to fail or produce incorrect results.
🔧 Proposed fix
if ! test -d "${EMSDK_DIR}"
then
echo "SDK not found - cloning from Github"
git clone https://github.com/emscripten-core/emsdk.git "${EMSDK_DIR}"
pushd "${EMSDK_DIR}" && git pull && ./emsdk install latest && ./emsdk activate latest
- popd
+ popd || exit 1
fi🧰 Tools
🪛 Shellcheck (0.11.0)
[warning] 20-20: Use 'popd ... || exit' or 'popd ... || return' in case popd fails.
(SC2164)
🤖 Prompt for AI Agents
In @thirdparty/signalsmith-stretch/web/emscripten/compile.sh around lines 15 -
21, The script can continue in the wrong directory if pushd/popd fail; update
the block that clones and enters ${EMSDK_DIR} so directory operations are
checked: after pushd "${EMSDK_DIR}" use "&&" chaining or explicit checks (e.g.,
pushd "${EMSDK_DIR}" || { echo 'pushd failed'; exit 1; }) before running git
pull/install/activate, and ensure popd is followed by a similar check (popd || {
echo 'popd failed'; exit 1; }) or use a trap to restore the original directory
on failure; reference the existing pushd/popd usage around the EMSDK_DIR clone
block and ensure the script exits on any directory operation failure.
|
|
||
| INPUT_CPP="$1" | ||
| OUTPUT_JS="$2" | ||
| mkdir -p $(dirname $OUTPUT_JS) |
There was a problem hiding this comment.
Quote command substitution to prevent word splitting.
If OUTPUT_JS contains spaces or special characters, the unquoted command substitution will cause word splitting and potentially create incorrect directories or fail.
🔧 Proposed fix
-mkdir -p $(dirname $OUTPUT_JS)
+mkdir -p "$(dirname "$OUTPUT_JS")"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| mkdir -p $(dirname $OUTPUT_JS) | |
| mkdir -p "$(dirname "$OUTPUT_JS")" |
🧰 Tools
🪛 Shellcheck (0.11.0)
[warning] 34-34: Quote this to prevent word splitting.
(SC2046)
🤖 Prompt for AI Agents
In @thirdparty/signalsmith-stretch/web/emscripten/compile.sh at line 34, The
mkdir invocation uses unquoted command substitution which can cause
word-splitting if OUTPUT_JS contains spaces; update the line that currently
reads mkdir -p $(dirname $OUTPUT_JS) to quote both the command substitution and
the variable inside it so that dirname receives the full OUTPUT_JS value and
mkdir sees a single argument (i.e., use quoted dirname and quoted OUTPUT_JS) to
prevent incorrect directory creation or failures.
| if (outputList[0].length != this.channels) { | ||
| this.channels = outputList[0]?.length || 0; | ||
| configure(); | ||
| } |
There was a problem hiding this comment.
Bug: Missing this. prefix on configure() call.
At line 240, configure() is called without this., which will throw a ReferenceError at runtime since configure is a method of the class, not a global function.
Proposed fix
if (outputList[0].length != this.channels) {
this.channels = outputList[0]?.length || 0;
- configure();
+ this.configure();
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (outputList[0].length != this.channels) { | |
| this.channels = outputList[0]?.length || 0; | |
| configure(); | |
| } | |
| if (outputList[0].length != this.channels) { | |
| this.channels = outputList[0]?.length || 0; | |
| this.configure(); | |
| } |
🤖 Prompt for AI Agents
In @thirdparty/signalsmith-stretch/web/web-wrapper.js around lines 238 - 241,
The code calls configure() without the instance reference, causing a
ReferenceError because configure is a class method; change the call to
this.configure() where outputList[0].length is compared to this.channels (the
block that updates this.channels based on outputList) so the instance method is
invoked correctly and respects the class context.
2 participants
Ok this one is MIT. You can edit audio streams temp and pitch, separately. If you increase tempo, it won't increase pitch unless you set it to. That's it.
I'll include the project file I used to test it, but here's a video (warning - Sound, obviously):
out.mp4
and here is the project file:
redot-signalsmith_proj.zip
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.