feat(dsp): add experimental switch for lower and finer buffer size control#3
feat(dsp): add experimental switch for lower and finer buffer size control#3
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ 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: 1
🧹 Nitpick comments (1)
app/src/main/java/me/timschneeberger/rootlessjamesdsp/service/RootlessAudioProcessorService.kt (1)
425-431: Consider usingmaxOffor cleaner buffer size clamping.The buffer size clamping logic is correct, but could be simplified. Additionally, the magic numbers
2and128are duplicated inSettingsAudioFormatFragment.ktas constants. Consider extracting these to a shared location to prevent drift.♻️ Suggested simplification
val allowLowSamples = preferences.get<Boolean>(R.string.key_audioformat_allow_low_samples) val minBufferSize = if (allowLowSamples) 2 else 128 val requestedBufferSize = preferences.get<Float>(R.string.key_audioformat_buffersize).toInt() -var bufferSize = if (requestedBufferSize < minBufferSize) minBufferSize else requestedBufferSize +var bufferSize = maxOf(requestedBufferSize, minBufferSize) if ((bufferSize % 2) != 0) { bufferSize += 1 }🤖 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/service/RootlessAudioProcessorService.kt` around lines 425 - 431, Replace the manual clamp and evenness logic with a clearer maxOf-based clamp and centralize the magic numbers: use allowLowSamples, requestedBufferSize and bufferSize in RootlessAudioProcessorService.kt to set bufferSize = maxOf(requestedBufferSize, MIN_BUFFER_SIZE) (then ensure evenness), where MIN_BUFFER_SIZE and DEFAULT_MIN_BUFFER_SIZE_LOW (or similarly named constants) are pulled from a shared constants location (the same shared constants used instead of the duplicated 2 and 128 in SettingsAudioFormatFragment.kt) so both files reference the single source of truth.
🤖 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/java/me/timschneeberger/rootlessjamesdsp/fragment/settings/SettingsAudioFormatFragment.kt`:
- Around line 119-122: When allowLowSamples is toggled the UI updates via
applyBufferSizeRange(allowLowSamples) but the running audio service is not
restarted; update the allowLowSamples?.setOnPreferenceChangeListener block to
also send the same hard-reboot intent used for bufferSize and encoding changes
(ACTION_SERVICE_HARD_REBOOT_CORE) so the service re-reads the preference (the
service currently reads it in startRecording()); locate allowLowSamples and
applyBufferSizeRange in SettingsAudioFormatFragment and add the
intent-broadcast/Service restart call identical to the existing handlers for
bufferSize/encoding.
---
Nitpick comments:
In
`@app/src/main/java/me/timschneeberger/rootlessjamesdsp/service/RootlessAudioProcessorService.kt`:
- Around line 425-431: Replace the manual clamp and evenness logic with a
clearer maxOf-based clamp and centralize the magic numbers: use allowLowSamples,
requestedBufferSize and bufferSize in RootlessAudioProcessorService.kt to set
bufferSize = maxOf(requestedBufferSize, MIN_BUFFER_SIZE) (then ensure evenness),
where MIN_BUFFER_SIZE and DEFAULT_MIN_BUFFER_SIZE_LOW (or similarly named
constants) are pulled from a shared constants location (the same shared
constants used instead of the duplicated 2 and 128 in
SettingsAudioFormatFragment.kt) so both files reference the single source of
truth.
...in/java/me/timschneeberger/rootlessjamesdsp/fragment/settings/SettingsAudioFormatFragment.kt
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
app/src/main/java/me/timschneeberger/rootlessjamesdsp/fragment/settings/SettingsAudioFormatFragment.kt (1)
138-148: Potential unnecessary service restart on settings screen open.
applyBufferSizeRangeis called duringonCreatePreferences(line 111). If the stored buffer size happens to be below 128 whileallowLowSamplesis false (e.g., from a previous session with the flag enabled), this will triggerACTION_SERVICE_HARD_REBOOT_COREevery time the settings screen is opened, even if the service has already applied the corrected value.Consider adding a flag or check to avoid broadcasting the reboot intent during initial preference loading, or only send the broadcast when the value actually changes from user interaction.
♻️ Proposed fix
- private fun applyBufferSizeRange(allowLow: Boolean) { + private fun applyBufferSizeRange(allowLow: Boolean, sendBroadcast: Boolean = true) { if (allowLow) { return } val currentValue = preferences.get<Float>(R.string.key_audioformat_buffersize) if(currentValue < Constants.AUDIO_BUFFER_MIN_DEFAULT) { bufferSize?.setValue(Constants.AUDIO_BUFFER_MIN_DEFAULT.toFloat()) - context?.sendLocalBroadcast(Intent(Constants.ACTION_SERVICE_HARD_REBOOT_CORE)) + if (sendBroadcast) { + context?.sendLocalBroadcast(Intent(Constants.ACTION_SERVICE_HARD_REBOOT_CORE)) + } } }Then update the initial call:
- applyBufferSizeRange(preferences.get<Boolean>(R.string.key_audioformat_allow_low_samples)) + applyBufferSizeRange(preferences.get<Boolean>(R.string.key_audioformat_allow_low_samples), sendBroadcast = false)🤖 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/settings/SettingsAudioFormatFragment.kt` around lines 138 - 148, applyBufferSizeRange currently always sends ACTION_SERVICE_HARD_REBOOT_CORE when it corrects the stored buffer size during onCreatePreferences, causing a reboot every time settings open; modify applyBufferSizeRange (and its callers) to accept a boolean like suppressBroadcast or to take an explicit "isInitialLoad" flag and only send the broadcast when the buffer size is actually changed by user action (i.e., compare previousValue from preferences.get<Float>(...) to the corrected value and call context?.sendLocalBroadcast(...) only if previousValue != correctedValue and suppressBroadcast is false), and ensure the initial call from onCreatePreferences passes suppressBroadcast=true so opening the settings won't trigger a reboot.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
app/src/main/java/me/timschneeberger/rootlessjamesdsp/fragment/settings/SettingsAudioFormatFragment.ktapp/src/main/java/me/timschneeberger/rootlessjamesdsp/service/RootlessAudioProcessorService.ktapp/src/main/java/me/timschneeberger/rootlessjamesdsp/utils/Constants.ktapp/src/main/res/values/defaults.xmlapp/src/main/res/values/keys.xmlapp/src/main/res/values/strings.xmlapp/src/main/res/xml/app_audio_format_preferences.xmlbuildError.txt
🤖 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/res/xml/app_audio_format_preferences.xml`:
- Around line 16-26: The seekbar still allows sliding to values below the
effective minimum because applyBufferSizeRange() only clamps stored values;
update applyBufferSizeRange(allowLow: Boolean) to call bufferSize?.setMin(...)
(using Constants.AUDIO_BUFFER_MIN_LOW or Constants.AUDIO_BUFFER_MIN_DEFAULT) so
the UI minimum updates, then read the current value via preferences.get(...) and
if it's below the new min call bufferSize?.setValue(...) and send the reboot
broadcast Intent(Constants.ACTION_SERVICE_HARD_REBOOT_CORE); this uses
MaterialSeekbarPreference.setMin(), bufferSize, preferences.get,
bufferSize.setValue, and the ACTION_SERVICE_HARD_REBOOT_CORE constant.
---
Nitpick comments:
In
`@app/src/main/java/me/timschneeberger/rootlessjamesdsp/fragment/settings/SettingsAudioFormatFragment.kt`:
- Around line 138-148: applyBufferSizeRange currently always sends
ACTION_SERVICE_HARD_REBOOT_CORE when it corrects the stored buffer size during
onCreatePreferences, causing a reboot every time settings open; modify
applyBufferSizeRange (and its callers) to accept a boolean like
suppressBroadcast or to take an explicit "isInitialLoad" flag and only send the
broadcast when the buffer size is actually changed by user action (i.e., compare
previousValue from preferences.get<Float>(...) to the corrected value and call
context?.sendLocalBroadcast(...) only if previousValue != correctedValue and
suppressBroadcast is false), and ensure the initial call from
onCreatePreferences passes suppressBroadcast=true so opening the settings won't
trigger a reboot.
| app:seekBarIncrement="2" | ||
| app:precision="0" | ||
| app:unit="@string/audio_format_buffer_size_unit" | ||
| app:labelMinWidth="120dp" | ||
| app:minValue="128" | ||
| app:minValue="2" | ||
| app:maxValue="16384" | ||
| app:defaultValue="@integer/default_audioformat_buffersize" | ||
| app:allowDividerAbove="false" | ||
| app:updatesContinuously="false" | ||
| app:showSeekBarValue="true" | ||
| app:iconSpaceReserved="false"/> |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, find the XML file to confirm the hardcoded values
find . -name "app_audio_format_preferences.xml" -type fRepository: JSJ-Experiments/RootlessJamesDSP
Length of output: 131
🏁 Script executed:
# Find the SettingsAudioFormatFragment to verify the clamping logic
find . -name "SettingsAudioFormatFragment.kt" -type fRepository: JSJ-Experiments/RootlessJamesDSP
Length of output: 180
🏁 Script executed:
# Search for MaterialSeekbarPreference implementation
find . -name "*MaterialSeekbar*" -o -name "*Seekbar*" | grep -E "\.(kt|java)$"Repository: JSJ-Experiments/RootlessJamesDSP
Length of output: 171
🏁 Script executed:
# Read the XML file to confirm the hardcoded values
cat -n ./app/src/main/res/xml/app_audio_format_preferences.xmlRepository: JSJ-Experiments/RootlessJamesDSP
Length of output: 4868
🏁 Script executed:
# Read the SettingsAudioFormatFragment to check for change listeners and clamping
wc -l ./app/src/main/java/me/timschneeberger/rootlessjamesdsp/fragment/settings/SettingsAudioFormatFragment.ktRepository: JSJ-Experiments/RootlessJamesDSP
Length of output: 184
🏁 Script executed:
# Read the MaterialSeekbarPreference to see if minValue can be set dynamically
cat -n ./app/src/main/java/me/timschneeberger/rootlessjamesdsp/preference/MaterialSeekbarPreference.ktRepository: JSJ-Experiments/RootlessJamesDSP
Length of output: 21634
🏁 Script executed:
# Read the SettingsAudioFormatFragment to find the change listener and applyBufferSizeRange method
cat -n ./app/src/main/java/me/timschneeberger/rootlessjamesdsp/fragment/settings/SettingsAudioFormatFragment.ktRepository: JSJ-Experiments/RootlessJamesDSP
Length of output: 8635
UI allows selecting invalid buffer sizes when "Allow low buffer sizes" is disabled.
The seekbar is configured with minValue="2" in XML, but when the toggle is off, the fragment's change listener (lines 115-118 in SettingsAudioFormatFragment.kt) clamps values below 128. Users can slide to invalid values like 64 only to have them silently adjusted to 128.
The applyBufferSizeRange() method currently only clamps the stored value but doesn't update the seekbar's visible minimum. Since MaterialSeekbarPreference exposes a public setMin() method that triggers UI updates via notifyChanged(), you should call it to reflect the effective range:
♻️ Update applyBufferSizeRange() to dynamically set seekbar minimum
private fun applyBufferSizeRange(allowLow: Boolean) {
val minValue = if (allowLow) {
Constants.AUDIO_BUFFER_MIN_LOW.toFloat()
} else {
Constants.AUDIO_BUFFER_MIN_DEFAULT.toFloat()
}
bufferSize?.setMin(minValue)
val currentValue = preferences.get<Float>(R.string.key_audioformat_buffersize)
if(currentValue < minValue) {
bufferSize?.setValue(minValue)
context?.sendLocalBroadcast(Intent(Constants.ACTION_SERVICE_HARD_REBOOT_CORE))
}
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/src/main/res/xml/app_audio_format_preferences.xml` around lines 16 - 26,
The seekbar still allows sliding to values below the effective minimum because
applyBufferSizeRange() only clamps stored values; update
applyBufferSizeRange(allowLow: Boolean) to call bufferSize?.setMin(...) (using
Constants.AUDIO_BUFFER_MIN_LOW or Constants.AUDIO_BUFFER_MIN_DEFAULT) so the UI
minimum updates, then read the current value via preferences.get(...) and if
it's below the new min call bufferSize?.setValue(...) and send the reboot
broadcast Intent(Constants.ACTION_SERVICE_HARD_REBOOT_CORE); this uses
MaterialSeekbarPreference.setMin(), bufferSize, preferences.get,
bufferSize.setValue, and the ACTION_SERVICE_HARD_REBOOT_CORE constant.
|
I guess other parts of the chain cause more latency then the processing part. i still got like 300ms worth of delay with the buffersize set to 2 |
1 participant
Summary by CodeRabbit