Re: [hatari-devel] Hatari plays all voices in phase

[David Savinkoff <dsavnkff@xxxxxxxxx>]

----- Nicolas Pomarède wrote:
> Le 21/12/2015 21:06, David Savinkoff a écrit :
> > Hi,
> >
> > I noticed on atari-forum that some people have found
> > a problem with the YM2149 sound.
> > I looked at the problem and wasn't able to nail it down,
> > so maybe someone can check the code too.
> >
> > Here is the link:
> > http://www.atari-forum.com/viewtopic.php?f=18&t=27521&sid=a0d1b43f0f66988900541b039ab5f6d4#p285292
> 
> Hi
> 
> I saw this message by troed too, but I don't see what "in phase" means. 
> Does it mean all 3 voices have the same square wave happening at the 
> same time if they are played at the same freq, instead of having 3 
> similar square wave, but each one with a random phase ?

I believe this is what is meant. The 'phase problem' must be subtle
in some way because:
1) I hear many demos that have a phasing effect (I did'nt test his demo).
2) The YM2149 code allows for phase shifts between channels.
3) I just examined and experimented with the YM2149 code.

> Could this be an effect of the filters we used ? If so, it's possible to 
> disable them by commenting a few lines to see if the problem persists.

The problem could be with how the filters are integrated into the
YM2149 code, not the filters. However, the phase problem is
just as likely to be elsewhere (it is reasonable that a real Atari
ST will play a tune phased differently each time, causing an
erroneous bug report).

I'm including the broken test patch I was experimenting with
(The patch is only useful as an insight to what 'I' was pondering).
Note that this patch worsens several things with no benefits,
and is actually an example of what is not wrong.

There may be problems with how this patch changed
Ym2149_ToneStepCompute() because I got unexpected
aliasing.

> 
> Nicolas
> 
> 
> 

--- sound.old.c	2015-11-21 09:54:27.000000000 -0800
+++ sound.c	2015-12-20 20:19:47.890271865 -0800
@@ -199,6 +199,7 @@
 
 static ymu32	stepA , stepB , stepC;
 static ymu32	posA , posB , posC;
+static ymu32	highA , highB , highC;
 static ymu32	mixerTA , mixerTB , mixerTC;
 static ymu32	mixerNA , mixerNB , mixerNC;
 
@@ -715,6 +716,10 @@
 	posB = 0;
 	posC = 0;
 
+	highA = 0;
+	highB = 0;
+	highC = 0;
+
 	currentNoise = 0xffff;
 
 	RndRack = 1;
@@ -753,10 +758,10 @@
  * Compute tone's step based on the input period.
  * Although for tone we should have the same result when per==0 and per==1,
  * this gives some very sharp and unpleasant sounds in the emulation.
- * To get a better sound, we consider all per<=5 to give step=0, which will
- * produce a constant output at value '1'. This should be handled with some
- * proper filters to remove high frequencies as on a real ST (where per<=9
- * gives nearly no audible sound).
+ * To get a better sound, we consider all per<=5 to give step = -step, which
+ * will produce a constant output at value '1'. This should be handled with
+ * some proper filters to remove high frequencies as on a real ST
+ * (where per<=9 gives nearly no audible sound).
  * A common replay freq of 44.1 kHz will also not be high enough to correctly
  * render possible tone's freq of 125 or 62.5 kHz (when per==1 or per==2)
  */
@@ -771,14 +776,17 @@
 	per = rHigh&15;
 	per = (per<<8)+rLow;
 
-	if  (per <= (int)(YM_ATARI_CLOCK/(YM_REPLAY_FREQ*7)) )
-		return 0;
+	if ( per == 0 )
+		per = 1;				/* result for Per=0 is the same as for Per=1 */	
 
 	step = YM_ATARI_CLOCK;
 	step <<= (15+16-3);
 	step /= (per * YM_REPLAY_FREQ);
 
-	return step;
+	if  (per <= (int)(YM_ATARI_CLOCK/(YM_REPLAY_FREQ*7)) )
+		return -step;
+	else
+		return step;
 }
 #else
 static ymu32	Ym2149_ToneStepCompute(ymu8 rHigh , ymu8 rLow)
@@ -789,20 +797,18 @@
 	per = rHigh&15;
 	per = (per<<8)+rLow;
 
-#if 0							/* need some high freq filters for this to work correctly */
 	if ( per == 0 )
 		per = 1;				/* result for Per=0 is the same as for Per=1 */	
-#else
-	if  (per <= (int)(YM_ATARI_CLOCK/(YM_REPLAY_FREQ*7)) )
-		return 0;				/* discard frequencies higher than 80% of nyquist rate. */
-#endif
 
 	step = YM_ATARI_CLOCK;
 	step <<= 24;
 
 	step /= (per * 8 * YM_REPLAY_FREQ);		/* 0x5ab9 < step < 0x5ab3f46 at 44.1 kHz */
 
-	return step;
+	if  (per <= (int)(YM_ATARI_CLOCK/(YM_REPLAY_FREQ*7)) )
+		return -step;				/* discard frequencies higher than 80% of nyquist rate. */
+	else
+		return step;
 }
 #endif
 
@@ -933,28 +939,28 @@
 //fprintf ( stderr , "env %x %x %x\n" , Env3Voices , envStep , envPos );
 
 	/* Tone3Voices will contain the output state of each voice : 0 or 0x1f */
-	bt = ((((yms32)posA)>>31) | mixerTA) & (bn | mixerNA);	/* 0 or 0xffff */
-	Tone3Voices = bt & YM_MASK_1VOICE;			/* 0 or 0x1f */
-	bt = ((((yms32)posB)>>31) | mixerTB) & (bn | mixerNB);
+	bt = ((((yms32)posA)>>31) | highA | mixerTA) & (bn | mixerNA);	/* 0 or 0xffff */
+	Tone3Voices = bt & YM_MASK_1VOICE;				/* 0 or 0x1f */
+	bt = ((((yms32)posB)>>31) | highB | mixerTB) & (bn | mixerNB);
 	Tone3Voices |= ( bt & YM_MASK_1VOICE ) << 5;
-	bt = ((((yms32)posC)>>31) | mixerTC) & (bn | mixerNC);
+	bt = ((((yms32)posC)>>31) | highC | mixerTC) & (bn | mixerNC);
 	Tone3Voices |= ( bt & YM_MASK_1VOICE ) << 10;
 
 	/* Combine fixed volumes and envelope volumes and keep the resulting */
 	/* volumes depending on the output state of each voice (0 or 0x1f) */
 	Tone3Voices &= ( Env3Voices | Vol3Voices );
 
-	/* When a step period is 0, the represented frequency was filtered from the */
+	/* When high[A B C] != 0, the represented frequency is filtered from the    */
 	/* ouput of the YM2149. Thus, use the transient DC component of the sample. */
 	/* Note that the "-1" table offset is a "good fit" for the DC component.    */
 
-	if (stepA == 0  &&  (Tone3Voices & YM_MASK_A) > 1)
+	if (highA != 0  &&  (Tone3Voices & YM_MASK_A) > 1)
 		Tone3Voices -= 1;     /* Voice A AC component removed; Transient DC component remains */
 
-	if (stepB == 0  &&  (Tone3Voices & YM_MASK_B) > 1<<5)
+	if (highB != 0  &&  (Tone3Voices & YM_MASK_B) > 1<<5)
 		Tone3Voices -= 1<<5;  /* Voice B AC component removed; Transient DC component remains */
 
-	if (stepC == 0  &&  (Tone3Voices & YM_MASK_C) > 1<<10)
+	if (highC != 0  &&  (Tone3Voices & YM_MASK_C) > 1<<10)
 		Tone3Voices -= 1<<10; /* Voice C AC component removed; Transient DC component remains */
 
 	/* D/A conversion of the 3 volumes into a sample using a precomputed conversion table */
@@ -1003,14 +1009,14 @@
 //fprintf ( stderr , "env %x %x %x\n" , Env3Voices , envStep , envPos );
 
 	/* Tone3Voices will contain the output state of each voice : 0 or 0x1f */
-	bt = -( (posA>>24) & 1);			/* 0 if bit24=0 or 0xffffffff if bit24=1 */
-	bt = (bt | mixerTA) & (bn | mixerNA);		/* 0 or 0xffff */
-	Tone3Voices = bt & YM_MASK_1VOICE;		/* 0 or 0x1f */
+	bt = -( (posA>>24) & 1);				/* 0 if bit24=0 or 0xffffffff if bit24=1 */
+	bt = (bt | highA | mixerTA) & (bn | mixerNA);		/* 0 or 0xffff */
+	Tone3Voices = bt & YM_MASK_1VOICE;			/* 0 or 0x1f */
 	bt = -( (posB>>24) & 1);
-	bt = (bt | mixerTB) & (bn | mixerNB);
+	bt = (bt | highB | mixerTB) & (bn | mixerNB);
 	Tone3Voices |= ( bt & YM_MASK_1VOICE ) << 5;
 	bt = -( (posC>>24) & 1);
-	bt = (bt | mixerTC) & (bn | mixerNC);
+	bt = (bt | highC | mixerTC) & (bn | mixerNC);
 	Tone3Voices |= ( bt & YM_MASK_1VOICE ) << 10;
 
 	/* Combine fixed volumes and envelope volumes and keep the resulting */
@@ -1019,13 +1025,13 @@
 
 	/* D/A conversion of the 3 volumes into a sample using a precomputed conversion table */
 
-	if (stepA == 0  &&  (Tone3Voices & YM_MASK_A) > 1)
+	if (highA != 0  &&  (Tone3Voices & YM_MASK_A) > 1)
 		Tone3Voices -= 1;     /* Voice A AC component removed; Transient DC component remains */
 
-	if (stepB == 0  &&  (Tone3Voices & YM_MASK_B) > 1<<5)
+	if (highB != 0  &&  (Tone3Voices & YM_MASK_B) > 1<<5)
 		Tone3Voices -= 1<<5;  /* Voice B AC component removed; Transient DC component remains */
 
-	if (stepC == 0  &&  (Tone3Voices & YM_MASK_C) > 1<<10)
+	if (highC != 0  &&  (Tone3Voices & YM_MASK_C) > 1<<10)
 		Tone3Voices -= 1<<10; /* Voice C AC component removed; Transient DC component remains */
 
 	sample = ymout5[ Tone3Voices ];			/* 16 bits signed value */
@@ -1055,11 +1061,6 @@
  * Update internal variables (steps, volume masks, ...) each
  * time an YM register is changed.
  */
-#ifndef NEWSTEP
-#define BIT_SHIFT 31
-#else
-#define BIT_SHIFT 24
-#endif
 void	Sound_WriteReg( int reg , Uint8 data )
 {
 	switch (reg)
@@ -1067,37 +1068,67 @@
 		case 0:
 			SoundRegs[0] = data;
 			stepA = Ym2149_ToneStepCompute ( SoundRegs[1] , SoundRegs[0] );
-			if (!stepA) posA = 1u<<BIT_SHIFT;		// Assume output always 1 if 0 period (for Digi-sample)
+			if (stepA < 0){
+				stepA = -stepA;
+				highA = 1;	// Assume output always 1 (for Digi-sample)
+			}else{
+				highA = 0;
+			}
 			break;
 
 		case 1:
 			SoundRegs[1] = data & 0x0f;
 			stepA = Ym2149_ToneStepCompute ( SoundRegs[1] , SoundRegs[0] );
-			if (!stepA) posA = 1u<<BIT_SHIFT;		// Assume output always 1 if 0 period (for Digi-sample)
+			if (stepA < 0){
+				stepA = -stepA;
+				highA = 1;	// Assume output always 1 (for Digi-sample)
+			}else{
+				highA = 0;
+			}
 			break;
 
 		case 2:
 			SoundRegs[2] = data;
 			stepB = Ym2149_ToneStepCompute ( SoundRegs[3] , SoundRegs[2] );
-			if (!stepB) posB = 1u<<BIT_SHIFT;		// Assume output always 1 if 0 period (for Digi-sample)
+			if (stepB < 0){
+				stepB = -stepB;
+				highB = 1;	// Assume output always 1 (for Digi-sample)
+			}else{
+				highB = 0;
+			}
 			break;
 
 		case 3:
 			SoundRegs[3] = data & 0x0f;
 			stepB = Ym2149_ToneStepCompute ( SoundRegs[3] , SoundRegs[2] );
-			if (!stepB) posB = 1u<<BIT_SHIFT;		// Assume output always 1 if 0 period (for Digi-sample)
+			if (stepB < 0){
+				stepB = -stepB;
+				highB = 1;	// Assume output always 1 (for Digi-sample)
+			}else{
+				highB = 0;
+			}
 			break;
 
 		case 4:
 			SoundRegs[4] = data;
 			stepC = Ym2149_ToneStepCompute ( SoundRegs[5] , SoundRegs[4] );
-			if (!stepC) posC = 1u<<BIT_SHIFT;		// Assume output always 1 if 0 period (for Digi-sample)
+			if (stepC < 0){
+				stepC = -stepC;
+				highC = 1;	// Assume output always 1 (for Digi-sample)
+			}else{
+				highC = 0;
+			}
 			break;
 
 		case 5:
 			SoundRegs[5] = data & 0x0f;
 			stepC = Ym2149_ToneStepCompute ( SoundRegs[5] , SoundRegs[4] );
-			if (!stepC) posC = 1u<<BIT_SHIFT;		// Assume output always 1 if 0 period (for Digi-sample)
+			if (stepC < 0){
+				stepC = -stepC;
+				highC = 1;	// Assume output always 1 (for Digi-sample)
+			}else{
+				highC = 0;
+			}
 			break;
 
 		case 6: