Hatari sound lowpass filters running at wrong frequency

[rainwarrior]

At some point the sound emulation moved to a 250kHz model of the YM sound, but was still applying the same lowpass filters which were written with coefficients for 44100-48000 Hz. This means that the effect of the lowpass filter is essentially disabled, as the cutoff frequency is now 5x too high, and out of the range of human hearing.

The fix is simple, move the relevant lowpass filter code in sound.c into YM2149_NextSample_250 where they can apply at the output audio rate instead.

This restores the previous behaviour of these filters as intended. However, this brings me to a second suggestion.

The two lowpass filters provided have very poor quality, extremely distorted sound. This is due to them applying asymmetrically with a hard break point in the middle. I assume this is modeled on some version of STF circuitry, and probably there exists some hardware that does sound this bad, but my own STs certainly don't sound anywhere near as distorted as this.

As a substitute, which I would suggest using as a default, I made a third filter option which uses coefficients borrowed from the other filter, but with only a symmetrical application as a simple IIR filter. This produces a very clean lowpass filter, attenuating the undesirable high frequencies with no added distortion.

Finally I added a config option to select both the LPF and HPF filters, for users that have preferences about them.

The code changes are very small and simple, I've included them below. This is a modification of the 2.4.1 release source. As a summary:

• In sound.c moved the lowpass filters into YM2149_NextSample_250.
• In sound.c added IIRLowpassFilter as an alternative, made the default.
• In configuration.c added YmLpf and YmHpf options.

Edit: see my post below for a patch applicable to the latest Hatari git repository.

[Cyprian]

good topic for Hatari Dev mailing list

[czietz]

I vaguely remember that some work was done on the sound code after 2.4.1 release. Did you check that the issues you found still persist in the current source and that your patch can still be applied to that?

[metalages]

Sound emulation has been moved to a 250kHz with 2.4 if I well remember ?

[rainwarrior]

I vaguely remember that some work was done on the sound code after 2.4.1 release. Did you check that the issues you found still persist in the current source and that your patch can still be applied to that?

Checking the log here: https://git.tuxfamily.org/hatari/hatari.git/log/src/sound.c

I only see two commits since the 2.4.1 release date (2022-08-03), and they are both just formatting changes, replacing some SDL type names with standard ones. Definitely nothing that would change this situation or invalidate the patch I'm suggesting.

Was this the correct place to look for the changes you're mentioning?

[rainwarrior]

Here is a patch starting from the last commit from 2023-08-01, updated for those recent code changes.

Summary:

• Moves the application of lowpass filter into YM2149_NextSample_250 to restore its original function at the audio samplerate, and not 250 kHz.
• Adds a simpler lowpass filter IIRLowPassFilter which has a cleaner sound.
• Adds YmLpf and YmHpf configuration options to allow users to configure it.

Code: Select all

From 97d5f7f6d464855120c83729b1293c6edeade561 Mon Sep 17 00:00:00 2001
From: bbbradsmith <bbbradsmith@users.noreply.github.com>
Date: Sun, 6 Aug 2023 17:50:21 -0400
Subject: [PATCH] fix lowpass filter application frequenc add simpler IIR
 lowpass filter default

---
 src/configuration.c          |  6 ++++
 src/includes/configuration.h |  2 ++
 src/includes/sound.h         |  1 +
 src/sound.c                  | 56 +++++++++++++++++++++++-------------
 4 files changed, 45 insertions(+), 20 deletions(-)

diff --git a/src/configuration.c b/src/configuration.c
index de6006d0..6d487776 100644
--- a/src/configuration.c
+++ b/src/configuration.c
@@ -272,6 +272,8 @@ static const struct Config_Tag configs_Sound[] =
 	{ "nSdlAudioBufferSize", Int_Tag, &ConfigureParams.Sound.SdlAudioBufferSize },
 	{ "szYMCaptureFileName", String_Tag, ConfigureParams.Sound.szYMCaptureFileName },
 	{ "YmVolumeMixing", Int_Tag, &ConfigureParams.Sound.YmVolumeMixing },
+	{ "YmLpf", Int_Tag, &ConfigureParams.Sound.YmLpf },
+	{ "YmHpf", Int_Tag, &ConfigureParams.Sound.YmHpf },
 	{ NULL , Error_Tag, NULL }
 };
 
@@ -732,6 +734,8 @@ void Configuration_SetDefault(void)
 	                 psWorkingDir, "hatari", "wav");
 	ConfigureParams.Sound.SdlAudioBufferSize = 0;
 	ConfigureParams.Sound.YmVolumeMixing = YM_TABLE_MIXING;
+	ConfigureParams.Sound.YmLpf = YM2149_LPF_FILTER_IIR;
+	ConfigureParams.Sound.YmHpf = YM2149_HPF_FILTER_IIR;
 
 	/* Set defaults for Rom */
 	File_MakePathBuf(ConfigureParams.Rom.szTosImageFileName,
@@ -863,6 +867,8 @@ void Configuration_Apply(bool bReset)
 		ConfigureParams.Sound.YmVolumeMixing = YM_TABLE_MIXING;
 
 	YmVolumeMixing = ConfigureParams.Sound.YmVolumeMixing;
+	YM2149_LPF_Filter = ConfigureParams.Sound.YmLpf;
+	YM2149_HPF_Filter = ConfigureParams.Sound.YmHpf;
 	Sound_SetYmVolumeMixing();
 
 	/* Falcon : update clocks values if sound freq changed  */
diff --git a/src/includes/configuration.h b/src/includes/configuration.h
index 86e26c37..66015893 100644
--- a/src/includes/configuration.h
+++ b/src/includes/configuration.h
@@ -74,6 +74,8 @@ typedef struct
   int SdlAudioBufferSize;
   char szYMCaptureFileName[FILENAME_MAX];
   int YmVolumeMixing;
+  int YmLpf;
+  int YmHpf;
 } CNF_SOUND;
 
 
diff --git a/src/includes/sound.h b/src/includes/sound.h
index d72b5dc4..3903d921 100644
--- a/src/includes/sound.h
+++ b/src/includes/sound.h
@@ -51,6 +51,7 @@ extern int	YmVolumeMixing;
 #define		YM2149_LPF_FILTER_NONE			0
 #define		YM2149_LPF_FILTER_LPF_STF		1
 #define		YM2149_LPF_FILTER_PWM			2
+#define		YM2149_LPF_FILTER_IIR			3
 extern int	YM2149_LPF_Filter;
 
 #define		YM2149_HPF_FILTER_NONE			0
diff --git a/src/sound.c b/src/sound.c
index 6ce935e0..25747755 100644
--- a/src/sound.c
+++ b/src/sound.c
@@ -117,7 +117,7 @@
 /* 2008/10/26	[NP]	Correctly save/restore all necessary variables in		*/
 /*			Sound_MemorySnapShot_Capture.					*/
 /* 2008/11/23	[NP]	Clean source, remove old sound core.				*/
-/* 2011/11/03	[DS]	Stereo DC filtering which accounts for DMA sound.               */
+/* 2011/11/03	[DS]	Stereo DC filtering which accounts for DMA sound.		*/
 /* 2017/06/xx	[NP]	New cycle exact emulation method, all counters are incremented	*/
 /*			using a simulated freq of 250 kHz. Some undocumented cases	*/
 /*			where also measured on real STF to improve accuracy.		*/
@@ -125,6 +125,8 @@
 /*			downsampling of the internal 250 kHz sound buffer.		*/
 /* 2021/07/23	[NP]	Default to 250 kHz cycle accurate emulation and remove older	*/
 /*			rendering and associated functions/variables.			*/
+/* 2023/08/06	[BS]	Restore lowpass filter to audio samplerate, had been at 250 khz */
+/*			incorrectly. Add cleaner IIR lowpass filter default.		*/
 
 
 const char Sound_fileid[] = "Hatari sound.c";
@@ -285,7 +287,7 @@ uint8_t		SoundRegs[ 14 ];
 
 int		YmVolumeMixing = YM_TABLE_MIXING;
 
-int		YM2149_LPF_Filter = YM2149_LPF_FILTER_PWM;
+int		YM2149_LPF_Filter = YM2149_LPF_FILTER_IIR;
 // int		YM2149_LPF_Filter = YM2149_LPF_FILTER_NONE;	/* For debug */
 int		YM2149_HPF_Filter = YM2149_HPF_FILTER_IIR;
 // int		YM2149_HPF_Filter = YM2149_HPF_FILTER_NONE;	/* For debug */
@@ -338,6 +340,7 @@ static CLOCKS_CYCLES_STRUCT	YM2149_ConvertCycles_250;
 
 static ymsample	LowPassFilter		(ymsample x0);
 static ymsample	PWMaliasFilter		(ymsample x0);
+static ymsample	IIRLowPassFilter	(ymsample x0);
 
 static void	interpolate_volumetable	(ymu16 volumetable[32][32][32]);
 
@@ -486,7 +489,20 @@ static ymsample	PWMaliasFilter(ymsample x0)
 	return y0;
 }
 
-
+/**
+ * A simpler IIR LPF filter which doesn't have the asymmterical
+ * push-pull distortion of the filters above, making it much cleaner sounding.
+ * Cutoff frequency is the same as the pull up of LowPassFilter above.
+ * fc = 7586.1 Hz (44.1 KHz), fc = 8257.0 Hz (48 KHz)
+ * 2023/08/06 Brad Smith.
+ */
+static ymsample	IIRLowPassFilter(ymsample x0)
+{
+	static	yms32 y0 = 0, x1 = 0;
+	y0 = (3*(x0 + x1) + (y0<<1)) >> 3;
+	x1 = x0;
+	return y0;
+}
 
 /*--------------------------------------------------------------*/
 /* Build the volume conversion table used to simulate the	*/
@@ -1106,12 +1122,6 @@ static void	YM2149_DoSamples_250 ( int SamplesToGenerate_250 )
 
 		sample = ymout5[ Tone3Voices ];			/* 16 bits signed value */
 
-		/* Apply low pass filter ? */
-		if ( YM2149_LPF_Filter == YM2149_LPF_FILTER_LPF_STF )
-			sample = LowPassFilter ( sample );
-		else if ( YM2149_LPF_Filter == YM2149_LPF_FILTER_PWM )
-			sample = PWMaliasFilter ( sample );
-
 		/* Store sample */
 		YM_Buffer_250[ pos ] = sample;
 		pos = ( pos + 1 ) & YM_BUFFER_250_SIZE_MASK;
@@ -1385,17 +1395,23 @@ static ymsample	YM2149_Next_Resample_Weighted_Average_2 ( void )
 
 static ymsample	YM2149_NextSample_250 ( void )
 {
-	if ( YM2149_Resample_Method == YM2149_RESAMPLE_METHOD_WEIGHTED_AVERAGE_2 )
-		return YM2149_Next_Resample_Weighted_Average_2 ();
-
-	else if ( YM2149_Resample_Method == YM2149_RESAMPLE_METHOD_NEAREST )
-		return YM2149_Next_Resample_Nearest ();
-
-	else if ( YM2149_Resample_Method == YM2149_RESAMPLE_METHOD_WEIGHTED_AVERAGE_N )
-		return YM2149_Next_Resample_Weighted_Average_N ();
-
-	else
-		return 0;
+	ymsample sample = 0;
+	switch (YM2149_Resample_Method)
+	{
+		case YM2149_RESAMPLE_METHOD_NEAREST:            sample = YM2149_Next_Resample_Nearest();            break;
+		case YM2149_RESAMPLE_METHOD_WEIGHTED_AVERAGE_2: sample = YM2149_Next_Resample_Weighted_Average_2(); break;
+		case YM2149_RESAMPLE_METHOD_WEIGHTED_AVERAGE_N: sample = YM2149_Next_Resample_Weighted_Average_N(); break;
+		default: break;
+	}
+	switch (YM2149_LPF_Filter)
+	{
+		default:
+		case YM2149_LPF_FILTER_NONE:                                          break;
+		case YM2149_LPF_FILTER_LPF_STF: sample = LowPassFilter ( sample );    break;
+		case YM2149_LPF_FILTER_PWM:     sample = PWMaliasFilter ( sample );   break;
+		case YM2149_LPF_FILTER_IIR:     sample = IIRLowPassFilter ( sample ); break;
+	}
+	return sample;
 }
 

[czietz]

I only see two commits since the 2.4.1 release date (2022-08-03)

In that case I must have remembered wrong. Sorry! (The Hatari maintainers will for sure still appreciate your patch against the current files.)

[rainwarrior]

Oh, I didn't realize Hatari also had a github mirror. I have submitted a pull request there.

I found an old sound comparison I'd made regarding the distortion, so if anyone wanted to hear a sound example of the distortion, it's attached to the PR thread.

[ThorstenOtto]

The main repository is hosted on gitlab, so i doubt they will accept any PRs there. Maybe best to send the patch to the mailing list (see https://listengine.tuxfamily.org/lists. ... ari-devel/)

[czietz]

Actually, the main repository is hosted on tuxfamily: https://git.tuxfamily.org/hatari/hatari.git/. But I saw on the mailing list that the maintainers noticed the patch (https://listengine.tuxfamily.org/lists. ... 00007.html); hence it might be advantageous to join the conversation there.

[npomarede]

Hi
I'm away on hollidays, I didn't have time to check the change, I need to be back home with proper audio setup to hear the result (using a laptop at the moment, not the greatest result speaker-wise )

About the difference you hear compared to your real HW, what ST model is that ? STF or STE ?

The filters in Hatari were contributed by david savinkoff in dec 2011 (you can check old mails thread "YM2149 + C10 lowpass filter mapping" in hatari-devel mailing list) and he wrote that STF has different filtering than other model STE/TT/Falcon :

Here are 2 patches to provide YM2149 + C10 lowpass filtering for the STf. Filtering is acivated in ST mode for 44100 Hz and 48000 Hz.
The STe, TT and Falcon do not have C10 so they sound brighter.

So maybe the changes you write here don't apply to all models ?

Anyway, I'm always glad to improve sound quality, so your patch is appreciated

Nicolas

[rainwarrior]

I have an 1040 STF and a 520 ST, but I can't currently run the 520 ST because its power supply has broken.

I made a recording of my STF and have posted it here, along with a recording of each filter under consideration: Github comment with recordings

I'll copy the comment here:

I made a recording of my 1040STF and the 4 filter options for comparison:
filter_comparison_bubble_bobble.zip

Each contains 16 seconds of audio, 2 excerpts from Bubble Bobble, the start of the tune at boot, and the introduction jingle that starts when you press the "1" or "2" key to begin the game. I would recommend listening to them side by side, or line them up in an audio-editor and switch between them to compare.

There are 5 recordings:

• 1040stf.flac - Recorded from my ST.
• hatari_iir.flac - The new filter I am proposing to use as default. (IIR)
• hatari_stf.flac - The previous default ST filter (LPF_STF)
• hatari_pwm.flac - The alternate filter which applied to later machine types? (PWM)
• hatari_none.flac - No lowpass filer (NONE)

Commentary:

• 1040STF - We can hear some lowpass, but the distortion effect is quite low.
• IIR - I feel it's the closest in sound to the STF recording, but slightly cleaner, idealized.
• STF - The distortion here is severe, the square waves do not sound square, it like a different instrument altogether.
• PWM - Very distorted as well, with hardly any lowpass effect. Harsh sound.
• NONE - No distortion, but far too much high frequency. Harsh and piercing. We need some lowpass by default.

The comments in the code indicate some theoretical basis of circuit modelling for the LPF_STF and PWM filters, but I think in practice at least the LPF_STF one does do not sound anything at all like the machine they are intended to emulate. I can't offer direct commentary on the PWM version because I do not have an STE or Falcon to record, but since its filter implementation is a direct modification of LPF_STF (disabling the filter for half of its input phase), I suspect it may suffer from the same problem of introducing uncharacteristic amounts of distortion.

Unfortunately I do not have a theoretical model to substitute at this time, I am only offering a simpler model of a standard RC lowpass filter, derived from the existing LPF_STF filter but removing the part that causes distortion by operating on the two phases asymmetrically.

However, theory or not, ultimately the theory is no good if it leads us to the wrong sound. I would love to see a re-evaluation of the circuitry, and to test a new model against reference hardware recordings, but for now I want to suggest the new filter as a default, as I think it does sound much closer in the STF case, and otherwise is a more pleasant sound.

I also made a visual comparison of the waveforms, which maybe helps illustrate the asymmetry issue:

[npomarede]

Thanks for the detailed report + wav files. As I wrote earlier I can't really test these changes at the moment due to being away from home. I can do this in 2 weeks.