jamulussoftware / jamulussoftware/jamulus

Improve performance/correctness of FlipNBits in sound.cpp

Open
#2,993 15 comments 0 reactions 0 assignees View on GitHub
bug
Dominant language
C
Stars
1.1k
Forks
248
Avg merge
2d 3h
Merged PRs (30d)
9

Description

I assume that the code for Flip16,32,64 bits in sound.cpp is not efficient (or doesn't do what it should do): https://github.com/jamulussoftware/jamulus/blob/main/src/sound/asio/sound.cpp#L1168

I made some heuristic tests on what the function Flip16Bits outputs (Disclaimer: I haven't fully understood what the code exactly does) and it seems that all it does is a left shift by one. Thus, at least Flip16Bits can be simplified (and I assume it's the same for the other code).
By the function name, it could do a bitwise invert which it doesn't. Also it doesn't seem to flip the whole word around.
What confuses me most, is that the if statement doesn't flip anything. So it might well be a bug.

The following code outputs "ok", which I consider as proof that the function Flip16Bits can be implemented as leftshift by 1 only.

```c++
#include
#include
#include
#include
using namespace std;

int32_t Flip32Bits(int32_t iIn) {

uint32_t iMask = ( static_cast ( 1 ) << 31 );

int32_t iOut = 0;

for ( unsigned int i = 0; i < 32; i++ )

{

// copy current bit to correct position

iOut |= ( iIn & iMask ) ? 1 : 0;

// shift out value and mask by one bit

iOut <<= 1;

iMask >>= 1;

}

return iOut;
}

int32_t Flip32BitHist(int32_t iIn) {
return (iIn<<1);
}

int16_t Flip16Bits ( const int16_t iIn )

{

uint16_t iMask = ( 1 << 15 );

int16_t iOut = 0;

for ( unsigned int i = 0; i < 16; i++ )

{

// copy current bit to correct position

iOut |= ( iIn & iMask ) ? 1 : 0;

// shift out value and mask by one bit

iOut <<= 1;

iMask >>= 1;

}

return iOut;

}

int16_t Flip16BitsHist(const int16_t iIn) {
int16_t iOut = (iIn<< 1);
return iOut;
}

int64_t Flip64Bits ( const int64_t iIn )

{

uint64_t iMask = ( static_cast ( 1 ) << 63 );

int64_t iOut = 0;

for ( unsigned int i = 0; i < 64; i++ )

{

// copy current bit to correct position
iOut |= ( iIn & iMask ) ? 1 : 0;

// shift out value and mask by one bit

iOut <<= 1;

iMask >>= 1;

}

return iOut;

}

int64_t Flip64BitHist(int64_t iIn) {
return (iIn << 1);
}

int main(int argc, char** argv) {
for (uint16_t check = 0;check<=(uint16_t)-1;check++) {
cout << bitset<16> (check) << endl;
//cout << b1 << endl;
//cout << bitset<16>(Flip16BitsHist(check)) << endl;
//cout << bitset<16>(Flip16Bits(check)) << endl << endl;
assert(Flip16BitsHist(check) == Flip16Bits(check));
//assert(Flip32Bits(check) == Flip32BitHist(check));
//assert(Flip64Bits(check) == Flip64BitHist(check));
//cout << check;
if (check == (uint16_t)-1) {// not so elegant termination!
break;
}
}
cout << "ok";
}

```
The other functions can't be tested this way as uint64_t is just too large to get an answer, thus analysis of the code is needed.
(I might run the 32 bit over night)

Contributor guide

Open the contributing guide

Research direction

Start in src/sound/asio/sound.cpp around line 1168 and inspect Flip16Bits, Flip32Bits, and Flip64Bits in their call context to establish the intended bit transformation. Compare that behavior with the reported outputs, then add focused coverage for all three widths; done means the implementation matches the intended transformation and the tests pass.

Written by the indexing model from the issue text.

Assessment

Tech stack
cpp
Domain
audio-video-rtc
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Quiet
Clarity
Mostly clear
Newbie friendliness
48/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.