Add a line per channel on spectral density graph #265
Conversation
WalkthroughThis pull request involves significant changes to the audio frequency analysis functionality within the OrcanodeMonitor system. The Changes
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: 3
🧹 Nitpick comments (9)
OrcanodeMonitor/Core/FfmpegCoreAnalyzer.cs (2)
Line range hint
10-19: Ensure robust handling of edge cases in AnalyzeFrequencies.
Consider validating thatdatais not empty,sampleRateis positive, andchannelsis at least 1 before creating theFrequencyInfoobject. This helps avoid potential out-of-bounds or invalid arguments.
Line range hint
30-65: Enhance logging and exception-handling around FFMpeg processing.
While the exception is thrown ifFFMpegprocessing fails, consider adding logging or more descriptive error messages to ease debugging ifFFMpegor the pipeline misbehaves.OrcanodeMonitor/Core/FrequencyInfo.cs (2)
33-81: Address large memory usage for very long audio inputs.
The approach of allocatingcomplexData[ch]arrays the size of the entire sample can become large for long inputs. If memory constraints are a concern, consider chunk-based or streaming approaches for computing FFT.
83-119: Use consistent naming and documentation for silence and noise thresholds.
The code uses “MaxSilenceMagnitude” and “MinNoiseMagnitude” but does not elaborate on how they interact. Clarifying references or re-checking naming might help future maintainers grasp the silence/noise boundary logic more easily.OrcanodeMonitor/Pages/SpectralDensity.cshtml (2)
31-43: Avoid possible label collisions for multi-channel datasets.
Multiple channel lines on the same chart can unintentionally overlap visually if they share similar color or label styles. Consider varying colors or using a legend to distinguish channels in a more user-friendly manner.
90-115: Prevent confusion in the channel statistics block.
The loop at lines 103-114 prints multiple channel stats, each preceded only by “Channel @i”. Adding an offset (like “Channel @i+1”), or a heading for each channel can improve clarity when more than one channel is present.OrcanodeMonitor/Pages/SpectralDensity.cshtml.cs (3)
4-4: Remove unused imports.The following imports appear to be unused and can be safely removed:
-using Microsoft.CodeAnalysis; -using System.Diagnostics; -using System.Linq.Expressions;Also applies to: 8-8, 9-9
86-96: Consider optimizing bucket magnitude lookup.The current implementation uses a linear search through the labels list. For better performance, consider using a dictionary to map labels to magnitudes:
-private double GetBucketMagnitude(string label, List<string> labels, List<double> magnitudes) -{ - double sum = 0; - for (int i = 0; i < labels.Count; i++) - { - if (labels[i] == label) - { - sum += magnitudes[i]; - } - } - return sum; -} +private double GetBucketMagnitude(string label, List<string> labels, List<double> magnitudes) +{ + var labelToMagnitudes = new Dictionary<string, double>(); + for (int i = 0; i < labels.Count; i++) + { + if (!labelToMagnitudes.ContainsKey(labels[i])) + { + labelToMagnitudes[labels[i]] = 0; + } + labelToMagnitudes[labels[i]] += magnitudes[i]; + } + return labelToMagnitudes.GetValueOrDefault(label, 0); +}
99-155: Consider breaking down the UpdateFrequencyInfo method.The method handles multiple responsibilities:
- Computing graph points
- Collecting and aligning labels
- Creating datasets
- JSON serialization
- Updating status properties
Consider splitting these into separate methods for better maintainability:
+private void ComputeGraphPoints(...) +private void CollectAndAlignLabels(...) +private void CreateDatasets(...) +private void UpdateStatusProperties(...) private void UpdateFrequencyInfo() { if (_frequencyInfo == null) { return; } ComputeGraphPoints(); CollectAndAlignLabels(); CreateDatasets(); UpdateStatusProperties(); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
OrcanodeMonitor/Core/FfmpegCoreAnalyzer.cs(1 hunks)OrcanodeMonitor/Core/FrequencyInfo.cs(1 hunks)OrcanodeMonitor/Pages/NodeEvents.cshtml(1 hunks)OrcanodeMonitor/Pages/SpectralDensity.cshtml(2 hunks)OrcanodeMonitor/Pages/SpectralDensity.cshtml.cs(7 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: build_windows (Release)
- GitHub Check: build_windows (Debug)
- GitHub Check: Analyze (javascript)
- GitHub Check: Analyze (csharp)
🔇 Additional comments (7)
OrcanodeMonitor/Core/FfmpegCoreAnalyzer.cs (1)
Line range hint
66-77: Consider guarding against invalid paths or streams in AnalyzeFileAsync and AnalyzeAudioStreamAsync.
Validate input parameters before processing in case the file path or stream is inaccessible or corrupted. This can improve resilience and debugging.OrcanodeMonitor/Core/FrequencyInfo.cs (2)
19-31: Check channel index boundaries in the constructor.
When you allocate arrays of sizechannelsand later try to populate them in loops, ensure that the value ofchannelsis always valid (≥1). Also verify thatdata.Lengthis sufficient for the expected number of channels.
245-277: Revisit the fallback logic in GetStatus.
When the magnitude falls between silent and online thresholds, the current code reverts tooldStatus. Confirm that this behavior is intended—sustained borderline audio could incorrectly remain as a prior status if not updated.OrcanodeMonitor/Pages/SpectralDensity.cshtml (1)
26-29: Confirm the correctness of Model.JsonSummaryDataset for multi-channel data.
If the summary dataset is meant to represent an aggregate or single-channel data, ensure that it is clearly documented or labeled. Otherwise, merging multi-channel data into one dataset can be confusing.OrcanodeMonitor/Pages/SpectralDensity.cshtml.cs (2)
Line range hint
54-84: LGTM! Well-implemented logarithmic bucketing with channel support.The implementation correctly handles both summary and per-channel frequency data visualization with proper logarithmic scaling.
157-170: LGTM! Well-implemented channel-specific metrics.The implementation provides a comprehensive set of per-channel metrics with proper null handling.
OrcanodeMonitor/Pages/NodeEvents.cshtml (1)
29-37: LGTM! Good improvement in status labels consistency.Converting numeric keys to strings and adding the new '-1' status label maintains consistency in the status label mapping.
Signed-off-by: Dave Thaler <[email protected]>
Signed-off-by: Dave Thaler <[email protected]>
Signed-off-by: Dave Thaler <[email protected]>
Signed-off-by: Dave Thaler <[email protected]>
Signed-off-by: Dave Thaler <[email protected]>
Signed-off-by: Dave Thaler <[email protected]>
Signed-off-by: Dave Thaler <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
OrcanodeMonitor/Pages/SpectralDensity.cshtml.cs (1)
Line range hint
54-84: Add null check and extract magic numbers as constants.The method needs the following improvements:
- Add null check for
_frequencyInfo- Extract magic numbers as named constants at class level
+private const int MAX_FREQUENCY = 24000; +private const int POINT_COUNT = 1000; private void FillInGraphPoints(List<string> labels, List<double> maxBucketMagnitudeList, int? channel = null) { - const int MaxFrequency = 24000; - const int PointCount = 1000; + if (_frequencyInfo == null) + { + return; + } // Compute the logarithmic base needed to get PointCount points. - double b = Math.Pow(MaxFrequency, 1.0 / PointCount); + double b = Math.Pow(MAX_FREQUENCY, 1.0 / POINT_COUNT); double logb = Math.Log(b); - var maxBucketMagnitude = new double[PointCount]; - var maxBucketFrequency = new int[PointCount]; + var maxBucketMagnitude = new double[POINT_COUNT]; + var maxBucketFrequency = new int[POINT_COUNT];
🧹 Nitpick comments (8)
OrcanodeMonitor/Pages/SpectralDensity.cshtml.cs (3)
4-4: Remove unused imports.The following imports appear to be unused and should be removed:
Microsoft.CodeAnalysisSystem.Linq.Expressions-using Microsoft.CodeAnalysis; -using System.Linq.Expressions;Also applies to: 9-9
86-96: Optimize bucket magnitude lookup performance.Consider using a Dictionary<string, double> for O(1) lookup instead of O(n) list search.
-private double GetBucketMagnitude(string label, List<string> labels, List<double> magnitudes) +private double GetBucketMagnitude(string label, Dictionary<string, double> magnitudesByLabel) { - double sum = 0; - for (int i = 0; i < labels.Count; i++) - { - if (labels[i] == label) - { - sum += magnitudes[i]; - } - } - return sum; + return magnitudesByLabel.TryGetValue(label, out double magnitude) ? magnitude : 0; }
99-155: Extract data processing logic into separate methods.The method is handling multiple responsibilities and could be split into smaller, focused methods:
- Computing graph points
- Collecting and processing labels
- Aligning datasets
- Updating model properties
Consider extracting these into separate private methods:
+private void ComputeGraphPoints(out List<string> summaryLabels, out List<double> summaryMaxBucketMagnitude, + out List<string>[] channelLabels, out List<double>[] channelMaxBucketMagnitude) +{ + summaryLabels = new List<string>(); + summaryMaxBucketMagnitude = new List<double>(); + FillInGraphPoints(summaryLabels, summaryMaxBucketMagnitude); + + channelLabels = new List<string>[_frequencyInfo.ChannelCount]; + channelMaxBucketMagnitude = new List<double>[_frequencyInfo.ChannelCount]; + for (int i = 0; i < _frequencyInfo.ChannelCount; i++) + { + channelLabels[i] = new List<string>(); + channelMaxBucketMagnitude[i] = new List<double>(); + FillInGraphPoints(channelLabels[i], channelMaxBucketMagnitude[i], i); + } +}OrcanodeMonitor/Pages/SpectralDensity.cshtml (2)
26-43: Differentiate channel colors for better visualization.All channels currently use the same color (rgba(75, 192, 192, 1)), making it difficult to distinguish between them. Consider using different colors for each channel.
- @: backgroundColor: 'rgba(75, 192, 192, 0.2)', - @: borderColor: 'rgba(75, 192, 192, 1)', + @: backgroundColor: GetChannelColor(@i, 0.2), + @: borderColor: GetChannelColor(@i, 1),Add this helper function to generate distinct colors:
@functions { private string GetChannelColor(int channelIndex, double alpha) { var colors = new[] { (75, 192, 192), // Teal (255, 99, 132), // Pink (54, 162, 235), // Blue (255, 206, 86), // Yellow (153, 102, 255), // Purple (255, 159, 64) // Orange }; var (r, g, b) = colors[channelIndex % colors.Length]; return $"rgba({r}, {g}, {b}, {alpha})"; } }
101-115: Use 1-based indexing for channel display.Channel numbering starts from 0, which might be confusing for users. Consider using 1-based indexing for display purposes.
- <p>Channel @i:</p> + <p>Channel @(i + 1):</p>OrcanodeMonitor/Core/FrequencyInfo.cs (3)
12-19: Improve constructor parameter documentation.The XML documentation is incomplete. Add descriptions for each parameter to improve code maintainability.
/// <summary> /// Given an audio clip, compute frequency info for it. /// </summary> -/// <param name="data"></param> -/// <param name="sampleRate"></param> -/// <param name="channels"></param> -/// <param name="oldStatus"></param> +/// <param name="data">Raw audio data as float array</param> +/// <param name="sampleRate">Audio sample rate in Hz</param> +/// <param name="channels">Number of audio channels</param> +/// <param name="oldStatus">Previous online status for hysteresis</param>
83-119: Optimize environment variable access and document magic numbers.Consider these improvements:
- Cache the environment variable values to avoid repeated lookups
- Define constants for the known threshold values mentioned in comments
+ private static readonly Lazy<double> _maxSilenceMagnitude = new(() => + { + string? maxSilenceMagnitudeString = Environment.GetEnvironmentVariable("ORCASOUND_MAX_SILENCE_MAGNITUDE"); + return double.TryParse(maxSilenceMagnitudeString, out var magnitude) ? magnitude : _defaultMaxSilenceMagnitude; + }); + + // Known threshold values from empirical testing + private const double KNOWN_UNINTELLIGIBLE_MAX_RATIO = 0.53; // 53% + private const double KNOWN_GOOD_MIN_RATIO = 1.14; // 114% + public static double MaxSilenceMagnitude { - get - { - string? maxSilenceMagnitudeString = Environment.GetEnvironmentVariable("ORCASOUND_MAX_SILENCE_MAGNITUDE"); - double maxSilenceMagnitude = double.TryParse(maxSilenceMagnitudeString, out var magnitude) ? magnitude : _defaultMaxSilenceMagnitude; - return maxSilenceMagnitude; - } + get => _maxSilenceMagnitude.Value; }
255-287: Consider breaking down status determination logic.The status determination logic is complex with multiple conditions. Consider extracting each condition into a separate method for better readability and testability:
+ private bool IsSilent(double maxMagnitude) => maxMagnitude < MinNoiseMagnitude; + + private bool ShouldMaintainSilentStatus(double maxMagnitude, OrcanodeOnlineStatus oldStatus) => + maxMagnitude <= MaxSilenceMagnitude && oldStatus == OrcanodeOnlineStatus.Silent; + + private bool IsUnintelligibleDueToLackOfSignal(double maxNonHumMagnitude) => + maxNonHumMagnitude < MinNoiseMagnitude; + + private bool IsUnintelligibleDueToHumRatio(double totalNonHumMagnitude, double totalHumMagnitude) => + totalNonHumMagnitude / totalHumMagnitude < MinSignalRatio; + private OrcanodeOnlineStatus GetStatus(OrcanodeOnlineStatus oldStatus, int? channel = null) { double max = GetMaxMagnitude(channel); - if (max < MinNoiseMagnitude) + if (IsSilent(max)) { return OrcanodeOnlineStatus.Silent; } - if ((max <= MaxSilenceMagnitude) && (oldStatus == OrcanodeOnlineStatus.Silent)) + if (ShouldMaintainSilentStatus(max, oldStatus)) { return oldStatus; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
OrcanodeMonitor/Core/FfmpegCoreAnalyzer.cs(0 hunks)OrcanodeMonitor/Core/FrequencyInfo.cs(1 hunks)OrcanodeMonitor/Pages/NodeEvents.cshtml(1 hunks)OrcanodeMonitor/Pages/SpectralDensity.cshtml(2 hunks)OrcanodeMonitor/Pages/SpectralDensity.cshtml.cs(7 hunks)
💤 Files with no reviewable changes (1)
- OrcanodeMonitor/Core/FfmpegCoreAnalyzer.cs
🚧 Files skipped from review as they are similar to previous changes (1)
- OrcanodeMonitor/Pages/NodeEvents.cshtml
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: build_windows (Release)
- GitHub Check: build_windows (Debug)
- GitHub Check: Analyze (csharp)
🔇 Additional comments (5)
OrcanodeMonitor/Pages/SpectralDensity.cshtml.cs (3)
160-170: LGTM! Well-implemented null handling.The channel-specific getter methods are well-implemented with proper null handling and default values.
181-189: LGTM! Error handling implemented as suggested.The error handling implementation follows the best practices suggested in previous reviews.
209-216: LGTM! Error handling implemented as suggested.The error handling implementation follows the best practices suggested in previous reviews.
OrcanodeMonitor/Pages/SpectralDensity.cshtml (1)
90-100: LGTM! Well-organized statistics display.The statistics are clearly organized with a proper hierarchy and good readability.
OrcanodeMonitor/Core/FrequencyInfo.cs (1)
146-150: LGTM! Safe implementation of GetSignalRatio.The implementation safely handles division by zero by using Math.Max to ensure the denominator is at least 1.
| private void ComputeFrequencyMagnitudes(float[] data, int sampleRate, int channelCount) | ||
| { | ||
| int n = data.Length / channelCount; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add input validation.
The method should validate input parameters to prevent potential issues:
- Check if data array is null or empty
- Verify that data length is divisible by channelCount
- Ensure sampleRate is positive
private void ComputeFrequencyMagnitudes(float[] data, int sampleRate, int channelCount)
{
+ if (data == null || data.Length == 0)
+ throw new ArgumentException("Audio data cannot be null or empty", nameof(data));
+ if (data.Length % channelCount != 0)
+ throw new ArgumentException("Data length must be divisible by channel count", nameof(data));
+ if (sampleRate <= 0)
+ throw new ArgumentException("Sample rate must be positive", nameof(sampleRate));
+
int n = data.Length / channelCount;📝 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.
| private void ComputeFrequencyMagnitudes(float[] data, int sampleRate, int channelCount) | |
| { | |
| int n = data.Length / channelCount; | |
| private void ComputeFrequencyMagnitudes(float[] data, int sampleRate, int channelCount) | |
| { | |
| if (data == null || data.Length == 0) | |
| throw new ArgumentException("Audio data cannot be null or empty", nameof(data)); | |
| if (data.Length % channelCount != 0) | |
| throw new ArgumentException("Data length must be divisible by channel count", nameof(data)); | |
| if (sampleRate <= 0) | |
| throw new ArgumentException("Sample rate must be positive", nameof(sampleRate)); | |
| int n = data.Length / channelCount; |
Signed-off-by: Dave Thaler <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
OrcanodeMonitor/Core/FrequencyInfo.cs (4)
19-31:⚠️ Potential issueFix array size and add input validation.
- The
StatusForChannelarray is initialized with sizechannels + 1but only indices0tochannels - 1are used.- Input parameters should be validated.
Apply these changes:
public FrequencyInfo(float[] data, int sampleRate, int channels, OrcanodeOnlineStatus oldStatus) { + if (data == null || data.Length == 0) + throw new ArgumentException("Audio data cannot be null or empty", nameof(data)); + if (data.Length % channels != 0) + throw new ArgumentException("Data length must be divisible by channel count", nameof(channels)); + if (sampleRate <= 0) + throw new ArgumentException("Sample rate must be positive", nameof(sampleRate)); + if (channels <= 0) + throw new ArgumentException("Channel count must be positive", nameof(channels)); + ChannelCount = channels; FrequencyMagnitudesForChannel = new Dictionary<double, double>[channels]; - StatusForChannel = new OrcanodeOnlineStatus[channels + 1]; + StatusForChannel = new OrcanodeOnlineStatus[channels]; FrequencyMagnitudes = new Dictionary<double, double>(); ComputeFrequencyMagnitudes(data, sampleRate, channels); Status = GetStatus(oldStatus); for (int i = 0; i < channels; i++) { StatusForChannel[i] = GetStatus(oldStatus, i); } }
19-31:⚠️ Potential issueFix array size allocation for StatusForChannel.
The array
StatusForChannelis initialized with sizechannels + 1but only indices0tochannels - 1are used.Apply this diff to fix the array size:
- StatusForChannel = new OrcanodeOnlineStatus[channels + 1]; + StatusForChannel = new OrcanodeOnlineStatus[channels];
33-35:⚠️ Potential issueAdd input validation for FFT computation.
The method should validate input parameters to prevent potential issues.
Apply this diff to add input validation:
private void ComputeFrequencyMagnitudes(float[] data, int sampleRate, int channelCount) { + if (data == null || data.Length == 0) + throw new ArgumentException("Audio data cannot be null or empty", nameof(data)); + if (data.Length % channelCount != 0) + throw new ArgumentException("Data length must be divisible by channel count", nameof(channelCount)); + if (sampleRate <= 0) + throw new ArgumentException("Sample rate must be positive", nameof(sampleRate)); + int n = data.Length / channelCount;
19-31:⚠️ Potential issueFix array size allocation and add input validation.
- The
StatusForChannelarray is initialized with an incorrect size as noted in past reviews.- Input parameters should be validated.
Apply these fixes:
public FrequencyInfo(float[] data, int sampleRate, int channels, OrcanodeOnlineStatus oldStatus) { + if (data == null || data.Length == 0) + throw new ArgumentException("Audio data cannot be null or empty", nameof(data)); + if (data.Length % channels != 0) + throw new ArgumentException("Data length must be divisible by channel count", nameof(channels)); + if (sampleRate <= 0) + throw new ArgumentException("Sample rate must be positive", nameof(sampleRate)); + if (channels <= 0) + throw new ArgumentException("Channel count must be positive", nameof(channels)); + ChannelCount = channels; FrequencyMagnitudesForChannel = new Dictionary<double, double>[channels]; - StatusForChannel = new OrcanodeOnlineStatus[channels + 1]; + StatusForChannel = new OrcanodeOnlineStatus[channels]; FrequencyMagnitudes = new Dictionary<double, double>(); ComputeFrequencyMagnitudes(data, sampleRate, channels); Status = GetStatus(oldStatus); for (int i = 0; i < channels; i++) { StatusForChannel[i] = GetStatus(oldStatus, i); } }
🧹 Nitpick comments (11)
OrcanodeMonitor/Pages/SpectralDensity.cshtml.cs (7)
52-87: Consider making frequency analysis parameters configurable.The magic numbers
MAX_FREQUENCYandPOINT_COUNTcould be moved to configuration to allow easier tuning without code changes.- private const int MAX_FREQUENCY = 24000; - private const int POINT_COUNT = 1000; + private static int MaxFrequency => int.TryParse( + Environment.GetEnvironmentVariable("ORCASOUND_MAX_FREQUENCY"), + out var freq) ? freq : 24000; + private static int PointCount => int.TryParse( + Environment.GetEnvironmentVariable("ORCASOUND_POINT_COUNT"), + out var points) ? points : 1000;
109-149: Configure JSON serialization options for better control.Consider adding JSON serialization options to control the output format and handle potential circular references.
- JsonSummaryDataset = JsonSerializer.Serialize(summaryDataset); - JsonChannelDatasets = channelDatasets.Select(dataset => JsonSerializer.Serialize(dataset)).ToList(); + var options = new JsonSerializerOptions + { + WriteIndented = false, + ReferenceHandler = ReferenceHandler.IgnoreCycles + }; + JsonSummaryDataset = JsonSerializer.Serialize(summaryDataset, options); + JsonChannelDatasets = channelDatasets.Select(dataset => + JsonSerializer.Serialize(dataset, options)).ToList();
52-54: Document the significance of the magic numbers.Consider adding XML documentation to explain:
- Why
MAX_FREQUENCYis set to 24000- Why
POINT_COUNTis set to 1000Add documentation above the constants:
+/// <summary> +/// Maximum frequency to analyze in Hz. 24kHz covers the typical human hearing range. +/// </summary> private const int MAX_FREQUENCY = 24000; +/// <summary> +/// Number of points to plot on the graph. 1000 points provides a good balance +/// between resolution and performance. +/// </summary> private const int POINT_COUNT = 1000;
160-161: Add XML documentation for JSON serialization properties.Consider adding documentation to explain the purpose and format of these properties.
Add documentation above the properties:
+/// <summary> +/// Gets or sets the JSON-serialized dataset containing summary frequency magnitudes. +/// Used by Chart.js for visualization. +/// </summary> public string JsonSummaryDataset { get; set; } +/// <summary> +/// Gets or sets the JSON-serialized datasets containing per-channel frequency magnitudes. +/// Used by Chart.js for visualization when multiple channels are present. +/// </summary> public List<string> JsonChannelDatasets { get; set; }
52-87: Consider caching the logarithmic base calculation.The logarithmic base calculation could be optimized by caching the results since
MAX_FREQUENCYandPOINT_COUNTare constants.- double b = Math.Pow(MAX_FREQUENCY, 1.0 / POINT_COUNT); - double logb = Math.Log(b); + private static readonly double _logBase = Math.Pow(MAX_FREQUENCY, 1.0 / POINT_COUNT); + private static readonly double _logBaseLog = Math.Log(_logBase); + // In FillInGraphPoints method: + double logb = _logBaseLog;
130-149: Add null checks for JSON serialization.Consider adding null checks to prevent potential
NullReferenceExceptionduring JSON serialization:- JsonSummaryDataset = JsonSerializer.Serialize(summaryDataset); - JsonChannelDatasets = channelDatasets.Select(dataset => JsonSerializer.Serialize(dataset)).ToList(); + JsonSummaryDataset = summaryDataset != null ? JsonSerializer.Serialize(summaryDataset) : "[]"; + JsonChannelDatasets = channelDatasets?.Select(dataset => + dataset != null ? JsonSerializer.Serialize(dataset) : "[]" + ).ToList() ?? new List<string>();
177-187: Add XML documentation for channel-specific methods.Consider adding XML documentation to improve code maintainability:
+ /// <summary> + /// Gets the maximum magnitude for a specific channel. + /// </summary> + /// <param name="channel">The channel index to get the magnitude for.</param> + /// <returns>The maximum magnitude for the specified channel, or 0 if no data is available.</returns> public int GetMaxMagnitude(int channel) => (int)Math.Round(_frequencyInfo?.GetMaxMagnitude(channel) ?? 0); + /// <summary> + /// Gets the maximum non-hum magnitude for a specific channel. + /// </summary> + /// <param name="channel">The channel index to get the magnitude for.</param> + /// <returns>The maximum non-hum magnitude for the specified channel, or 0 if no data is available.</returns> public int GetMaxNonHumMagnitude(int channel) => (int)Math.Round(_frequencyInfo?.GetMaxNonHumMagnitude(channel) ?? 0);OrcanodeMonitor/Core/FrequencyInfo.cs (4)
33-81: Optimize memory allocation in frequency computation.Consider pre-allocating dictionaries with capacity hints to reduce memory reallocations during population.
- FrequencyMagnitudesForChannel[ch] = new Dictionary<double, double>(); + FrequencyMagnitudesForChannel[ch] = new Dictionary<double, double>(n / 2);
33-81: Optimize memory usage in frequency magnitude computation.Consider using array pooling to reduce memory allocations in the frequency magnitude computation:
+using System.Buffers; + private void ComputeFrequencyMagnitudes(float[] data, int sampleRate, int channelCount) { int n = data.Length / channelCount; - Complex[][] complexData = new Complex[channelCount][]; + Complex[][] complexData = ArrayPool<Complex[]>.Shared.Rent(channelCount); for (int ch = 0; ch < channelCount; ch++) { - complexData[ch] = new Complex[n]; + complexData[ch] = ArrayPool<Complex>.Shared.Rent(n); } + try + { // ... existing computation code ... + } + finally + { + for (int ch = 0; ch < channelCount; ch++) + { + ArrayPool<Complex>.Shared.Return(complexData[ch]); + } + ArrayPool<Complex[]>.Shared.Return(complexData); + } }
146-150: Add detailed documentation for signal ratio calculation.Consider adding more detailed XML documentation to explain the signal ratio calculation:
/// <summary> -/// Compute the sound-to-hum ratio. +/// Computes the ratio between non-hum and hum frequencies in the audio signal. +/// This ratio helps determine if the signal is intelligible or just noise. /// </summary> /// <param name="channel">Channel number, or null for an aggregate</param> -/// <returns>Ratio</returns> +/// <returns> +/// The ratio of non-hum to hum frequencies. A higher ratio indicates a clearer signal. +/// Returns 0 when no hum is detected to avoid division by zero. +/// </returns> +/// <remarks> +/// The ratio is calculated by dividing the total magnitude of non-hum frequencies +/// by the total magnitude of hum frequencies (50Hz and 60Hz bands). +/// A minimum value of 1 is used for hum magnitude to prevent division by zero. +/// </remarks> public double GetSignalRatio(int? channel = null)
255-287: Extract magic numbers into named constants.Consider extracting the magic number for better code maintainability:
+ // Tolerance for frequency comparison in Hz + private const double FREQUENCY_TOLERANCE = 1.0; + private OrcanodeOnlineStatus GetStatus(OrcanodeOnlineStatus oldStatus, int? channel = null) { double max = GetMaxMagnitude(channel); if (max < MinNoiseMagnitude) { return OrcanodeOnlineStatus.Silent; } if ((max <= MaxSilenceMagnitude) && (oldStatus == OrcanodeOnlineStatus.Silent)) { return oldStatus; } if (GetMaxNonHumMagnitude(channel) < MinNoiseMagnitude) { return OrcanodeOnlineStatus.Unintelligible; } double totalNonHumMagnitude = GetTotalNonHumMagnitude(channel); double totalHumMagnitude = GetTotalHumMagnitude(channel); if (totalNonHumMagnitude / totalHumMagnitude < MinSignalRatio) { return OrcanodeOnlineStatus.Unintelligible; } return OrcanodeOnlineStatus.Online; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
OrcanodeMonitor/Core/FrequencyInfo.cs(1 hunks)OrcanodeMonitor/Pages/SpectralDensity.cshtml(2 hunks)OrcanodeMonitor/Pages/SpectralDensity.cshtml.cs(5 hunks)
👮 Files not reviewed due to content moderation or server errors (3)
- OrcanodeMonitor/Pages/SpectralDensity.cshtml
- OrcanodeMonitor/Pages/SpectralDensity.cshtml.cs
- OrcanodeMonitor/Core/FrequencyInfo.cs
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: build_windows (Release)
- GitHub Check: build_windows (Debug)
- GitHub Check: Analyze (javascript)
- GitHub Check: Analyze (csharp)
🔇 Additional comments (5)
OrcanodeMonitor/Pages/SpectralDensity.cshtml.cs (3)
198-205: LGTM! Proper error handling implementation.The error handling follows the suggested pattern from previous reviews, with appropriate error logging.
Also applies to: 226-233
198-205: LGTM! Error handling implemented as requested.The error handling has been properly implemented for both node and event audio sample fetching, with appropriate error logging.
Also applies to: 226-233
198-205: LGTM! Proper error handling implementation.The error handling implementation follows the suggested pattern from previous reviews, with clear error messages and proper logging.
Also applies to: 226-233
OrcanodeMonitor/Core/FrequencyInfo.cs (2)
146-150: LGTM! Proper signal ratio implementation.The implementation correctly handles division by zero and uses appropriate magnitude thresholds.
146-150: LGTM! Division by zero handled correctly.The signal ratio calculation now properly handles the case when hum magnitude is zero.
Signed-off-by: Dave Thaler <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
OrcanodeMonitor/Pages/SpectralDensity.cshtml (1)
80-105: Consider improving the accessibility of the statistics section.While the statistics section now shows per-channel data, it could benefit from some accessibility improvements:
- The channel count should be wrapped in a semantic HTML element.
- The statistics should use a more semantic structure like
<dl>(description list) instead of line breaks.Consider this improvement:
-# Channels: @Model.ChannelCount<br /> +<p aria-label="Channel count">Number of channels: @Model.ChannelCount</p> -<p> - Max magnitude: @Model.MaxMagnitude <br /> - Max magnitude outside hum range: @Model.MaxNonHumMagnitude <br /> - Total magnitude outside hum range: @Model.TotalNonHumMagnitude <br /> - Total magnitude of hum range: @Model.TotalHumMagnitude <br /> - Signal ratio: @Model.SignalRatio %<br /> - Status: @Model.Status <br /> -</p> +<dl> + <dt>Max magnitude</dt> + <dd>@Model.MaxMagnitude</dd> + <dt>Max magnitude outside hum range</dt> + <dd>@Model.MaxNonHumMagnitude</dd> + <dt>Total magnitude outside hum range</dt> + <dd>@Model.TotalNonHumMagnitude</dd> + <dt>Total magnitude of hum range</dt> + <dd>@Model.TotalHumMagnitude</dd> + <dt>Signal ratio</dt> + <dd>@Model.SignalRatio%</dd> + <dt>Status</dt> + <dd>@Model.Status</dd> +</dl>OrcanodeMonitor/Pages/SpectralDensity.cshtml.cs (3)
52-64: Consider documenting the performance implications of POINT_COUNT.While the constant's purpose is well-documented, it would be helpful to add a note about the memory and CPU implications of increasing this value.
Add performance implications to the documentation:
/// <summary> /// Number of points to plot on the graph. 1000 points provides a good balance -/// between resolution and performance. +/// between resolution and performance. +/// +/// Performance implications: +/// - Memory: O(n) where n is POINT_COUNT +/// - CPU: O(n) for data processing and rendering /// </summary>
65-97: Consider optimizing the bucket calculation for better performance.The current implementation has a few potential performance improvements:
- Pre-calculate the bucket indices for frequencies
- Use a more efficient data structure for frequency-magnitude pairs
Consider this optimization:
private void FillInGraphPoints(List<string> labels, List<double> maxBucketMagnitudeList, int? channel = null) { if (_frequencyInfo == null) { return; } // Compute the logarithmic base needed to get PointCount points. double b = Math.Pow(MAX_FREQUENCY, 1.0 / POINT_COUNT); double logb = Math.Log(b); var maxBucketMagnitude = new double[POINT_COUNT]; var maxBucketFrequency = new int[POINT_COUNT]; + // Pre-calculate frequency to bucket mapping for common frequencies + var frequencyToBucket = new Dictionary<int, int>(); + for (int f = 1; f <= MAX_FREQUENCY; f *= 2) + { + frequencyToBucket[f] = Math.Min(POINT_COUNT - 1, (int)(Math.Log(f) / logb)); + } foreach (var pair in _frequencyInfo.GetFrequencyMagnitudes(channel)) { double frequency = pair.Key; double magnitude = pair.Value; - int bucket = (frequency < 1) ? 0 : Math.Min(POINT_COUNT - 1, (int)(Math.Log(frequency) / logb)); + int bucket; + if (frequency < 1) + { + bucket = 0; + } + else + { + // Find nearest power of 2 and interpolate + int lowerPow2 = 1 << (int)Math.Log2(frequency); + int upperPow2 = lowerPow2 << 1; + bucket = frequencyToBucket[lowerPow2] + + (int)((frequency - lowerPow2) * (frequencyToBucket[upperPow2] - frequencyToBucket[lowerPow2]) / (upperPow2 - lowerPow2)); + } if (maxBucketMagnitude[bucket] < magnitude) { maxBucketMagnitude[bucket] = magnitude; maxBucketFrequency[bucket] = (int)Math.Round(frequency); } }
182-195: Consider using CSS custom properties for channel colors.Instead of hardcoding the colors in C#, consider moving them to CSS custom properties for better maintainability and theming support.
- Add CSS variables to your stylesheet:
:root { --channel-1-color: rgb(54, 235, 127); --channel-2-color: rgb(153, 102, 255); /* ... other colors ... */ }
- Update the C# code to use CSS variables:
- var colors = new[] { - (54, 235, 127), // Green - (153, 102, 255), // Purple - (255, 159, 64), // Orange - (255, 206, 86), // Yellow - (75, 192, 192), // Teal - (255, 99, 132), // Pink - (54, 162, 235), // Blue - }; - var (r, g, b) = colors[channelIndex % colors.Length]; - return $"'rgba({r}, {g}, {b}, {alpha})'"; + return $"'var(--channel-{(channelIndex % 7) + 1}-color, rgba(128, 128, 128, {alpha}))'";
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
OrcanodeMonitor/Core/Fetcher.cs(1 hunks)OrcanodeMonitor/Pages/SpectralDensity.cshtml(2 hunks)OrcanodeMonitor/Pages/SpectralDensity.cshtml.cs(5 hunks)
✅ Files skipped from review due to trivial changes (1)
- OrcanodeMonitor/Core/Fetcher.cs
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: build_windows (Release)
- GitHub Check: build_windows (Debug)
- GitHub Check: Analyze (javascript)
- GitHub Check: Analyze (csharp)
🔇 Additional comments (2)
OrcanodeMonitor/Pages/SpectralDensity.cshtml (1)
24-33: LGTM! The Chart.js visualization now supports multiple channels.The implementation correctly generates datasets for each channel with unique colors and labels.
OrcanodeMonitor/Pages/SpectralDensity.cshtml.cs (1)
228-235: LGTM! Error handling has been properly implemented.The error handling for audio sample fetching has been implemented as suggested in the previous review.
Also applies to: 256-263
Signed-off-by: Dave Thaler <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
OrcanodeMonitor/Core/FrequencyInfo.cs (3)
95-115: Cache parsed configuration values for better performance.The configuration values are parsed from environment variables on every access. Consider caching the parsed values to improve performance.
public static double MaxSilenceMagnitude { + private static double? _cachedMaxSilenceMagnitude; get { + if (_cachedMaxSilenceMagnitude.HasValue) + return _cachedMaxSilenceMagnitude.Value; string? maxSilenceMagnitudeString = Environment.GetEnvironmentVariable("ORCASOUND_MAX_SILENCE_MAGNITUDE"); - double maxSilenceMagnitude = double.TryParse(maxSilenceMagnitudeString, out var magnitude) ? magnitude : _defaultMaxSilenceMagnitude; - return maxSilenceMagnitude; + _cachedMaxSilenceMagnitude = double.TryParse(maxSilenceMagnitudeString, out var magnitude) ? magnitude : _defaultMaxSilenceMagnitude; + return _cachedMaxSilenceMagnitude.Value; } }
191-207: Optimize GetMaxNonHumMagnitude using LINQ.The method can be simplified and potentially optimized using LINQ.
private double GetMaxNonHumMagnitude(Dictionary<double, double> frequencyMagnitudes) { - double maxNonHumMagnitude = 0; - foreach (var pair in frequencyMagnitudes) - { - double frequency = pair.Key; - double magnitude = pair.Value; - if (!IsHumFrequency(frequency)) - { - if (maxNonHumMagnitude < magnitude) - { - maxNonHumMagnitude = magnitude; - } - } - } - return maxNonHumMagnitude; + return frequencyMagnitudes + .Where(pair => !IsHumFrequency(pair.Key)) + .Select(pair => pair.Value) + .DefaultIfEmpty(0) + .Max(); }
274-306: Extract magic numbers into named constants.The status determination logic uses magic numbers that should be extracted into named constants for better maintainability and clarity.
+ private const double SILENCE_HYSTERESIS_THRESHOLD = 1.0; + private const double SIGNAL_RATIO_THRESHOLD = 1.0; + private OrcanodeOnlineStatus GetStatus(OrcanodeOnlineStatus oldStatus, int? channel = null) { double max = GetMaxMagnitude(channel); if (max < MinNoiseMagnitude) { return OrcanodeOnlineStatus.Silent; } - if ((max <= MaxSilenceMagnitude) && (oldStatus == OrcanodeOnlineStatus.Silent)) + if ((max <= MaxSilenceMagnitude + SILENCE_HYSTERESIS_THRESHOLD) && + (oldStatus == OrcanodeOnlineStatus.Silent)) { return oldStatus; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
OrcanodeMonitor/Core/FrequencyInfo.cs(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: build_windows (Release)
- GitHub Check: build_windows (Debug)
- GitHub Check: Analyze (csharp)
🔇 Additional comments (2)
OrcanodeMonitor/Core/FrequencyInfo.cs (2)
37-41: Track and resolve the data length validation issue.The commented-out validation check suggests a known issue that needs to be investigated and fixed. This could lead to runtime errors if the data length is not properly validated.
Would you like me to help create an issue to track this TODO? The issue would include:
- Investigation of why the validation check is failing
- Steps to reproduce the issue
- Proposed solutions
131-143: LGTM! Properties are well-organized and documented.
Fix "-1" label on node events page
Summary by CodeRabbit
New Features
FrequencyInfoclass for advanced audio signal analysis.Refactor
Bug Fixes
Chores
FrequencyInfoclass from the codebase.