Bug: Pinmame has buzzing/crackling with DCS machines (timing issue?) about pinmame HOT 45 CLOSED

There is one little error I ran across in the DCS board code that might be worth fixing, although I doubt it's responsible for anything as noticeable as this. The code that handles the autobuffer transfers is off by one, so it ends up dropping 1 sample out of every 480. In src/wpc/wmssnd.c, at line 1667, you'll notice that the calculation of the next autobuffer read pointer is decremented by one after figuring the buffer quadrant. It took me a lot of head-scratching to figure out why it's doing that, but after some experimentation I think it was basically an ad hoc hack because the ROM code won't work if you use the "obvious correct" calculation without the -1. The reason the "correct" calculation doesn't work is that the 4th quadrant pointer will point one word past the end of the buffer, which can't happen on the ADSP-2105 hardware - the original hardware always wraps the pointer at the end. The ROM code seems to be written in such a way that it depends upon that, so the buffer never gets filled if that 4th quadrant offset isn't inside the buffer. I believe the correct solution is to remove the -1 and replace it with a (% adsp_aBufData.size) applied to the whole calculation, so that the 4th quadrant offset wraps back to zero, like in the original hardware.

  next = (adsp_aBufData.size / adsp_aBufData.step * adsp_aBufData.irqCount /  DCS_IRQSTEPS) 
         * adsp_aBufData.step;

 cpunum_set_reg(dcslocals.brdData.cpuNo, ADSP2100_I0 + adsp_aBufData.iReg,
                adsp_aBufData.start + (next % adsp_aBufData.size));

Changing this doesn't result in any difference that's audible to my ears, but even so, I'm pretty sure that the existing code is wrong, both with respect to the original hardware and just in terms of the sample generation. adsp_irqGen's timer is set up to assume that exactly adsp_aBufData.size samples will be generated in a certain number of emulated clock cycles, and the code as-is actually generates adsp_abufData.size - 1 samples per cycle, so the overall cycle timing is just slightly off, by one part in 480. The CPU throttling code might be making up for that speed difference, so maybe that's not introducing any glitching on its own. But the ROM code doesn't know about the off-by-one error, so it's happily generating 480 samples on every cycle anyway, and only 479 are making it into the MAME audio stream, so we're definitely not reconstructing the original stream completely faithfully.

(It is interesting to note that one sample in 480 at the DCS 31250Hz sampling rate corresponds to 65 incorrect samples per second - that is, 65 samples in the decompression stream that differ from the corresponding samples in the original stream. Maybe that could be audible as a noise signal centered around 65Hz.)

from pinmame.

I think i tracked it down: @djrobx changed something in the same timeframe: 4f6a5ed#diff-38fdb5dc0c7009edb3085a85cfe3a7b89d445f479874077da5bfb1ac903fc9c0

I don't know exactly why this buzzing happens yet, but it seems like its gone if i remove it again.

from pinmame.

Thanks just did a quick test and hot damn, it sounds amazing now! Thanks a lot mjrgh!

from pinmame.

Some explanations on this:

Yes, you're right, some games now feature some slightly audible buzz (or whatever one wants to call it), so i would also be interested in a more complete list if possible (as you mentioned that you can hear it on more machines), which would simplify my tweaking of the code tremendously. From my experience, especially DCS-based games are affected at the moment, but still a detailed list of specific games would be nice.

So why is this the case in comparison to 2.7?
2.8 completely revamped the up/downsampling and filtering of all audio output, bringing it to modern standards (i.e. proper resampling like professional audio software does do it since years/decades, along with better low pass filtering, also matching the actual hardware better, and a lot of tweaks to the audio cores themselves to be a bit more precise).
While this usually tremendously improves the audio quality in comparison to the old interpolation-based code (Gottliebs are a prominent example), it also reveals some issues on some machines that were not audible before.
These can be seperated into: Whacky PinMAME sound emulation code that worked before by accident (not the case with e.g. TOM) and less aggressive filtering (most likely the case here).

With DCS my theory is that this 'buzz' is actually an artefact of the DCS (lossy) compression itself, and the filtering hardware of real pins aggressively filtering that annoying part out in practice (low pass).
I didn't want to be too aggressive with the filtering though (to keep the samples 'clearer'), but maybe we need to be, so to match real HW better. :/

So please provide a list on which machines you think we should do better.

Also: Is 'Resampling Quality' set to '1' in your PinMAME options?

from pinmame.

But then again, i just checked the MAME codebase, and they do not seem to use any DCS filtering at all AND have sound without buzz/noise???

from pinmame.

But then again, i just checked the MAME codebase, and they do not seem to use any DCS filtering at all AND have sound without buzz/noise???

Keep in mind that the original DCS boards were doing actual, PHYSICAL D/A conversion, so they required the standard post-DAC analog domain filters to remove the reconstruction noise inherent in D/A conversion. MAME/VPM don't need reconstruction filters, because they operate purely in the digital domain - the physical conversion to analog happens in the PC sound card hardware, which necessarily has its own post-DAC reconstruction filters. Those would take the place of the corresponding reconstruction filters in the DCS hardware.

Now, there are two possibilities about the purpose of the filters on the original DCS hardware. Possibility 1 is that they're purely post-DAC reconstruction filters. If that's the case, then it's unnecessary to replicate them in MAME/VPM, since there's not actually a DAC in the emulated audio chain at that stage. Possibility 2 is that they're meant to be combined reconstruction filters and (per your theory) "compression artifact" removal filters. My guess would be possibility 1 - that the low-pass filters are there purely as reconstruction filters. Compression artifacts don't usually manifest as simple high-frequency noise - if you think back to the early low-bandwidth MP3 days, the problem that those tracks had was NOT a high-frequency hiss. This would accord with the observation that MAME doesn't implement the filters.

If this new buzzy artifact was introduced in work on the sample rate conversion code, I'd immediately suspect that there's simply an error somewhere in one of the sample rate converters. Square-wave buzzy noise is EXACTLY what buggy digital-domain sample rate conversion tends to sound like.

from pinmame.

..i would like to go with this theory, but i just finished up testing a whole bunch of different non-DCS machines, and none is showing that issue (if somebody knows of some machines though, i'm happy to know in order to further nail down this investigation!), e.g. AlvinG, GTS3, Sys11, DE/Sega, SAM.

And the sample rate converter is basically one monolithic code path, so all machines have to go through the same code in there, plus that lib is used by a whole bunch of other apps in the wild (unless there was some other weird bug sneaking into 2.8 in the DCS code).

from pinmame.

Hi again! Sorry for the lack of testing and reporting what games are affected, I only started checking this out a couple of days ago since I found a local pinball place (a couple of enthusiasts who have their machines available once a week). So I wanted to get some practice in and learn some rulesets since my access is limited :) excellent stuff from the tables I've played.

Congo was another that had this issue, so it seems like it's just DCS stuff.

Maybe some kind of filter setting would help, to not have to mess around with speaker/EQ settings on our setups. Admittedly you don't really notice this much through speakers, but through headphones it's very apparent (which is what I was using as I just got into virtual pinball, definitely need to look into building a VP table in the future though!).

Edit: Yes I've tried setting the resampling quality to 1.

from pinmame.

And the sample rate converter is basically one monolithic code path,

Just to clarify, I didn't mean that there was necessarily a bug in the sample rate converter code itself - that's presumably a third-party library that's been well tested. My first suspicion would actually be in something calling the sample rate converter code, most likely just someone setting the wrong sample rate somewhere in a setup call.

I don't remember the DCS stuff well enough, but is there any chance that some of the games just use a different sampling rate out of the DCS decoder? There are something like three different DCS decoder versions in there with slight differences.

from pinmame.

In theory there is an assert that should trigger in my own build if its different from the fixed one (31250Hz), but i'll double check. Maybe i also screwed up some other part during the DCS conversion, i'll also double check.

from pinmame.

Nope, sorry. Don't know what i did different this afternoon (tested the wrong version by accident???), but the buzz is now back, even when reverting that one. :/

@mjrgh I doublechecked the sampling rate, its always just set to 31250, same as the init (so no change).
I also ruled out the resampling by setting the output rate to 31250 (mixer.c has a special codepath that gets used then).
And i ruled out the low pass filtering by disabling the define.
Buzz still there. :/

from pinmame.

Are there tags or commit numbers for the 2.7 and 2.8 releases? I don't see them in the tag lists. I was thinking I could try diffing the two versions and see if I can spot anything suspicious - the culprit pretty much has to be a change made between those two versions, if the bug reliably reproduces after but not before.

from pinmame.

You can just go with the dates from the release notes/whatsnew.txt for that

from pinmame.

Here's the diff, for anyone else who wants to try finding it by inspection:

https://github.com/vpinball/pinmame/compare/f2476cbb361068575146b505c5f4d7d2473ffbce..2ddfb10e4f60c9b0ed7122ca655e36b30bc27bbc

It's a large diff, but I suspect there are only a few files that are likely to be involved:

• src/cpu/adsp2100/2100ops.c
• src/sound/filter.c
• src/sound/filter.h
• src/sound/mixer.c
• src/sound/mixer.h
• src/sound/streams.c
• src/sound/streams.h
• src/windows/sound.c
• src/wpc/core.c
• src/wpc/core.h
• src/wpc/wmssnd.c
• src/wpc/wpc.c
• src/wpc/wpc.h

...and of those, probably only the last 3 are the most likely problem points, and the most likely of all is src/wpc/wmssnd.c, since that's specifically where the DCS decoder is implemented. The others are all common code paths used by many games, which makes them less suspect, but doesn't rule them out entirely - it's always possible that it's a subtle interaction between a change in common code and some specific use case of the common code that's only exercised by the one caller.

from pinmame.

@toxieainc, one thing that might be worth a quick try is reverting the changes at the following lines in src/wpc/wmssnd.c (line numbers reflect old/new sides in the diff linked above):

1425/1474
1431/1480
2293/2342
2299/2348

I think I made those changes on the theory that the extra shifts were guaranteed to have no effect and could be removed for efficiency, but the efficiency difference would be trivial, and looking at it now, I'm wondering if the high bit could actually be set in some cases (that's where the extra shift would matter - it has the effect of masking the high bit). Maybe the extra bit is there in the data streams for certain games.

from pinmame.

from pinmame.

Nice suggestions, but the issue also happens if one disables the WPCSPEEDUP thingie.. (so that code is not triggered then)

from pinmame.

but the issue also happens if one disables the WPCSPEEDUP thingie..

Ah, okay, that rules out that part of the code, at least.

Couldn't it rather be the cast: (INT32)mx0 * my0 ?
In the above case, will only mx0 be casted, or the product of the two?

That code is correct as written. The cast is intentionally applied to the operand prior to the multiply, which is necessary to ensure correct results portably (in particular, in environments where int is 16 bits). Applying the cast post-multiply (i.e., writing it as (INT32)(mx0 * my0)) wouldn't have the desired effect of widening the operands pre-multiply, and would in fact have no effect at all, since the result is already implicitly converted to INT32 anyway in the assignment to tmp (which is declared as an INT32). You could for the sake of clarity add a separate (INT32) cast to my0, but that's unnecessary as it's already implicit (from the C binary op type promotion rules). You could likewise add parens around the whole mx0 expression (as in ((INT32)mx0)) to make the precedence explicit to a human reader, and to convey that the pre-multiply widening is the intent rather than an accident - so perhaps writing ((INT32)mx0) * ((INT32)my0) would be the way to make the meaning and intention most apparent to a human reader. It's unambiguous to the compiler either way, but the C integer promotion rules do tend to be a bit arcane for human readers. The trade-off is that it's a lot of extra verbiage, and someone down the road is probably going to be tempted to "clean it up" by changing it back; maybe a macro with a commented explanation would be in order, since the same expression is repeated a few times in there.

from pinmame.

some more intermediate findings:
reverting the whole wmssnd.c and patching it up to work again doesn't change anything
same for all of adsp2100

then, as said, the resampler is also out, as changing the output to 31250Hz skips it

so by now i would suspect some timing difference or another part of the sound refactor work

from pinmame.

@volkenborn as you still use an ancient compiler ;) , could you please check if you also hear this subtle buzz/noise, e.g. on TOM (just start the machine and keep toggling the volume up/down and you should hear it)
just to rule out a compilation based change that happened (compiler optimization and the like)

from pinmame.

reverting the whole wmssnd.c and patching it up to work again doesn't change anything
same for all of adsp2100 [and the resampler]

That's pretty perplexing. I'd propose trying to narrow it down to a single commit by building each commit sequentially, starting at 2ddfb10, and testing each one until identifying the one where the buzz appears. But it seems prohibitively tedious with 264 commits in the range. Maybe a binary search is in order - it should only take about log2(264) ~ 8 attempts to reach the leaf node. :)

from pinmame.

Yup, already on it.. And now comes the sad part:
If i just build 2.7: Same issue :/
And now even more sad: If i download the official 2.7 from vpforums: Same issue

@riggles1: Are you 100% sure this was 2.7 you tested with for the first test?

@ everybody: Can you guys please do a test with 2.7 and see if you also get the buzzing there, same as me?

from pinmame.

Initially I tested 2.6, but I also tested 2.7 to see where the change was made, unless I did something wrong when changing from 2.6 to 2.7. If I didn't do anything wrong while testing then the audio performance is the same between 2.6 and 2.7.

from pinmame.

from pinmame.

So i fear we are in some timing issue territory here??
It does not make any sense to me. @mjrgh Do you think you could also take a look maybe and test a bit, so that we know how things work/react on your setup?

from pinmame.

I'm not sure how I can help. I have noticed popping/distortion in Theatre of magic/Totan/World Cup soccer 94. I'm using the base version of vpinmame that's bundled in the latest baller.

from pinmame.

Yup, thanks for the tests! The problem here is now that this is not really a regression (see my test above, plus i also hear exactly the same issue with the old builds). It seems like you were just lucky to not hear it before. Same for gaston who apparently is still lucky, even with the latest releases. :/
So it must be some evil timing bug that we have to track down.

from pinmame.

from pinmame.

You have to listen with headphones, otherwise its (at least for me) impossible to hear.

from pinmame.

Depends on how sensitive you are to buzzing, no other games give me crackling noises like that, listen to the example audio I posted, this same buzz is present in videos where people are playing those tables. It's just DCS machines that have the issue, so my workaround right now is to just not play DCS machines for the time being.

from pinmame.

from pinmame.

I notice it either way, lowering vpin and internal ROM sounds, only thing that helps a bit is going into Windows EQ settings and lowpass filtering the sound through that, but it's annoying to do every time only for DCS tables.

If you notice it using your particular speakers or not, it's still an issue that can be addressed and some investigation to the cause has already progressed above in this thread, so it's worth having this issue open until there's an answer to what is causing it.

from pinmame.

Commited, thanks a lot!!

from pinmame.

Hmmmm.. Actually i cannot hear the TOM crackling anymore now with the official build. :)

@riggles1 Can you also please verify?

@mjrgh Maybe this subtle fix actually has even more implications as the ones in your detailed analysis (i.e. some of the weird special case handling that i never fully understood is now not triggered anymore)?

from pinmame.

Will verify next weekend, never built pinmame from source before either, but I have done so for regular mame so I'll figure it out I think. Great news! Super excited to test this :D

from pinmame.

@riggles1 No need to build it by yourself, there are automatic builds available here: https://github.com/vpinball/pinmame/actions
You need to select the one you want (pinmame, vpinmame,...) then you get the latest

from pinmame.

It seems to me that this also fixed the issue with noise when cranking up the volume to the max in the menu??

from pinmame.

Thanks just did a quick test and hot damn, it sounds amazing now!

That's great news!

Perhaps related, we can probably remove the DCS_LOWPASS code at this point. I think it was there mostly in the hope that it would have some effect on the mystery noise problem, and now that it looks like we've found the actual source of the mystery noise, we don't have to resort to attenuating the whole signal in an (unsuccessful) attempt to remove it. I don't notice a difference after disabling DCS_LOWPASS now, and if you think about it at a DSP theory level, you shouldn't be able to hear a difference. You can't filter noise components above 15.6 kHz out of a 31250Hz discrete signal because a 31250Hz discrete signal can't mathematically represent frequencies that high. The filter isn't a brick wall, so it's still doing a little filtering a little at the high end below the 1/2 sampling rate point, but I suspect that's not having any beneficial effect and is only degrading the audio fidelity by rolling off the treble end excessively. We do still need post-physical-DAC filtering, of course, but the PC sound card necessarily does that, plus the sample rate conversion library has to do its own filtering post up-sampling. Anyway, seems at least worth experimenting with to see if anyone can actually hear a reason to leave it in place.

from pinmame.

It seems to me that this also fixed the issue with noise when cranking up the volume to the max in the menu??

Yeah, the volume-to-max problem was probably always a manifestation of this same bug. The noise was coming from the slope discontinuities where you went straight from sample N to sample N+2, which made little pointy corners in the waveform where it should have been smooth. The bigger the gap between the PCM levels of the two samples, the sharper the corner, and the louder the noise - I'm guessing it's superlinear with respect to the nominal volume, in which case it would become more noticeable at higher settings and especially loud at the last couple of volume notches. The problem was probably never "DCS gets distorted when turned up all the way" - it was probably actually "DCS distortion that's always there becomes less obnoxious when you turn the volume way down".

from pinmame.

I thought about the same with the lowpass filter. But here it gets tricky: Do we want the 'clean' quality, or do we actually want to mimic the original sound board, which after all includes these filters? As you say, this is not a brick wall, it will also remove/dampen some frequencies before the cutoff.

from pinmame.

btw: Please also double check my recent commit. Its just a detail, but you're the expert with DCS now.

from pinmame.

I thought about the same with the lowpass filter. But here it gets tricky: Do we want the 'clean' quality, or do we actually want to mimic the original sound board, which after all includes these filters? As you say, this is not a brick wall, it will also remove/dampen some frequencies before the cutoff.

That's a fair point. Given that my whole rationale here is that it probably doesn't make any audible difference, I think it's fine either way as a practical matter. Plus it's already all #ifdef'd for easy configuration, so this is all just a question of what the default setting for the public build ought to be. If a number of people were to A/B test it and find that it really doesn't make an audible difference to anyone, I'd say it's better to set the default configuration to remove it, because if no one can hear it, it's just burning up CPU cycles for no good reason. The emulation purism is a separate axis, I suppose, but I think for something relatively modern like DCS I'd favor audio fidelity (playing back the original recordings to the best ability of modern hardware) over strict re-creation (sounding exactly like an original Medieval Madness machine). I don't think most desktop VP users would care about sounding exactly like the original (since their playing experience is already so far removed from that), and I even think most pin cab users would come down on the audio fidelity side (since they're all building 200W/channel 11-channel Dolby Atmos systems these days).

from pinmame.

Good point.
Although also quite some emulation users buy higher end gfx boards and 4k monitors in order to being able to simulate CRT 'defects' properly (including myself). ;)

But i also tend towards removing the lowpass now, as you say the HW is new enough in order to prefer quality over accuracy in this case.

from pinmame.

..and undefd again! Thanks once more @mjrgh !

from pinmame.

btw: Please also double check my recent commit. Its just a detail, but you're the expert with DCS now.

I just took a look - looks good to me. (Is that status == 0 case in dcs_txData() what you were referring to with your comment about the weird special case? If so I agree there might be some better approach there, but that code should only get hit when the sound board gets rebooted, which as far as I can tell happens only at startup and when entering and exiting the operator menus. Sound should be muted at those times anyway, so there shouldn't be much chance of glitches. I wonder if we can just delete that gap-filling special case, and instead let any gaps fill with zeroes in dcs_dacupdate() when it runs the buffer dry. The timing of the gaps might be a little off that way, but I'm not sure the timing is any more accurate with the existing approach anyway, and given that this only should happen during periods of silence, maybe it just doesn't really matter.)

from pinmame.