Integer overflow in Opal resampling for mix rates >= 65536 Hz

[sagamusix]

Note: I'm not a libADLMIDI user, so I don't know if this condition can practically occur in that context. But as the Opal C port found in this repository has been advertised as usable for libxmp for example, I think it would make sense to fix this issue even if libADLMIDI is not directly affected.

Recently it was reported that the OPL emulation (via the original Opal C++ implementation) in OpenMPT causes clicks when the mix rate is very high. I narrowed it down to the linear interpolation code: Once the mix rate exceeds 65536 Hz, the signed 16-bit integer output of Opal will be multiplied with the linear interpolation fractional position which at that point will be 17 bits or more, so the multiplication result may be a number that needs 33 bits or more to be represented. But the code as it is written will result in a 32 bit signed integer and thus causes wraparounds and thus large clicks in the output.

There are at least two ways to avoid this:

• Change multiplication result to be a 64 bit integer by casting one of the two multiplicands to int64_t or equivalent. Easy solution but involves four 64-bit multiplications, which may or may not be desirable.
• Scale the fractional position to always be between 0 and 65535. This has the advantage that you only need to divide by SampleRate once when computing the fractional position. In the left and right channel resampling, you will simply be dividing by 65536 which can be expressed by shifting 16 bits to the right. This variant is potentially faster especially on platforms with slow integer division. This is the variant I chose for OpenMPT's fix.

[Wohlstand]

Hello! Interesting, I should take the fix to my end too. Thanks for the report!

[Wohlstand]

Actually, it would fail if run the libADLMIDI with the "PCM sample rate" mode, and set such of sample rate here.

[sagamusix]

Note that if you want to verify this yourself, the chances of producing an integer wraparound are naturally bigger if

1. The OPL output is very loud (lots of voices playing simultaneously) or
2. The mix rate is very high. So at 96 kHz the probability of clicks is lower than at 192 kHz, for example.

[Wohlstand]

Fixed just now! Please test out the thing how it works for you.

[sagamusix]

Looks good in general but I don't think it's necessary to emulate OpenMPT's helper function in every detail :) mul32to64 exists in our codebase because it offers a faster way of doing a 32x32->64 multiplication on some platforms (well, MSVC specifically), while falling back to a full 64-bit multiplication on other platforms. Since you only implement the general (non-optimized) case, I think it's fine to inline that multplication. Also for this particular computation the clamping to 32-bit range is not needed, because the end result of the multiplication-division is always between 0 and 65536. So the muldiv implementation can probably also be inlined. :)

[Wohlstand]

I did on a quick hand, but right, I could simplify the thing as it's no necessary to have that as multiple separate functions

[Wohlstand]

Okay, I closing this as this is fully completed now.