4
\$\begingroup\$

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.

\$\endgroup\$
0

4 Answers 4

4
\$\begingroup\$

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 < micBufferEnd; ++s)
    {
        Test[Tindex++] = (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 here, but that's not portable, and it's even possible that it could change from version to version of an individual compiler. An alternative, also noted in the comments, was to define the structure differently. One way to do it might be this:

class Sample
{
private:
    float chan[2];
public:
    float left() const { return chan[0]; }
    float right() const { return chan[1]; }
};

Using it would be almost identical except that left and right are now member functions rather than data items.

Sample *micBufferEnd = (Sample *)&micBuffer[bufferSize];
if (MixMono) 
{
    for (Sample *s = (Sample *)micBuffer; s < micBufferEnd; ++s)
    {
        Test[Tindex++] = (s->left() + s->right()) * halfScaleFactor;
    }
}
\$\endgroup\$
6
  • \$\begingroup\$ ...but note that the last might not work. The compiler is free to treat the struct as having padding between the items, so left and right might not refer to consecutive items in the input. \$\endgroup\$ Commented Jul 3, 2014 at 15:22
  • \$\begingroup\$ @JerryCoffin: That's good point. An alternative might be to define the struct as an array of 2 floats and provide left and right as aliases. \$\endgroup\$
    – Edward
    Commented Jul 3, 2014 at 15:32
  • \$\begingroup\$ Quite interested in that Left Right thing, but i see no way for it to know what left and right means?, left will need to be 1 index behind right (and vice versa). \$\endgroup\$
    – Zerowalker
    Commented Jul 4, 2014 at 5:20
  • \$\begingroup\$ @Edward, i tried the left/right, it works, but i don't get why. There is nothing showing why ever even is stored in left, and odd in right. \$\endgroup\$
    – Zerowalker
    Commented Jul 4, 2014 at 6:22
  • \$\begingroup\$ @Zerowalker: I've updated my answer to try to better explain how that part works and also to show an alternative that doesn't have the flaw that JerryCoffin noted in his comment. \$\endgroup\$
    – Edward
    Commented Jul 4, 2014 at 11:20
4
\$\begingroup\$

Right now, your code is structured something like:

for i = 1 to number of samples
    if x 
        result[i] = foo(samples[i])
    else if y 
        results[i] = bar(samples[i])
    else
        result[i] = baz(samples[i])

As a first step, I would turn that inside out (so to speak) and make each of foo, bar and baz implement its own iteration over the inputs, to get something like this:

if x
    result = foo(samples)
else if y
    result = bar(samples)
else
    result = baz(samples)

That's still fairly repetitive, so a possible next step would be to carry out the selection, then apply that selection:

if x
    algorithm = foo
else if y
    algorithm = bar
else
    algorithm = baz

result = algorithm(samples)

A possible next step after that would be to convert x and y (currently Booleans) into a single variable, and use that to index into a collection where you store the algorithms:

Algorithm algorithms[] = { baz, foo, bar, baz };

index = (x << 1) | y

algorithm = algorithms[index]

result = algorithm(samples)

If you really want the code to be succinct, you can combine some of those steps to get something like this:

Algorithm algorithms[] = { baz, foo, bar, baz };

result = algorithms[(x<<1) | y](samples);

Frankly, I'm a lot less than excited about that though--it combines enough different "stuff" into a single statement that I think it's substantially harder to read and understand than the previous version.

As far as how to implement that in C++ goes, you'd probably end up with something like this:

std::vector<int> mix(std::vector<int> const &inputs) {
    std::vector<int> result;
    for (int i=0; i<inputs.size(); i+=2)
        result.push_back((inputs[i] + inputs[i+1])*16384;
    return result;
}

std::vector<int> left(std::vector<int> const &inputs) { 
    std::vector<int> result;
    for (int i=0; i<inputs.size(); i+=2)
        result.push_back(inputs[i]);
    return result;
}

// and so on.

Of course, the same caveat as Edward raised continues to apply--you don't really want to use 16384 directly in the code. You want to define some meaningful name for it, then use that.

You could also implement these as generic algorithms instead of plain functions the way they are now. For your purpose, I don't think this would really be a big win though--it would add extra work to invocation with little chance of a real benefit.

\$\endgroup\$
4
  • \$\begingroup\$ Hmm interesting, don't quite understand the "Algorithm" part though. However, using vector, is there any benefit for it (My float array will Always be either 960 or 480). \$\endgroup\$
    – Zerowalker
    Commented Jul 4, 2014 at 17:19
  • \$\begingroup\$ Yes, there are benefits from it, such as not having to deal with the size separately from the data. \$\endgroup\$ Commented Jul 4, 2014 at 17:35
  • \$\begingroup\$ Jerry Coffin, even though it's only always 2 sizes? I could probably just go with 960 all the time though. But if Vector is the same speed as a float array, i will use that. \$\endgroup\$
    – Zerowalker
    Commented Jul 5, 2014 at 3:39
  • \$\begingroup\$ @Zerowalker: I've never been able to measure a difference in speed. \$\endgroup\$ Commented Jul 5, 2014 at 3:41
3
\$\begingroup\$

Does this code actually work? I haven't figured out what it's supposed to do yet, so I can't be sure.

This looks wrong to me:

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

You set the booleans to true, then test them in the if. The if condition will always be true, so what's the point?

Also, you declare an inner version of Test in the if statement: int16_t Test[960 / 2];. But this version only exists in the body of the if. Once you leave the if, the original Test[960] is active again.

This line confuses me:

int s = (micBuffer[i] * 32768.0f); //32bit Float to 16Bit Int

Where's the declaration of micBuffer? If s is supposed to be a 16-bit integer, why isn't it int16_t? (The parentheses around the RHS expression seem useless.)

Is there a relationship between bufferSize and the size of Test (which is 960 or 960/2)? Why are you looping over elements of micBuffer, instead of looping over elements of Test? Have you tried writing it with the if-else if-else on the outside, with a (different) loop inside each block?

Maybe this code should be split into 3 different functions, one for MixMono, one for left, and one for "other".

What's your convention for variable names? Some of them are capitalized, some aren't. I don't care what convention you use (some folks would care, a lot), but you should be consistent.

\$\endgroup\$
5
  • \$\begingroup\$ Woah, many stuff here, will try explain a bit. Left only takes every Even index in micbuffer (0,2 etc) which is the Left channel. MixMono downmixes to stereo. Both of them have the same parameters (half the size of Stereo). however int16_t Test shouldn't be there, it's weird that it works indeed. micBuffer is declared way back, and contains 960 indexes of float numbers. isn't int = int16_t? 960 = Stereo, 960/2 = Mono, very simple, half size, half indexes. If you know a more efficient way, please give examples. Much appreciated for your information, thanks! \$\endgroup\$
    – Zerowalker
    Commented Jul 3, 2014 at 3:16
  • \$\begingroup\$ int might be 16 bits for some compilers, but it would be unusual these days outside of embedded, single-chip systems. int_16t must be 16 bits if the compiler conforms to the standard. A plain int is typically the natural word size of the target system, but it's up to the compiler implementor. If the size of the integer needs to be 16 bits in your algorithm, you should declare it int_16t`. \$\endgroup\$ Commented Jul 3, 2014 at 12:57
  • \$\begingroup\$ ah, didn't know that, thanks. - Is "short" the same as int_16t? \$\endgroup\$
    – Zerowalker
    Commented Jul 4, 2014 at 4:50
  • \$\begingroup\$ short has to be at least 16 bits, but it might be bigger. short and int are allowed to be the same size, for example. Look in limits.h to see what your compiler does. If the size matters to you, always use the int_Nt types, which are portable. \$\endgroup\$ Commented Jul 4, 2014 at 11:14
  • \$\begingroup\$ Ah i see, i am guessing short stays 16bit until you reach a number above it. Kinda complicated, but others have also told me to use the _Nt, will have to start doing that so i know it's properly used. \$\endgroup\$
    – Zerowalker
    Commented Jul 4, 2014 at 17:24
2
\$\begingroup\$

I'm going to steal your second version, change the formatting a bit, and fix what appear to be bugs.

//For Loop Conversions
if (MixMono) //For Loop for Downmixing to Mono
{
    for (int i = 0; i < bufferSize; i += 2)         
    {
        //__m128 t = *mixBuffer[i];
        Test[i/2] = ((mixBuffer[i] + mixBuffer[i+1]) * 32768.0f) / 2;
    }
}
else if (left) //For Loop for Getting Left Channel
{
    for (int i = 0; i < bufferSize; i += 2)         
    {
        Test[i/2] = (mixBuffer[i] * 32768.0f);
    }
}
else //For Loop for BitDepth Conversion
{
    for (int i = 0; i < bufferSize; ++i)            
    {
        Test[i] = (mixBuffer[i] * 32768.0f); //32bit Float to 16Bit Int
    }
 }

A few things:

  • I changed the three independent if statements to an if-else if-else sequence. Your if statements really weren't independent, since the conditions were related. The new structure reflects the intent better, I think.
  • The expression (mixBuffer[i] + mixBuffer[i++]) is almost certainly wrong. If the LH side of the + expression is evaluated first, the result is mixBuffer[i]+mixBuffer[i]. If the RH side is evaluated first, you (probably) get mixBuffer[i+1]+mixBuffer[i+1]. The compiler can choose either order. There's also the side effect from i++ to worry about -- exactly when does the new value get stored back into i? I have to worry about sequence points (there are none in this sub-expression) and whether the compiler is allowed to delay updating the value of i enough for it to matter. Yuck! Let's say (mixBuffer[i] + mixBuffer[i+1] instead, and eliminate all the ambiguity. And we'll have to increment by 2 instead of 1 in the for loop header, since we no longer increment in the loop body.
  • for loops are easier to understand if you treat the loop control variable(s) as constant inside the body of the loop. If you change the variable in the loop header ((; ; i++)) and in the body ([i++]), it's very easy to get confused and introduce an error. When you need complex behavior in the loop's control variable, consider using a while loop instead. (We can forget about the issue in this case, since the control variable is a constant now (in the loop body)).
  • For standalone expressions, prefer ++i instead of i++. This link explains why it often matters for user-defined types: Why use ++i instead of i++? It doesn't matter in your code, but it's a good habit to prefer preincrement/predecrement instead of postincrement/postdecrement when you can.
  • In the second for loop, i + 2 doesn't do what you want. i + 2 is evaluated and then the result is discarded. i does not change, so this is an infinite loop. I changed i + 2 to i += 2.
  • I don't think you need Tindex. Isn't Tindex always equal to i/2 when you use it?
\$\endgroup\$
4
  • \$\begingroup\$ Although it's technically true that Tindex is not needed, I think it may be better to leave it. That way, Test is always indexed with Tindex and micBuffer is always indexed with i. It's a small consistency that costs very little computationally but, in my opinion, aids comprehension for human programmers reading and maintaining the code. \$\endgroup\$
    – Edward
    Commented Jul 3, 2014 at 15:17
  • \$\begingroup\$ True, i do not need Tindex, hmm, is it possible to heave some kind of pointer variable that always is /2 of i? (Guess not as it only exist in the for loop). - Also the [i] + [i++] thing, does that really happen, i mean, that would ruin whole calculations if the compiler messes the order of things, not just this particular one? \$\endgroup\$
    – Zerowalker
    Commented Jul 4, 2014 at 5:13
  • \$\begingroup\$ Yes, order of evaluation in most expressions is up to the compiler. It's a cause of many mysterious errors in C and C++. \$\endgroup\$ Commented Jul 4, 2014 at 11:20
  • \$\begingroup\$ @GraniteRobert Looked it up, interesting indeed. i+1 seems to force it to become 1 higher than i. i++ keeps i in temp memory while Then giving the command the i+1 (which in turn let's the command use i normally as it's "delayed". ++i does the other way around, adding 1 before i, so when it reaches i it has already been increased. Disturbing facts, but extremely useful to know about. \$\endgroup\$
    – Zerowalker
    Commented Jul 5, 2014 at 3:52

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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