current community

your communities

more stack exchange communities

Stack Exchange
tour help
Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 5 down vote favorite
1

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_Init(_DeviceCode, 0, 0, BASSWASAPIInit.BASS_WASAPI_BUFFER, 1f, 0.05f, _WasapiProcess, IntPtr.Zero);

        if (!result)
        {
            throw new BassWasapiInitException(Bass.BASS_ErrorGetCode().ToString());
        }

        BassWasapi.BASS_WASAPI_Start();
        System.Threading.Thread.Sleep(500);
    }


    ~SpectrumBeatDetector()
    {
        // Kill working thread and clean after BASS
        if(_AnalysisThread != null && _AnalysisThread.IsAlive)
        {
            _AnalysisThread.Abort();
        }

        Free();
    }

    // Sensivity Setters
    public void SetBassSensivity(SensivityLevel Sensivity)
    {
        _BASSSensivity = Sensivity;
    }

    public void SetMidsSensivity(SensivityLevel Sensivity)
    {
        _MIDSSensivity = Sensivity;
    }

    #endregion

    #region BASS-dedicated Methods

    // WASAPI callback, required for continuous recording
    private static int Process(IntPtr buffer, int length, IntPtr user)
    {
        return length;
    }

    // Cleans after BASS
    public void Free()
    {
        BassWasapi.BASS_WASAPI_Free();
        Bass.BASS_Free();
    }

    #endregion

    #region Analysis public methods

    // Starts a new Analysis Thread
    public void StartAnalysis()
    {
        // Kills currently running analysis thread if alive
        if (_AnalysisThread != null && _AnalysisThread.IsAlive)
        {
            _AnalysisThread.Abort();
        }

        // Starts a new high-priority thread
        _AnalysisThread = new Thread(delegate()
            {
                while (true)
                {
                    //Stopwatch SW = new Stopwatch();
                    //SW.Start();
                    Thread.Sleep(5);
                    PerformAnalysis();
                    //SW.Stop();
                    //Console.WriteLine(SW.Elapsed);
                }
            });

        _AnalysisThread.Priority = ThreadPriority.Highest;
        _AnalysisThread.Start();
    }

    // Kills running thread
    public void StopAnalysis()
    {
        if(_AnalysisThread != null && _AnalysisThread.IsAlive)
        {
            _AnalysisThread.Abort();
        }
    }

    #endregion

    #region Event handling

    public void Subscribe(BeatDetectedHandler Delegate)
    {
        OnDetected += Delegate;
    }

    public void UnSubscribe(BeatDetectedHandler Delegate)
    {
        OnDetected -= Delegate;
    }

    #endregion

    #region Analysis private methods

    // Shifts history n places to the right
    private void ShiftHistory(int n)
    {
        for (int i = 0; i < BANDS; i++)
        {
            for (int j = HISTORY - 1 - n; j >= 0; j--)
            {
                _History[i, j + n] = _History[i, j];
            }
        }
    }

    // Performs FFT analysis in order to detect beat
    private void PerformAnalysis()
    {
        // Specifes on which result end which band (dividing it into 10 bands)
        // 19 - bass, 187 - mids, rest is highs
        int[] BandRange = { 4, 8, 18, 38, 48, 94, 140, 186, 466, 1022, 22000};
        double[] BandsTemp = new double[BANDS];
        int n = 0;
        int level = BassWasapi.BASS_WASAPI_GetLevel();

        // Get FFT
        int ret = BassWasapi.BASS_WASAPI_GetData(_FFTData, (int)BASSData.BASS_DATA_FFT1024 | (int)BASSData.BASS_DATA_FFT_COMPLEX); //get channel fft data
        if (ret < -1) return;

        // Calculate the energy of every result and divide it into subbands
        float sum = 0;

        for (int i = 2; i < 2048; i = i + 2)
        {
            float real = _FFTData[i];
            float complex = _FFTData[i + 1];
            sum += (float)Math.Sqrt((double)(real * real + complex * complex));

            if(i == BandRange[n])
            {
                BandsTemp[n++] = (BANDS * sum) / 1024;
                sum = 0;
            }
        }

        // Detect beat basing on FFT results
        DetectBeat(BandsTemp, level);

        // Shift the history register and save new values
        ShiftHistory(1);

        for (int i = 0; i < BANDS; i++)
        {
            _History[i, 0] = BandsTemp[i];
        }
    }

    // Calculate the average value of every band
    private double[] CalculateAverages()
    {
        double[] avg = new double[BANDS];

        for (int i = 0; i < BANDS; i++)
        {
            double sum = 0;

            for (int j = 0; j < HISTORY; j++)
            {
                sum += _History[i, j];
            }

            avg[i] = (sum / HISTORY);
        }

        return avg;
    }

    // Detects beat basing on analysis result
    // Beat detection is marked on the first three bits of the returned value
    private byte DetectBeat(double[] Energies, int volume)
    {
        // Sound height ranges (1, 2 is bass, next 6 is mids)
        int Bass = 3;
        int Mids = 6;

        double[] avg = CalculateAverages();
        byte result = 0;
        double volumelevel = (double)volume / 32768 * 100;   // Volume level in %

        for (int i = 0; i < BANDS && result == 0; i++)                    
        {
            // Set the C parameter
            double C = 0;

            if (i < Bass)
            {
                C = 2.3 * ((double)_BASSSensivity / 100);
            }
            else if (i < Mids)
            {
                C = 2.89 * ((double)_MIDSSensivity / 100);
            }
            else
            {
                C = 3 * ((double)_MIDSSensivity / 100);
            }

            // Compare energies in all bands with C*average
            if(Energies[i] > (C * avg[i]) && volumelevel > 1)   // Second rule is for noise reduction
            {
                byte res = 0;
                if(i<Bass)
                {
                    res = 1;
                }
                else if (i < Mids)
                {
                    res = 2;
                }
                else
                {
                    res = 4;
                }
                result = (byte)(result | res);
            }
        }

        if(result > 0)
        {
            OnDetected(result);
        }

        return result;
    }

    #endregion

}
}
share|improve this question
    
Does it work? Does it recognize beats? –  Bob Dalgleish Jan 26 at 1:42
2  
It depends on a song and a source (works really well with Spotify, a bit worse with YouTube) but as far as I checked it recognized beats in most of the songs I tested it on. –  Szab Jan 26 at 1:52

1 Answer 1

up vote 2 down vote accepted

Naming

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 < Bass) is more readable than if(i<Bass).

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 <= 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;
}

Free() versus IDisposable

Because you are having unmanaged reassources 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)

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.