Closed
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughRefactors C++ DSP wrapper for guarded, frame-based processing, removes direct Clarity ownership in the wrapper, adds a native VIPER "original" EQ JNI path, tightens preference parsing and compatibility fallbacks, adds harmonics validation, field‑surround mapping/utilities, tests, and extensive DSP documentation. Changes
Sequence Diagram(s)mermaid Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~50 minutes Poem
🚥 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
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/src/main/res/xml/dsp_spectrum_ext_preferences.xml (1)
20-135:⚠️ Potential issue | 🟡 MinorConfirm the safety default change is intentional.
key_spectrum_ext_safetynow defaults tofalse, which changes behavior. If parity requires this, consider a short hint in the UI; otherwise keep the previous default to avoid regressions.🔧 Optional revert if not intended
- android:defaultValue="false" + android:defaultValue="true"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/res/xml/dsp_spectrum_ext_preferences.xml` around lines 20 - 135, The SwitchPreferenceCompat for the safety toggle (android:key="@string/key_spectrum_ext_safety") now has android:defaultValue="false" which changes runtime behavior; either restore the previous defaultValue to the original setting (e.g., "true") or explicitly confirm and document the new default in the UI (add a summary/hint) so users are aware. Update the SwitchPreferenceCompat element (the preference with key key_spectrum_ext_safety) to the intended default or add a short summary string resource to clarify the intentional behavior change.
🧹 Nitpick comments (4)
app/src/main/cpp/libjdspimptoolbox/main/JdspImpResToolbox.c (1)
851-854: Use JNI_ABORT for read-only arrays to avoid unnecessary copy-back.Arrays
freqs,gains, anddispFreqare read-only inputs. Using mode0causes the JVM to copy data back, which is wasteful. UseJNI_ABORTto discard changes.♻️ Proposed fix
- (*env)->ReleaseDoubleArrayElements(env, jfreq, freqs, 0); - (*env)->ReleaseDoubleArrayElements(env, jgain, gains, 0); - (*env)->ReleaseDoubleArrayElements(env, jdispFreq, dispFreq, 0); + (*env)->ReleaseDoubleArrayElements(env, jfreq, freqs, JNI_ABORT); + (*env)->ReleaseDoubleArrayElements(env, jgain, gains, JNI_ABORT); + (*env)->ReleaseDoubleArrayElements(env, jdispFreq, dispFreq, JNI_ABORT); (*env)->ReleaseFloatArrayElements(env, jresponse, response, 0);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/cpp/libjdspimptoolbox/main/JdspImpResToolbox.c` around lines 851 - 854, The three read-only JNI arrays are being released with mode 0 which forces copy-back; change the calls releasing jfreq/jgain/jdispFreq to use JNI_ABORT instead of 0 so ReleaseDoubleArrayElements(env, jfreq, freqs, JNI_ABORT), ReleaseDoubleArrayElements(env, jgain, gains, JNI_ABORT) and ReleaseDoubleArrayElements(env, jdispFreq, dispFreq, JNI_ABORT) to avoid unnecessary copying back of freqs, gains and dispFreq while leaving ReleaseFloatArrayElements(env, jresponse, response, 0) unchanged if response is an output buffer.app/src/main/cpp/libjamesdsp-wrapper/clarity/ClarityProcessor.h (1)
159-162: Storing Mode as int reduces type safety.Changing from
Mode modetoint modeloses compile-time type checking. Consider keeping the enum class type or adding validation in the setter to ensure only valid mode values are accepted.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/cpp/libjamesdsp-wrapper/clarity/ClarityProcessor.h` around lines 159 - 162, The field declaration changed from the enum class type to an int (Mode -> int mode) which weakens type safety; restore the enum class type by declaring the member as Mode mode (or if you must keep int for ABI reasons, add validation in the setter such as setMode or set_mode to check the incoming integer is within the Mode enum range and convert to Mode before storing) and update any uses that assume an int accordingly; reference the Mode enum and the mode member in ClarityProcessor.h and adjust constructors/setters that initialize mode to use static_cast<Mode>(...) only after validating the value.app/src/main/java/me/timschneeberger/rootlessjamesdsp/interop/JamesDspLocalEngine.kt (1)
175-210: Consider de-duplicating EQ normalization logic across engines.
normalizeMultiEqBandsand the scale constants mirrorJamesDspRemoteEngine.normalizeMultiEqBands; extracting a shared helper avoids drift.Also applies to: 330-341
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/me/timschneeberger/rootlessjamesdsp/interop/JamesDspLocalEngine.kt` around lines 175 - 210, The EQ normalization logic and constants (normalizeMultiEqBands, VIPER_SCALE, VIPER_EXT_SCALE, STANDARD_SCALE, EQ_FIELDS) are duplicated in JamesDspLocalEngine and JamesDspRemoteEngine; extract them into a single shared helper (e.g., an object/class like EqNormalization or a util file) that exposes a normalizeMultiEqBands(filterType: Int, bands: DoubleArray): DoubleArray and the scale constants, move the constants out of the engine classes into that helper, update both JamesDspLocalEngine.normalizeMultiEqBands and JamesDspRemoteEngine.normalizeMultiEqBands to delegate to the shared helper, and keep behavior identical (same branching for EQ_FILTER_TYPE_VIPER_ORIGINAL and default) to avoid drift.app/src/main/java/me/timschneeberger/rootlessjamesdsp/interop/JamesDspBaseEngine.kt (1)
97-99: Clamp EQ interpolation mode to the supported range.
Preferences can be edited externally; clamping avoids passing unexpected values downstream.♻️ Suggested change
- val eqInterpolationMode = cache.get(R.string.key_eq_interpolation, "0").toIntOrNull() ?: 0 + val eqInterpolationMode = cache.get(R.string.key_eq_interpolation, "0") + .toIntOrNull() + ?.coerceIn(0, 1) ?: 0🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/me/timschneeberger/rootlessjamesdsp/interop/JamesDspBaseEngine.kt` around lines 97 - 99, Parse eqInterpolationMode as you're doing now, then clamp it into the supported range before using it: replace the current eqInterpolationMode assignment with logic that converts the cached string to Int (as done) and then bounds it with min/max (e.g., eqInterpolationMode = eqInterpolationMode.coerceIn(0, MAX_EQ_INTERPOLATION_MODE)). Reference the eqInterpolationMode variable and introduce or reuse a constant named MAX_EQ_INTERPOLATION_MODE (or the appropriate existing constant) to define the upper bound so out-of-range preference edits cannot propagate downstream.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@app/src/main/cpp/libjamesdsp-wrapper/fieldsurround/FieldSurroundProcessor.cpp`:
- Around line 319-321: FieldSurroundProcessor::setDepthFromParamInt currently
casts the incoming int to int16_t which can truncate out-of-range values; clamp
the incoming value to the int16_t range (e.g. using std::clamp(value,
std::numeric_limits<int16_t>::min(), std::numeric_limits<int16_t>::max()))
before the static_cast and then call
depthSurround.setStrength(static_cast<int16_t>(clampedValue)) so truncation is
prevented and behavior is well-defined.
- Around line 117-119: The b-coefficient expressions (b0, b1, b2) in
FieldSurroundProcessor.cpp are incorrectly multiplied by omega; remove the
trailing "* omega" from each so they match the RBJ high-shelf form used for the
a-coefficients—set b0 to A * ((A + 1.0) + (A - 1.0) * cosOmega + 2.0 * sqrtA *
z), b1 to -2.0 * A * ((A - 1.0) + (A + 1.0) * cosOmega), and b2 to A * ((A +
1.0) + (A - 1.0) * cosOmega - 2.0 * sqrtA * z) to restore the standard
high-shelf formula for the FieldSurroundProcessor.
In `@app/src/main/cpp/libjdspimptoolbox/main/JdspImpResToolbox.c`:
- Around line 776-779: The calls to GetDoubleArrayElements/GetFloatArrayElements
(for jfreq, jgain, jdispFreq, jresponse) may return NULL on OOM; add null checks
after each call (freqs, gains, dispFreq, response) and handle failure by
releasing any previously acquired arrays and returning/throwing an appropriate
JNI exception or error code; use
ReleaseDoubleArrayElements/ReleaseFloatArrayElements for any non-NULL pointers
before exiting to avoid leaks and ensure you operate only when all four pointers
are non-NULL.
In
`@app/src/main/java/me/timschneeberger/rootlessjamesdsp/fragment/PreferenceGroupFragment.kt`:
- Line 36: The String.format("%.1f dB", it / 10.0f) call in
PreferenceGroupFragment should use an explicit locale to ensure the decimal
separator matches the device locale; update the call to use
String.format(Locale.getDefault(), "%.1f dB", it / 10.0f) and add an import for
java.util.Locale (keep the existing isRootless import untouched) so the
crossfeed label formats consistently across locales.
---
Outside diff comments:
In `@app/src/main/res/xml/dsp_spectrum_ext_preferences.xml`:
- Around line 20-135: The SwitchPreferenceCompat for the safety toggle
(android:key="@string/key_spectrum_ext_safety") now has
android:defaultValue="false" which changes runtime behavior; either restore the
previous defaultValue to the original setting (e.g., "true") or explicitly
confirm and document the new default in the UI (add a summary/hint) so users are
aware. Update the SwitchPreferenceCompat element (the preference with key
key_spectrum_ext_safety) to the intended default or add a short summary string
resource to clarify the intentional behavior change.
---
Nitpick comments:
In `@app/src/main/cpp/libjamesdsp-wrapper/clarity/ClarityProcessor.h`:
- Around line 159-162: The field declaration changed from the enum class type to
an int (Mode -> int mode) which weakens type safety; restore the enum class type
by declaring the member as Mode mode (or if you must keep int for ABI reasons,
add validation in the setter such as setMode or set_mode to check the incoming
integer is within the Mode enum range and convert to Mode before storing) and
update any uses that assume an int accordingly; reference the Mode enum and the
mode member in ClarityProcessor.h and adjust constructors/setters that
initialize mode to use static_cast<Mode>(...) only after validating the value.
In `@app/src/main/cpp/libjdspimptoolbox/main/JdspImpResToolbox.c`:
- Around line 851-854: The three read-only JNI arrays are being released with
mode 0 which forces copy-back; change the calls releasing jfreq/jgain/jdispFreq
to use JNI_ABORT instead of 0 so ReleaseDoubleArrayElements(env, jfreq, freqs,
JNI_ABORT), ReleaseDoubleArrayElements(env, jgain, gains, JNI_ABORT) and
ReleaseDoubleArrayElements(env, jdispFreq, dispFreq, JNI_ABORT) to avoid
unnecessary copying back of freqs, gains and dispFreq while leaving
ReleaseFloatArrayElements(env, jresponse, response, 0) unchanged if response is
an output buffer.
In
`@app/src/main/java/me/timschneeberger/rootlessjamesdsp/interop/JamesDspBaseEngine.kt`:
- Around line 97-99: Parse eqInterpolationMode as you're doing now, then clamp
it into the supported range before using it: replace the current
eqInterpolationMode assignment with logic that converts the cached string to Int
(as done) and then bounds it with min/max (e.g., eqInterpolationMode =
eqInterpolationMode.coerceIn(0, MAX_EQ_INTERPOLATION_MODE)). Reference the
eqInterpolationMode variable and introduce or reuse a constant named
MAX_EQ_INTERPOLATION_MODE (or the appropriate existing constant) to define the
upper bound so out-of-range preference edits cannot propagate downstream.
In
`@app/src/main/java/me/timschneeberger/rootlessjamesdsp/interop/JamesDspLocalEngine.kt`:
- Around line 175-210: The EQ normalization logic and constants
(normalizeMultiEqBands, VIPER_SCALE, VIPER_EXT_SCALE, STANDARD_SCALE, EQ_FIELDS)
are duplicated in JamesDspLocalEngine and JamesDspRemoteEngine; extract them
into a single shared helper (e.g., an object/class like EqNormalization or a
util file) that exposes a normalizeMultiEqBands(filterType: Int, bands:
DoubleArray): DoubleArray and the scale constants, move the constants out of the
engine classes into that helper, update both
JamesDspLocalEngine.normalizeMultiEqBands and
JamesDspRemoteEngine.normalizeMultiEqBands to delegate to the shared helper, and
keep behavior identical (same branching for EQ_FILTER_TYPE_VIPER_ORIGINAL and
default) to avoid drift.
Comment on lines
117
to
119
| const double b0 = ((A + 1.0) + (A - 1.0) * cosOmega + 2.0 * sqrtA * z) * A * omega; | ||
| const double b1 = -2.0 * A * ((A - 1.0) + (A + 1.0) * cosOmega) * omega; | ||
| const double b2 = ((A + 1.0) + (A - 1.0) * cosOmega - 2.0 * sqrtA * z) * A * omega; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "FieldSurroundProcessor.cpp" -type fRepository: JSJ-Experiments/RootlessJamesDSP
Length of output: 155
🏁 Script executed:
cat -n app/src/main/cpp/libjamesdsp-wrapper/fieldsurround/FieldSurroundProcessor.cpp | sed -n '100,130p'Repository: JSJ-Experiments/RootlessJamesDSP
Length of output: 1617
🌐 Web query:
standard biquad high shelf filter coefficient formula audio DSP
💡 Result:
RBJ “Audio EQ Cookbook” high-shelf biquad (standard)
Given sample rate Fs, shelf midpoint/corner f0, gain dBgain, and shelf slope S:
Intermediates
A = 10^(dBgain/40)(sosqrt(A)=10^(dBgain/80))w0 = 2*pi*f0/Fsalpha = sin(w0)/2 * sqrt( (A + 1/A)*(1/S - 1) + 2 )(shelving form) [1]
Coefficients (unnormalized)
b0 = A*((A+1) + (A-1)*cos(w0) + 2*sqrt(A)*alpha)b1 = -2*A*((A-1) + (A+1)*cos(w0))b2 = A*((A+1) + (A-1)*cos(w0) - 2*sqrt(A)*alpha)a0 = (A+1) - (A-1)*cos(w0) + 2*sqrt(A)*alphaa1 = 2*((A-1) - (A+1)*cos(w0))a2 = (A+1) - (A-1)*cos(w0) - 2*sqrt(A)*alpha[1]
Normalize for direct-form implementations (typical):
b0/=a0; b1/=a0; b2/=a0; a1/=a0; a2/=a0;(then seta0 = 1). [2]
Common “fixed-slope” variant (S = 1) using K = tan(pi*f0/Fs)
If you prefer the widely used K/V form (effectively a fixed shelf slope), see the EarLevel implementation (with V = 10^(|dBgain|/20), K = tan(pi*Fc/Fs)) and its high-shelf coefficient equations. [3]
Sources: RBJ cookbook formulas (high-shelf) [1][2] and a standard reference implementation [3].
[1] https://gist.github.com/RyanMarcus/d3386baa6b4cb1ac47f4
[2] https://shepazu.github.io/Audio-EQ-Cookbook/audio-eq-cookbook.html
[3] https://www.earlevel.com/main/2011/01/02/biquad-formulas/
🏁 Script executed:
cat -n app/src/main/cpp/libjamesdsp-wrapper/fieldsurround/FieldSurroundProcessor.cpp | sed -n '90,125p'Repository: JSJ-Experiments/RootlessJamesDSP
Length of output: 1734
🏁 Script executed:
rg -i "omega.*scaling|scaling.*coefficient|high.*shelf|biquad" app/src/main/cpp/libjamesdsp-wrapper/fieldsurround/ -A 2 -B 2Repository: JSJ-Experiments/RootlessJamesDSP
Length of output: 3642
🏁 Script executed:
cat -n app/src/main/cpp/libjamesdsp-wrapper/fieldsurround/FieldSurroundProcessor.cpp | head -50Repository: JSJ-Experiments/RootlessJamesDSP
Length of output: 1711
Remove the omega scaling factor from b coefficients—this deviates from standard high-shelf filter formulas and appears to be unintentional.
The b0, b1, and b2 coefficients on lines 117-119 are incorrectly scaled by omega. According to the RBJ Audio EQ Cookbook (the standard reference for biquad filter design), high-shelf coefficients use only the gain factor A, not A * omega. The a coefficients (lines 114-116) correctly follow the standard formula, but the b coefficients introduce a non-standard omega multiplication that will distort the intended filter response.
Correct the coefficients to match the standard high-shelf formula:
b0 = A * ((A + 1.0) + (A - 1.0) * cosOmega + 2.0 * sqrtA * z)
b1 = -2.0 * A * ((A - 1.0) + (A + 1.0) * cosOmega)
b2 = A * ((A + 1.0) + (A - 1.0) * cosOmega - 2.0 * sqrtA * z)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@app/src/main/cpp/libjamesdsp-wrapper/fieldsurround/FieldSurroundProcessor.cpp`
around lines 117 - 119, The b-coefficient expressions (b0, b1, b2) in
FieldSurroundProcessor.cpp are incorrectly multiplied by omega; remove the
trailing "* omega" from each so they match the RBJ high-shelf form used for the
a-coefficients—set b0 to A * ((A + 1.0) + (A - 1.0) * cosOmega + 2.0 * sqrtA *
z), b1 to -2.0 * A * ((A - 1.0) + (A + 1.0) * cosOmega), and b2 to A * ((A +
1.0) + (A - 1.0) * cosOmega - 2.0 * sqrtA * z) to restore the standard
high-shelf formula for the FieldSurroundProcessor.
Member
Author
There was a problem hiding this comment.
Removing * omega would change behavior away from ViPER behavior.
There was a problem hiding this comment.
@JadenJSJ, understood! Thank you for clarifying that this implementation intentionally matches ViPER's behavior. I now see that the * omega scaling is deliberate to maintain compatibility with ViPER's filter response, even though it deviates from the standard RBJ high-shelf formula.
Consider adding a brief comment above the coefficient calculations to document this intentional deviation from standard formulas, for example:
// Note: b-coefficients include omega scaling to match ViPER behavior
const double b0 = ((A + 1.0) + (A - 1.0) * cosOmega + 2.0 * sqrtA * z) * A * omega;This would help future maintainers understand the design decision.
(\_(\
(•ㅅ•) *noted for future!*
/>📝
✏️ Learnings added
Learnt from: JadenJSJ
Repo: JSJ-Experiments/RootlessJamesDSP PR: 1
File: app/src/main/cpp/libjamesdsp-wrapper/fieldsurround/FieldSurroundProcessor.cpp:117-119
Timestamp: 2026-02-22T10:24:01.242Z
Learning: In `app/src/main/cpp/libjamesdsp-wrapper/fieldsurround/FieldSurroundProcessor.cpp`, the high-pass filter implementation in `Biquad::setHighPassParameter()` intentionally includes `* omega` scaling on the b-coefficients (b0, b1, b2) to match ViPER behavior, deviating from standard RBJ Audio EQ Cookbook formulas.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
app/src/main/cpp/libjamesdsp-wrapper/fieldsurround/FieldSurroundProcessor.cpp
Show resolved
Hide resolved
app/src/main/java/me/timschneeberger/rootlessjamesdsp/fragment/PreferenceGroupFragment.kt
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
app/src/main/java/me/timschneeberger/rootlessjamesdsp/interop/JamesDspBaseEngine.kt (1)
96-124:⚠️ Potential issue | 🟡 MinorUse safe parsing for
crossfeedMode(and other int prefs) to avoid crashes on malformed prefs.You used
toIntOrNull()for EQ filter type (great).crossfeedModestill usestoInt()(Line 180), which can throw if the stored value is corrupted or from an older format.Proposed fix
- val crossfeedMode = cache.get(R.string.key_crossfeed_mode, CROSSFEED_MODE_DEFAULT.toString()).toInt() + val crossfeedMode = cache.get(R.string.key_crossfeed_mode, CROSSFEED_MODE_DEFAULT.toString()) + .toIntOrNull() ?: CROSSFEED_MODE_DEFAULTAlso applies to: 178-183
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/me/timschneeberger/rootlessjamesdsp/interop/JamesDspBaseEngine.kt` around lines 96 - 124, The code uses unsafe toInt() conversions for some stored integer prefs (notably crossfeedMode) which can throw on malformed data; in JamesDspBaseEngine replace uses of toInt() on values returned from cache.get (e.g., the crossfeedMode retrieval and any other int prefs around the crossfeed/reverb/spectrum parsing) with safe parsing via toIntOrNull() and provide a sensible default/fallback (and coerceIn where applicable), mirroring the pattern used for eqFilterType (toIntOrNull() ?: default) to avoid crashes from corrupted or old preference formats.app/src/main/java/me/timschneeberger/rootlessjamesdsp/interop/JamesDspRemoteEngine.kt (1)
223-337:⚠️ Potential issue | 🟠 MajorClamp
depthbefore.toShort()to avoid silent wrap on out-of-range strength.In
fallbackToLegacy,depth.toShort()will wrap ifdepthever escapes[-32768, 32767](and that wrap would be very hard to debug in audio behavior). Even if current mappings “should” keep it in range, clamping here is cheap and makes the contract explicit.Proposed fix
if (enable) { val fallbackToLegacy = { + val depthClamped = depth.coerceIn(Short.MIN_VALUE.toInt(), Short.MAX_VALUE.toInt()) val widenResult = effect.setParameter(PARAM_FIELD_SURROUND_WIDENING, widening.toShort()) if (widenResult != AudioEffect.SUCCESS) { markUnsupported(widenResult, true) } else { val midResult = effect.setParameter(PARAM_FIELD_SURROUND_MID_IMAGE, midImage.toShort()) if (midResult != AudioEffect.SUCCESS) { markUnsupported(midResult, true) } else { - val depthResult = effect.setParameter(PARAM_FIELD_SURROUND_DEPTH, depth.toShort()) + val depthResult = effect.setParameter(PARAM_FIELD_SURROUND_DEPTH, depthClamped.toShort()) if (depthResult != AudioEffect.SUCCESS) { markUnsupported(depthResult, true) } else { true } } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/me/timschneeberger/rootlessjamesdsp/interop/JamesDspRemoteEngine.kt` around lines 223 - 337, The fallbackToLegacy lambda in setFieldSurroundInternal can silently wrap when converting depth.toShort(); clamp depth to the Short range before converting (e.g. val safeDepth = depth.coerceIn(Short.MIN_VALUE.toInt(), Short.MAX_VALUE.toInt()).toShort()) and use safeDepth when calling effect.setParameter(PARAM_FIELD_SURROUND_DEPTH, ...); update only the conversion inside fallbackToLegacy (keep other logic unchanged) so out-of-range depth values don't wrap.app/src/main/cpp/libjamesdsp-wrapper/JamesDspWrapper.cpp (1)
265-315:⚠️ Potential issue | 🔴 CriticalCritical: Bounds checking missing in offset/size handling; reading beyond array bounds.
When
size < 0(default),inputLengthis set to the full input array length, but the code then readsinput[offset + i]up tooffset + inputLength - 1. This exceeds bounds whenoffset > 0.Example:
offset=500,size=-1,input.length=1000→ reads indices 500–1499, which is out of bounds.Additionally, there is no validation that the output array is large enough for the computed
inputLength.All three methods (processInt16, processInt32, processFloat) have this issue. Correct the length calculation to account for offset:
jsize inputLength; - if(size < 0) - inputLength = env->GetArrayLength(inputObj); - else - inputLength = size; - if(offset < 0) - offset = 0; + const jsize inputArrayLength = env->GetArrayLength(inputObj); + const jsize outputArrayLength = env->GetArrayLength(outputObj); + if (offset < 0) offset = 0; + if (offset > inputArrayLength) offset = inputArrayLength; + + inputLength = (size < 0) ? (inputArrayLength - offset) : size; + if (inputLength < 0) inputLength = 0; + if (inputLength > (inputArrayLength - offset)) inputLength = (inputArrayLength - offset); + if (inputLength > outputArrayLength) inputLength = outputArrayLength; + // stereo interleaved: require even sample count + inputLength &= ~1;Apply to all three methods (processInt16, processInt32, processFloat).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/cpp/libjamesdsp-wrapper/JamesDspWrapper.cpp` around lines 265 - 315, The offset/size calculation in Java_me_timschneeberger_rootlessjamesdsp_interop_JamesDspWrapper_processInt16 (and analogs processInt32/processFloat) can read past the input/output arrays: when size < 0 you set inputLength = GetArrayLength(inputObj) but still index from offset, causing offset + inputLength overflow; fix by computing available = env->GetArrayLength(inputObj) - max(0, offset) and then set inputLength = (size < 0) ? available : min(static_cast<jsize>(size), available), ensure inputLength >= 0, compute frames from that bounded length, and likewise validate output array length via env->GetArrayLength(outputObj) - max(0, offset) to ensure output has enough space; on any bounds failure, release arrays and return (same error cleanup path). Apply the same bounded-length logic and checks in the corresponding Java_me_timschneeberger_rootlessjamesdsp_interop_JamesDspWrapper_processInt32 and processFloat functions.
🧹 Nitpick comments (8)
app/src/test/java/me/timschneeberger/rootlessjamesdsp/interop/FieldSurroundDepthMappingTest.kt (1)
8-30: Consider adding edge case tests for better coverage.The current tests validate key scenarios well. Consider adding tests for:
toWrapperCompatDepthStrength(0)should return 200 (lower bound)toDirectDepthStrengthwith negative values (verifies short conversion behavior)isDepthAtOrAboveBranchThreshold(499)should return false (boundary)💡 Suggested additional tests
`@Test` fun wrapperPathDepthZeroMapsTo200() { val strength = FieldSurroundDepthMapping.toWrapperCompatDepthStrength(0) assertEquals(200, strength) } `@Test` fun depth499BelowBranchThreshold() { val strength = FieldSurroundDepthMapping.toDirectDepthStrength(499) assertFalse(FieldSurroundDepthMapping.isDepthAtOrAboveBranchThreshold(strength)) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/test/java/me/timschneeberger/rootlessjamesdsp/interop/FieldSurroundDepthMappingTest.kt` around lines 8 - 30, Add edge-case unit tests to improve coverage: add a test that calls FieldSurroundDepthMapping.toWrapperCompatDepthStrength(0) and asserts it returns 200, add a test that calls toDirectDepthStrength with a negative input (e.g., -1) and asserts the resulting short/strength behaves as expected (non-positive conversion) using isDepthStageEnabled or direct equality as appropriate, and add a boundary test that calls toDirectDepthStrength(499) and asserts FieldSurroundDepthMapping.isDepthAtOrAboveBranchThreshold(strength) returns false; reference the methods toWrapperCompatDepthStrength, toDirectDepthStrength and isDepthAtOrAboveBranchThreshold and the CONFIG_DEFAULT_DEPTH constant when adding these tests.app/src/main/java/me/timschneeberger/rootlessjamesdsp/interop/JamesDspBaseEngine.kt (2)
554-687: FieldSurround depth mapping refactor is clean; consider clamping mapped strength to int16 range at the boundary.Since native strength is now
int16_t(and remotes may transmit depth as a short), clampingdepthStrengthtoShort.MIN_VALUE..Short.MAX_VALUEbefore callingsetFieldSurroundInternal(...)would make the contract explicit and prevent any future mapping changes from accidentally overflowing.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/me/timschneeberger/rootlessjamesdsp/interop/JamesDspBaseEngine.kt` around lines 554 - 687, The mapped depthStrength passed into setFieldSurroundInternal can exceed int16; clamp it to the int16 range to match the native int16_t contract: in setFieldSurroundMappedDepth compute a clampedDepth = depthStrength.coerceIn(Short.MIN_VALUE.toInt(), Short.MAX_VALUE.toInt()) (or equivalent) and pass clampedDepth to setFieldSurroundInternal instead of depthStrength, ensuring all callers of setFieldSurroundMappedDepth (setFieldSurround and setFieldSurroundWrapperCompat) remain unchanged.
301-324: Nice improvement to EQ string validation; consider rejecting non-finite numbers too.Right now
"NaN"/"Infinity"would parse viatoDoubleOrNull()and get forwarded to native. If that’s not intended, add anisFinite()check here (similar to harmonics parsing).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/me/timschneeberger/rootlessjamesdsp/interop/JamesDspBaseEngine.kt` around lines 301 - 324, The setMultiEqualizer method currently accepts values like "NaN" or "Infinity" because toDoubleOrNull() returns those as Doubles; update the parsing loop in setMultiEqualizer to reject non-finite values by checking number.isFinite() after toDoubleOrNull(), log a clear error (like other malformed cases) and return false if the value is not finite before assigning into doubleArray, so only finite doubles are passed to setMultiEqualizerInternal.app/src/main/cpp/libjamesdsp-wrapper/clarity/ClarityProcessor.cpp (2)
105-145: Guarded NoiseSharpening resets are good; consider clamping cutoff to a non-negative range.If
nyquistOffsetHzexceedssamplingRate * 0.5f,cutoff(Line 122) goes negative and gets fed into filter design. A simplestd::max(0.0f, cutoff)(orstd::clamp) would make this more robust.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/cpp/libjamesdsp-wrapper/clarity/ClarityProcessor.cpp` around lines 105 - 145, Clamp the computed cutoff in NoiseSharpening::reset to a non-negative value before passing it to filters[i].setLPF_BW to avoid negative bandwidths when nyquistOffsetHz > samplingRate*0.5; e.g., compute cutoff as static_cast<float>(samplingRate)*0.5f - nyquistOffsetHz and then use std::max(0.0f, cutoff) (or std::clamp) when calling setLPF_BW, leaving the rest of reset() and the setters (setNyquistOffset, setSamplingRate) unchanged.
268-338: ClarityProcessor guarded state changes are a big improvement; validate mode inputs.
setMode(int m)usesstatic_cast<Mode>(m)(Line 286) without range checking, andapplyModehas nodefault:case. Consider clampingmto the known enum range (or adding a default behavior) to keep invalid pref/plugin inputs from putting the processor into a silent no-op state.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/cpp/libjamesdsp-wrapper/clarity/ClarityProcessor.cpp` around lines 268 - 338, The setMode(int m) path casts an arbitrary int to Mode without validation (in setMode) and applyMode lacks a default branch; update setMode to validate/clamp m against the known Mode enum range (or map unknown values to a safe fall-back like Mode::BYPASS or Mode::OZONE) before doing static_cast and only change mode if valid, and add a default case in applyMode to handle unexpected Mode values (perform a safe reset/sync and/or log the event) so invalid pref/plugin inputs cannot put the processor into a silent no-op state.app/src/main/cpp/libjamesdsp-wrapper/fieldsurround/FieldSurroundProcessor.cpp (1)
241-253: Minor: consider enforcing a non-negative/finite gainCap at the boundary.
gain = max(0, min(gainCap, computedGain))behaves “safe” even whengainCapis negative (it collapses to 0), but clampinggainCapto>= 0(and finite) earlier would make the model less surprising.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/cpp/libjamesdsp-wrapper/fieldsurround/FieldSurroundProcessor.cpp` around lines 241 - 253, The clamp on gain in DepthSurround::refreshStrength is currently using gain = std::max(0.0f, std::min(gainCap, computedGain)), which silently treats negative or non-finite gainCap oddly; before clamping computedGain, validate and sanitize gainCap (e.g., ensure gainCap >= 0.0f and is finite using std::isfinite) and replace it with a safe non-negative finite value so subsequent clamping of computedGain/gain behaves predictably (refer to DepthSurround::refreshStrength, gainCap, computedGain, and gain).app/src/main/java/me/timschneeberger/rootlessjamesdsp/fragment/PreferenceGroupFragment.kt (1)
321-414: Strength unit round-tripping improvements are good; consider centralizing minDb/max constants.The
mapMinDbToZeromapping +linearToDbguard make percent↔dB switching much more stable. Minor maintainability note:minDb = -40.0fis duplicated here and elsewhere; consider sharing a single source of truth to avoid drift.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/me/timschneeberger/rootlessjamesdsp/fragment/PreferenceGroupFragment.kt` around lines 321 - 414, The local hardcoded minDb (-40.0f) is duplicated; create a single constant (e.g., MIN_DB = -40.0f) and replace the local minDb declaration in bindStrengthUnitPreferences, the literal in linearToDb (currently returns -40.0f when linear <= 0.0f), and any other places that rely on that value (references: the local variable minDb in bindStrengthUnitPreferences, linearToDb function, and dbToLinearMapped/linearToDbMapped helpers) so all places use the shared constant; update uses to refer to that constant and remove the duplicated literal.app/src/main/java/me/timschneeberger/rootlessjamesdsp/view/EqualizerSurface.kt (1)
23-38: Consider no-op guards in setters to avoid redundant redraws.
interpolationModenormalizes to 0/1 and always callsinvalidate()(same forisViperOriginalMode), which can cause extra redraws if the same value is re-applied via binding/prefs.Proposed tweak (optional)
var interpolationMode: Int = 0 set(value) { - field = if (value == 1) 1 else 0 - invalidate() + val normalized = if (value == 1) 1 else 0 + if (field == normalized) return + field = normalized + invalidate() } var isViperOriginalMode: Boolean = false set(value) { - field = value - visibleBands = if (value) VIPER_ORIGINAL_FULL_SCALE.size else SCALE.size - invalidate() + if (field == value) return + field = value + visibleBands = if (value) VIPER_ORIGINAL_FULL_SCALE.size else SCALE.size + invalidate() }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/me/timschneeberger/rootlessjamesdsp/view/EqualizerSurface.kt` around lines 23 - 38, Add no-op guards to avoid redundant invalidate() calls: for interpolationMode compute the normalized value (normalized = if value == 1 then 1 else 0) and if field == normalized return; otherwise set field = normalized and call invalidate(); for isViperOriginalMode if field == value return; otherwise set field = value, compute visibleBands = if (value) VIPER_ORIGINAL_FULL_SCALE.size else SCALE.size, then call invalidate(); similarly for sampleRate compute val = value.coerceAtLeast(1) and if field == val return else set field = val and invalidate(). These changes touch the setters for interpolationMode, isViperOriginalMode, and sampleRate and reference visibleBands, VIPER_ORIGINAL_FULL_SCALE, SCALE, and invalidate().
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/src/main/cpp/libjamesdsp-wrapper/JamesDspWrapper.cpp`:
- Around line 869-874: The call to FieldSurroundProcessor::setDepthFromParamInt
currently passes a truncated short (static_cast<short>(depth)) which can wrap
before the method clamps; change the call in JamesDspWrapper.cpp to pass the
original int value (depth) directly to fieldSurround->setDepthFromParamInt so
the internal clamping logic of setDepthFromParamInt(int) can operate correctly,
i.e., remove the static_cast<short> and pass depth as-is.
In
`@app/src/main/java/me/timschneeberger/rootlessjamesdsp/fragment/PreferenceGroupFragment.kt`:
- Around line 149-163: The two hard-coded toast messages in
PreferenceGroupFragment (the requireContext().toast calls inside the modePref
value reset and modePref.setOnPreferenceChangeListener when checking
supportsCustomCrossfeed and CUSTOM_CROSSFEED_MODE_VALUE) should be moved to
string resources; add string entries (e.g. crossfeed_unavailable_using_preset
and crossfeed_unavailable) to strings.xml, then replace the literal strings with
getString(R.string.crossfeed_unavailable_using_preset) and
getString(R.string.crossfeed_unavailable) in the PreferenceGroupFragment where
modePref and customFeedPref are referenced so the toasts use localized
resources.
---
Outside diff comments:
In `@app/src/main/cpp/libjamesdsp-wrapper/JamesDspWrapper.cpp`:
- Around line 265-315: The offset/size calculation in
Java_me_timschneeberger_rootlessjamesdsp_interop_JamesDspWrapper_processInt16
(and analogs processInt32/processFloat) can read past the input/output arrays:
when size < 0 you set inputLength = GetArrayLength(inputObj) but still index
from offset, causing offset + inputLength overflow; fix by computing available =
env->GetArrayLength(inputObj) - max(0, offset) and then set inputLength = (size
< 0) ? available : min(static_cast<jsize>(size), available), ensure inputLength
>= 0, compute frames from that bounded length, and likewise validate output
array length via env->GetArrayLength(outputObj) - max(0, offset) to ensure
output has enough space; on any bounds failure, release arrays and return (same
error cleanup path). Apply the same bounded-length logic and checks in the
corresponding
Java_me_timschneeberger_rootlessjamesdsp_interop_JamesDspWrapper_processInt32
and processFloat functions.
In
`@app/src/main/java/me/timschneeberger/rootlessjamesdsp/interop/JamesDspBaseEngine.kt`:
- Around line 96-124: The code uses unsafe toInt() conversions for some stored
integer prefs (notably crossfeedMode) which can throw on malformed data; in
JamesDspBaseEngine replace uses of toInt() on values returned from cache.get
(e.g., the crossfeedMode retrieval and any other int prefs around the
crossfeed/reverb/spectrum parsing) with safe parsing via toIntOrNull() and
provide a sensible default/fallback (and coerceIn where applicable), mirroring
the pattern used for eqFilterType (toIntOrNull() ?: default) to avoid crashes
from corrupted or old preference formats.
In
`@app/src/main/java/me/timschneeberger/rootlessjamesdsp/interop/JamesDspRemoteEngine.kt`:
- Around line 223-337: The fallbackToLegacy lambda in setFieldSurroundInternal
can silently wrap when converting depth.toShort(); clamp depth to the Short
range before converting (e.g. val safeDepth =
depth.coerceIn(Short.MIN_VALUE.toInt(), Short.MAX_VALUE.toInt()).toShort()) and
use safeDepth when calling effect.setParameter(PARAM_FIELD_SURROUND_DEPTH, ...);
update only the conversion inside fallbackToLegacy (keep other logic unchanged)
so out-of-range depth values don't wrap.
---
Nitpick comments:
In `@app/src/main/cpp/libjamesdsp-wrapper/clarity/ClarityProcessor.cpp`:
- Around line 105-145: Clamp the computed cutoff in NoiseSharpening::reset to a
non-negative value before passing it to filters[i].setLPF_BW to avoid negative
bandwidths when nyquistOffsetHz > samplingRate*0.5; e.g., compute cutoff as
static_cast<float>(samplingRate)*0.5f - nyquistOffsetHz and then use
std::max(0.0f, cutoff) (or std::clamp) when calling setLPF_BW, leaving the rest
of reset() and the setters (setNyquistOffset, setSamplingRate) unchanged.
- Around line 268-338: The setMode(int m) path casts an arbitrary int to Mode
without validation (in setMode) and applyMode lacks a default branch; update
setMode to validate/clamp m against the known Mode enum range (or map unknown
values to a safe fall-back like Mode::BYPASS or Mode::OZONE) before doing
static_cast and only change mode if valid, and add a default case in applyMode
to handle unexpected Mode values (perform a safe reset/sync and/or log the
event) so invalid pref/plugin inputs cannot put the processor into a silent
no-op state.
In
`@app/src/main/cpp/libjamesdsp-wrapper/fieldsurround/FieldSurroundProcessor.cpp`:
- Around line 241-253: The clamp on gain in DepthSurround::refreshStrength is
currently using gain = std::max(0.0f, std::min(gainCap, computedGain)), which
silently treats negative or non-finite gainCap oddly; before clamping
computedGain, validate and sanitize gainCap (e.g., ensure gainCap >= 0.0f and is
finite using std::isfinite) and replace it with a safe non-negative finite value
so subsequent clamping of computedGain/gain behaves predictably (refer to
DepthSurround::refreshStrength, gainCap, computedGain, and gain).
In
`@app/src/main/java/me/timschneeberger/rootlessjamesdsp/fragment/PreferenceGroupFragment.kt`:
- Around line 321-414: The local hardcoded minDb (-40.0f) is duplicated; create
a single constant (e.g., MIN_DB = -40.0f) and replace the local minDb
declaration in bindStrengthUnitPreferences, the literal in linearToDb (currently
returns -40.0f when linear <= 0.0f), and any other places that rely on that
value (references: the local variable minDb in bindStrengthUnitPreferences,
linearToDb function, and dbToLinearMapped/linearToDbMapped helpers) so all
places use the shared constant; update uses to refer to that constant and remove
the duplicated literal.
In
`@app/src/main/java/me/timschneeberger/rootlessjamesdsp/interop/JamesDspBaseEngine.kt`:
- Around line 554-687: The mapped depthStrength passed into
setFieldSurroundInternal can exceed int16; clamp it to the int16 range to match
the native int16_t contract: in setFieldSurroundMappedDepth compute a
clampedDepth = depthStrength.coerceIn(Short.MIN_VALUE.toInt(),
Short.MAX_VALUE.toInt()) (or equivalent) and pass clampedDepth to
setFieldSurroundInternal instead of depthStrength, ensuring all callers of
setFieldSurroundMappedDepth (setFieldSurround and setFieldSurroundWrapperCompat)
remain unchanged.
- Around line 301-324: The setMultiEqualizer method currently accepts values
like "NaN" or "Infinity" because toDoubleOrNull() returns those as Doubles;
update the parsing loop in setMultiEqualizer to reject non-finite values by
checking number.isFinite() after toDoubleOrNull(), log a clear error (like other
malformed cases) and return false if the value is not finite before assigning
into doubleArray, so only finite doubles are passed to
setMultiEqualizerInternal.
In
`@app/src/main/java/me/timschneeberger/rootlessjamesdsp/view/EqualizerSurface.kt`:
- Around line 23-38: Add no-op guards to avoid redundant invalidate() calls: for
interpolationMode compute the normalized value (normalized = if value == 1 then
1 else 0) and if field == normalized return; otherwise set field = normalized
and call invalidate(); for isViperOriginalMode if field == value return;
otherwise set field = value, compute visibleBands = if (value)
VIPER_ORIGINAL_FULL_SCALE.size else SCALE.size, then call invalidate();
similarly for sampleRate compute val = value.coerceAtLeast(1) and if field ==
val return else set field = val and invalidate(). These changes touch the
setters for interpolationMode, isViperOriginalMode, and sampleRate and reference
visibleBands, VIPER_ORIGINAL_FULL_SCALE, SCALE, and invalidate().
In
`@app/src/test/java/me/timschneeberger/rootlessjamesdsp/interop/FieldSurroundDepthMappingTest.kt`:
- Around line 8-30: Add edge-case unit tests to improve coverage: add a test
that calls FieldSurroundDepthMapping.toWrapperCompatDepthStrength(0) and asserts
it returns 200, add a test that calls toDirectDepthStrength with a negative
input (e.g., -1) and asserts the resulting short/strength behaves as expected
(non-positive conversion) using isDepthStageEnabled or direct equality as
appropriate, and add a boundary test that calls toDirectDepthStrength(499) and
asserts FieldSurroundDepthMapping.isDepthAtOrAboveBranchThreshold(strength)
returns false; reference the methods toWrapperCompatDepthStrength,
toDirectDepthStrength and isDepthAtOrAboveBranchThreshold and the
CONFIG_DEFAULT_DEPTH constant when adding these tests.
Comment on lines
869
to
874
| fieldSurround->setOutputModeFromParamInt(outputMode); | ||
| fieldSurround->setWidenFromParamInt(widening); | ||
| fieldSurround->setMidFromParamInt(midImage); | ||
| fieldSurround->setDepthFromParamInt(depth); | ||
| fieldSurround->setDepthFromParamInt(static_cast<short>(depth)); | ||
| fieldSurround->setPhaseOffsetFromParamInt(phaseOffset); | ||
| fieldSurround->setMonoSumMixFromParamInt(monoSumMix); |
There was a problem hiding this comment.
Avoid truncation before clamping: drop the static_cast<short>(depth) here.
FieldSurroundProcessor::setDepthFromParamInt(int value) already clamps to the int16_t range internally. Casting to short first can wrap and defeat the clamp (if an out-of-range ever slips through).
Proposed fix
- fieldSurround->setDepthFromParamInt(static_cast<short>(depth));
+ fieldSurround->setDepthFromParamInt(depth);📝 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.
Suggested change
| fieldSurround->setOutputModeFromParamInt(outputMode); | |
| fieldSurround->setWidenFromParamInt(widening); | |
| fieldSurround->setMidFromParamInt(midImage); | |
| fieldSurround->setDepthFromParamInt(depth); | |
| fieldSurround->setDepthFromParamInt(static_cast<short>(depth)); | |
| fieldSurround->setPhaseOffsetFromParamInt(phaseOffset); | |
| fieldSurround->setMonoSumMixFromParamInt(monoSumMix); | |
| fieldSurround->setOutputModeFromParamInt(outputMode); | |
| fieldSurround->setWidenFromParamInt(widening); | |
| fieldSurround->setMidFromParamInt(midImage); | |
| fieldSurround->setDepthFromParamInt(depth); | |
| fieldSurround->setPhaseOffsetFromParamInt(phaseOffset); | |
| fieldSurround->setMonoSumMixFromParamInt(monoSumMix); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/src/main/cpp/libjamesdsp-wrapper/JamesDspWrapper.cpp` around lines 869 -
874, The call to FieldSurroundProcessor::setDepthFromParamInt currently passes a
truncated short (static_cast<short>(depth)) which can wrap before the method
clamps; change the call in JamesDspWrapper.cpp to pass the original int value
(depth) directly to fieldSurround->setDepthFromParamInt so the internal clamping
logic of setDepthFromParamInt(int) can operate correctly, i.e., remove the
static_cast<short> and pass depth as-is.
Comment on lines
149
to
163
| if (modePref.value == CUSTOM_CROSSFEED_MODE_VALUE) { | ||
| modePref.value = CROSSFEED_MODE_DEFAULT_VALUE | ||
| requireContext().toast("Custom crossfeed is unavailable on this backend. Using preset mode.", false) | ||
| } | ||
| } | ||
|
|
||
| customFeedPref?.valueLabelOverride = fun(it: Float): String { | ||
| return String.format(Locale.getDefault(), "%.1f dB", it / 10.0f) | ||
| } | ||
|
|
||
| modePref?.setOnPreferenceChangeListener { _, newValue -> | ||
| val selectedMode = newValue as? String | ||
| if (!supportsCustomCrossfeed && selectedMode == CUSTOM_CROSSFEED_MODE_VALUE) { | ||
| requireContext().toast("Custom crossfeed is unavailable on this backend.", false) | ||
| false |
There was a problem hiding this comment.
Move new toast strings into resources for localization.
The messages on Line 151/162 are user-facing and currently hard-coded; they should be string resources to keep translations consistent across the app.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@app/src/main/java/me/timschneeberger/rootlessjamesdsp/fragment/PreferenceGroupFragment.kt`
around lines 149 - 163, The two hard-coded toast messages in
PreferenceGroupFragment (the requireContext().toast calls inside the modePref
value reset and modePref.setOnPreferenceChangeListener when checking
supportsCustomCrossfeed and CUSTOM_CROSSFEED_MODE_VALUE) should be moved to
string resources; add string entries (e.g. crossfeed_unavailable_using_preset
and crossfeed_unavailable) to strings.xml, then replace the literal strings with
getString(R.string.crossfeed_unavailable_using_preset) and
getString(R.string.crossfeed_unavailable) in the PreferenceGroupFragment where
modePref and customFeedPref are referenced so the toasts use localized
resources.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
docs/CLARITY_RE.md (1)
319-319: Minor: Add hyphen for compound adjective.
sample-rate dependentshould besample-rate-dependentwhen used as a compound adjective modifying "integer sample delays".📝 Suggested fix
-- XHiFi delays are sample-rate dependent integer sample delays (`sr/400`, `sr/200`), not fixed milliseconds. +- XHiFi delays are sample-rate-dependent integer sample delays (`sr/400`, `sr/200`), not fixed milliseconds.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/CLARITY_RE.md` at line 319, Update the compound adjective in the sentence about XHiFi delays: change "sample-rate dependent integer sample delays (`sr/400`, `sr/200`), not fixed milliseconds." to use a hyphenated compound adjective "sample-rate-dependent integer sample delays (`sr/400`, `sr/200`), not fixed milliseconds." — locate the phrase "XHiFi delays are sample-rate dependent integer sample delays" and replace it with the hyphenated form.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@docs/CLARITY_RE.md`:
- Line 319: Update the compound adjective in the sentence about XHiFi delays:
change "sample-rate dependent integer sample delays (`sr/400`, `sr/200`), not
fixed milliseconds." to use a hyphenated compound adjective
"sample-rate-dependent integer sample delays (`sr/400`, `sr/200`), not fixed
milliseconds." — locate the phrase "XHiFi delays are sample-rate dependent
integer sample delays" and replace it with the hyphenated form.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
app/src/main/cpp/libjamesdsp-wrapper/clarity/ClarityProcessor.cpp (1)
147-149: Guard log10 against non‑positive gain values.If
gainever reaches 0 or negative,log10will produce-inf/NaN. A small clamp keeps the filter stable.🔧 Suggested hardening
- gainDb = 20.0 * std::log10(static_cast<double>(gain)); + const double safeGain = std::max(1.0e-6, static_cast<double>(gain)); + gainDb = 20.0 * std::log10(safeGain);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/cpp/libjamesdsp-wrapper/clarity/ClarityProcessor.cpp` around lines 147 - 149, In HighShelf::setGainLinear replace the direct log10 call with a guarded clamp: ensure the incoming gain is coerced to a small positive minimum (e.g. std::numeric_limits<float>::min() or a defined epsilon) before calling std::log10 so that gainDb is never set to -inf/NaN; update setGainLinear to use the clamped value and keep assigning to gainDb so filters remain stable when gain <= 0.docs/UPDATED_SPECTRUM_EXT.md (1)
7-12: Consider varying the repeated “Treat” bullet starters.Small wording tweaks would improve flow without changing meaning.
✍️ Possible rewording
- - Treat sections 4 through 11 as the normative DSP and parameter contract. - - Treat sections 12 and 13 as app/wrapper compatibility behavior. - - Treat section 14 as a mandatory portability safeguard for clean-room implementations. - - Use section 18 for direct source-file lookup. + - Use sections 4 through 11 as the normative DSP and parameter contract. + - Refer to sections 12 and 13 for app/wrapper compatibility behavior. + - Reserve section 14 for portability safeguards in clean-room implementations. + - Use section 18 for direct source-file lookup.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/UPDATED_SPECTRUM_EXT.md` around lines 7 - 12, The repeated sentence starter "Treat" in the bullet list under "## 0. How to use this document" feels monotonous; revise the four bullets (the entries referencing sections 4–11, 12–13, 14, and 18) to vary phrasing while preserving meaning — e.g., use "Consider sections 4–11 as...", "Regard sections 12 and 13 as...", "Section 14 should be used as...", and "Use section 18 for..." — update the text around the unique heading "## 0. How to use this document" and the four listed items to apply these alternative starters.docs/CLARITY_RE.md (1)
1-370: Great reference doc; only minor copy‑edits to consider.Consider trimming the repeated “commonly,” hyphenating “sample‑rate‑dependent” and “high‑frequency,” and varying the repeated “Runtime output confirms …” sentence starts to smooth readability.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/CLARITY_RE.md` around lines 1 - 370, Doc copy edits: trim redundant uses of the word "commonly", hyphenate the compound adjectives "sample-rate-dependent" and "high-frequency", and vary the repeated sentence starts that begin "Runtime output confirms ..." for smoother flow. Locate occurrences in the CLARITY_RE.md content (search for "commonly", "sample rate dependent", "high frequency", and the repeated "Runtime output confirms" sentences), remove or reword duplicate "commonly" instances, replace the two compound adjective occurrences with hyphenated forms, and rewrite at least one of the repeated "Runtime output confirms ..." lines to a different lead-in while preserving the same facts.app/src/main/cpp/libjdspimptoolbox/main/JdspImpResToolbox.c (1)
854-862: Consider documenting the denMagSq threshold.The
1.0e-24threshold for skipping bands with near-zero denominator magnitude is extremely small. While this prevents division by zero, consider adding a brief comment explaining the rationale for this specific threshold value.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/cpp/libjdspimptoolbox/main/JdspImpResToolbox.c` around lines 854 - 862, The magic constant 1.0e-24 used in the denMagSq check needs a brief explanatory comment: update the block around variables denRe/denIm/denMagSq (and the subsequent hRe/hIm calculation that uses denMagSq) to document why this threshold was chosen (e.g., to avoid division by zero in double-precision, referencing DBL_EPSILON or the expected signal dynamic range) and note that it intentionally skips bands with near-zero denominator; prefer mentioning that the value is a conservative floor for denRe^2+denIm^2 (or replace with a named constant if desired) so future readers understand the rationale.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/src/main/cpp/libjamesdsp-wrapper/JamesDspWrapper.cpp`:
- Around line 269-327: The float->int16 conversion in
JamesDspWrapper::processInt16's FieldSurround path (the loop that computes
scaled = sample * kScale and assigns output[i] = static_cast<jshort>(...)) can
overflow for near-1.0 samples causing sign wrap; after rounding (or by using a
kScale of 32767) clamp the rounded value into the int16 range
(INT16_MIN..INT16_MAX) before casting and assignment to output[i]; apply the
same fix to the other float-to-int conversion block referenced in the diff (the
second float-path around the dsp->processFloatMultiplexd call at lines noted in
the comment). Ensure you update the loop that writes output[i] and use
INT16_MIN/INT16_MAX for clamping.
---
Nitpick comments:
In `@app/src/main/cpp/libjamesdsp-wrapper/clarity/ClarityProcessor.cpp`:
- Around line 147-149: In HighShelf::setGainLinear replace the direct log10 call
with a guarded clamp: ensure the incoming gain is coerced to a small positive
minimum (e.g. std::numeric_limits<float>::min() or a defined epsilon) before
calling std::log10 so that gainDb is never set to -inf/NaN; update setGainLinear
to use the clamped value and keep assigning to gainDb so filters remain stable
when gain <= 0.
In `@app/src/main/cpp/libjdspimptoolbox/main/JdspImpResToolbox.c`:
- Around line 854-862: The magic constant 1.0e-24 used in the denMagSq check
needs a brief explanatory comment: update the block around variables
denRe/denIm/denMagSq (and the subsequent hRe/hIm calculation that uses denMagSq)
to document why this threshold was chosen (e.g., to avoid division by zero in
double-precision, referencing DBL_EPSILON or the expected signal dynamic range)
and note that it intentionally skips bands with near-zero denominator; prefer
mentioning that the value is a conservative floor for denRe^2+denIm^2 (or
replace with a named constant if desired) so future readers understand the
rationale.
In `@docs/CLARITY_RE.md`:
- Around line 1-370: Doc copy edits: trim redundant uses of the word "commonly",
hyphenate the compound adjectives "sample-rate-dependent" and "high-frequency",
and vary the repeated sentence starts that begin "Runtime output confirms ..."
for smoother flow. Locate occurrences in the CLARITY_RE.md content (search for
"commonly", "sample rate dependent", "high frequency", and the repeated "Runtime
output confirms" sentences), remove or reword duplicate "commonly" instances,
replace the two compound adjective occurrences with hyphenated forms, and
rewrite at least one of the repeated "Runtime output confirms ..." lines to a
different lead-in while preserving the same facts.
In `@docs/UPDATED_SPECTRUM_EXT.md`:
- Around line 7-12: The repeated sentence starter "Treat" in the bullet list
under "## 0. How to use this document" feels monotonous; revise the four bullets
(the entries referencing sections 4–11, 12–13, 14, and 18) to vary phrasing
while preserving meaning — e.g., use "Consider sections 4–11 as...", "Regard
sections 12 and 13 as...", "Section 14 should be used as...", and "Use section
18 for..." — update the text around the unique heading "## 0. How to use this
document" and the four listed items to apply these alternative starters.
There was a problem hiding this comment.
🧹 Nitpick comments (3)
app/src/main/cpp/libjamesdsp-wrapper/clarity/ClarityProcessor.cpp (1)
386-391: Consider logging when falling back to NATURAL mode.The default case handles invalid mode values by resetting to NATURAL, which is defensive. However, modifying state during
process()could be surprising. Consider adding a log/warning when this fallback triggers, to help diagnose configuration issues.♻️ Optional: Add diagnostic logging
default: + // Log unexpected mode value for debugging + // LOGW("ClarityProcessor::applyMode: unexpected mode %d, falling back to NATURAL", static_cast<int>(mode)); mode = Mode::NATURAL; reset(); natural.process(samples, frames); break;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/cpp/libjamesdsp-wrapper/clarity/ClarityProcessor.cpp` around lines 386 - 391, The default switch branch silently falls back to Mode::NATURAL and calls reset() then natural.process(samples, frames); add a diagnostic log/warning so callers can see invalid mode falls back: in the default branch of the switch in ClarityProcessor::process (where Mode::NATURAL, reset(), and natural.process are invoked) emit a concise log or warning (including the invalid mode value if available) before resetting and processing, using the project's logging facility so the message is recorded for debugging configuration issues.app/src/main/cpp/libjamesdsp-wrapper/JamesDspWrapper.cpp (1)
385-396: Consider explicit clamping for edge-case safety in Int32 path.The float-level bounds checks (lines 388-391) handle ±1.0, but for values very close to 1.0, the
scaled + 0.5rounding could theoretically approach INT32_MAX. While practically unlikely due to float precision limits, adding explicit clamping (like in processInt16) would provide defense-in-depth.♻️ Optional: Add explicit clamping for consistency
} else { const double scaled = static_cast<double>(sample) * kScale; - output[i] = static_cast<jint>(scaled > 0.0 ? scaled + 0.5 : scaled - 0.5); + const double rounded = scaled > 0.0 ? scaled + 0.5 : scaled - 0.5; + const double clamped = std::clamp( + rounded, + static_cast<double>(INT32_MIN), + static_cast<double>(INT32_MAX) + ); + output[i] = static_cast<jint>(clamped); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/cpp/libjamesdsp-wrapper/JamesDspWrapper.cpp` around lines 385 - 396, The conversion loop using kScale in JamesDspWrapper.cpp should add explicit clamping after rounding to ensure values can't exceed INT32 bounds; inside the for-loop in the function that populates output[i] (the block using temp[], kScale and static_cast<jint>), compute the rounded value then clamp it to INT32_MIN/INT32_MAX before assigning to output[i] (mirroring the defensive pattern used in the processInt16 path) so that any rare edge-case rounding cannot overflow the jint range.app/src/main/cpp/libjdspimptoolbox/main/JdspImpResToolbox.c (1)
829-829: Verify the 0.636 gain scaling factor.The per-band gain calculation multiplies by
0.636:bandGain[i] = (float)(pow(10.0, gainDb / 20.0) * 0.636);This appears to be a calibration constant. If this is intended to match ViPER's original behavior, consider documenting its origin (e.g., as a comment referencing the source or as a named constant).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/cpp/libjdspimptoolbox/main/JdspImpResToolbox.c` at line 829, The per-band gain calculation uses a magic constant 0.636 which should be verified and documented; replace the bare literal in the expression that assigns bandGain[i] (the line using pow(10.0, gainDb / 20.0) * 0.636) with a named constant (e.g., VIPER_GAIN_SCALE or BAND_GAIN_SCALE) defined near the top of the C file or in the relevant header, and add a brief comment referencing the origin (e.g., “scale factor to match ViPER behavior — reference/issue/URL”) so future readers know why the value is used and where it came from.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@app/src/main/cpp/libjamesdsp-wrapper/clarity/ClarityProcessor.cpp`:
- Around line 386-391: The default switch branch silently falls back to
Mode::NATURAL and calls reset() then natural.process(samples, frames); add a
diagnostic log/warning so callers can see invalid mode falls back: in the
default branch of the switch in ClarityProcessor::process (where Mode::NATURAL,
reset(), and natural.process are invoked) emit a concise log or warning
(including the invalid mode value if available) before resetting and processing,
using the project's logging facility so the message is recorded for debugging
configuration issues.
In `@app/src/main/cpp/libjamesdsp-wrapper/JamesDspWrapper.cpp`:
- Around line 385-396: The conversion loop using kScale in JamesDspWrapper.cpp
should add explicit clamping after rounding to ensure values can't exceed INT32
bounds; inside the for-loop in the function that populates output[i] (the block
using temp[], kScale and static_cast<jint>), compute the rounded value then
clamp it to INT32_MIN/INT32_MAX before assigning to output[i] (mirroring the
defensive pattern used in the processInt16 path) so that any rare edge-case
rounding cannot overflow the jint range.
In `@app/src/main/cpp/libjdspimptoolbox/main/JdspImpResToolbox.c`:
- Line 829: The per-band gain calculation uses a magic constant 0.636 which
should be verified and documented; replace the bare literal in the expression
that assigns bandGain[i] (the line using pow(10.0, gainDb / 20.0) * 0.636) with
a named constant (e.g., VIPER_GAIN_SCALE or BAND_GAIN_SCALE) defined near the
top of the C file or in the relevant header, and add a brief comment referencing
the origin (e.g., “scale factor to match ViPER behavior — reference/issue/URL”)
so future readers know why the value is used and where it came from.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
app/src/main/cpp/libjdspimptoolbox/main/JdspImpResToolbox.c (1)
837-837: Consider validatingnPtsagainst actual array sizes.The function trusts that
nPtsmatches the actual sizes ofjdispFreqandjresponse. If the caller passes mismatched values, this could cause out-of-bounds access. While this follows the existing pattern in functions likeComputeEqResponse, adding bounds checks would improve robustness.🛡️ Optional: Add array size validation
+ jsize dispFreqLen = (*env)->GetArrayLength(env, jdispFreq); + jsize responseLen = (*env)->GetArrayLength(env, jresponse); + if (nPts > dispFreqLen || nPts > responseLen) + nPts = dispFreqLen < responseLen ? (int)dispFreqLen : (int)responseLen; + const double sampleRate = srate > 0 ? (double)srate : 1.0; for (int i = 0; i < nPts; i++)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/cpp/libjdspimptoolbox/main/JdspImpResToolbox.c` at line 837, Validate that the provided nPts does not exceed the actual Java array lengths before the for (int i = 0; i < nPts; i++) loop: use the JNI GetArrayLength results for jdispFreq and jresponse, compare them against nPts, and if they don’t match (or nPts is larger) short-circuit by throwing a Java IllegalArgumentException or returning an error code and avoid running the loop; update the code paths that reference jdispFreq and jresponse (the same pattern used in ComputeEqResponse) to use the validated length to prevent out-of-bounds access.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@app/src/main/cpp/libjdspimptoolbox/main/JdspImpResToolbox.c`:
- Line 837: Validate that the provided nPts does not exceed the actual Java
array lengths before the for (int i = 0; i < nPts; i++) loop: use the JNI
GetArrayLength results for jdispFreq and jresponse, compare them against nPts,
and if they don’t match (or nPts is larger) short-circuit by throwing a Java
IllegalArgumentException or returning an error code and avoid running the loop;
update the code paths that reference jdispFreq and jresponse (the same pattern
used in ComputeEqResponse) to use the validated length to prevent out-of-bounds
access.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/src/main/cpp/libjamesdsp-wrapper/clarity/ClarityProcessor.cpp`:
- Around line 124-126: In NoiseSharpening::reset(), the computed cutoff can
become exactly 0 (when nyquistOffsetHz >= samplingRate*0.5) which will make the
LPF coefficients freeze the output at prevSample (silence); clamp the cutoff to
a small positive floor instead of allowing 0. For example, compute the raw
cutoff from samplingRate and nyquistOffsetHz as now, then replace any value <= 0
with a tiny positive constant (e.g. 1e-3f or std::numeric_limits<float>::min())
before using it to compute LPF coefficients and reset prevSample/state so the
filter never becomes a 0 Hz integrator; update the cutoff assignment in
NoiseSharpening::reset() accordingly.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/src/main/cpp/libjamesdsp-wrapper/clarity/ClarityProcessor.cpp`:
- Around line 142-147: The inlined IIR update in ClarityProcessor.cpp uses the
input (xIn) for both a1 and b1 terms, breaking the filter transfer; update the
state to match IIR1::process by setting filters[ch].prevSample = out *
filters[ch].a1 + xIn * filters[ch].b1 (i.e., use the computed output variable
"out" for the a1 term and keep xIn for the b1 term) so the prevSample update
matches the IIR1 implementation and preserves the correct transfer function.
Comment on lines
142
to
147
| const float xIn = sample + (sample - prev) * gain; | ||
| IIR1& filter = filters[ch]; | ||
| const float hist = xIn * filter.b1; | ||
| const float out = filter.prevSample + xIn * filter.b0; | ||
| filter.prevSample = xIn * filter.a1 + hist; | ||
| buffer[idx] = out; |
There was a problem hiding this comment.
Incorrect IIR filter state update breaks filter transfer function.
The inlined IIR filter logic has a bug in the state update. Comparing with IIR1::process (lines 30-35):
- Original:
prevSample = output * a1 + input * b1 - Current:
prevSample = xIn * a1 + xIn * b1(uses input instead of output)
This changes the filter's transfer function, producing incorrect audio processing results.
🐛 Proposed fix
const float xIn = sample + (sample - prev) * gain;
IIR1& filter = filters[ch];
const float hist = xIn * filter.b1;
const float out = filter.prevSample + xIn * filter.b0;
- filter.prevSample = xIn * filter.a1 + hist;
+ filter.prevSample = out * filter.a1 + hist;
buffer[idx] = out;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/src/main/cpp/libjamesdsp-wrapper/clarity/ClarityProcessor.cpp` around
lines 142 - 147, The inlined IIR update in ClarityProcessor.cpp uses the input
(xIn) for both a1 and b1 terms, breaking the filter transfer; update the state
to match IIR1::process by setting filters[ch].prevSample = out * filters[ch].a1
+ xIn * filters[ch].b1 (i.e., use the computed output variable "out" for the a1
term and keep xIn for the b1 term) so the prevSample update matches the IIR1
implementation and preserves the correct transfer function.
JadenJSJ
force-pushed
the
feat/improve-viper-behavior-parity-dsp
branch
from
f1c05a4 to
41b19d5
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Improvements
Documentation