nukeykt / nuked-opl3 Goto Github PK

"Incorrect values read from a different memory region will cause incorrect computations. In OPL3_Reset: Out-of-bounds read from a buffer (CWE-125)"

After toggling 4-op mode, the slots are disconnected which causes the channel to be muted.

More info:

Hello,

I'm using this cool Nuked-OPL3 implementation in a little emulator project of mine, and encountered a problem. I'm more or less sure that it's my fault, so sorry about reporting as "bug" if it's not a bug of Nuked-OPL3 at all. If this is really the case I would really thank some suggestions.

The problem: recently (worked before very well, for a long time) my emulator always crashes when I try to run an AdLib player tool (also written by me many years ago) inside the emulated machine. Curiously the problem goes away if I compile the emulator with lower optimization level and debug symbols to be able to use gdb. Since debugging is not so much possible, I dropped in a tons of printfs throughout the code, and realized that in EnvelopeUpdateKSL() (called during executing OPL3_GenerateStream() called from "rendering" audio samples) slot->channel is a NULL pointer which is dereferenced there, causing the crash.

I guess the problem is probably not a Nuked-OPL3 issue (but I am not sure) but a problem how to use it. I use SDL2 in my emulator, with audio callback which is a thread then from the perspective of the OS. However in the main thread, the emulator also calls function from Nuked-OPL3 when the emulated OPL chip register is written by the software running in my emulation (I use OPL3_WriteRegBuffered() to do that).

Because the problem seems to go away with no C compiler optimization, the problem seems to be some race condition caused by using Nuked-OPL3 from multiple threads. However if it's true, I cannot easily see any solution to this issue, since I cannot change the major infrastructure of the emulator, SDL audio callback must be a thread (Pushing is not a solution here unfortunately, which is allowed by newer SDL2 versions), while the emulator code itself (which also calls register write events) must be in the "main thread/program".

Hopefully my issue and/or question is understandable, sorry for my lame English, and long text trying to explain what I mean in clumsy way. Please ignore me, if this issue is something you cannot do anything against.

Thanks a lot in advance!

Is there any sort of global state used in this synth? I am debugging adplug/adplug#120 and here there are two instances of the NukedOPL3 synth in the same program. If we call OPL3_GenerateStream on only one instance then playback is fine, but if we call that function on both instances then there is significant audio corruption. The song tempo also seems to increase, as if we are losing chunks of audio. It's almost like the OPL3_GenerateStream call advances time for all NukedOPL3 instances instead of just the one being called.

I have looked through the code but I cannot find any sort of global state. I tried removing the static qualifier on everything but it made no difference. Using other emulators like the one from DOSBox works fine with multiple instances, it's only NukedOPL3 that seems to have a problem.

Is there any reason you can think of why we are unable to run two instances of the synth at the same time?

Hi,

It looks like slot->mod is never assigned. I assume you are still working on OPL3, aren't you?

Thank you

Hi there,

I was wondering if you'd be open to relicense Nuked-OPL3 (and maybe the other Yamaha emulators) under a less restrictive license (maybe MIT or BSD, although BSD itself is incompatible with MIT so maybe the former is better)?

The main problem with the LGPL is that it cannot be used in certain platforms like iOS, because of the relink clause.

I understand if you're not open to it, but thanks for considering it.

Having a simple OPL3 demo script with SDL2:

// utility function for OPL3 programming
/* These are offsets from the base I/O address. */
const int FM = 8;       // SB (mono) ports (e.g. 228H and 229H)
const int PROFM1 = 0;   // On CT-1330, this is left OPL-2.  On CT-1600 and
                            // later cards, it's OPL-3 bank 0.
const int PROFM2 = 2;   // On CT-1330, this is right OPL-2.  On CT-1600 and
                            // later cards, it's OPL-3 bank 1.

void FMoutput(unsigned port, int reg, int val, std::shared_ptr<hardware::opl::OPL> opl)
/* This outputs a value to a specified FM register at a specified FM port. */
{
    opl->write(port, reg); // OPL3_WriteRegBuffered
    SDL_Delay(8);
    opl->write(port + 1, val); 
    SDL_Delay(55);
}

void fm(int reg, int val, std::shared_ptr<hardware::opl::OPL> opl)
/* This function outputs a value to a specified FM register at the Sound
 * Blaster (mono) port address.
 */
{
    FMoutput(FM, reg, val, opl);
}

void Profm1(int reg, int val, std::shared_ptr<hardware::opl::OPL> opl)
/* This function outputs a value to a specified FM register at the Sound
 * Blaster Pro left FM port address (or OPL-3 bank 0).
 */
{
    FMoutput(PROFM1, reg, val, opl);
}

void Profm2(int reg, int val, std::shared_ptr<hardware::opl::OPL> opl)
/* This function outputs a value to a specified FM register at the Sound
 * Blaster Pro right FM port address (or OPL-3 bank 1).
 */
{
    FMoutput(PROFM2, reg, val, opl);
}

// ------------------------------------------

// snippet of the OPL3 programming

fm(1, 0, opl);        /* must initialize this to zero */
    Profm2(5, 1, opl);  /* set to OPL3 mode, necessary for stereo */
    fm(0xC0, LEFT | RIGHT | 1, opl);     /* set both channels, parallel connection */

    /***************************************
    * Set parameters for the carrier cell *
    ***************************************/

    fm(0x23, 0x21, opl);  /* no amplitude modulation (D7=0), no vibrato (D6=0),
                     * sustained envelope type (D5=1), KSR=0 (D4=0),
                     * frequency multiplier=1 (D4-D0=1)
                     */

    fm(0x43, 0x0, opl);   /* no volume decrease with pitch (D7-D6=0),
                     * no attenuation (D5-D0=0)
                     */

    fm(0x63, 0xff, opl);  /* fast attack (D7-D4=0xF) and decay (D3-D0=0xF) */
    fm(0x83, 0x05, opl);  /* high sustain level (D7-D4=0), slow release rate (D3-D0=5) */


    /*****************************************
     * Set parameters for the modulator cell *
     *****************************************/

    fm(0x20, 0x20, opl);  /* sustained envelope type, frequency multiplier=0    */
    fm(0x40, 0x3f, opl);  /* maximum attenuation, no volume decrease with pitch */

    /* Since the modulator signal is attenuated as much as possible, these
     * next two values shouldn't have any effect.
     */
    fm(0x60, 0x44, opl);  /* slow attack and decay */
    fm(0x80, 0x05, opl);  /* high sustain level, slow release rate */


    /*************************************************
     * Generate tone from values looked up in table. *
     *************************************************/

    spdlog::info("440 Hz tone, values looked up in table.");
    fm(0xa0, 0x41, opl);  /* 440 Hz */
    fm(0xb0, 0x32, opl);  /* 440 Hz, block 0, key on */

    SDL_Delay(1000);

    fm(0xb0, 0x12, opl);  /* key off */

the callback for generating playing sound

void callback_nuked(void* userdata, uint8_t* stream, int len)
{
    
    OPL3_GenerateStream((opl3_chip*)userdata, (int16_t*)buffer, (uint16_t)length / 2);
}

the resulting sound is not a continuos wave 440Hz but is randomly not generated properly resulting in a clipped/noise sound.

Same script used with a DosBox OPL3 emulation instead have the right expected results.

the OPL3 demo programming is derived from an old DOS demo:

• http://www.dcee.net/Files/Programm/Sound/fmsample.zip

Yes, i know CQM is inaccurate, but in at least one case, i strongly believe it's what a game was actually composed for.

I submit the following tune.

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

not too shabby, right?

now listen to it on an actual OPL2.

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

very thin intro sound. horrible balance, and some notes an octave different (shades of Supaplex)

While it is known that CQM gets Supaplex wrong (because we have the amiga original tune to compare with) comparison with the jaguar original tune seems to suggest that the CQM card plays the tune closer to the original.

Because of this i wish to have the option for an emulated cqm card, for those games where the composer had a cqm card too.

Flagged by GCC 10.2 when code is instrumented with UBSAN:

../../src/libs/nuked/opl3.c:478:40: runtime error: left shift of negative value -512
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior ../../src/libs/nuked/opl3.c:478:40 in

Is there a way to read the register?
The basic functionality to read the status register looks like is missing as well as the 2 timers.

OPL is limited to 8 octaves by hardware design. Could this limit be lifted?

Apparently this is possible, yet it's not a case on a real OPL3.

The stereo extension is a rather odd way of accomplishing full range panning, if you don't mind me saying. What's wrong with sending the panning value 0x30 to every channel, so left and right are equal, then adjust the returning sample?
That could be done by the calling program rather than handled within Nuked.

chip->tremolo is being updated every sample, whereas it should be moved inside the previous "if" block which is only true every 64 samples. That is, chip->tremolo will only be updated when and after chip->tremolopos is.

I've written my own midi player called Midiplay and am currently using Nuked for synthesizing the output.
Each time I change midi tracks, I call OPL3_Reset, but it can (but not always) cause a Seg Fault.
The solution was to call OPL3_Reset only once during Midiplay's initialisation. Then, during track initialisation:

    opl3_chip   oplChip;
    int i;

    for (i = 0; i < 18; i++)
    {
        oplChip.channel[i].cha = 0;
        oplChip.channel[i].chb = 0;
    }

    oplChip.writebuf_samplecnt = 0;
    oplChip.writebuf_cur = 0;
    oplChip.writebuf_last = 0;
    oplChip.writebuf_lasttime = 0;
    for (i = 0; i < OPL_WRITEBUF_SIZE; i++)
    {
        oplChip.writebuf[i].reg = 0;
    }

Perhaps it would be advantageous to have an OPL3_Init function. Then OPL3_Reset could just do the minimal stuff.
Notice that I've initialised left & right channels to zero to silence them.

For example:
#define OPL_REGS_TREMOLO 0x20