BufferingAudioReader Broken with MP3 input sources using WindowsAudio or juce::MP3AudioFormat

I’ve spent the whole day trying to track down why MP3 playback has become stuttery and finally tracked it down to this commit:

I can’t quite see what the commit was intending to fix but I think the stuttering was introduced with this line: auto startPos = ((pos - 1024) / samplesPerBlock) * samplesPerBlock;
The JUCE and Windows MP3 reading can’t skip to arbitrary sample positions. I think the JUCE reader needs a few hundred samples to get going but the WindowsAudio reader is worse, it seems to quantise to a few ms. The CoreAudioReader seems to figure this out behind the scenes so it’s not a problem on macOS.

I imagine what the - 1024 above does is ensure the start position of the block has enough time to properly produce samples.

Is it possible to fix this or revert the change?

One of the main uses for BufferingAudioReader is to decompress files on a background thread i.e. be used in conjunction with MP3s so it really needs to work with that.

The change was to fix an infinite read bug that was reported on GitHub here -

I’ll take another look at this and see if there’s a more comprehensive fix since I’d prefer not to just revert the change.

That would be great. This one’s pretty serious for us.

The issue with

auto startPos = ((pos - 1024) / samplesPerBlock) * samplesPerBlock;

is that when the number of samples being read is < 1024 it can sometimes cause the readNextBufferChunk() method to always think it is up to date and no new blocks will be buffered. For example if pos == samplesPerBlock == 32768 and the buffer size being read in readSamples() is 512, then startPos will always be 0 and endPos will always be numBlocks * 32768. The next bit of readNextBufferChunk() will then assume that we have an up to date chunk and the method returns false:

    if (newBlocks.size() == numBlocks)
        newBlocks.clear (false);
        return false;

If the read timeout of the BufferingAudioReader has been set to -1 then this causes the hang that was reported in the GitHub issue.

If the issue is with pre-buffering some of the audio file then it might have been caused by this commit instead:

Does reverting that help?

No, the issue was introduced with this commit: BufferingAudioReader: Fixed an infinite read bug · juce-framework/JUCE@b3bdfdb · GitHub

If I set JUCE to the one before everything works, if I set it to this it’s broken.

If the problem is that newBlocks.size() == numBlocks is incorrectly breaking the loop, shouldn’t it be a more involved check and see if the blocks ranges are actually the same?

Ok, I see that isn’t actually why newBlocks.size() == numBlocks is breaking the loop, that statement is saying that “the maximum number of blocks has been buffered”.

What’s weird is that if I make this change:
auto pos = (nextReadPosition.load() / samplesPerBlock) * samplesPerBlock
then everything works.

I can’t see why though…
I assumed the intention was to snap blocks to samplesPerBlock boundaries but doesn’t the divide and the multiply cancel out?

Was that your original problem or was it specifically to do with the - 1024?

Yeah it’ll snap the start of each block to multiples of samplesPerBlock. The cause of the loop was the - 1024 so if that change is working for you and fixes the stuttering then I’ll get it added.

Yeah, I think it might inadvertently be ensuring the block starts before the position actually being read but seems to work so if that’s a safe fix that would be great to add.

Hi @ed95, just checking when you’re planning to make this change? If possible I’d like to release a version to my beta team this weekend for testing :crossed_fingers:

Ed’s out today, so I expect this change won’t be made available until next week.

Ah ok. Thanks for letting me know.

This is on develop now (thanks @t0m!)

1 Like