feat: add optional real-time frequency histogram visualizer#64
Conversation
There was a problem hiding this comment.
SMoC PR #64 Code Review
First of all, outstanding work on this feature! The real-time frequency visualizer looks phenomenal, and the implementation is highly creative. The integration of SoundFlow FFT spectrum data, vertical color gradients, logarithmic binning, and smooth physics-like transitions provides a highly polished premium feel to the TUI.
Here is a senior-level detailed review focusing on performance, correctness, thread safety, rendering fidelity, and test determinism to help elevate this contribution to production grade.
1. Performance & Memory Allocations (High Impact)
Issue: Excessive GC Allocations in Real-Time Audio Update Loop
Inside SoundFlowPlaybackService.cs's SpectrumData property:
public float[] SpectrumData {
get {
if (_spectrumAnalyzer == null) return Array.Empty<float>();
float[] raw = _spectrumAnalyzer.SpectrumData;
if (raw == null || raw.Length == 0) return Array.Empty<float>();
float peak = _levelMeterAnalyzer?.Peak ?? 1.0f;
float[] scaled = new float[raw.Length]; // <-- Allocates a new array on every access!
for (int i = 0; i < raw.Length; i++) {
scaled[i] = raw[i] * peak;
}
return scaled;
}
}- Why it matters:
SpectrumDatais called every 100ms insideFrequencyHistogramView.UpdateVisualizationduring active playback. This creates a continuous stream of allocations (~2-4 KB per second) which creates garbage collector pressure over extended music playing sessions. - Senior Recommendation: Redesign the interface to allow zero-allocation updates by reusing a single buffer.
- Option A: Implement a method that copies into an existing buffer:
public void GetSpectrumData(Span<float> destination)orpublic int GetSpectrumData(float[] destination). - Option B: Cache and reuse a single internal private array buffer
_cachedSpectrumDatainsideSoundFlowPlaybackServiceand only resize it ifraw.Lengthchanges, returning a read-only span or the same array (since the UI thread copies its values anyway).
- Option A: Implement a method that copies into an existing buffer:
2. Bugs & Rendering Fidelity
Issue: Missing Unicode Lower One-Eighth Block Character
In FrequencyHistogramView.cs, the switch expression maps the lowest bar height 1 to a standard ASCII space ' ':
char blockChar = cellLevel switch {
1 => ' ', // <-- standard ASCII space U+0020
2 => '▂', // U+2582
3 => '▃', // U+2583
...- Why it matters: Standard ASCII space
' 'is identical tocellLevel <= 0, which means you lose the finest 1/8th height resolution of the bar. It will jump abruptly from 0/8 (completely empty) to 2/8 (▂). - Senior Recommendation: Use the actual Unicode Lower One Eighth Block character
' '(U+2581) for case1:1 => ' ', // <-- U+2581 Lower One Eighth Block
3. UI Layout & Visibility Dependencies
Issue: Referencing Layout Dimensions of an Invisible View
In NowPlayingView.cs, _histogramView's layout is dynamically bound to _albumArtView:
_histogramView = new FrequencyHistogramView(playbackQueueService) {
...
Width = Dim.Func((v) => _albumArtView.Frame.Width),
Height = Dim.Func((v) => _albumArtView.Frame.Height),
Visible = false
};And during visualization toggle:
private void ToggleVisualization() {
_showVisualization = !_showVisualization;
_albumArtView.Visible = !_showVisualization; // <-- Becomes hidden
_histogramView.Visible = _showVisualization; // <-- Becomes visible
...
}- Why it matters: In Terminal.Gui (and many layout engines), setting a view's visibility to
falsecan bypass its layout pass or collapse its frame size to0. If this occurs,_histogramView's width and height (which depend on_albumArtView.Frame.Width/Height) can collapse to0, making the visualizer invisible. - Senior Recommendation: Avoid defining layout constraints based on a peer view that will be hidden. Instead:
- Bind both
_albumArtViewand_histogramViewto a shared helper method, layout configuration, or parent percentage bounds so they resolve independently of each other's visibility. - Alternatively, keep
_albumArtViewvisible with 0-opacity/no rendering, or simply bind_histogramViewusing the exact sameDim.Funclogic used for_albumArtView.
- Bind both
4. Test Suite Determinism & Robustness
Issue: Flaky UI Screenshot Tests on Real-Time Timers
The test suite in FrequencyHistogramViewTest.cs uses context.AdvanceTime(TimeSpan.FromMilliseconds(100)) to trigger and test the visualization updates.
- Why it matters: Real-time timers and main-loop clocks can be highly non-deterministic on different host environments (e.g., local machines vs slow CI/CD containers). Minor timing drifts or scheduling latencies in the event loop can result in a different number of timeout executions, leading to flaky test runs (as seen in our local run where procedural animations yielded different visual heights).
- Senior Recommendation:
- Avoid real-time main loops or timing dependencies in unit tests.
- Instead of invoking the main loop clock via
AdvanceTime, expose an internalUpdate()orUpdateVisualization()method directly on the view (or via a test interface) to allow the test suite to step through visual frames with 100% mathematical determinism. - To ensure colors in screenshot goldens are identical across all developer environments, force color-depth settings inside
TestInit.csby explicitly configuring theCOLORTERMenvironment variable:Environment.SetEnvironmentVariable("COLORTERM", ""); // Force standard 16-color ANSI output
5. Concurrency Safety
Issue: Concurrency in Spectrum Copying
Audio playback rendering runs on the hardware's separate audio callback thread, while spectrum drawing runs on Terminal.Gui's UI main thread.
- Why it matters: The
rawarray reference accessed via_spectrumAnalyzer.SpectrumDatamight be replaced or mutated concurrently. Under high load or when track changes occur, this can lead to out-of-bounds array reads if the analyzer is reconfigured. - Senior Recommendation: Ensure that SoundFlow's spectrum buffer access is thread-safe and properly locked/copied atomically. Since we copy it locally, we are safe from intermediate UI changes, but verifying that the underlying audio framework guarantees safe access to
SpectrumDataduring playback is highly recommended.
oca-agent
left a comment
There was a problem hiding this comment.
Re-approving PR per user request.
This pull request introduces an optional, dynamic, real-time frequency histogram/equalizer to the "Now Playing" screen in
smoc.Key Contributions:
SpectrumAnalyzerandLevelMeterAnalyzerinSoundFlowPlaybackService.Viewwith high-resolution vertical rendering using 8-level Unicode blocks (,▂,▃,▄,▅,▆,▇,█).minBin = 1) to completely prevent the leftmost channel from pegging.col + 1) to ensure perfect vertical bar spacing and prevent any terminal residual artifacts.4.5fand optimized edge weighting to at most70.4%to keep frequency bounds naturally active without capping.v/Vor the command:np-vis.