vsariola / sointu

Fork of 4klang that can target 386, amd64 and WebAssembly. Tools run on Windows, Mac & Linux
MIT License
239 stars 15 forks source link

Asm player bug related to multiple empty voices and oscillator shape parameter #102

Closed LeStahL closed 9 months ago

LeStahL commented 10 months ago

Ok, this is a weird one, but I often encounter it:

When an instrument has a substantial amount of voices and also the shape parameter of its oscillator is changed, the exported asm track will produce weird noises until the first note of the synth in question is played.

I attached a zip with minimal examples that reproduce the problem: sointubug.zip

sointu_nobug.yml should work. sointunobug.exe is a dsound playback executable for verification. sointubug.yml should reproduce the bug. sointubug.exe is a dsound playback executable with the bug.

the diff is actually only

44c44
<           parameters: {color: 64, detune: 64, gain: 64, phase: 0, shape: 117, stereo: 0, transpose: 64, type: 0, unison: 0}
---
>           parameters: {color: 64, detune: 64, gain: 64, phase: 0, shape: 64, stereo: 0, transpose: 64, type: 0, unison: 0}

interestingly, this only occurs in the asm exported player. I can play the song correctly in the tracker.

vsariola commented 10 months ago

I have a hunch why this is: probably because all envelopes are triggering by default and they don't release until first time released. This behaviour is sometimes desired: for example, envelope in the Global instrument with nothing to release it can be used to fade in the track. But instruments with a lot of voices are playing note 0 when (very low bass) when the track starts. Need to think what's the way to really fix this.

4klang / sointu retriggers voice by clearing all of its memory, (rep stosb) and the memory is also 0s at the beginning of the intro; I would not want to add a loop to set all envelopes to releasing state

vsariola commented 10 months ago

A decent quick fix is to put one empty pattern in the beginning; that should result in 16 releases for each instrument and thus likely stopping the weird noises before they get too bad

EDIT: wrong advice, that also releases only the first voice of each instrument, all the other voices still have their envelopes attacking

vsariola commented 10 months ago

If it's ok that envelopes are never attacking at the beginning of song (prevents making fade-ins using these, but I guess no-one ever used this anyway as it doesn't quite work out in the tracker or vsti, just in the compiled song), we can check if the note = 0 in the envelope, and if so, never go to the attacking.

So, change around the line here https://github.com/vsariola/sointu/blob/231e055faf0087988f268a2b3512b00be9af588a/vm/compiler/templates/amd64-386/sources.asm#L18

to something like:

    mov     eax, dword [{{.INP}}-su_voice.inputs+su_voice.note]   ; eax = note
    test    eax, eax                            ; if (note == 0) // note should be 0 only when the voice has never been triggered
    je      su_op_envelope_skipattack           ;   goto skipattack
    mov     eax, dword [{{.INP}}-su_voice.inputs+su_voice.release] ; eax = su_instrument.release
    test    eax, eax                            ; if (eax == 0)
    je      su_op_envelope_process              ;   goto process
su_op_envelope_skipattack:
    mov     al, {{.InputNumber "envelope" "release"}}  ; [state]=RELEASE
vsariola commented 10 months ago

If I counted correctly, that's like 7 bytes of uncompressed asm code. You are free to sizecode that if you can find a better way (e.g. doing the whole thing with a single branch) but the version I posted likely compresses quite ok as the two parts are quite repetitive.

vsariola commented 10 months ago

Alright so this is a bit scary change, but we could release the logic of the release bit so that 0 = released, and anything > 0 is attacking / sustaining. This makes it impossible to have envelopes in the global instrument, which is to me a small loss compared to fixing the noises in the beginning of the song.

I pushed a draft fix to fix/reverse_release branch; can you try if it would work for you?

https://github.com/vsariola/sointu/tree/fix/reverse_release

The commit is a draft because I should probably rename "Release" to "Hold" instead of release, to make it more clear that it is true when the note is holding and false when its released.

There's also super ugly mov dword [...], 0 which I need to sizecode, but couldn't see a register being guaranteed zero there.

vsariola commented 10 months ago

So if I counted correctly, this version only adds 1 or 5 bytes to the uncompressed size, depending if the synth is using polyphonism or not. 1) one extra stosd, setting both the note and the release to same value 2) (in the polyphonic case), that ugly mov dword [...], 0 adds four bytes, because I don't have a register as zero. There's gotta be a better way but that at least should compress pretty well, vs. xor eax, eax; mov dword [...], 0 might still be more bytes after compression.

Apart from that, couple of places I just mov dword [...], eax instead of inc dword [...] to clear the Release flag and then when testing the flag, I use jne instead of je. How these affect compressed size probably... depends.

vsariola commented 10 months ago

All the three versions (original, checking if note == 0, reversing Release logic) are within 1801-1803 bytes when compressing with light crinkler settings the cplay-winmm.exe... that's too small to draw any real conclusions, but the difference is pretty negligible.

vsariola commented 9 months ago

Merged to master branch, happy jamming!