tags 649274 + pending
thanks
On Sat, 19 Nov 2011, Hans de Goede wrote in part:
So about the issues I've found while reviewing all the changes
the Debian pkgs make to the upstream tarbal:
--- timidity-2.13.2.orig/timidity/reverb.c
+++ timidity-2.13.2/timidity/reverb.c
@@ -1624,8 +1630,8 @@ static void do_ch_reverb_panning_delay(i
buf[i] += r;
buf[++i] += l;
- if (++index0 == buf_size) {index0 = 0;}
- if (++buf_index == buf_size) {buf_index = 0;}
+ if (index0++ == buf_size) {index0 = 0;}
+ if (buf_index++ == buf_size) {buf_index = 0;}
}
memset(reverb_effect_buffer, 0, sizeof(int32) * count);
info->index[0] = index0;
This changes pre-increment to post increment, *at the end of a loop*,
causing index0 (and buf_index) to be ranged as:
0 <= index0 <= buf_index
which should of course be:
0 <= index0 < buf_index
So I believe this Debian patch to be wrong, and the original code to
be correct.
Having looked at the code, agreed. I'll remove this in the next upload.
--- timidity-2.13.2.orig/timidity/playmidi.c
+++ timidity-2.13.2/timidity/playmidi.c
@@ -7980,7 +7980,7 @@ int play_event(MidiEvent *ev)
channel[ch].temper_type = current_event->a;
ctl_mode_event(CTLE_TEMPER_TYPE, 1, ch,
channel[ch].temper_type
if (temper_type_mute) {
- if (temper_type_mute & 1 << current_event->a
+ if ((temper_type_mute & 1 << current_event->a)
- ((current_event->a >= 0x40) ? 0x3c
:
SET_CHANNELMASK(channel_mute, ch);
ctl_mode_event(CTLE_MUTE, 1, ch, 1);
@@ -8003,7 +8003,7 @@ int play_event(MidiEvent *ev)
ctl_mode_event(CTLE_TEMPER_TYPE, 1, i,
channel[i].tempe
}
if (temper_type_mute) {
- if (temper_type_mute & 1 << current_event->a
+ if ((temper_type_mute & 1 << current_event->a)
- ((current_event->a >= 0x40) ? 0x3c
:
FILL_CHANNELMASK(channel_mute);
for (i = 0; i < MAX_CHANNELS; i++)
I believe this patch is meant to fix a compiler warning, unfortunately
the patch does more then that, it actually changes the meaning of the code.
The original code calculates how much to shift the 1 by before masking
by seeing if a >= 0x40 and in that case shifting by (a - 0x3c), otherwise
it shifts by just a (actually (a - 0), but that is the same).
The precedence rules relevant here (for the original code) are, first do
the - op, then the << and then &. The added () change the order in which this
operations are done. Given that temper_type_mute is strictly a bitfield,
doing
a - operation on it makes no sense, and I believe the old code is correct.
Note
that my patchset has what I believe is a correct fix for the compiler warning
in patch 0010-Fix-silence-various-compiler-warnings.patch
Also agreed, and also will revert. This code looks pretty funny, but it
makes slightly more sense in the context of the -Q manpage options:
On the other hand, to put `t' character after -Q option
or to use --temper-mute describes temperament mute.
This mutes channels of specific temperament type n. For
preset temperament, n can range 0 to 3. For
user-defined temperament, n can range 4 to 7.
So the target cases are a = 0 through 3, or a = 0x40 through 0x43.
+++ timidity-2.13.2/timidity/instrum.c
@@ -1070,7 +1071,7 @@ Instrument *load_instrument(int dr, int
}
/* panning */
if (ip != NULL && bank->tone[prog].pan != -1) {
- pan = ((int) bank->tone[prog].pan & 0x7f) - 64;
+ pan = ((int) bank->tone[prog].pan & 0x7f); /* - 64
*/;
for (i = 0; i < ip->samples; i++) {
panning = (int) ip->sample[i].panning + pan;
panning = (panning < 0) ? 0
This is (I believe) a fix for Debian bug 536252:
http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=536252
See the original bug for the rational of the fix. I believe this rationale
is wrong, as the code here is adding 2 pans together, so it is correct to
change one from 0-127 to -64 - 63 to get the desired effect.
Before looking at the Debian changes I spend an entire day tracking down
what I believe is the real cause for Debian bug 536252, after a similar
issue was reported in Fedora bug 710927:
https://bugzilla.redhat.com/show_bug.cgi?id=710927
What is going on (and a workaround-ish way to fix it) can be found in
0009-sndfont-Work-around-soundfonts-with-missing-links-be.patch
I agree that the Debian patch looks wrong and will revert this too. I've
looked briefly at the RHBZ report and patch 0009, and am currently reading
up on the Soundfont spec so I can make sense of the bug / your patch. :-)
I haven't yet looked at the remainder of your patches, but will do so now.
That rounds up the "wrong" (or so I believe) fixes in the Debian pkg.
As said I hope to do a new upstream release soon, amongst a lot of bugfixes
this will also include IPV6 support for the relevant bits of timidity.
Thanks, and thanks for identifying these issues. I see there's been a bit
of activity a week or two ago in upstream (and upstream looks to have most
the patches originally submitted with this bug report), so I'll see if I
can just make a new orig tarball based on upstream git and drop our
patches entirely.
--
Geoffrey Thomas
http://ldpreload.com
geo...@ldpreload.com
--
To UNSUBSCRIBE, email to debian-bugs-dist-requ...@lists.debian.org
with a subject of "unsubscribe". Trouble? Contact listmas...@lists.debian.org
