For-loop for mixing audio (left channel, stereo to mono)

This is a very simple for loop, but I find it very, complex-looking. I hope you can give me some insight on how it may be improved.

int Tindex = 0;
int16_t Test[960];
int TestSize = SampleSize / 2; //Test is 16Bit, Half size of 32bit
int TestChannels = 2; //Stereo
float copy = 0; //Used to store Audio data used to Mixdown to Mono
bool left = true; //Only use Left Channel
bool MixMono = true; //MixMono has priority over left
if (MixMono || left) //Change the approprite variables if it's Mono instead of Stereo
{
    int16_t Test[960 / 2];
    TestSize = TestSize / 2; //If Mono is used, Half Channel/Size.
    TestChannels = 1; //Mono
}
int ChannelCheck = 0;

for (int i = 0; i < bufferSize; i += 1)         //For Loop for Downmixing and BitDepth Conversion
{
    int s = (micBuffer[i] * 32768.0f); //32bit Float to 16Bit Int

    ChannelCheck++;

    if (MixMono)//Mixdown Stereo to Mono
    {
        copy += s;
        if (ChannelCheck == 2)
        {
            Test[Tindex] = copy / 2;
            copy = 0;
            Tindex++;
            ChannelCheck = 0;
        }

    }
    else if (left)
    {
        if (ChannelCheck == 1)
        {
            Test[Tindex] = s;
            ChannelCheck -= 2;
            Tindex++;
        }
    }
    else
    {
        Test[Tindex] = s;
        Tindex++;
    }
}
//End of For Loop

I tried commenting most stuff, so it should be self-explained.

It's pretty much a mix stereo to mono. It combines 2 indices from the float array (add them), then divide them (to keep the correct volume).

16-bit conversion just multiplies by Int16 Max; not much there. I would like to have some kind of dithering, so if anyone knows some easy references, please offer them.

But as you can see, it's overly complicated with checks all over the place. I think it can be improved, but I can't really figure out a good way to do it.

Provide a complete sample

For future reference, it's much easier to review code if it's complete. That is, instead of just writing a loop, you could instead wrap in a small but complete program. Your code refers to SampleSize, bufferSize, and micBuffer but none of those are defined in the sample you've posted.

Establish and use consistent naming

Some of your variable names, such as SampleSize are capitalized, while others, such as bufferSize are not. One common C++ convention is to use lowercase names for variables (as with bufferSize) and capitalized names for classes and structure types such as Vector. There is no significance to these conventions to the compiler, but a consistently applied convention makes it easier for others to read and understand your code.

Avoid the use of hard-coded "magic numbers"

There are numbers, such as 960 and 32768.0f in your code that don't have an obvious meaning. Better would be to declare them as constants with a meaningful name, or at least to have more explanatory comments.

Eliminate unused variables

This code declares a variable Test at the top of the sample, and then redefines it within the scope of the if statement. The second declaration of Test is never used within the code. TestChannels and TestSize are also unused within this code. You can (and should) eliminate unused variables like this.Your compiler is smart enough to help you find this kind of problem if you know how to ask it to do so.

Remove loop invariants from the loop

A loop invariant is a value used within a loop construct that doesn't change. Right now your code checks the value of MixMono and left for every iteration through the loop. Since they don't change within the loop, it's better to check them just once and then select a loop, since you're looking for performance.

As an example of this, here's what your MixMono loop would look like when extracted:

if (MixMono) 
{
    for (int i = 0; i < bufferSize; ++i)
    {
        int s = micBuffer[i] * scaleFactor; //32bit Float to 16Bit Int
        ChannelCheck++;
        copy += s;
        if (ChannelCheck == 2)
        {
            Test[Tindex] = copy / 2;
            copy = 0;
            Tindex++;
            ChannelCheck = 0;
        }
    }
}

Now that it's extracted, it's easier to see what's happening and to identify inefficiencies. One thing to note is that we're doing two multiplies, an add and a division for each mono sample. The math is currently (left 32768 + right 32768)/2 with each channel being a float and the result being an int16_t. Mathematically, this is the same as (left+right)*16384 so as long as you can assure that the samples are within range and that you always have an even number of samples, you can simplify this loop to this:

const float scaleFactor = 32768;
const float halfScaleFactor = scaleFactor/2;
if (MixMono) 
{
    for (int i = 0; i < bufferSize; i+=2)
    {
        Test[Tindex++] = (micBuffer[i] + micBuffer[i+1]) * halfScaleFactor;
    }
}

The other loops can be similarly compressed and the s, ChannelCheck and copy variables completely eliminated:

else if (left)
{
    for (int i = 0; i < bufferSize; i+=2)         
    {
        Test[Tindex++] = micBuffer[i] * scaleFactor;
    }
}
else 
{
    for (int i = 0; i < bufferSize; ++i)
    {
        Test[Tindex++] = micBuffer[i] * scaleFactor;
    }
}

Consider using pointers

Depending on the architecture and compiler, using pointers instead of array indexing may be faster. If your application requires more speed than indexing can provide, you might want to test using pointers and see if that will allow your code to meet its performance requirements. As always, you should prefer clearly written code and only attempt such speedups if you have measured the performance of the code and found it insufficient.

Consider using a struct

Another possibility is to define a struct like this:

struct Sample
{
    float left;
    float right;
};

and then processing your micBuffer data by casting it as an array of such structures. Your code might be easier to read and understand this way and may be faster. An example is this:

Sample *micBufferEnd = (Sample *)&micBuffer[bufferSize];
if (MixMono) 
{
    for (Sample *s = (Sample *)micBuffer; s left + s->right) * halfScaleFactor;
    }
}

This works by treating the samples a pair at a time. When the ++s is executed at the end of each loop, it advances the pointer by the size of the structure which happens to be the size of two floats.

Note that as @JerryCoffin noted in his comment, that this is not guaranteed to work because the compiler is free to insert padding between values. For instance, on a 64-bit machine, the compiler may decide to align each member on a 64-bit boundary, possibly resulting in a gap between member items. Compilers often implement something like a #pragma pack to force the behavior used

StackExchange Code Review Q#55949, answer score: 4