Created attachment 95065 [details] [review]
Fix which fixes the bug, but breaks testsuite

Here is the trivial way to fix it, that is just change the code, and don't be backward compatible. It breaks the test suite, too. 

However, since we have lots of legacy data (such as in the test suite), the question is this: shall we preserve the original (buggy) behaviour when people load old songs?

If yes (I'd recommend that), then I haven't quite figured out how to do it, because compat_setup() appears to be executed before the properties are restored. However, what would be needed is something that gets executed after the properties have been restored, and checks ctrl_mul, and if ctrl_mul is set, assigns ostrength = ostrength * 0.5.

That way, old instruments/sounds would still sound the way they did before. I've tried using beast with the patch applied to play a few of my songs, and they sound broken, for instance

  http://space.twc.de/~stefan/music/pixel_ice2.bse

which sounds quite broken with the patch applied without compat code in place.

Created attachment 95743 [details] [review]
Fix with compatibility code in place

Here is a fix which passes the test suite, too. Note that since the compatibility code needs to know whether the file was saved with the new or old BseAmplifier, the version needs to be bumped to 0.7.2 - but since the release is very close, I think its acceptable to do this now.

Care needs to be taken when applying the patch now that the version number has been bumped to 0.7.2. Since the patch decides on wether to run the compat code or not depending on the version number, files edited with a version of beast 0.7.2 without the patch applied will eventually get messed up (wrong amplification).

Right now there are none in SVN:

stefan@lotrien:/usr/local/src/beast$ grep '0.7.2' $(find . -name '*.bse')
stefan@lotrien:/usr/local/src/beast$

But that could change for instance if editing existing library/testsuite files or adding new ones is performed with a version of beast-0.7.2-rc1 without the patch applied.

Comment on attachment 95743 [details] [review]
Fix with compatibility code in place

thanks, however there are a couple issues with this patch:

>--- bse/ChangeLog	(revision 4376)
>+++ bse/ChangeLog	(working copy)
>@@ -1,3 +1,9 @@
>+Mon Sep 17 17:42:18 2007  Stefan Westerfeld  <stefan@space.twc.de>
>+
>+	* bsecxxbase.hh:
>+	* bsecxxbase.cc: Add virtual method restore_finish() which overrides
>+	and chains the BseObject base class method.

i chose to provide restore_finished() instead which doesn't need to chain back to the BseObjectClass handler which restore_finish() should have. (basically, we implement chaining to parent classes in C++ the same way as in C, so if cxxbase does the chaining and the C++ method doesn't need to chain up, it should be called differently). also, versioning is required in this method as you'll see further down.


>--- plugins/bseamplifier.cc	(revision 4376)
>+++ plugins/bseamplifier.cc	(working copy)
>@@ -1,5 +1,6 @@
> /* BSE - Bedevilled Sound Engine
>  * Copyright (C) 2002, 2003 Tim Janik
>+ * Copyright (C) 2007 Stefan Westerfeld
>  *
>  * This library is free software; you can redistribute it and/or
>  * modify it under the terms of the GNU Lesser General Public
>@@ -38,7 +38,9 @@ class Amplifier : public AmplifierBase {
>       cl2 = params->clevel2 * 0.01;
>       ctrl_mul = params->ctrl_mul;
>       ctrl_exp = params->ctrl_exp;
>-      ocs = params->ostrength * 0.5 * 0.01;
>+      ocs = params->ostrength * 0.01;
>+      if (!ctrl_mul)
>+        ocs *= 0.5;
>       bl = params->base_level * 0.01;
>       double master = params->master_volume;
>       al1 *= master;
>@@ -220,11 +222,15 @@ protected:
>       }
>     return false;
>   }
>+
>+  bool m_restore_compat_071;
>+

this has multiple issues. first, we declare variables always at the top of C++ classes. second, we use bitfields to implement booleans to save space. third, using a full int throughout the full runtime in all bse amplifier instances just to patch up restoration behavior is very wastefull. most of the time this variable isn't needed, so if at all, this should have been object QData which could be used during parsing and deleted afterwards. however, since further up in the call stack the versioning information is available anyway, it just needs to be passed down, so this method can operate just like compat_setup. (see my commits to SVN)


>   void
>   compat_setup (guint          vmajor,
>                 guint          vminor,
>                 guint          vmicro)
>   {
>+    m_restore_compat_071 = BSE_VERSION_CMP (vmajor, vminor, vmicro, 0, 7, 1) <= 0;
>     if (BSE_VERSION_CMP (vmajor, vminor, vmicro, 0, 6, 2) <= 0)
>       set ("olevel", 100.0, NULL);
>     if (BSE_VERSION_CMP (vmajor, vminor, vmicro, 0, 5, 4) <= 0)

>--- plugins/ChangeLog	(revision 4376)
>+++ plugins/ChangeLog	(working copy)
>@@ -1,3 +1,9 @@
>+Wed Sep  5 23:34:00 2007  Stefan Westerfeld  <stefan@space.twc.de>
>+
>+	* bseamplifier.cc: Only multiply control signals with 0.5 which are
>+	added up (keeps them in [0,1] range), but don't do the same for
>+	control signals which are multiplied - fixes #474244.

when mentioning bugs in changelog, please use this exact syntax:
  "bug" <whitespace> "#" [0-9]+
that way, the bug numbers are parsable by release scripts such as this:
  http://blogs.gnome.org/mr/2007/05/30/maintainer-06/

(In reply to comment #2)
> Created an attachment (id=95743) [edit]
> Fix with compatibility code in place
> 
> Here is a fix which passes the test suite, too. Note that since the
> compatibility code needs to know whether the file was saved with the new or old
> BseAmplifier, the version needs to be bumped to 0.7.2 - but since the release
> is very close, I think its acceptable to do this now.

has been done meanwhile in SVN. also, a fix similar to yours in spirit has been applied, thanks for your work. please verify that your amplification issues are fixed with this commit:

2007-09-20 21:59:18  Tim Janik  <timj@gtk.org>

        * bseamplifier.cc: fixed bug #474244, based on a patch by Stefan
        Westerfeld which changes multiplied control signals to not get
        halved as we do with summated signals to constrain the range to [0,1].
        compatibility code handles value adjustments for bse files <= 0.7.1.