naudio / NLayer

MPEG 1 & 2 Decoder for Layers 1, 2, & 3
MIT License
123 stars 30 forks source link

FIxed Index out of range exception that can occure for some MP3 songs… #22

Closed taranasus closed 3 years ago

taranasus commented 3 years ago

Because of the way it was written, idx could become bigger than SBLIMIT * SSLIMIT within the while, causing an index out of range exception inside the Dequantize method, throwing and stopping to play the song altogether. This fixed that issue allowing the otherwise valid MP3 song to continue playing.

ioctlLR commented 3 years ago

Hmmm... Doing it the suggested way means the last sample will likely be overwritten with an incorrect value. Seems like we should instead just break the loop immediately. Another option is to subtract 3 from the exit case in the while definition (line 1388) so we always have a multiple of 4 samples decoded, which is probably closer to what the spec calls for (I've not read it in years so I don't remember offhand). Thoughts?

taranasus commented 3 years ago

I agree that this is probably not the best approach, and the place where the file has the problem and this case is hit it does produce a tiny audio artefact. I'd say let's go with the second option that you think is closer to the spec, as my fix creates fake data while breaking the loop might produce incorrect / incomplete data.

I'm no authority on MP3 decoding, I just want this to work. I'll make the subtract 3 change now

taranasus commented 3 years ago

Awesome, thanks for approving. Do I still have to do anything or is that it and it will be merged at some point?

basisbit commented 3 years ago

Where does the - 3 origin from? -1 could have been plausible, but where are those other two increases done?

taranasus commented 3 years ago

Where does the - 3 origin from? -1 could have been plausible, but where are those other two increases done?

If you expand the file, to see the whole while loop you'll see it looks like this:

while (part3end > _bitRes.BitsRead && idx < SBLIMIT * SSLIMIT - 3)
{
    Huffman.Decode(_bitRes, h, out x, out y, out v, out w);
    _samples[ch][idx] = Dequantize(idx, v, gr, ch); ++idx;
    _samples[ch][idx] = Dequantize(idx, w, gr, ch); ++idx;
    _samples[ch][idx] = Dequantize(idx, x, gr, ch); ++idx;
    _samples[ch][idx] = Dequantize(idx, y, gr, ch); ++idx;
}

So we're doing -3 to make sure that it gets through the last Dequantize without going IndexOutOfRange

LeatonChuter commented 3 years ago

Is this going to be merged in? I'm trying to do MP3 > WAV conversion in a docker environment using NLayer and getting this error.