Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[YM2612] Incorrect sampling/clock rates (highly modulated timbres sound wrong) #50

Closed
freq-mod opened this issue Nov 27, 2021 · 21 comments

Comments

@freq-mod
Copy link

Should test it at the very beginning, before checking SSG-EG and DAC distortion...
output.zip - contains ROM file and recording how should it sound like. On picodrive (all versions) it doesn't sound right. I suspect, YM2612 clock rate, or sampling rate (should be 53267 khz iirc) aren't right

@irixxxx
Copy link
Owner

irixxxx commented Nov 28, 2021

It can't be that, else all music would sound wrong, which it definitely doesn't. Is this using some rarely used feature tm the OPN?

@freq-mod
Copy link
Author

Nah, the OPN chips all use the different sampling rates. OPN(A) runs at 55466, OPN2 at 53267 khz. Some OPNA instruments that rely on aliasing noise (like the one from the ROM file) sound wrong on OPN2, because the rates are mismatching, and pitch adjustments must be made. Here, picodrive sounds differently than MAME/Nuked, so I suppose, that's because the sampling rate is incorrect at least.

@irixxxx
Copy link
Owner

irixxxx commented Nov 28, 2021

No, I'm sure the clock is correct for ntsc. 53693100/7/(6*24) = 53267.

@irixxxx
Copy link
Owner

irixxxx commented Nov 28, 2021

An aliasing problem? It all sounds completely different if the emu sample rate is adjusted.

@freq-mod
Copy link
Author

So, what else can cause the difference between MAME/Nuked and this?
BambooTracker ulility for OPNA composing had the same problem - certain instruments sounded incorrectly. It was caused by sampling rate: BambooTracker/BambooTracker#25

@irixxxx
Copy link
Owner

irixxxx commented Nov 28, 2021

That supports my theory that it's an aliasing problem. Or better said, a resampling problem. The resampling filter in picodrive is the most simple one - it just takes the last calculated FM sample at the 44.1 KHz sample points (line 1047 ff in picoo/sound/ym2612.c). I experimented with nearest neighbour (line 1035), 3-level quantized linear (line 1009), and linear (line 998). You could try yourself, but I think linear should improve the sound. However, without a proper resampling FIR filter it won't really get much better. I haven't checked, but I reckon such a filter is probably too expensive for picodrive which is very much optimized for speed. Any suggestions for a simple and fast resampler welcome.
I also can't use the Bambootracker approach, since then I'd have to resample CD audio from 44100 to 53267 Hz - same problem for another audio source :-/ .

@freq-mod
Copy link
Author

You could try yourself

I either got a ton of compile errors, or white noise. Still, the problem is how a different resampler would affect weaker ARM devices like GP2X, Miyoo, etc...

@irixxxx
Copy link
Owner

irixxxx commented Nov 29, 2021

It's actually a worst case. I'd normally use a polyphase FIR filter for this. Basically that's "splitting" the task to resample by an M/N factor into upsampling by M and downsampling by N. That works well if you can reduce the fraction to some small numbers. That's not possible here since N=53267 is a prime number :-/ So, no dice for a polyphase filter here. Even "cheating" by using a frequency slightly off isn't of much use.
Using blargg's blip buffer would be an alternative. I don't know how efficient that is, but AFAICS it's about 30 mults and adds per sample. Compare that to the about a dozen instructions without any multiplications the current code uses...

@irixxxx
Copy link
Owner

irixxxx commented Nov 30, 2021

Hmm, I could maybe create a polyphase 5/6 filter (with 15 and 12 taps for the FIRs), that would amount to 44389 Hz, then do nearest neighbour to reach 44100. No idea if that would give proper results. Rather brute force, and the filters would probably be of mediocre quality.
Minimally it introduces an aliasing at 289 Hz, because the nearest neighbour sample selection would introduce this. No idea if this could be heard, and also no idea if this would preserve the aliasing in the source signal. IIRC it would only amount to about half a dozen mult/add ops per sample, though.

@irixxxx
Copy link
Owner

irixxxx commented Dec 1, 2021

I made some experiments with a 40 tap polyphase FIR for resampling of the 53267 Hz signal to 44389 Hz (ratio 5/6), close enough for some experiments.
The filter requires 8 mul/add ops per sample, and it also requires that all YM output operators must be calculated - ATM in picodrive it's nearest neighbour for resampling of the YM output, which drops roughly every 6th value for which the output isn't computed. So, it creates a noticeable overhead, but I haven't measured this on a slow system for now.

The filter has a sample rate of 5*53267 (roughly 233.3 KHz), a stop band from 20 KHz to Nyquist, and an attenuation of about -50 dB. It's not very good, but it works good enough to show that the resampling apparently destroys the sounds created by the aliasing effect in the YM :-/
Besides it produces some audible crackling which sounds like quantization noise to me. I haven't looked for the reason though, but if anyone has an idea let me know.

Changing the picodrive output rate to that of the YM produces the effect. It's no viable solution though, since I then would have to resample all CD output which is always at 44100 Hz to 53267 Hz, which isn't probably any better.

I'm out of ideas for now. I'm however not a dsp engineer. Any help would be appreciated.
BTW, have you checked if this is working correctly with +GX at 44100 Hz sample rate?

@irixxxx
Copy link
Owner

irixxxx commented Dec 10, 2021

Here is a very experimental non-final patch (non-ARM CPUs only). Could you check if this is better?
y.txt

@freq-mod
Copy link
Author

I will test it at evening...

@freq-mod
Copy link
Author

MUCH better! Still, very slightly off from intended sound, but the difference is nowhere as apparent as it was earlier. Thanks!

@irixxxx
Copy link
Owner

irixxxx commented Dec 11, 2021

This needs much more scrutiny. In the original code there are a lot of shortcuts to save on processing. The patch removes nearly all of this, at the price of needing a lot of more cpu in certain cases. It needs more analysis to find a good balance.

@irixxxx
Copy link
Owner

irixxxx commented Dec 22, 2021

Here's a more elaborate patch. Please give this some in-depth scrutiny if possible. There's both the generic C as well as the ARM version. I had to change quite a bit to minimize the speed loss introduced by this. At 44100 Hz sample rate it's not much worse than before, but at lower bitrates it needs more cpu power.
y.txt
(updated patch - removed some leftover testing stuff from ARM code)

@freq-mod
Copy link
Author

OK, so: on x86_64 Linux it works, though it's not 100% perfect. Very close, still (I might upload a recording of what is it now, if needed) ARM is the same, works, doesn't have any slowdowns, I think nothing else got broken. Don't know what else should I check out.

@irixxxx
Copy link
Owner

irixxxx commented Dec 23, 2021

OK thanks... I'm going to modify the patch once more. I have an idea to improve load for low bitrates, at the expense of lower sound accuracy. Since I reckon lower bitrates are only selected where the computational overhead hurts it's probably ok that way ;-)

@freq-mod
Copy link
Author

I got notified of a new patch, don't see it? It was broken?

@DrUm78
Copy link

DrUm78 commented Dec 24, 2021

Yep, too many sound artifacts at low bitrate, so irixxxx removed it I guess.

@irixxxx
Copy link
Owner

irixxxx commented Dec 24, 2021

Yes. Nice idea, but too many audible artifacts at low bitrates. I need to ponder over this some more.

@irixxxx
Copy link
Owner

irixxxx commented Dec 24, 2021

Should be preliminarily fixed by d127b3f, although speed considerations remain open. That's something for next year...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants