オリジナル timidity-0.2i のバグレポート
              「バグるなら，直してしまえ，TiMidity」

オリジナルの timidity-0.2i にはいろんなバグが潜んでいます．
このページはそれまでのメモです．バグが見つかったり，新たに報告されたバグを
このページにメモっています．他にもこんなバグがあるよってのがあったら，
私宛までメールください．現在 10 ヶ所以上の明らかなバグを発見しています．


■テンポチェンジの発生タイミングがずれる
=========================== readmidi.c =================================
readmidi.c のテンポチェンジは

	  compute_sample_increment(tempo, divisions);

でテンポを変更した後、デルタタイムを足し込んでいます。これは明らかに
バグで、デルタタイムを足し込んだ後にテンポを変更しなければなりません。
以下のように修正してください。
修正前：
  for (i=0; i<event_count; i++)
    {
      skip_this_event=0;
      ctl->cmsg(CMSG_INFO, VERB_DEBUG_SILLY,
		"%6d: ch %2d: event %d (%d,%d)",
		meep->event.time, meep->event.channel + 1,
		meep->event.type, meep->event.a, meep->event.b);

      if (meep->event.type==ME_TEMPO)
	{
	  tempo=
	    meep->event.channel + meep->event.b * 256 + meep->event.a * 65536;
	  compute_sample_increment(tempo, divisions);
	  skip_this_event=1;
	}
      else if ((quietchannels & (1<<meep->event.channel)))
	skip_this_event=1;
＜中略＞
	  st += samples_to_do;
	}
      else if (counting_time==1) counting_time=0;
      if (!skip_this_event)
	{

修正後：
  for (i=0; i<event_count; i++)
    {
      skip_this_event=0;
      ctl->cmsg(CMSG_INFO, VERB_DEBUG_SILLY,
		"%6d: ch %2d: event %d (%d,%d)",
		meep->event.time, meep->event.channel + 1,
		meep->event.type, meep->event.a, meep->event.b);

      if (meep->event.type==ME_TEMPO)
	{
	  skip_this_event=1;
	}
＜中略＞
	  st += samples_to_do;
	}
      else if (counting_time==1) counting_time=0;
      if (meep->event.type==ME_TEMPO)
	{
	  tempo=
	    meep->event.channel + meep->event.b * 256 + meep->event.a * 65536;
	  compute_sample_increment(tempo, divisions);
	}
      if (!skip_this_event)
	{


■resample.c でのバッファオーバーラン
=========================== resample.c =================================
PRECALC_LOOPS マクロを定義してコンパイルすると，resample.c の何ヵ所かで
バッファがオーバーランしてしまうバグがあります．
直すべきところは関数 rs_loop() と rs_vib_loop() の 2 ヶ所あります．

-- rs_loop():
修正前：
      if (ofs >= le)
	/* NOTE: Assumes that ll > incr and that incr > 0. */
	ofs -= ll;

修正後：
      while (ofs >= le)
	ofs -= ll;

-- rs_vib_loop():
修正前：
      /* Hopefully the loop is longer than an increment */
      if(ofs >= le)
	ofs -= ll;
修正後：
      /* Hopefully the loop is longer than an increment */
      while(ofs >= le)
	ofs -= ll;


■エンベロープリリースのバグ
=========================== mix.c ======================================
Note Off のリリース時に，エンベロープレベルが上がってしまうという
バグがあります．Note Off の瞬間，音量のレベルが急増し，変な感じになること
があります．このバグは 関数 recompute_envelope() の定義の中の

  if (voice[v].envelope_volume==voice[v].sample->envelope_offset[stage])
    return recompute_envelope(v);

というところを，

  if (voice[v].envelope_volume==voice[v].sample->envelope_offset[stage] ||
      (stage > 2 && voice[v].envelope_volume <
       voice[v].sample->envelope_offset[stage]))
    return recompute_envelope(v);

とすれば直ります．
# ちなみに，ここは典型的な Tail Recursion なので，ちょっと賢いコンパイラなら
# ジャンプ命令に展開してくれるはずです．
=========================================================================


■コメントの間違い
controls.h で

  void (*master_volume)(int mv);
  void (*program)(int channel, int val); /* val<0 means drum set -val */
  void (*volume)(int channel, int val);

と program() の val が負の場合はドラムを意味するとコメントがありますが，
これは間違いです．val は必ず 0 以上の値を取り，ドラムチャネルであっても
そのチャネル番号を指定します．


■timidity 付属の getopt.c を用いた場合に発生するバグ
timidity 付属の getopt.c を用いた場合 getopt() でエラーを起した後に
再度 getopt() を呼び出すと optarg のポインタが不正な位置になってしまい，
次の引数を参照するとおかしな状況になります．timidity 付属の getopt() に
限らず，他の getopt() を用いても同じようなバグに出会う危険性があるため，
getopt() がエラーを起したら，もう getopt() は呼び出さない方が安全でしょう．
という訳で，以下のように修正しましょう．
============ getopt.c (オリジナル) ===============================
gopError:
        optarg = NULL;
        errno  = EINVAL;

============ getopt.c (修正後) ===================================
gopError:
	if (argc > optind)
		optind++;
        optarg = letP = NULL;
        errno  = EINVAL;

============ timidity.c (オリジナル) =============================
  while ((c=getopt(argc, argv, "UI:P:L:c:A:C:ap:fo:O:s:Q:FD:hi:"
#if defined(AU_LINUX) || defined(AU_WIN32)
			"B:" /* buffer fragments */
#endif
#ifdef __WIN32__
			"e"	/* evil (be careful) */
#endif
			))>0)
	 switch(c)
		{
＜中略＞
		default:
	cmderr++; break;
		}

============ timidity.c (修正後) ====================================
  while ((c=getopt(argc, argv, "UI:P:L:c:A:C:ap:fo:O:s:Q:FD:hi:"
#if defined(AU_LINUX) || defined(AU_WIN32)
			"B:" /* buffer fragments */
#endif
#ifdef __WIN32__
			"e"	/* evil (be careful) */
#endif
			))>0)
    {
	 switch(c)
		{
＜中略＞
		default:
	cmderr++; break;
		}
	if(cmderr) break;
    }
====================================================================


■おそらくバグ
========================= playmidi.c ===============================
playmidi.c:dumb_pass_playing_list() 内で RC_QUIT 時に

		play_mode->close_output();
		ctl->close();

を呼び出しています．timidity.c:main() でも，この関数からリターンした後，

		/* Return only when quitting */
		ctl->pass_playing_list(argc-optind, &argv[orig_optind]);

		play_mode->close_output();
		ctl->close();

のように close() を呼び出しています．2 重に close しています．ctl の方は
内部で 2 重 close をチェックしており，問題が発生しませんが．play_mode の
方は 2 重 close がチェックされていません．よって曲の終了時になんらかの
不都合を伴う危険性があります．また，新たに ctl を追加したときに，
2 重 close のチェックを忘れると問題を引き起こしかねません．
よって，playmidi.c:dumb_pass_playing_list() 内の

	    case RC_QUIT:
		play_mode->close_output();
		ctl->close();
		return;
	    }

というところは

	    case RC_QUIT:
		return;
	    }

と，close 処理の 2 行を消すべきでしょう．
============================================================================


■mix.c:mix_voice() のバグ
================================ mix.c ===============================
ramp_out() 関数が c==0 で呼ばれたら「Division by zero」で落っこちます．
可能性はかなり低いのですが，resample_voice(v, &c); が呼ばれたのち，c==0 に
なる可能性があります．mix_voice() で

  if (vp->status==VOICE_DIE)
    {
      if (c>=MAX_DIE_TIME)
	c=MAX_DIE_TIME;
      sp=resample_voice(v, &c);
      ramp_out(sp, buf, v, c);
      vp->status=VOICE_FREE;
    }

というところを，

  if (vp->status==VOICE_DIE)
    {
      if (c>=MAX_DIE_TIME)
	c=MAX_DIE_TIME;
      sp=resample_voice(v, &c);
      if(c > 0)
	  ramp_out(sp, buf, v, c);
      vp->status=VOICE_FREE;
    }

としてください．
============================================================================


■WAV 出力のバグ (Reported by Masaaki Koyanagi <koya@k2.t.u-tokyo.ac.jp>)
=========================== wave_a.c ===================================
修正前
  if ((dpm.encoding & (PE_MONO | PE_16BIT)) == PE_MONO)
    RIFFheader[32]='\001';
  else if (!(dpm.encoding & PE_MONO) || (dpm.encoding & PE_16BIT))
    RIFFheader[32]='\002';

修正後

  if ((dpm.encoding & (PE_MONO | PE_16BIT)) == PE_MONO)
    RIFFheader[32]='\001';
  else if (!(dpm.encoding & PE_MONO) && (dpm.encoding & PE_16BIT))
    RIFFheader[32]='\004';
  else 
    RIFFheader[32]='\002';
===========================================================================


■バグ (Reported by Masaaki Koyanagi <koya@k2.t.u-tokyo.ac.jp>)
同一音の NoteON -> NoteOFF の組が重なると正しく演奏できない．
※このバグは小柳雅明さんのレポートを元に 0.2i+W15 で FIX しました．
※ただし，0.2i+W17 以降でないと不都合があると思います．


■多分バグ
実際の音色はプログラムチェンジが発行された時に変化しなければならない．
バンクセレクト -> Note ON -> プログラムチェンジ
の間の Note ON は，まだ，古いバンクの音色でなければならないのに，
TiMidity では既に変化してしまっている．
TiMidity ではバンクセレクトの段階で既に音色が変化してしまっているが，
実際はプログラムチェンジが発行されるまで変化してはならない．
※このバグは 0.2i+W15 で FIX しました．


■バンクセレクトのバグ (Reported by Masaaki Koyanagi <koya@k2.t.u-tokyo.ac.jp>)
readmidi.cにおいて

                  case 32:
                    if (b!=0)
                      ctl->cmsg(CMSG_INFO, VERB_DEBUG,
                                "(Strange: tone bank change 0x%02x)", b);
                    else
                      control=ME_TONE_BANK;
                    break;

を

                  case 32:
                    if (b!=0)
                      ctl->cmsg(CMSG_INFO, VERB_DEBUG,
                                "(Strange: tone bank change 0x%02x)", b);
/*                    else
                      control=ME_TONE_BANK;*/
                    break;

としてください。

コントロールチェンジ０と３２はそれぞれBank Select MSBと
Bank Select LSBなのですが、GS音源においてはLSBは０に固定されていて
MSBのみが意味があります。

修正前ではLSBをMSBとして設定してしまいます。
例えばMSBを8、LSBを0とするBank selectに対して
本来はcfgのBank 8の音色とならなければならないのに、
Bank 0(←LSBの値)となってしまうのです。

＃ソースのコメントを読むと作者自身もこのあたりは良く知らないようです。


■Sustain のバグ
Sustain (Hold Pedal) コントロールでは，0〜63 で off, 64〜127 で on と
なります．ところが，0.2i では 1〜63 でも on になったままになり，
音がなり続けてしまう場合があります．以下，FIX した周辺の一部のコードです．
=========================== readmidi.c =================================
		  case 11: control=ME_EXPRESSION; break;
		  case 64: control=ME_SUSTAIN; b = (b >= 64); break;
		  case 120: control=ME_ALL_SOUNDS_OFF; break;
========================================================================


■アンチエイリアシング処理のバグ
アンチエイリアシング処理のコードにバグがあります．
config.h で LOOKUP_HACK を定義してコンパイルし，
アンチフィルタリング (-a) オプションで実行すると，ノイズだらけになります．
一部だけを直したのでは完全な解決にはならないのですが，とりあえずなら
以下のようにすれば直ります(でっちあげの FIX)．
=========================== filter.c ================================
<中略>

/*
 * FIR filtering -> apply the filter given by coef[] to the data buffer
 * Note that we simulate leading and trailing 0 at the border of the
 * data buffer
 */

/* This is quick hack for antialiasing filter's bug fix. */
#define sample_t int16
static void filter(sample_t *result,sample_t *data, int32 length,FLOAT_T coef[])
{

<中略>

    /* We apply the filter we have designed on a copy of the patch */
    temp = safe_malloc(sp->data_length);
    memcpy(temp,sp->data,sp->data_length);

    filter((int16 *)sp->data,temp,sp->data_length/sizeof(sample_t),fir_symetric);

    free(temp);
}
========================================================================


■FSCALE, FSCALENEG のバグ
ldexp を用いる場合，FSCALE, FSCALENEG の第一引数を double に cast しないと
ならない．
=========================== config.h =================================
#ifdef USE_LDEXP
#  define FSCALE(a,b) ldexp((double)(a),(b))
#  define FSCALENEG(a,b) ldexp((double)(a),-(b))
#else
#  define FSCALE(a,b) ((a) * (double)(1<<(b)))
#  define FSCALENEG(a,b) ((a) * (1.0L / (double)(1<<(b))))
#endif
======================================================================


■wav2pat のバグ
wav2pat のオリジナルのコードでは，SunOS とかだったら Bus error になっていた
はず．．．？以下のように直しました．
=========================== wav2pat.c ================================
  /* hammer together something that looks like a GUS patch header */

#define pound(a) *point++=(a);
#define pounds(a) { int16 x = LE_SHORT(a); memcpy(point, &x, 2); point+=2; }
#define poundl(a) { int32 x = LE_LONG(a);  memcpy(point, &x, 4); point+=4; }
#define bounce(a) point += a;
  
  memset(thing, 0, 335);
  point=thing;
=======================================================================


■pre_resample() のバグ
timidity で midi ファイルを演奏すると，時たまプツってソルトノイズが聞こえて
くると思います．このバグは以前から気が付いていたのですが，どこにバグが
あるか，はっきりと分かりませんでした．もちろん，patch ファイルにソルトノイズが
入っている場合もあるのですが(例えば power/h-tomlo2.pat など)，パッチの
ソルトノイズはすでに除いてあるから，原因は別にあるはずだ，と探していました．
そしてとうとう，やっとそのバグを発見しました．以下，そのバグを FIX した
周辺のコードです．
========================== resample.c ==================================
void pre_resample(Sample * sp)
{
  double a, xdiff;
  int32 incr, ofs, newlen, count;
  int16 *newdata, *dest, *src = (int16 *) sp->data, *vptr;
  int32 v, v1, v2, v3, v4, i;

  static const char note_name[12][3] =
  {
    "C", "C#", "D", "D#", "E", "F", "F#", "G", "G#", "A", "A#", "B"
  };

  ctl->cmsg(CMSG_INFO, VERB_NOISY, " * pre-resampling for note %d (%s%d)",
	    sp->note_to_use,
	    note_name[sp->note_to_use % 12], (sp->note_to_use & 0x7F) / 12);

  a = ((double) (sp->sample_rate) * freq_table[(int) (sp->note_to_use)]) /
    ((double) (sp->root_freq) * play_mode->rate);
  newlen = (int32)(sp->data_length / a);
  dest = newdata = safe_malloc((newlen >> (FRACTION_BITS - 1)) + 2);

  count = (newlen >> FRACTION_BITS) - 1;
  ofs = incr = (sp->data_length - (1 << FRACTION_BITS)) / count;

  if (--count)
    *dest++ = src[0];

  /* Since we're pre-processing and this doesn't have to be done in
     real-time, we go ahead and do the full sliding cubic interpolation. */
  count--;
  for(i = 0; i < count; i++)
    {
      vptr = src + (ofs >> FRACTION_BITS);
      v1 = *(vptr - 1);
      v2 = *vptr;
      v3 = *(vptr + 1);
      v4 = *(vptr + 2);
      xdiff = FSCALENEG(ofs & FRACTION_MASK, FRACTION_BITS);
      v = (int32)(v2 + (xdiff / 6.0) * (-2 * v1 - 3 * v2 + 6 * v3 - v4 +
       xdiff * (3 * (v1 - 2 * v2 + v3) + xdiff * (-v1 + 3 * (v2 - v3) + v4))));
      if(v < -32768)
	  *dest++ = -32768;
      else if(v > 32767)
	  *dest++ = 32767;
      else
	  *dest++ = (int16)v;
      ofs += incr;
    }

  if (ofs & FRACTION_MASK)
    {
      v1 = src[ofs >> FRACTION_BITS];
      v2 = src[(ofs >> FRACTION_BITS) + 1];
      *dest++ = (int16)(v1 + (((v2 - v1) * (ofs & FRACTION_MASK)) >> FRACTION_BITS));
    }
  else
    *dest++ = src[ofs >> FRACTION_BITS];
  *dest = *(dest - 1) / 2;
  *dest = *(dest - 1) / 2;

  sp->data_length = newlen;
  sp->loop_start = (int32)(sp->loop_start / a);
  sp->loop_end = (int32)(sp->loop_end / a);
  free(sp->data);
  sp->data = (sample_t *) newdata;
  sp->sample_rate = 0;
}
=======================================================================


■FINALINTERP のバグ
============================ resamp.c ================================
resamp.c で

#define FINALINTERP if (ofs == le) *dest++=src[ofs>>FRACTION_BITS];
/* So it isn't interpolation. At least it's final. */

というコードがあります．このままでは，もし，データの loop end がデータの
最後に位置している場合に，確保したデータの領域を越えてアクセスしてしまう可能性
があります．ここは

#define FINALINTERP if (ofs == le) *dest++=src[(ofs>>FRACTION_BITS)-1]/2;
/* So it isn't interpolation. At least it's final. */

のようにしなければならないでしょう．
======================================================================


■以下，恐らくバグだろうとにらんでいるところ．

オリジナルでは，以下の read_config_file() 関数のエラーが無視されていた．
エラーで読み込まれないファイルがあっても何もいわずに成功したかのように
振る舞う場合があったような気が．．．以下のように FIX しました．
============================ timidity.c ==============================
      else if (!strcmp(w[0], "source"))
	{
	  if (words < 2)
	    {
	      fprintf(stderr, "%s: line %d: No file name given\n", name, line);
	      return -2;
		 }
	  for (i=1; i<words; i++)
	    {
		int status;
		rcf_count++;
		status = read_config_file(w[i]);
		rcf_count--;
		if(status != 0)
		    return status;
	    }
	}
      else if (!strcmp(w[0], "default"))
	{
======================================================================


■以下,バグではないが，移植性を損なうところ．
(pipe に対して ioctl/FIONREAD がサポートされていない OS がある．)
====================== motif_p.c, tk_c.c =============================
#if defined(sgi)
#include <sys/time.h>
#endif

#if defined(SOLARIS)
#include <fileio.h>
#endif

int pipe_read_ready(void)
{
#if defined(sgi)
    fd_set fds;
    int cnt;
    struct timeval timeout;

    FD_ZERO(&fds);
    FD_SET(fpip_in, &fds);
    timeout.tv_sec = timeout.tv_usec = 0;
    if((cnt = select(fpip_in + 1, &fds, NULL, NULL, &timeout)) < 0)
    {
	perror("select");
	return -1;
    }

    return cnt > 0 && FD_ISSET(fpip_in, &fds) != 0;
#else
    int num;

    if(ioctl(fpip_in,FIONREAD,&num) < 0) /* see how many chars in buffer. */
    {
	perror("ioctl: FIONREAD");
	return -1;
    }
    return num;
#endif
}
======================================================================

■シンタックスエラーを引き起こすバグ
============================== resamp.c ==============================
LINEAR_INTERPOLATION マクロが未定義の場合，

#define INTERPVARS

が定義されます．ところが，関数の宣言のところで，

static sample_t *rs_???(...)
{
  INTERPVARS;
  Voice 
    *vp=&voice[v];
  ...
}

と，INTERPVARS の後ろに 「;」がついているため，これが展開されると，

static sample_t *rs_???(...)
{
  ;
  Voice 
    *vp=&voice[v];
  ...
}

となってしまいます．この残った「;」があるためにエラーとなってコンパイ
ルできません．「;」が空の実行文とみなされ，次に続く宣言文が文法違反と
なってしまうからです．頑張って INTERPVARS の後ろのセミコロンを取りましょう．
全部で 6 ヶ所あります．
======================================================================


■おまけ
あちらこちらに

while(i--) { ... }

のようなコードをよく見かけます．

int j;
for(j = 0; j < i; j++) { ... }

と書き換えた方が高速になる場合があるようです．(IRIX の cc で 7% 上がった)．
というのは，for 文で書いておくと最適化オプションをつけてコンパイルしたときに
コンパイラが unroll の展開を行ってくれるからです．アセンブルコードに
コンパイルしてみるとよく分かります．unroll とは，例えば，

for(j = 0; j < i; j++)
    val[j]++;

のような計算を，

for(j = 0; j < i - 4; j += 5)
{
    val[j]++;
    val[j+1]++;
    val[j+2]++;
    val[j+3]++;
    val[j+4]++;
}
for(; j < i; j++)
    val[j]++;

のように展開して，比較回数とジャンプ命令を減らす最適化のテクニックです．
gcc では -funroll-all-loops オプションで上記展開を行ってくれます．
一部の CPU ではあまり高速化の効果が現れませんが，Intel チップなどの CPU では
結構その効果が現れます．

■もひとつおまけ
浮動小数点数に float が多く使われていますが，これを全部 double にしたら
高速になるはず(多分)．特に double の精度の FPU を搭載しているマシンなどは
試してみる価値はあるはず．

■またまたおまけ
#define LINEAR_INTERPOLATION
#undef LOOKUP_HACK
という config.h の設定において，resample.c の最初の方にある

#  define INTERPVARS sample_t v1, v2;

ってところを

#  define INTERPVARS int32 v1, v2;

にしたら高速になった．dec/osf1 や sgi/irix などでは，short (16 ビット) の
演算より int (32 ビット) の演算の方が速いのだ．

■ちょっとした疑問
int play_midi(MidiEvent *eventlist, int32 events, int32 samples) { ... }
という関数で，仮引数 `events' を全く使っていないんだけど，なんでしょうね？

■ちょっとした疑問 part II
static Instrument *load_instrument(char *name, int percussion,
				   int panning, int amp, int note_to_use,
				   int strip_loop, int strip_envelope,
				   int strip_tail) { ... }
あれ，ここでも `percussion' を使ってないぞ．

■ちょっとした疑問 part III
static FILE *try_to_open(char *name, int decompress, int noise_mode) { ... }
ここでも，`noise_mode' が使われてないよ．