Fix incorrect y-axis labeling of spectrogram #4011
Conversation
|
Warning Rate limit exceeded@lucastheis has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 16 minutes and 49 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 ignored due to path filters (1)
📒 Files selected for processing (2)
WalkthroughThis pull request introduces comprehensive improvements to the spectrogram functionality in WaveSurfer. The changes focus on frequency scaling and visualization, updating the Changes
Sequence DiagramsequenceDiagram
participant Test as Cypress Test
participant WaveSurfer as WaveSurfer
participant Spectrogram as SpectrogramPlugin
Test->>WaveSurfer: Initialize with scale type
WaveSurfer->>Spectrogram: Configure plugin
Spectrogram-->>WaveSurfer: Apply frequency scaling
WaveSurfer-->>Test: Render spectrogram
Test->>Test: Validate snapshot
Possibly related PRs
Suggested reviewers
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 (
|
lucastheis
changed the title
![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 (3)
src/plugins/spectrogram.ts (2)
387-392: Reevaluate default filter sizes
AllocatingfftSamples / 2filters for mel/log/bark/erb might be excessive for some use cases and can impact performance. Consider making these configurable or adjusting them based on real-world scenarios.
545-551: Minor comment typo
"freqwMax" should likely be "freqMax."- // Only relevant if `freqwMax > freqFrom` + // Only relevant if `freqMax > freqFrom`cypress/e2e/spectrogram.cy.js (1)
86-114: Solid coverage of scale tests
Iterating through multiple scale types is a great testing approach. However, note the'erbs'vs.'erb'issue, which can invalidate the intended test.Would you like assistance in harmonizing the test suite with the plugin’s recognized scale types and possibly opening a new issue for extended scale support?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
examples/audio/1khz.mp3is excluded by!**/*.mp3
📒 Files selected for processing (2)
cypress/e2e/spectrogram.cy.js(2 hunks)src/plugins/spectrogram.ts(8 hunks)
🔇 Additional comments (12)
src/plugins/spectrogram.ts (12)
258-258: Documentation alignment
The updated docstring clarifies thatfrequencyMaxshould be set to the Nyquist frequency (samplerate/2), which aligns well with the revised scaling logic.
509-510: Clear statement on raw data range
The comment succinctly clarifies that the rawfrequenciesDatacovers up to half the sample rate (Nyquist).
512-513: Straightforward min/max handling
ReferencingfrequencyMinandfrequencyMaxhere is direct and clear, aiding readability.
521-526: Background fill beyond Nyquist
Filling the unused canvas area with the last colormap color is a reasonable solution iffreqMaxexceeds the actual data range.
537-537: Vertical indexing
Subtracting frombitmapHeight - 1flips the rows correctly, ensuring the top row corresponds to higher frequencies.
666-678: Implicit linear scale
Falling back toreturn hzwhenthis.scaleis neither'mel','logarithmic','bark', nor'erb'effectively handles the'linear'case.
680-692: Consistent inverse scaling
ThescaleToHzmethod mirrors the logic inhzToScale, ensuring proper two-way conversion for each scale.
713-713: Sensible default for frequencyMax
Defaulting tobuffer.sampleRate / 2aligns with standard audio processing assumptions around Nyquist frequency.
796-798: Accurate label interpolation
Interpolating betweenscaleMinandscaleMaxin scale-space ensures frequency labels match the selected scale.
851-851: Good use of getLabelFrequency
This leverages your new scale logic for frequency labeling. No issues here.
556-558: Validate cropping height
Ensure that(rMax1 - rMin)remains non-negative—otherwise, the cropped bitmap could have an invalid height.✅ Verification successful
Height calculation is mathematically guaranteed to be non-negative
SincerMinis derived fromfreqMinandrMax1is clamped to be ≤ 1, the expression(rMax1 - rMin)is guaranteed to be non-negative due to the monotonic nature of the frequency scaling function.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Investigate any usage pattern of rMin/rMax1 that might produce negative dimensions: rg --context 5 'rMax1|rMin'Length of output: 3851
560-566: Avoid potential division by zero
rMaxcan become zero or negative iffreqMax≤ 0. Consider clamping or adding a guard to prevent runtime errors.
Short description
Unfortunately the y-axis labels of the spectrogram are still incorrect. I am mostly to blame as I had used and sent some faulty test files to @jonom (#3796) such that the linear labels ended up off by a factor of 2.
This PR also improves the implementation for nonlinear scales. The previous approach did not seem to work unless
frequencyMaxmatches half the buffer's sampling rate.This PR's implementation still does not work when
numMelFilters(ornumLogFilters, etc.) is different fromfftSamples / 2, so I changed these values for now so that the spectrogram and labels are aligned.Implementation details
frequencyMin/frequencyMax)How to test it
I added
examples/audio/1khz.mp3and a basic Cypress test to check if the 1 kHz label is where you'd want it.Screenshots
Before:
1 kHz,
scale: linear,frequencyMax: 5000,fftSamples: 1024,height: 2561 kHz,
scale: mel,frequencyMax: 5000,fftSamples: 1024,height: 2561 kHz,
scale: erb,frequencyMax: 5000,fftSamples: 1024,height: 2568 kHz,
scale: linear,frequencyMax: 20000,fftSamples: 1024,height: 2561 kHz,
scale: linear,frequencyMax: 5000,fftSamples: 1024,height: 256,frequencyMin: 500After:
1 kHz,
scale: linear,frequencyMax: 5000,fftSamples: 1024,height: 2561 kHz,
scale: mel,frequencyMax: 5000,fftSamples: 1024,height: 2561 kHz,
scale: erb,frequencyMax: 5000,fftSamples: 1024,height: 2568 kHz,
scale: linear,frequencyMax: 20000,fftSamples: 1024,height: 2561 kHz,
scale: linear,frequencyMax: 5000,fftSamples: 1024,height: 256,frequencyMin: 500Checklist
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Tests