feat: Add high pass filter to audio player #473
Conversation
… improved interaction
|
Warning Rate limit exceeded@tphakala has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 17 minutes and 4 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughThe pull request enhances the audio player's functionality by introducing new constants for high-pass filter settings and additional utility methods for creating and managing control buttons and sliders. The audio processing chain is updated to include a high-pass filter, and keyboard controls have been added for gain and filter frequency adjustments. Additionally, the HTML layout in the list detections fragment has been slightly modified by adding a margin to the recording section. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant UI
participant SliderManager
participant AudioEngine
participant Keyboard
User->>UI: Launch Audio Player
UI->>AudioEngine: Initialize audio nodes (includes high-pass filter)
User->>UI: Click control button / Adjust slider
UI->>SliderManager: Manage slider interaction (createSliderManager)
SliderManager->>AudioEngine: Update gain/filter settings (updateGainValue/updateFilterDisplay)
Keyboard->>UI: Press key for adjustment
UI->>AudioEngine: Update corresponding audio node parameter
Possibly related PRs
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![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 (5)
assets/audioplayer.js (5)
11-14: Constants look good, consider inline documentation.These constants are self-explanatory, but adding brief comments (e.g., describing the rationale behind the 24 dB maximum) can help future maintainers quickly grasp their purpose.
58-89: Check chain order for the high-pass filter.Currently, the compressor acts after the high-pass filter. In some scenarios, you may want the compressor to process the full bandwidth before filtering. Ensure the order aligns with desired audio behavior.
96-111: Solid approach to building sliders.The ARIA attributes and vertical orientation are handled well. As a minor improvement, consider adding
aria-valueminandaria-valuemaxhere (instead of setting them externally) for clearer encapsulation.
516-549: Duplicated audio chain logic.Much of this code replicates what
createAudioNodesalready does (lines 58-89). Factor out common logic to reduce code duplication and centralize updates to audio node configuration.
551-564: Duplicate gain UI logic.This function largely mirrors logic in the
createUIUpdatersfunction (lines 264-280). Extracting a shared utility or reusing the existing method would help maintain DRY principles.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
assets/audioplayer.js(8 hunks)views/fragments/listDetections.html(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- views/fragments/listDetections.html
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: unit-tests
- GitHub Check: Analyze (go)
🔇 Additional comments (3)
assets/audioplayer.js (3)
16-20: FILTER_TYPES are well-structured.No immediate concerns. The added enum-like object is clear and extensible in case more filter types are needed in the future.
22-24: Verify default high-pass frequency range.The 20 Hz lower bound is typical, but confirm whether 20 Hz is sufficiently low for intended use cases. Some audio applications might benefit from a slightly higher default cutoff (e.g., 30 Hz).
656-679: Prevent default may disrupt page scrolling.While intercepting the wheel event is necessary for controlling gain/filter, be aware that on smaller screens, users might expect to scroll the page. Confirm that this behavior doesn’t negatively impact the overall UX.
assets/audioplayer.js
Outdated
| // Slider Interaction Setup | ||
| const createSliderInteraction = (slider, manager, updateFn) => { | ||
| const handleUpdate = (e) => { | ||
| e.preventDefault(); | ||
| manager.resetTimer(); | ||
| const sliderRect = slider.querySelector('.relative').getBoundingClientRect(); | ||
| let y = e.type.includes('touch') ? e.touches[0].clientY : e.clientY; | ||
|
|
||
| let pos = (sliderRect.bottom - y) / sliderRect.height; | ||
| pos = Math.max(0, Math.min(1, pos)); | ||
|
|
||
| return pos; | ||
| }; | ||
|
|
||
| return handleUpdate; | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Potential duplication with “setupSliderInteraction”.
This createSliderInteraction function appears to overlap with setupSliderInteraction (lines 185-211). Consider unifying or removing one to avoid confusion and improve maintainability.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
assets/audioplayer.js (1)
369-384:⚠️ Potential issueRemove duplicate slider interaction logic.
The
createSliderInteractionfunction duplicates the functionality ofsetupSliderInteraction(lines 186-224). This duplication makes the code harder to maintain and increases the risk of inconsistencies.Remove the duplicate function and use
setupSliderInteractionconsistently throughout the code.
🧹 Nitpick comments (3)
assets/audioplayer.js (3)
22-24: Consider using more descriptive names for filter frequency constants.The constants could be more descriptive to better convey their purpose and units:
-const FILTER_HP_DEFAULT_FREQ = 20; // Default highpass filter frequency -const FILTER_HP_MIN_FREQ = 20; // Minimum highpass filter frequency -const FILTER_HP_MAX_FREQ = 10000; // Maximum highpass filter frequency +const FILTER_HP_DEFAULT_FREQ_HZ = 20; // Default highpass filter frequency in Hertz +const FILTER_HP_MIN_FREQ_HZ = 20; // Minimum highpass filter frequency in Hertz +const FILTER_HP_MAX_FREQ_HZ = 10000; // Maximum highpass filter frequency in Hertz
58-89: Consider modularizing the audio nodes creation.The
createAudioNodesfunction could be split into smaller, more focused functions for better maintainability:const createAudioNodes = (audioContext, audio) => { - const audioSource = audioContext.createMediaElementSource(audio); - const gainNode = audioContext.createGain(); - gainNode.gain.value = 1; - - const highPassFilter = audioContext.createBiquadFilter(); - highPassFilter.type = 'highpass'; - highPassFilter.frequency.value = FILTER_HP_DEFAULT_FREQ; - highPassFilter.Q.value = 1; - - const compressor = audioContext.createDynamicsCompressor(); - compressor.threshold.value = -24; - compressor.knee.value = 30; - compressor.ratio.value = 12; - compressor.attack.value = 0.003; - compressor.release.value = 0.25; + const audioSource = createAudioSource(audioContext, audio); + const gainNode = createGainNode(audioContext); + const highPassFilter = createHighPassFilter(audioContext); + const compressor = createCompressor(audioContext); audioSource .connect(highPassFilter) .connect(gainNode) .connect(compressor) .connect(audioContext.destination); return { source: audioSource, gain: gainNode, compressor, filters: { highPass: highPassFilter } }; }; + +const createAudioSource = (audioContext, audio) => { + return audioContext.createMediaElementSource(audio); +}; + +const createGainNode = (audioContext) => { + const gainNode = audioContext.createGain(); + gainNode.gain.value = 1; + return gainNode; +}; + +const createHighPassFilter = (audioContext) => { + const filter = audioContext.createBiquadFilter(); + filter.type = 'highpass'; + filter.frequency.value = FILTER_HP_DEFAULT_FREQ; + filter.Q.value = 1; + return filter; +}; + +const createCompressor = (audioContext) => { + const compressor = audioContext.createDynamicsCompressor(); + compressor.threshold.value = -24; + compressor.knee.value = 30; + compressor.ratio.value = 12; + compressor.attack.value = 0.003; + compressor.release.value = 0.25; + return compressor; +};
649-671: Consider debouncing the wheel event handler.The wheel event can fire rapidly, potentially causing performance issues. Consider debouncing the handler:
+const debounce = (fn, delay) => { + let timeoutId; + return (...args) => { + if (timeoutId) { + clearTimeout(timeoutId); + } + timeoutId = setTimeout(() => { + fn.apply(null, args); + }, delay); + }; +}; + +const handleWheel = (e) => { + if (gainManager.isActive() || filterManager.isActive()) { + e.preventDefault(); + + if (gainManager.isActive()) { + // ... existing gain manager code ... + } else if (filterManager.isActive()) { + // ... existing filter manager code ... + } + } +}; + -spectrogramContainer.addEventListener('wheel', (e) => { - // ... existing code ... -}, { passive: false }); +spectrogramContainer.addEventListener('wheel', debounce(handleWheel, 50), { passive: false });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
assets/audioplayer.js(8 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: unit-tests
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (go)
🔇 Additional comments (1)
assets/audioplayer.js (1)
96-129: Well-implemented UI creation functions with proper accessibility!The functions make good use of ARIA attributes and follow accessibility best practices.
| const resizeObserver = new ResizeObserver(() => { | ||
| updateControlsVisibility(); | ||
| }); | ||
| resizeObserver.observe(spectrogramContainer); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider cleaning up the ResizeObserver.
The ResizeObserver should be disconnected when it's no longer needed to prevent memory leaks.
const resizeObserver = new ResizeObserver(() => {
updateControlsVisibility();
});
resizeObserver.observe(spectrogramContainer);
+
+// Clean up the observer when the audio player is destroyed
+const cleanup = () => {
+ resizeObserver.disconnect();
+};
+
+// Add cleanup to the audio element for later use
+audio.cleanup = cleanup;📝 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.
| const resizeObserver = new ResizeObserver(() => { | |
| updateControlsVisibility(); | |
| }); | |
| resizeObserver.observe(spectrogramContainer); | |
| const resizeObserver = new ResizeObserver(() => { | |
| updateControlsVisibility(); | |
| }); | |
| resizeObserver.observe(spectrogramContainer); | |
| // Clean up the observer when the audio player is destroyed | |
| const cleanup = () => { | |
| resizeObserver.disconnect(); | |
| }; | |
| // Add cleanup to the audio element for later use | |
| audio.cleanup = cleanup; |
This PR adds high pass filter for audio player controls, high pass filter allows to remove unwanted rumble from audio clips at playback time