libertyernie / brawltools

BrawlBox and BrawlLib
http://forums.kc-mm.com/index.php?topic=67847.0
143 stars 31 forks source link

Problem With PCM16 RSTM Encoding: Inconsistency Found Near End of Stream #115

Closed gheskett closed 7 years ago

gheskett commented 7 years ago

While encoding with PCM16, it seems that the audio near the very end of the resulting stream tends to get messed up in unusual ways. Through inspection, I've found instances of non-uniform audio jumps and even silence just sitting at the end of the stream just before looping. Here, I've provided an example of what is going on near the looping point with PCM16 and an example of how it should sound with ADPCM encoding. I've also included copies of the WAV file I used before and after the encoding in case it helps to inspect the audio through an external program.

https://www.mediafire.com/folder/eyguyucbvl709/Looping_Inconsistency

libertyernie commented 7 years ago

Do you think this could be the same as issue #113?

On Apr 16, 2017 6:18 AM, "gheskett" notifications@github.com wrote:

While encoding with PCM16, it seems that the audio near the very end of the resulting stream tends to get messed up in unusual ways. Through inspection, I've found instances of non-uniform audio jumps and even silence just sitting at the end of the stream just before looping. Here, I've provided an example of what is going on near the looping point with PCM16 and an example of how it should sound with ADPCM encoding. I've also included copies of the WAV file I used before and after the encoding in case it helps to inspect the audio through an external program.

https://www.mediafire.com/folder/eyguyucbvl709/Looping_Inconsistency

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/libertyernie/brawltools/issues/115, or mute the thread https://github.com/notifications/unsubscribe-auth/ADj1S-42BK6O4OHDybvaaldtXnv9JZjnks5rwfj7gaJpZM4M-mkg .

gheskett commented 7 years ago

The two issues are definitely different problems. The strange inconsistencies have seemingly nothing to do with the preview player. I actually built BrawlBox with the Issue 113 fix on my own in order to ensure that the two were completely unrelated. Both versions of BrawlBox seem to have the exact same problem.

Not sure if this is particularly helpful, but it seems like the inconsistencies produced are exactly the same if you provide the same looping points. However, if you provide 2 different looping points that scale up to the same point (i.e. becoming a multiple of 14336), the inconsistencies produced appear to be different, some hardly significant.

libertyernie commented 7 years ago

See if it works now. I think I found the issue.

gheskett commented 7 years ago

A great bulk of the issue appears to be resolved, yet there still remains a slight error that I managed to find. Using the same audio and loop points as the previous example, I managed to encode a nearly flawless RSTM with PCM16, except for that 5 samples in the right channel were silenced and offset starting at sample 962,560. Because of this, there exists loss at the loop point and a slight popping sound where the silence occurs in the right channel. Here, I've included the original WAV, the PCM16 RSTM file, the extracted resulting WAV, and an image that shows where the issue is occurring and how it offsets the rest of the right channel by 5 samples.

https://www.mediafire.com/folder/bfrsgnj7tqfuc/Sub-Issue

EDIT: After looking at the RSTM file more closely, I have discovered that the issue is likely not a fault of the actual encoder, but a problem with the PCM16 decoder. I created a different WAV from the RSTM file using the vgmstream decoder, and the resulting file did not appear to exhibit the strange silence/offset issue like the other one.

libertyernie commented 7 years ago

There's a little bit of padding that the encoder puts at the end of the last block for each channel (to round the block sizes up to a multiple of 32 bytes.) Since vgmstream's decoder handles this OK, mine should too.

The problem is coming from the decoder assuming the right channel starts 5 bytes earlier than it actually does - it's not expecting the rounding-to-32-bytes padding there.

gheskett commented 7 years ago

Also, please forgive me for adding this here since this is not necessarily a bug, but I do have one quick request: Is there any way you can set the type of RSTM encoding to default to the most recent option selected upon a previous encoding? (If that's not clear, for example, say I code one file as a PCM16 RSTM and decide I want to create another PCM16 file within the same BrawlBox session. After selecting a new file to encode, the default encoding standard would be set to PCM16 instead of ADPCM.) This would be convenient if one wants to loop multiple files at once in the same encoding format, as it can be very easy to accidentally code something as ADPCM when the intention was to encode a PCM16 file. (This feature would only be necessary to save within the same session of BrawlBox. If the application closes, it would probably be fine to default it back to ADPCM.)

libertyernie commented 7 years ago

I think I've fixed this one now. (Unless I've forgotten something else!) Yeah, I can have it "remember" the encoding for the length of a session by just storing that in a static variable. That's not a bad idea.

gheskett commented 7 years ago

I'm not able to reproduce any more bugs with the encoder/decoder, so I think (and hope) the issue is fixed for good (although the static variable does not appear to have been implemented as of yet).

libertyernie commented 7 years ago

Good! I'll do the static variable thing later today.

gheskett commented 7 years ago

Okay, I think I found another bug related to the PCM16. So when you manually set a loop point for the stream, the loop gets shifted to a factor of a particular number (for ADPCM it's 14336; not sure what it is for PCM16). With ADPCM, the extra data tacked on to the end is written after the starting loop point. However, with PCM16 it seems like the extra data is written following the end loop point, which is prone to leaving some static at the final loop point. What's weird is that if you set the end looping point to the last sample of the stream, it seems to work fine, but if you place the starting loop at 1 and the end at the total number of samples - 1, BrawlBox will crash.

libertyernie commented 7 years ago

Nice catch! I think I've now fixed that one too. I didn't realize that ReadSamples would go past the loop end point, but it makes sense,