Good stuff! I went into it expecting that you hadn’t dealt with seeking issues, but using mpg123_replace_reader is definitely the way to go!
I’m actually trying to sort of “clean room” create my own reader - because unfortunately any code directly derivative of the original is still GPL’ed and thus can’t be used in my commercial, closed-source project. So stay tuned anyway…
BTW, I’m pretty sure you’ve fixed the potential threading issue I saw - because there seem to be no static variables aside from constants. But the fact that you can seek in the file without having to keep it all in memory is even better.
Now that someone else has looked at this code, I have a question - or an idea, or something.
Look at the original, lines 255-256 - you have the same code.
It seems to me that this is far too complicated for what it does. Here’s the code with a little extra whitespace… [code]sampleBuffer[0][j] =
readBuffer[j * byteAlign + 3] * 0x1000000 +
readBuffer[j * byteAlign + 2] * 0x10000 +
readBuffer[j * byteAlign + 1] * 0x100 +
readBuffer[j * byteAlign + 0];
sampleBuffer[1][j] =
readBuffer[j * byteAlign + 7] * 0x1000000 +
readBuffer[j * byteAlign + 6] * 0x10000 +
readBuffer[j * byteAlign + 5] * 0x100 +
readBuffer[j * byteAlign + 4];[/code]
First, I claim that this code only makes sense if “byteAlign” is always 8. If it’s less than 8, samples will overlap in the buffer - if greater than 8, there will be gaps.
Second, I claim that byteAlign should really be called something like sizeOfTwoSamplesInBytes - so all the samples here are assumed to be 4 bytes long, and all the ints we’re reading should really be int32s (I know on most processors we are running on, int and int32 are the same, but this isn’t universally true.
Now, let’s look at the definition of readBuffer:unsigned char readBuffer[65536 * 4 * 2 * 2 * sizeof(int)];
and suppose we changed this to unsigned int32 readBuffer[65536 * 4 * 2 * 2];
The “* sizeof(Type)” in a malloc or array is a code smell that often results in a refactoring like this…
Now the long lines of code above become… sampleBuffer[0][j] = readBuffer[2 * j];
sampleBuffer[1][j] = readBuffer[2 * j + 1];
But then I can apply that same refactoring again to readBuffer, which is really a two-dimensional array… unsigned int32 readBuffer[65536 * 4 * 2][2];
// ...
for (int j = 0; j < samps; j++)
for (int i = 0; i < 2; ++i)
sampleBuffer[i][j] = readBuffer[j][i];
- just a matrix transposition!
I haven’t actually tried this on your code yet. You implied you were testing it, what are you doing to test, are you running it within this guy’s application?
In fact, the original code isn’t strictly right because of endianness - the code assumes that the samples are “little-endian” which is true of most but not all machines. If you run this code on a big-endian machine, you’ll get garbled samples… though I don’t think any of the platforms that Juce now supports are big-endian.
But why do all this arithmetic, anyway? Just do the matrix transposition.
The final question is whether the matrix transposition itself is necessary. Unfortunately, Juce’s sample buffer has a different memory layout than mpg123s - in JUCE, each track is a separate, continuous track of data, whereas in mpg123 the tracks are interleaved.