Fix: [Spectrogram] fix cropping and scaling issues #3796
Conversation
|
@jonom thanks for the PR! How did your tests go, did it solve those issues for you? |
|
I'm not exactly sure how the e2e tests are supposed to be ran - after a while I worked out I could run them manually, but I didn't see snapshots to compare with expected results. One thing I have found since is that setting sampleRate on the ws instance doesn't seem to affect rendering, except that if it's too small, the upper range of the spectrogram may be missing. So I've been setting it to 88200 and then it seems to always work 🤷♂️ |
|
I've commented out Spectrogram tests for now, they mysteriously always fail on GitHub Actions but work locally. |
jonom
force-pushed
the
spectrogram-fixes
branch
from
ccd8362 to
2b9bb14
Compare
WalkthroughThe pull request introduces modifications to the Spectrogram plugin in WaveSurfer.js, focusing on enhancing the frequency analysis and visualization capabilities. The changes include updating the sample rate, adjusting frequency scaling parameters, and improving the rendering logic of the spectrogram. The modifications aim to provide more flexible and accurate spectrogram generation across different audio sample rates and FFT sample sizes. Changes
Assessment against linked issues
Poem
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command 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 (
|
Credit: Lucas Theis https://github.com/lucastheis
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (7)
src/plugins/spectrogram.ts (7)
339-340: Default height of 200 px may be too small for some use cases.
If the user’s container is large, consider making the default scale more adaptive. This might also address potential visual issues.
352-352: Explain the 4x downsampling of fftSamples for Mel filters.
The new logic sets numMelFilters to fftSamples / 4. If the intention is to reduce computational load, consider documenting or making this configurable to allow different channel granularity.
463-463: Ensure canvas width is appropriately recalculated.
Calling “this.getWidth()” multiple times is fine, but be mindful of reflows if the container changes size often or if the plugin is continuously redrawn. Caching the width might be more performant.Also applies to: 468-468
555-557: Confirm safe usage of “offsetWidth”.
“offsetWidth” returns an integer that excludes margins. If the element has non-zero margins or is inside a flex container, the actual content width might be different. For more precise sizing, consider “clientWidth” or a bounding rectangle.
563-563: Defaulting “frequencyMax” to samplerate/4.
Be cautious that some usage might require the full Nyquist range. If this is truly the intended new default, highlight it in release notes or usage docs for clarity.
689-690: Protect label drawing with boundary checks.
The label position clamp is a good approach, but multiple computations might cause “flickering” if the spectrogram height changes over time. Consider caching layout data or injecting a stable reference.Also applies to: 691-692
707-707: Potential repeated measurement with “getWidth()”.
Same comment as line 463 and 468: consider storing the width in a local variable to avoid repeated layout calculations.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
examples/spectrogram.js(2 hunks)src/plugins/spectrogram.ts(9 hunks)
🔇 Additional comments (6)
src/plugins/spectrogram.ts (4)
469-469: Validate shift from samplerate/2 to samplerate/4.
The line “freqFrom = this.buffer.sampleRate / 4” could limit your spectrogram to half of the typical Nyquist range. Verify that you actually intend to skip the higher frequencies.
486-486: Offset expression might invert frequency axis unintentionally.
“(bitmapHeight / 2 - j)” suggests half-based vertical offset. Confirm if the design requires a mirrored view or if there’s a user-facing reason for the inverted indexing.
677-685: Validate frequency scale transitions.
Ensure that switching from “mel” scale to a “linear” scale preserves consistent label spacing. Also check numeric rounding for high frequencies.
250-250: Clarify “samplerate/4” usage in docstring.
The updated comment suggests setting the max frequency to “samplerate/4” to draw the “whole range” of the spectrogram. Typically, the Nyquist frequency is samplerate/2. If you intend to visualize the entire Nyquist range, consider double-checking this value and updating the docstring accordingly.
Run the following script to find all docstring references to “samplerate/4” or “samplerate/2” and confirm whether they need updating:
examples/spectrogram.js (2)
12-12: Check interplay between “sampleRate=44100” and actual audio sampling.
Browsers often ignore custom sampleRate during decoding and might resample internally. If you’re relying on 44100 for display math, confirm that the actual decoded data sample rate matches.
21-25: Helpful additions for advanced spectrogram customization.
The new options (scale, frequencyMax, frequencyMin, fftSamples, labelsBackground) give users more granular control. Just ensure your docs or example usage describe their interactions clearly (especially how “frequencyMax=8000” relates to “sampleRate=44100”).
| const bitmapHeight = pixels[0].length | ||
| const imageData = new ImageData(width, bitmapHeight) |
There was a problem hiding this comment.
Check potential out-of-bounds index.
Accessing “pixels[0]” might fail if “pixels” is empty. Ensure “frequenciesData[c]” is never empty or handle errors gracefully.
|
@katspaugh sorry for the slow reply, I didn't realise the Mel scale had been added and the fixes I supplied previously only worked for the Linear scale. I've worked with @lucastheis to update the fixes and the Mel scale should work a lot better now, being rendered at the appropriate height and also fixing a few bugs with the scale labels. If you could let me know what I would need to do to get this over the line that would be great. No rush - I'll be off until the new year now. If you're taking some time off as well, happy holidays!
|
Short description
Resolves #3663, #3140
Implementation details
I've attempted to address some issues with the spectrogram rendering creating an image of the wrong height for the target container, causing it to be too tall or short to fill the area, and meaning the frequency labels do not line up with data in the image. There are some magic numbers in here (E.g. division by 2 or 4) I don't understand so appreciate if someone else can vet this!
(Marked as draft for now as I'm running tests locally but it's taking quite a while.)
Separate to this I've implemented some caching of the result of
getFrequencies()as well as the rendered bitmaps as this can be quite slow, and for my use case we allow zooming in and out so I wanted to speed up the rendering. If this is something you'd like included I can add to this PR or open another one later.How to test it
Screenshots
Checklist
Summary by CodeRabbit