Add AdLib Gold Surround Module emulation #1715

johnnovak · 2022-06-10T13:34:46Z

Here we go, now you can enjoy the Dune soundtrack with all the glorious spatial effects thanks to the emulation of the surround add-on module! 😎 🎧 🎉

As far as I'm aware, this is the only game in existence that makes use of the surround module, if present. All other games that can be configured for AdLib Gold just use the card like a regular OPL3 device (including KGB which I had high hopes for, but nope, it's just standard OPL3 music, and the game never even touches the extended registers).

Note that auto-detection won't work in most games as the official way of detecting the card (according to the official AdLib Gold SDK) is by performing some timed writes & reads to the MMA registers which have something to do with PCM sample playback. Now, PCM playback is only used by the official jukebox program included with the card and some obscure demoscene productions, therefore it's fair to say it's quite pointless to emulate just to make auto-detection work correctly. PCem and 86box do emulate it, though, but frankly, it's a lot of code, and porting all that over would be a quite pointless exercise because of the aforementioned reasons...

Making Dune sound close to the real hardware recordings have necessitated a couple of hacks, please see the individual commit descriptions for details. Like I said, no other game uses the surround module, so in practice we only need to make it sound right for this single game and job done. Here's the recordings (note that the L/R channels are swapped):

https://www.youtube.com/watch?v=gUfGyfbzl9k

About the implementation: emulation of both the stereo processor and the surround processor is needed; with just the surround processor we're only halfway there. Dune uses the pseudo stereo effect that the stereo processor provides, and it also boosts the bass by a hefty +15dB (!) via the stereo processor's equaliser. It turns out both of these effects are vital as the music had been composed with this specific processing chain in place.

I'm using the "ideal" implementation of the surround chip which sounds quite faithful compared to the original recordings to my ear (maybe not in the mathematical sense, but who cares really as long as it sounds good). The library also provides a supposedly more accurate mode that emulates the fixed-point arithmetic of the chip, but on one hand it's quite buggy (fails the sanitizers checks with lots out-of-bound writes), and on the other hand the differences are so minuscule that using the fixed-precision emulation is pointless (pardon the pun!). You'd also need to resample before feeding data into it, which would introduce latency and complexity, etc.

A few ideas for testing (apart from Dune):

Other games that support the AdLib Gold but don't write to the extended control registers still work fine:

http://www.vgmpf.com/Wiki/index.php?title=Category:Games_That_Use_AdLib_Gold_1000_For_Music
http://www.vgmpf.com/Wiki/index.php/Category:Games_That_Use_AdLib_Gold_1000_For_Sound

oplmode=opl3 is 100% unaffected
oplemu modes other than nuked work fine

shermp · 2022-06-11T02:27:20Z

src/hardware/adlib.cpp

+struct {
+	AdlibGoldSurroundProcessor *surround_processor = nullptr;
+	AdlibGoldStereoProcessor *stereo_processor     = nullptr;
+	bool surround_enabled = false;
+} adlib_gold = {};
+
+void adlib_gold_postprocess_and_add_samples(mixer_channel_t &chan,
+                                            int16_t *data, const uint32_t frames)
+{
+	auto frames_left = frames;
+	auto buf         = data;
+
+	while (frames_left--) {
+		StereoFrame frame = {buf[0], buf[1]};
+
+		const auto wet = adlib_gold.surround_processor->Process(frame);
+		frame.left += wet.left;
+		frame.right += wet.right;
+
+		frame = adlib_gold.stereo_processor->Process(frame);
+
+		buf[0] = frame.left;
+		buf[1] = frame.right;
+		buf += 2;
+	}
+
+	chan->AddSamples_s16(frames, data);
+}


I've had a bit of a read, of the code, and I wonder if encapsulation could be improved here.

I have a suggestion (and it's just a suggestion, feel free to tell me to sod off if it's unworkable).

Instead of having the global adlib_gold struct, consider making it an actual class/struct with a constructor, and the adlib_gold_postprocess_and_add_samples function as a method. For bonus points, make the surround_processor and stereo_processor unique pointers.

You can then add an instance of them to the respective OPL3 handlers and init them via the handler Init() methods.

If you use unique_ptr for the two processor variables, they will automatically delete themselves when the handler destructor is called, so they don't need to be explicitly deleted.

No, it's a good recommendation. The same idea had passed my mind, but I had abandoned it because it would introduce another pointer redirection and the global struct approach does the job... which is probably not a good reason, because the performance difference would be insignificant. We should optimise for code clarity & maintainability first, so I'm gonna rework this now.

Once I've done this, I think I'll also move all this stuff into its own adlib_surround.cpp file because adlib.cpp is getting a little bit crowded...

It wouldn't even necessarily have to be more pointer redirection, store the object itself in the handler struct (I'm assuming there's only ever one handler at any given time).

I've encapsulated all new AdLib Gold stuff in a class and separated it out into its own file, but the global stays because it's too convoluted to shoehorn it in into the existing structure in any other way (I've tried and gave up halfway).

As a variation to your current solution, how about adding the AdlibGold adlib_gold object (default constructed, not a pointer) to the Adlib::Module class. Then guard the AdlibGold methods, basically making them no-ops if the adlib gold is not enabled. Maybe with an enabled variable in AdlibGold for the couple of times you need to do something different from standard OPL3.

That would save a bunch of if (adlib_gold) checks, especially in methods like Module::CtrlWrite.

Again, just my thoughts from seeing the current implementation. I'm all for reducing nullptr checks where possible. If you think this would harm performance, or cause other issues, or is in the "too-hard basket", feel free to disregard!

Ahem, or maybe disregard, because you wouldn't be able to use it in the Adlib::Handler would you?

As a variation to your current solution, how about adding the AdlibGold adlib_gold object (default constructed, not a pointer) to the Adlib::Module class.

Yeah that's what I tried to do, basically.

Ahem, or maybe disregard, because you wouldn't be able to use it in the Adlib::Handler would you?

Bingo, and without refactoring the whole Adlib module substantially it's just about pushing the problem somewhere else. You'd still do those checks, just slightly differently. So I opted for the simplest approach than to complicate everything quite a lot, without really solving the underlying problem.

The approach is a nice local maximum, given the OO Adlib scaffolding available. It's clear what's happening (with only a single if-per-call), which is perfect for the scope of the PR.

This is a great discussion though, and I got curious: can we bake out the ifs? This a31abf3 adds a std::function member in the Handler to bake out the if-gold-else branches in the Handler::Generate functions (I didn't go further than that, as it's just a proof-of-concept)

It includes a bit more change to how the Handler's are constructed, but that can be ignored.

The idea is we assign the OPL-specific std::function at the time of Hander-creation, and then use that call for the remainder.

This function-pointer approach is orthogonal to OO-derived design, so it's not a good fit, but just an idea for thought.

Overall, I think the existing Adlib+Handler OO design works to keep the long port-read/write calls DRY, but it's a bit of a maze to work with supporting all the implementations. If possible, I think we should get down to best OPL2 & 3 implementation (Nuked?) and do away with the OO layering. (in future PRs!)

Just adding discussion, not asking for change. Happy with whatever resolution falls out of the prior discussion.

Cheers, I'll peruse your std::function example in detail a bit later, but in general yes, I agree. I've done some testing to make sure all OPL3 implementations are still working, but to be honest, only one gave semi-acceptable results, the rest were quite broken compared to Nuked. I can't imagine anyone wanting to use those. Well, maybe if Nuked is still a performance problem on RPi4... but if that's no longer the case, I'd say nuke the rest (can't stop the puns coming, I'm so very sorry... 🤣)

include/envelope.h

include/mixer.h

src/hardware/adlib_gold.cpp

src/hardware/adlib.cpp

kcgen · 2022-06-12T04:56:22Z

This is a huge functional addition and a pleasure to read and review. Thanks, @johnnovak! 🚀

This library aims to emulate the Yamaha YM7128B Surround Processor. The original goal is to contribute to the emulation of the AdLib Gold sound card, which used such integrated circuit to add surround effects to the audio output, as heard in the beautiful soundtrack of the Dune videogame, made specifically for that rare sound card. https://github.com/TexZK/YM7128B_emu

Further simplify implementation & cleanup

johnnovak · 2022-06-12T05:03:04Z

This is a huge functional addition and a pleasure to read and review. Thanks, @johnnovak! 🚀

Cheers! You surely must have seen that I started using bit_view, and I've noticed that you can't really use bit_view types as function arguments easily (it complained about the copy constructor missing or something). So I figured it's supposed to be used only internally, e.g. function receives some uint8_t, then constructs a bit_view of that value for internal manipulation, then possibly returns another uint8_t. Is that how it's supposed to be used, am I on the right track here?

kcgen · 2022-06-12T05:36:06Z

It's probably still missing some functionality if it's not flexible enough.

Here's the basic use-case:

Create a register type, as a union:

with uint8_t data being the raw bits, which you'd historically have to twiddle
add bit_views into those bits, which gives you named accessors and avoids the twiddling

union MyReg {
    uint8_t data;
    bit_view<3, 1> is_enabled; // bit 3, one-bit-wide
};

Now we can put MyReg my_reg; in some global struct, and assign all 8-bits in an IO port-write call, say:

io_port_write(const uint8_t value) {

    my_reg = {value};

    if (my_reg.is_enabled)
        do_something();
}

Or maybe we don't want to clobber all of its bits, so we use a temporary:

io_port_write(const uint8_t value) {

    // say we have another union+bitview type .. 
    StateRequest request = {value};

    if (request.try_to_enable) {
        do_something_that_might_fail;
        check_if_it_worked;
        my_reg.is_enabled = 1;
    }
}

The bit_view items can:

be assigned to a value: my_reg.is_enabled = 1
be assigned from other views: my_reg.is_enabled = other_reg.is_enabled
have bit-functions called on them, like my_reg.is_enabled.flip().

The entire union data value can be assigned using my_reg = {value}; the parethesis are critical because this {syntax} says "assign to the first struct member", which in our example, is the .data value.

Or you can write to the .data value itself as well my_reg.data = value.

(But my_reg = value won't work, just like my_big_struct = 5 wouldn't work, either)

Union+bit_view types can be passed across function arguments (see the bottom if the bit_view test cases) by reference, pointer, and value. The union+bit_view types can be held in arrays, new'd, and delete'd.

If something breaks or is erroring out - drop it here, and we'll figure it out!

johnnovak · 2022-06-12T06:02:11Z

If something breaks or is erroring out - drop it here, and we'll figure it out!

Cheers, how you described the usage is how I'm basically using it now, that's fine. I hit the problem when I tried to do something like this:

union MyReg {
    uint8_t data;
    bit_view<3, 1> is_enabled;
};

void do_stuff(const MyReg value) {
    ...
}

Then when I try to pass in MyReg from somewhere else, it starts complainig about missing copy constructors, e.g.:

const uint8_t data;
const MyReg value = {data};
do_stuff(data);

kcgen · 2022-06-12T06:18:17Z

Then when I try to pass in MyReg from somewhere else, it starts complaining about missing copy constructors.

Ok, that makes sense.

If we've got do_stuff(const MyReg reg) , the argument type is MyReg which is a struct, so it won't be able to accept a scalar-value (the uint8_t data value). That's like passing do_stuff(5), which under the hood is trying to do reg = 5 (the only way to get the value into the union is reg = {5} or reg.data = 5).

So in the example bit, data won't work.
But value is a MyReg, so that'll will!

const uint8_t data;
const MyReg value = {data};
do_stuff(value); // <- should work

The best way to think of MyReg is that it's a struct, and a struct would have the same problem being directly assigned a scalar-value.

Does that get you past the sticking point?

johnnovak · 2022-06-12T06:27:51Z

do_stuff(value); // <- should work

Sorry, that was a typo; I meant to write do_stuff(value); like you wrote in your example, and that was when I ran into trouble.

kcgen · 2022-06-12T06:52:07Z

do_stuff(value); // <- should work 
Sorry, that was a typo; I meant to write do_stuff(value); like you wrote in your example, and that was when I ran into trouble.

Excellent; thanks!

I didn't have a pass-by-value unit test, and indeed I hit error: call to implicitly-deleted copy constructor of '(anonymous namespace)::Register' when trying it.

I've switched to a trivial copy constructor in the bit_view, so now the compiler should be able to understand how to do copy-by-value for compound objects containing bit_views.

Pushed to main, should be good to go!

johnnovak · 2022-06-12T07:42:30Z

I'm feeling brave @kcgen; I've just turned on check narrowing in a few files I've touched. Here goes nothing! 🤣

kcgen · 2022-06-12T08:00:03Z

Around the ~60s mark into the Dune intro sequence, the waveforms gets pretty hefty and start clipping out on my side using oplemu = compat and mame.

With oplemu = fast, I get the baseline standard OPL3 (expected, I think?)
With oplemu = nuked, I get silence, but still see the debug adlib gold log messages
This is with the last fetch of remotes/origin/jnovak/adlib-gold-surround-fixed-1 (before narrowing checks)
I'm using the floppy images (https://archive.org/details/000739-Dune)
I've configured the game to use Adlib Gold (autodetection seems to work)

Using this INI (and changing oplemu = )

[CPU]
cycles = 6000

[render]
aspect = true

[mixer]
rate = 48000

[sblaster]
sbtype  = none
oplmode = opl3gold
oplemu = compact

[gus]
gus = false

[speaker]
pcspeaker = false
tandy     = off
disney    = false

[autoexec]
imgmount a *.img -t floppy
mount c .
c:
cd dune
dune.bat

Recordings (Ctrl+F6):

Compat

opl3gold_compat.mp4

Mame

opl3gold_mame.mp4

Fast

opl3gold_fast.mp4

Visual comparison, in above order:

johnnovak · 2022-06-12T08:25:00Z

Around the ~60s mark into the Dune intro sequence, the waveforms gets pretty hefty and start clipping out on my side using oplemu = compat and mame.

Yes, so this is expected because the floppy version doesn't scale back the FM volume to about ~25% like the CD version does. The game adds +15dB of bass boost via the Philips stereo processor, so that overdrives the output very badly in case of the floppy version.

With the CD version things work out fine because they scaled the volume back to 25% at the FM output generation level (so not by using the mixer!)

The only course of action for the floppy version to get rid of the clipping is to lower the volume of the FM channel manually in the mixer. Because we're adding the samples as floats, that will get rid of the clipping because the volume scaling is applied to float samples that are over the int16 min/max range.

We're not emulating the full analog path here, so the clipping is probably different and maybe not as severe with real analog circuitry in case of the floppy version. But surely it must have been a real problem, otherwise they wouldn't have scaled back the volume of the CD release to 25%; that's a massive volume drop! My theory is that few users must have complained because the AdLib Gold was a bit of rarity even back then, but they decided to fix the bass boost issue in the CD release anyway (although they could have attenuated the volume only for the AdLib Gold and left it at 100% for plain OPL3, but that's another story...)

The floppy version is expected not to clip when not emulating the AdLib Gold because there's no +15dB bass boost in place without the AdLib Gold's stereo processor. Now the only mystery is why isn't the AdLib Gold processor being applied to the fast OPL type... Looking into this.

Btw, the mame OPL emu sounds absolutely awful, it's missing notes all over the place. So even if we decide to keep other OPL emus other than Nuked, I think the MAME emu has to go. The fast one does a good job at hitting the right notes at the very least (but the subtle details in how notes are modulated are different).

This is with the last fetch of remotes/origin/jnovak/adlib-gold-surround-fixed-1 (before narrowing checks)

That was a bad idea, this PR is big enough already. I've reverted it, sorry.

johnnovak · 2022-06-12T08:37:24Z

Ah, so the AdLib Gold post-processing is not applied in fast OPL mode because that lives in dblopl.cpp and I haven't touched that file at all... Well, that explains it! 😅

I guess I could make it work by exporting adlib_gold, etc. But you were saying before to only keep Nuked, after all, so I'm a bit hesitant to do that.

kcgen · 2022-06-12T08:44:17Z

Thanks for getting me up to speed! That's a relief that it's actually expected; will track down the CD version tomorrow.

I guess I could make it work by exporting adlib_gold, etc. But you were saying before to only keep Nuked, after all, so I'm a bit hesitant to do that.

Yes, I agree it makes sense to hold off. If Nuked can do the job of all of them, and you've determined it's the best of the bunch, then let's run with it (in a subsequent cleanup/reduction PR).

Worst case, if anything bad happens we've always got the others in source control history, ready for revival.

johnnovak · 2022-06-12T11:04:37Z

Thanks for getting me up to speed! That's a relief that it's actually expected; will track down the CD version tomorrow.

This mirror is handy if you just want to grab a single eXoDOS game quickly:
https://the-eye.eu/public/Games/eXo/eXoDOS_v5/

Yes, I agree it makes sense to hold off. If Nuked can do the job of all of them, and you've determined it's the best of the bunch, then let's run with it (in a subsequent cleanup/reduction PR).

Sounds like a plan!

johnnovak · 2022-06-12T13:19:35Z

For reference, with these settings I'm not getting clipping in the floppy version. The key things are sbmixer = false and mixer fm 25 -- yes, you really need to go that low. This makes the volume level similar to that of the CD version. Alternatively, you can just set the master volume to 25, that bypasses the clipping because now we're using a float mixbuffer 😏

[sblaster]
oplmode = opl3gold
sbmixer = false

[autoexec]
mount C "C"
c:
mixer fm 25
call dune
exit

I've also re-enabled the DC offset removal in TDA8425_emu because apparently it has some serious DC drift issues without it...

And some bad news: now I'm quite sure that the filters in TDA8425_emu are unstable. You might notice that sometimes there's just a loud click during the intro and no sound whatsoever after that. If you inspect the rendered output, you'll see that the waveform suddenly jumps from zero to max value and it stays there -- most likely to max infinity because of unstable filters with chaotic behaviour. On some runs they're fine, on some runs they aren't... I ran into this a few times before, but because it's sporadic I was never completely sure whether it's my code or the library -- now it's clear that the library is at fault. 😞

Well, it seems the guy who wrote this wasn't exactly an expert on DSP filter design. Neither am I, but it's fairly easy to reimplement the TDA8425 emulation with the iir1 filters that I trust a lot more, so that's what I'll do instead of trying to troubleshoot & fix the guy's filter code (I'm not really qualified to do that...) After all, we're only talking about a low-shelf, a high-shelf, and an all-pass for the pseudo-stereo mode, and I can use his code (or PCem) as a guide for the control register stuff.

You gotta do what you gotta do, I guess 🤷🏻

You can still test most of this, though; on most runs it works fine 😅 Oh, and the surround emulation library is fine as there's no filter code there, just delays, which are a lot harder to screw up 😜

kcgen · 2022-06-13T03:20:25Z

you inspect the rendered output, you'll see that the waveform suddenly jumps from zero to max value and it stays there

Oh yes, I actually thought Nuked was somehow incompatible with the Gold, because I get the negative-pop + full negative bias 100% of the time.

It was only when I switched to compat and mame that those gold-processed samples started flowing.

On some runs they're fine, on some runs they aren't... I ran into this a few times before, but because it's sporadic I was never completely sure whether it's my code or the library -- now it's clear that the library is at fault.

Given It's 100% reproduceable for me and not for you, says there's probably something quite small that's sending it off kilter, and it might need just a tiny bit of help to reign it in.

In parallel, I can try adding a bunch of asserts to the TDA8425_emu code to try to understand why the samples are heading off into the ether.

johnnovak · 2022-06-13T04:43:48Z

Oh yes, I actually thought Nuked was somehow incompatible with the Gold, because I get the negative-pop + full negative bias 100% of the time.

Really weird, I'm getting a 50-60% success rate with Nuked...

Given It's 100% reproduceable for me and not for you, says there's probably something quite small that's sending it off kilter, and it might need just a tiny bit of help to reign it in.

Okay but keep in my that without the DC offset removal turned on things get really weird at the waveform level, even when you scale the volume back to 25 in the floppy version (or use the CD version). Disable the DC offset, render the intro tune again at 25 volume that you posted above, and see it for yourself. Well, you can actually see it at right side of the waveform images you posted, the DC offset starts to rise really high there.

a755b88

In parallel, I can try adding a bunch of asserts to the TDA8425_emu code to try to understand why the samples are heading off into the ether.

I've been running it with the sanitizer and it didn't catch anything on my machine. I guess I just don't want to turn this in to a wild goose chase as I'm not a DSP expert, so I cannot determine whether he made a mistake in his filter DSP code or it's something else. But I know it's possible to design filters incorrectly so they become unstable and exhibit chaotic behaviour, depending on the input (because they use feedback, so anything goes, really...)

So try adding your check and maybe run it through the sanitizer, but if that doesn't shed light on the issue in say a day, I'm inclined to scrap it and reimplement the (quite simple) filters with IIR1.

johnnovak · 2022-06-14T03:13:27Z

In parallel, I can try adding a bunch of asserts to the TDA8425_emu code to try to understand why the samples are heading off into the ether.

Just wondering, any luck with the asserts @kcgen?

kcgen · 2022-06-14T04:45:18Z

In parallel, I can try adding a bunch of asserts to the TDA8425_emu code to try to understand why the samples are heading off into the ether.

Just wondering, any luck with the asserts @kcgen?

Nothing to report (zero progress), and barely moved the Mute PR much today; but if I manage to make any progress in the coming days I will surely report how it's going. In the meantime, certainly don't hold up for me! If any fruit comes from this effort, we'll propose it up stream first and see what the maintainer thinks.

In the meantime, it sounds like constructing your own IIR filter will be fast and robust - so is probably going to be right approach regardless. 👍

johnnovak force-pushed the jnovak/adlib-gold-surround-fixed-1 branch from c6d573c to dcdb230 Compare June 10, 2022 13:35

shermp reviewed Jun 11, 2022

View reviewed changes

johnnovak force-pushed the jnovak/adlib-gold-surround-fixed-1 branch 4 times, most recently from 9f5ed43 to 59a5c78 Compare June 11, 2022 08:00

johnnovak marked this pull request as ready for review June 11, 2022 08:23

johnnovak requested review from kcgen, GranMinigun and shermp June 11, 2022 08:45

johnnovak force-pushed the jnovak/adlib-gold-surround-fixed-1 branch 6 times, most recently from 0bcc621 to b3d9a10 Compare June 11, 2022 12:39