surge-synthesizer / surge

Synthesizer plug-in (previously released as Vember Audio Surge)
https://surge-synthesizer.github.io/
GNU General Public License v3.0
3.06k stars 392 forks source link

Resolve int16 wt interpretation documentation and comparability #7694

Closed abique closed 1 month ago

abique commented 1 month ago

Hi,

According to the specification, if you want to encode 16 bits samples, you need the flags = 0x0C while it currently sets flags = 0x04 which is for 15 bits samples.

Reference: https://github.com/surge-synthesizer/surge/blob/main/resources/data/wavetables/WT%20fileformat.txt#L11

When looking at https://github.com/surge-synthesizer/surge/blob/main/scripts/wt-tool/wt-tool.py#L43

The bug seems to be here: https://github.com/surge-synthesizer/surge/blob/94c8c5b02fb2dadf62dae70a146d67ae487309a1/scripts/wt-tool/wt-tool.py#L43

Cheers, Alex

baconpaul commented 1 month ago

Yeah this is a remnant from before the format supported full height 16

The full fix here is

  1. Retire the --norm flag
  2. add a "--int-as-15" flag
  3. If you set int as 15 then set to 0x04 else 0x0C
  4. If you set the int as 15 then >>1 the samples on the encoding

Sort of one of those "quick hours" so let me tag it into 1.3.3 so we remember

mkruselj commented 1 month ago

@baconpaul IMO this basically means keep the "peak" normalization mode (not as an option but to use it when encoding) because it will make sure all waveforms are hitting that 2^15. Right?

EDIT: Actually... no. Normalization should still be an option - for some WTs it makes more sense to have all waveforms peak normed, for some it doesn't. What int-as-15 flag should make sure is just that things don't go over 2^15, UNLESS peak normalizing option is set, which would then drive through all frames and peak norm them.

mkruselj commented 1 month ago

Also update the .wt format spec (especially that maximum frame length is 4096 now).

abique commented 1 month ago

I think here the spec could be a bit more explicit:

The goal is to have the same sound for everyone.

baconpaul commented 1 month ago

Lemme look at what we do in the surge engine and then we can compare notes. Will add it to my list for this week

abique commented 1 month ago

FYI, Bitwig was previously loading the 16 bits value into a short, then scaling accordingly to the flag then clipping to -1..1. So Bitwig never ignored the MSB.

abique commented 1 month ago

For the mitigation of the wavetable I propose this:

int16_t MAX15 = 1 << 14;
int16_t IS16_THR = MAX15 + MAX15 / 4;
int16_t sample = ...;
if ((sample <= -IS16_THR) || (IS16_THR <= sample))
   setIs16Bits(true);
abique commented 1 month ago

Another option for the spec is to say: no mitigation, if the flags says 15 bits, then it is 15 bits and hard clip.

abique commented 1 month ago

Note for myself: 36445 (related issue number in Bitwig's bug tracker)

baconpaul commented 1 month ago

Here's what surge does today

https://github.com/surge-synthesizer/surge/blob/4fce6dd641758ddfac055f301cc822a243a30af6/src/common/dsp/Wavetable.cpp#L248

If we get full 16 bits flag we pre-process the int with i16toi15_block and then call i152float_block in either case

https://github.com/surge-synthesizer/surge/blob/4fce6dd641758ddfac055f301cc822a243a30af6/src/common/dsp/vembertech/basic_dsp.h#L57

i16toi15_block basically does a >>1 so in 16 bit mode we drop the LSB and go to 15 bit

https://github.com/surge-synthesizer/surge/blob/4fce6dd641758ddfac055f301cc822a243a30af6/src/common/dsp/vembertech/basic_dsp.h#L48C13-L48C28

i152float_block converts to float by multiplying by const float scale = 1.f / 16384.f;, or 2^14

Except for the i16to15 addition this is all still claes-era code.

We never check if the i15 inbound has bit 16 set. But the >>1 will make sure i16 inbound doesnt

Reading this now, a more accurate approach would be to have an i162float method and then call that where we dont shift and instead divide by 32768. We have very few 16 bit wavetables in the code base and this is a 1 bit difference (the amplitude would be the same since we move the >>1 from the integer to the float denom).

I propose the following then

  1. Surge modifies the wavetable load to instead of drop the LSB and multiply by 2, don't do that for int16
  2. Surge stays unchanged for int15. So int15 signals with an overflow will clip still.
  3. I'll change wt-tool.py to allow an -int16 flag which allows you to create an int16 and does no normalization

and that seems to be it right?

mkruselj commented 1 month ago

I'd keep the --peak flag (rename it to --norm) that would simply peak normalize all frames, optionally.

baconpaul commented 1 month ago

Right normalize them to max is +/-2^14 in int15 leave 16 unchanged? Or do you want to re-peak the 16 also so the highest value is 2^15

mkruselj commented 1 month ago

Repeak in both cases to the max value (0 dBFS or -6 dBFS)

baconpaul commented 1 month ago

Gotcha.

So -i15 no repeak might clip -i16 no repeak might have max < 2^15 -i15 and -i16 repeak will lower or raise

I think we want also a -reduce16to15 which just does a <<1 and leaves it alone. But that's an easy mode to add as well.

abique commented 1 month ago

My advice for wt-tool.py would be to remove the option to encode to 15 bits.

baconpaul commented 1 month ago

Oh that does solve all the problems right

duh me, good idea Alex

baconpaul commented 1 month ago

Another option for the spec is to say: no mitigation, if the flags says 15 bits, then it is 15 bits and hard clip.

I agree with this

baconpaul commented 1 month ago

Ugh this turns out to be a bit uglier to fix for a variety of bad choices in surge. Those choices are

  1. The patch always streams the wavetable as int16
  2. The window oscillator expects the 15 not 16 bit version
  3. So right now when we load we load as 15 and then save as 15
  4. which means if we don't scale each load save roundtrip cuts us by a factor of two
  5. and multiplying by 2 in that case when we save just destroys the bit

so this one won't be a 'this afternoon' thing. I think what I will need to do is preserve the original wavetable data to stream it into the patch. A bit tricky.

baconpaul commented 1 month ago

Wow turns out surge at head does the same today if you load a 16-as-16 wavetable. I guess no-one has been importing the bitwig factory wavetables into surge! This definitely needs a fix. Ugh. Probably the way to do it is to preserve the data and then patch the window oscillator

baconpaul commented 1 month ago

OK I got it all dealt with, including making the options to wt-tool more rational and testing them fully. Will merge shortly.

abique commented 1 month ago

That was fast! If you do a PR I can review it.

baconpaul commented 1 month ago

https://github.com/surge-synthesizer/surge/pull/7703

baconpaul commented 1 month ago

Anyway I merged it - lemme know what you think. If it looks good to you we can close this too

abique commented 1 month ago

Do you have plan for older presets to sound like they did before?

abique commented 1 month ago

@baconpaul I think your fix didn't update the specification.

I'd change it to something like:

        0008: if set, int16 data is in range 2^16; if not set it is in range 2^15 (legacy) and must be hard clipped
abique commented 1 month ago

Wow turns out surge at head does the same today if you load a 16-as-16 wavetable. I guess no-one has been importing the bitwig factory wavetables into surge! This definitely needs a fix. Ugh. Probably the way to do it is to preserve the data and then patch the window oscillator

You'll have -6dB on the oscillator compared to what you had before. Question is are you OK with breaking the sound of old presets? I think it is OK to break the sound here, it isn't so many wavetables that are involved, and adding backward compat may lead to other issues.

baconpaul commented 1 month ago

Wow turns out surge at head does the same today if you load a 16-as-16 wavetable. I guess no-one has been importing the bitwig factory wavetables into surge! This definitely needs a fix. Ugh. Probably the way to do it is to preserve the data and then patch the window oscillator

You'll have -6dB on the oscillator compared to what you had before. Question is are you OK with breaking the sound of old presets? I think it is OK to break the sound here, it isn't so many wavetables that are involved, and adding backward compat may lead to other issues.

No i mean no-one could have used i16 because each time you saved and loaded your session you got an extra 6db!!!!

Got it all fixed without a sound break in the PR. But it was very wrong.

baconpaul commented 1 month ago

@baconpaul I think your fix didn't update the specification.

I'd change it to something like:

        0008: if set, int16 data is in range 2^16; if not set it is in range 2^15 (legacy) and must be hard clipped

I think the correct statement is

if set, int16 data provides the full 16 bit range. If not set (legacy) int16 data provides only 15 bits, and values above +/- 2^14 may be clipped by wavetable renderers.

yeah?

abique commented 1 month ago

May or must be clipped? I'd go for must because we want the wt to sound the "same" everywhere if possible.

baconpaul commented 1 month ago

well that's the question right.

And what gets clipped

"The wavetable data will be clamped to +/- 2^14" is what we really mean then?

Depending what you do with the wavetable they won't sound the same (window and wt oscillator in surge sound very different for instance) but i think you are right. You want to be clear that this is a clamp on the oscillator input not output.

So that leads me to

if set, int16 data uses the full 16 bit range (+/- 2^15). If not set (legacy) int16 data provides only 15 bits, and all wavetable values must either be clamped to +/- 2^14 on wavetable read or fail to load if out of range.

I think that's what we want right?

baconpaul commented 1 month ago

We could also remove the or fail to load and just insist on a clamp

abique commented 1 month ago

We could also remove the or fail to load and just insist on a clamp

Yeah failing to load is tricky because you have older presets or projects for bitwig that still needs to play. For instance our package builder will reject "invalid" wavetables, but our oscillator will still be able to play them. I think clipping is right from a spec perspective, from an historical perspective nobody ever clipped it, it seems...

Another option would be to says that if the bit 8 isn't present in the flags, then add +6dB to the wavetable, which would be more in line with all the previous implementation.

Maybe let's not rush this questions ? :-) We'll have to live with the new spec forever.

abique commented 1 month ago

Today nothing prevents a wavetable made using float that will peak at 80 dB right?

So let's say it is OK to have a wt that goes beyond -1..1, then it'd be fine to have a 16 bits sample that can go to +6dB. The oscillator then could have an option to normalize/clip/identity the wavetable.

What do you think?

baconpaul commented 1 month ago

Yeah I think that’s right so “int15 means peal values of 2^14 are 0db signals rather than 2^15. If a file has data outside that range Tools can choose to clip or overflow in a way appropriate to the algorithm”

abique commented 1 month ago

Yeah, as of now I feel that it is the best option.

Let's take a moment to think about it? I'll also adjust Bitwig's code and try it.

mkruselj commented 1 month ago

“int15 means peal values of 2^14 are 0db signals rather than 2^15"

Doesn't this change the sound of existing patches?

abique commented 1 month ago

“int15 means peal values of 2^14 are 0db signals rather than 2^15"

Doesn't this change the sound of existing patches?

Actually it preserves it. In bitwig we would have played with up to +6dB already and maybe surge too?

mkruselj commented 1 month ago

I'm fine with that spec, then!

baconpaul commented 1 month ago

Yeah the nice thing about what I wrote is it is actually what surge does after my push

I’m off to the dentist now but let me add that sentence to the spec and then maybe cherry pick it and one or two other things into a surge and surge rack point release yeah!

abique commented 1 month ago

Yeah the nice thing about what I wrote is it is actually what surge does after my push

I’m off to the dentist now but let me add that sentence to the spec and then maybe cherry pick it and one or two other things into a surge and surge rack point release yeah!

If you don't mind waiting a bit before racking a point release, I'll need a bit of time but I'd like to check this with Claes as well. (we're very busy atm).

abique commented 1 month ago

By the way the spec doesn't mention if the cycle size has to be a power of 2. If it isn't mandatory maybe it could be mentioned that the cycle size is an arbitrary number?

baconpaul commented 1 month ago

It had to be power of 2 in surge for a variety of reasons and think that’s the case in serum and vital also. May be a good thing to put in spec

I’ll reopen this and change the title - and no rush on a point release but would be nice for surge and rack to have working access to their bitwig wt content properly

abique commented 1 month ago

would be nice for surge and rack to have working access to their bitwig wt content properly

You can install all bitwig packs, then right click on a wavetable -> reveal file and you'll find them. The folder structure is quite simple, so go down a few directories and you can find all *.wt files.

We have wavetables spread over a few packs I think.

mkruselj commented 1 month ago

I wouldn't mind if BWS offers a pack that is all WTs only (merging WTs from disparate packs). :)

baconpaul commented 1 month ago

would be nice for surge and rack to have working access to their bitwig wt content properly

You can install all bitwig packs, then right click on a wavetable -> reveal file and you'll find them. The folder structure is quite simple, so go down a few directories and you can find all *.wt files.

We have wavetables spread over a few packs I think.

Yeah this is how I found one to test and found the bug I fixed yesterday

I’m debating modifying surge storage wavetable scans to look for the bitwig install locations and add them to the menu …. I know the location in Mac but don’t suppose you can share where the packs install win lin could you?

abique commented 1 month ago

This can be overridden by the user: image

The location on Linux is $HOME/.BitwigStudio/installed-packages/. I'm not sure for Windows, I'll let you know once I boot Windows.

mkruselj commented 1 month ago

I guess it should read the BWS prefs file to get the user overriden path, too, then.

abique commented 1 month ago

I guess it should read the BWS prefs file to get the user overriden path, too, then.

I think it is a bad idea. But you're free to try. Also Bitwig may change its preference file format anytime.

You could try the standard locations, if they exists show them to the user. Then let the user add additional wavetables locations in Surge's settings.

baconpaul commented 1 month ago

I agree alex. If the user wants additional things they can use soft links to our search paths today. I was just thinking of the "I haven't screwed with my settings lets just make it work magically"

abique commented 1 month ago

I've implemented the discussed change in bitwig (allow the +6dB) it'll be in the next beta release.