patterncsharpMinor
Beat detection algorithm implementation
Viewed 0 times
beatalgorithmimplementationdetection
Problem
What is the quality of the code I've written? Is it easy to read or is it a low-quality piece of code? Also, is there any way I can improve the algorithm itself (beside changing C parameters)?
```
using System;
using System.Threading;
using Un4seen.Bass;
using Un4seen.BassWasapi;
namespace Szab.BeatDetector
{
public enum SensivityLevel
{
VERY_LOW = 120,
LOW = 110,
NORMAL = 100,
HIGH = 80,
VERY_HIGH = 70
};
public sealed class SpectrumBeatDetector
{
#region Fields
// Constants
private const int BANDS = 10;
private const int HISTORY = 50;
// Events
public delegate void BeatDetectedHandler(byte Value);
private event BeatDetectedHandler OnDetected;
// Threading
private Thread _AnalysisThread;
// BASS Process
private WASAPIPROC _WasapiProcess = new WASAPIPROC(SpectrumBeatDetector.Process);
// Analysis settings
private int _SamplingRate;
private int _DeviceCode;
private SensivityLevel _BASSSensivity;
private SensivityLevel _MIDSSensivity;
// Analysis data
private float[] _FFTData = new float[4096];
private double[,] _History = new double[BANDS, HISTORY];
#endregion
#region Setup methods
public SpectrumBeatDetector(int DeviceCode, int SamplingRate = 44100, SensivityLevel BASSSensivity = SensivityLevel.NORMAL, SensivityLevel MIDSSensivity = SensivityLevel.NORMAL)
{
_SamplingRate = SamplingRate;
_BASSSensivity = BASSSensivity;
_MIDSSensivity = MIDSSensivity;
_DeviceCode = DeviceCode;
Init();
}
// BASS initialization method
private void Init()
{
bool result = false;
// Initialize BASS on default device
result = Bass.BASS_Init(0, _SamplingRate, BASSInit.BASS_DEVICE_DEFAULT, IntPtr.Zero);
if (!result)
{
throw new BassInitException(Bass.BASS_ErrorGetCode().ToString());
}
// Initialize WASAPI
result = BassWasapi.BASS_WASAPI_
```
using System;
using System.Threading;
using Un4seen.Bass;
using Un4seen.BassWasapi;
namespace Szab.BeatDetector
{
public enum SensivityLevel
{
VERY_LOW = 120,
LOW = 110,
NORMAL = 100,
HIGH = 80,
VERY_HIGH = 70
};
public sealed class SpectrumBeatDetector
{
#region Fields
// Constants
private const int BANDS = 10;
private const int HISTORY = 50;
// Events
public delegate void BeatDetectedHandler(byte Value);
private event BeatDetectedHandler OnDetected;
// Threading
private Thread _AnalysisThread;
// BASS Process
private WASAPIPROC _WasapiProcess = new WASAPIPROC(SpectrumBeatDetector.Process);
// Analysis settings
private int _SamplingRate;
private int _DeviceCode;
private SensivityLevel _BASSSensivity;
private SensivityLevel _MIDSSensivity;
// Analysis data
private float[] _FFTData = new float[4096];
private double[,] _History = new double[BANDS, HISTORY];
#endregion
#region Setup methods
public SpectrumBeatDetector(int DeviceCode, int SamplingRate = 44100, SensivityLevel BASSSensivity = SensivityLevel.NORMAL, SensivityLevel MIDSSensivity = SensivityLevel.NORMAL)
{
_SamplingRate = SamplingRate;
_BASSSensivity = BASSSensivity;
_MIDSSensivity = MIDSSensivity;
_DeviceCode = DeviceCode;
Init();
}
// BASS initialization method
private void Init()
{
bool result = false;
// Initialize BASS on default device
result = Bass.BASS_Init(0, _SamplingRate, BASSInit.BASS_DEVICE_DEFAULT, IntPtr.Zero);
if (!result)
{
throw new BassInitException(Bass.BASS_ErrorGetCode().ToString());
}
// Initialize WASAPI
result = BassWasapi.BASS_WASAPI_
Solution
Naming
Based on the naming guidelines input parameters should be named using
Although it isn't mentioned in the guidelines, you should consider to use the same casing for variables which are local to a method.
You have a lot of magic numbers in your code which should be extracted to well named constants. In this way Mr.Maintainer wouldn't have to think to much about them.
If you need a comment to know what a variable is about, then this variable is poorly named.
General
-
Comments should describe why something is done. Let the code itself tell what is done by using meaningful names.
-
commented code is dead code which should be removed
-
leave your variables some space to breathe.
Read more about disposing finalizable types here : https://msdn.microsoft.com/en-us/library/b1yfkh5e%28v=vs.110%29.aspx (scroll down)
Based on the naming guidelines input parameters should be named using
camelCase casing. Although it isn't mentioned in the guidelines, you should consider to use the same casing for variables which are local to a method.
You have a lot of magic numbers in your code which should be extracted to well named constants. In this way Mr.Maintainer wouldn't have to think to much about them.
If you need a comment to know what a variable is about, then this variable is poorly named.
General
-
Comments should describe why something is done. Let the code itself tell what is done by using meaningful names.
-
commented code is dead code which should be removed
-
leave your variables some space to breathe.
if(i
DetectBeat()
You don't need to assign 0 to the C variable, because an assignment changing this value is happening anyhow.
If the condition if(Energies[i] > (C * avg[i]) && volumelevel > 1) evaluates to true, you can break out of the for loop instead of again checking the for conditions.
By extracting the calculating of the C parameter into a separate method, the DetectBeat() method will be reduced to its core responsibility.
private const int _Bass = 3;
private const int _Mids = 6;
private double CalculateCParameter(int band)
{
if (band < _Bass)
{
return C = 2.3 * ((double)_BASSSensivity / 100);
}
if (band < _Mids)
{
return 2.89 * ((double)_MIDSSensivity / 100);
}
return C = 3 * ((double)_MIDSSensivity / 100);
}
And by extracting the calculation of the result to a separate method
private byte CalculateResultByBand(int band)
{
if(band < _Bass)
{
return 1;
}
if (i < _Mids)
{
return 2;
}
return 4;
}
the DetectBeat() method can, after adding a guard clause for volumelevel <= 1 be simplified to
private byte DetectBeat(double[] Energies, int volume)
{
double[] avg = CalculateAverages();
byte result = 0;
double volumelevel = (double)volume / 32768 * 100; // Volume level in %
if (volumelevel (C * avg[i]))
{
result = CalculateResultByBand(i);
break;
}
}
if(result > 0)
{
OnDetected(result);
}
return result;
}
Free() versus IDisposable
Because you are having unmanaged resources you really should implement IDisposable` to free the unmanaged resources. Read more about disposing finalizable types here : https://msdn.microsoft.com/en-us/library/b1yfkh5e%28v=vs.110%29.aspx (scroll down)
Code Snippets
private const int _Bass = 3;
private const int _Mids = 6;
private double CalculateCParameter(int band)
{
if (band < _Bass)
{
return C = 2.3 * ((double)_BASSSensivity / 100);
}
if (band < _Mids)
{
return 2.89 * ((double)_MIDSSensivity / 100);
}
return C = 3 * ((double)_MIDSSensivity / 100);
}private byte CalculateResultByBand(int band)
{
if(band < _Bass)
{
return 1;
}
if (i < _Mids)
{
return 2;
}
return 4;
}private byte DetectBeat(double[] Energies, int volume)
{
double[] avg = CalculateAverages();
byte result = 0;
double volumelevel = (double)volume / 32768 * 100; // Volume level in %
if (volumelevel <= 1) { return result;}
for (int i = 0; i < BANDS && result == 0; i++)
{
// Set the C parameter
double C = CalculateCParameter(i);
// Compare energies in all bands with C*average
if(Energies[i] > (C * avg[i]))
{
result = CalculateResultByBand(i);
break;
}
}
if(result > 0)
{
OnDetected(result);
}
return result;
}Context
StackExchange Code Review Q#78589, answer score: 3
Revisions (0)
No revisions yet.