SoX - Sound eXchange

I've noticed checks for separate functions HAVE_ID3TAG_SET_FIELDVALUE and
HAVE_LAME_SET_VBR_QUALITY. I can see how using CMake invites this approach.
But as more and more individual functions are independently tested this way,
the possible build combinations grow exponentially which makes testing them
all impractical.

 

I've searched lame.def in the LAME project's source history to find the time
line for id3tag_set_fieldvalue and lame_set_VBR_quality. lame.def is the
Windows dynamic library definition file where exports are defined. I used it
because it's only a single file and tells us when the functions became
available for dynamic linking.

 

id3tag_set_fieldvalue was added:

 

lame.def rev 1.2

Wed Jun 27 21:22:22 2007 UTC

 

The first release after that was:

 

LAME 3.98 beta 5   August 12 2007

 

 

lame_set_VBR_quality was added:

 

lame.def rev 1.6

Sat Apr 12 18:18:06 2008 UTC

 

The first release after that was:

 

LAME 3.98 beta 8   April 13 2008

 

I think it's possible to simplify the build situation by decreasing the
number of HAVE_LAME_* defs.

 

What's the oldest version of LAME that's supported? The SoX INSTALL file
says:

 

    == Requirements ==

 

    The following requirements have been tested. 

    Nevertheless, other versions may work as well.

 

    LAME source distribution version 3.97

    available on [http://lame.sourceforge.net]

 

So I'd guess LAME 3.97 is the oldest supported version. Is that right?

 

The easiest thing to do would be to raise the requirement to LAME 3.98 and
drop support for LAME3.97. Old versions of SoX could still be used with old
versions of LAME. Despite the many benefits of this approach, I'm not
advocating for this to happen now. Right now I can see how this could be
thoughtfully rejected.

 

The second approach is to replace HAVE_ID3TAG_SET_FIELDVALUE and
HAVE_LAME_SET_VBR_QUALITY (and avoid future HAVE_LAME_functionname), and
instead use HAVE_LAME_VER_398. This I do advocate for the following reasons:

 

* It cuts the active LAME related build variants from 4 to 2 (3 if you count
no LAME). (Multiply this by 2 if you want to include dynamic vs. static
linking.)

 

* It prevents future explosion of build combinations as additional functions
are used.

 

* It explicitly allows contributors to use any new LAME 3.98 function
without extra work as long as they use "#ifdef HAVE_LAME_VER_398" with some
reasonable fallback if it's not defined.

 

* It creates an easier framework for contributors. Testing requirements will
be clearer and by eliminating the need to create and test new
HAVE_LAME_functionnames, a full understanding of CMake will not be required.

 

* It enables a reasonable policy of actively supporting, say, two versions
of LAME. When LAME 4.00 ships with some compelling new feature, Support for
3.97 can be phased out, 3.98 retained (as the "old" version) and new
features will be enabled with HAVE_LAME_VER_400

 

Jim

Chris, appreciate your thoughts on what compatibility we should be aiming for here.

/Rob

--- On Sun, 27/9/09, Jim Harkins <jimh@...> wrote:

> From: Jim Harkins <jimh@...>
> Subject: [SoX-devel] MP3 LAME version requirements
> To: sox-devel@...
> Date: Sunday, 27 September, 2009, 5:13 AM
> 
> 
> 
>  
>  
> 
> 
> 
> 
> 
> 
>  
> 
> 
> 
> I've noticed checks for separate
> functions HAVE_ID3TAG_SET_FIELDVALUE
> and HAVE_LAME_SET_VBR_QUALITY. I can see how using CMake
> invites this approach.
> But as more and more individual functions are independently
> tested this way,
> the possible build combinations grow exponentially which
> makes testing them all
> impractical. 
> 
>    
> 
> I've searched lame.def in the LAME
> project's source history to find the
> time line for id3tag_set_fieldvalue and
> lame_set_VBR_quality. lame.def is the
> Windows dynamic library definition file where exports are
> defined. I used it
> because it's only a single file and tells us when the
> functions became
> available for dynamic linking. 
> 
>    
> 
> id3tag_set_fieldvalue was
> added: 
> 
>    
> 
> lame.def rev 1.2 
> 
> Wed Jun 27 21:22:22 2007
> UTC 
> 
>    
> 
> The first release after that
> was: 
> 
>    
> 
> LAME 3.98 beta 5   August 12
> 2007 
> 
>    
> 
>    
> 
> lame_set_VBR_quality was
> added: 
> 
>    
> 
> lame.def rev 1.6 
> 
> Sat Apr 12 18:18:06 2008
> UTC 
> 
>    
> 
> The first release after that
> was: 
> 
>    
> 
> LAME 3.98 beta 8   April 13
> 2008 
> 
>    
> 
> I think it's possible to simplify
> the build situation by decreasing the
> number of HAVE_LAME_* defs. 
> 
>    
> 
> What's the oldest version of LAME
> that's supported? The SoX INSTALL
> file says: 
> 
>    
> 
>     == Requirements
> == 
> 
>    
> 
>     The following
> requirements have been tested.  
> 
>     Nevertheless, other
> versions may work as well. 
> 
>    
> 
>     LAME source
> distribution version 3.97 
> 
>     available on
> [http://lame.sourceforge.net] 
> 
>    
> 
> So I'd guess LAME 3.97 is the oldest
> supported version. Is that right? 
> 
>    
> 
> The easiest thing to do would be to
> raise the requirement to LAME 3.98
> and drop support for LAME3.97. Old versions of SoX could
> still be used with old
> versions of LAME. Despite the many benefits of this
> approach, I'm not
> advocating for this to happen now. Right now I can see how
> this could be
> thoughtfully rejected. 
> 
>    
> 
> The second approach is to replace
> HAVE_ID3TAG_SET_FIELDVALUE and
> HAVE_LAME_SET_VBR_QUALITY (and avoid future
> HAVE_LAME_functionname), and
> instead use HAVE_LAME_VER_398. This I do advocate for the
> following reasons: 
> 
>    
> 
> * It cuts the active LAME related build
> variants from 4 to 2 (3 if you
> count no LAME). (Multiply this by 2 if you want to include
> dynamic vs. static
> linking.) 
> 
>    
> 
> * It prevents future explosion of build
> combinations as additional
> functions are used. 
> 
>    
> 
> * It explicitly allows contributors to
> use any new LAME 3.98 function
> without extra work as long as they use "#ifdef
> HAVE_LAME_VER_398"
> with some reasonable fallback if it's not
> defined. 
> 
>    
> 
> * It creates an easier framework for contributors.
> Testing requirements will be clearer and by eliminating the
> need to create and
> test new HAVE_LAME_functionnames, a full understanding of
> CMake will not be
> required. 
> 
>    
> 
> * It enables a reasonable policy of
> actively supporting, say, two
> versions of LAME. When LAME 4.00 ships with some compelling
> new feature,
> Support for 3.97 can be phased out, 3.98 retained (as the
> "old"
> version) and new features will be enabled with
> HAVE_LAME_VER_400 
> 
>    
> 
> Jim 
> 
> 
> 
>  
> 
> 
> 
> -----Inline Attachment Follows-----
> 
> ------------------------------------------------------------------------------
> Come build with us! The BlackBerry&reg; Developer
> Conference in SF, CA
> is the only developer event you need to attend this year.
> Jumpstart your
> developing skills, take BlackBerry mobile applications to
> market and stay 
> ahead of the curve. Join us from November 9-12, 2009.
> Register now!
> http://p.sf.net/sfu/devconf
> -----Inline Attachment Follows-----
> 
> _______________________________________________
> SoX-devel mailing list
> SoX-devel@...
> https://lists.sourceforge.net/lists/listinfo/sox-devel
>

I've currently marked lame 3.97 as deprecated in ChangeLog which gives it a
1 year life span at most.  If we need to we can reduce this to shorter
period if mp3.c changes drastically soon.

Jim gave some ideas on what to do once support is removed... Hopefully, we
will remember to look at mailing list archive when its time to remove and
take the comments into account.

Chris

On Mon, Sep 28, 2009 at 9:33 AM, Chris Bagwell <chris@...> wrote:

>
>
> On Sun, Sep 27, 2009 at 10:49 AM, robs <aquegg@...> wrote:
>
>> Chris, appreciate your thoughts on what compatibility we should be aiming
>> for here.
>>
>> /Rob
>>
>>
> Since 3.98 is over 1 year old, I guess we shouldn't sweat to much if we
> want to require it as minimum... but seems recent emails may require 3.98.2
> which also barely meets 1 year old date as well.
>
> As this is  such a highly requested feature, I think Jim's suggestion makes
> sense and benefits outweigh negatives.
>
> If this goes in immediately, we should probably not call this a dot release
> any more though to better reflect minimum versions changing.
>
> Chris
>

On Fri, Nov 27, 2009 at 10:42 PM, Doug Cook
<idigdoug@...:

>
>
> On Fri, Nov 27, 2009 at 6:51 PM, Chris Bagwell <chris@...:
>
>> I've currently marked lame 3.97 as deprecated in ChangeLog which gives it
>> a 1 year life span at most.  If we need to we can reduce this to shorter
>> period if mp3.c changes drastically soon.
>>
>> Jim gave some ideas on what to do once support is removed... Hopefully, we
>> will remember to look at mailing list archive when its time to remove and
>> take the comments into account.
>>
>> Chris
>>
>>   If this goes in immediately, we should probably not call this a dot
>>> release any more though to better reflect minimum versions changing.
>>>
>>> Chris
>>>
>>
> Tracker patch *2870196*  includes changes that improve support for LAME
> 3.97. Main point is to enable most of the ID3 tags to be set via the ID3
> APIs supplied by 3.97. The current CVS code relies entirely on a function
> that was added in 3.98.1, but 90% of the ID3 support can be implemented
> using functions available in 3.97. Other change is to provide better
> fallback logic and better informational messages in the case where the
> current version of LAME doesn't support something (esp. important if using
> DL_OPEN since we don't know what version we're going to get in that case).
> Final change (for the non-dlopen case only) is to more accurately specify
> the autoconf variables needed for each situation. This calls for 3 autoconf
> variables to be used:
>
>

Thanks, Doug.  Today I've been trying to figure out current state of patches
in tracker and at least close out the ones that seem to have already been
merged into CVS.  I'll look into this specific patch and see if I can't
incorporate it for 14.3.1 release.

I'm also trying to get DL_SNDFILE working with autoconf... Its got a couple
of issues still in my initial attempt but they should be easy to fix.

Chris

>
>  Thanks, Doug.  Today I've been trying to figure out current state of
> patches in tracker and at least close out the ones that seem to have already
> been merged into CVS.  I'll look into this specific patch and see if I can't
> incorporate it for 14.3.1 release.
>
> I'm also trying to get DL_SNDFILE working with autoconf... Its got a couple
> of issues still in my initial attempt but they should be easy to fix.
>
> Chris
>
Right. I noticed some activity on the list and just wanted to call some
attention to this one, since I think it is significant. If support for LAME
3.97 is at all important, this patch should probably be included. On the
other hand, if 3.97 is truly deprecated, then it probably isn't such a big
deal.

Unfortunately, I'm not particularly skilled with autoconf (and I'm not sure
I ever want to become more skilled with it), so I can't help much on the
autoconf side of things. Good luck on that!

On Fri, Nov 27, 2009 at 10:49 PM, Chris Bagwell <chris@...:

>
>
> On Fri, Nov 27, 2009 at 10:42 PM, Doug Cook <
> idigdoug@...> wrote:
>
>>
>>
>> On Fri, Nov 27, 2009 at 6:51 PM, Chris Bagwell <chris@...:
>>
>>> I've currently marked lame 3.97 as deprecated in ChangeLog which gives it
>>> a 1 year life span at most.  If we need to we can reduce this to shorter
>>> period if mp3.c changes drastically soon.
>>>
>>> Jim gave some ideas on what to do once support is removed... Hopefully,
>>> we will remember to look at mailing list archive when its time to remove and
>>> take the comments into account.
>>>
>>> Chris
>>>
>>>   If this goes in immediately, we should probably not call this a dot
>>>> release any more though to better reflect minimum versions changing.
>>>>
>>>> Chris
>>>>
>>>
>> Tracker patch *2870196*  includes changes that improve support for LAME
>> 3.97. Main point is to enable most of the ID3 tags to be set via the ID3
>> APIs supplied by 3.97. The current CVS code relies entirely on a function
>> that was added in 3.98.1, but 90% of the ID3 support can be implemented
>> using functions available in 3.97. Other change is to provide better
>> fallback logic and better informational messages in the case where the
>> current version of LAME doesn't support something (esp. important if using
>> DL_OPEN since we don't know what version we're going to get in that case).
>> Final change (for the non-dlopen case only) is to more accurately specify
>> the autoconf variables needed for each situation. This calls for 3 autoconf
>> variables to be used:
>>
>>
>
> Thanks, Doug.  Today I've been trying to figure out current state of
> patches in tracker and at least close out the ones that seem to have already
> been merged into CVS.  I'll look into this specific patch and see if I can't
> incorporate it for 14.3.1 release.
>
> I'm also trying to get DL_SNDFILE working with autoconf... Its got a couple
> of issues still in my initial attempt but they should be easy to fix.
>
> Chris
>

OK, I just committed that patch to CVS.  Makes code read much nicer having
HAVE_LAME_398 type #defines.

I have only one minor concern.  For mad precision, we may want to keep it at
16-bits even though we read in higher.  The reason is pretty simple.  Users
expect the following command line:

sox infile.mp3 outfile.wav

to create a 16-bit WAV file instead of a 24-bit... Similar to how:

madplay -o outfile.wav infile.mp3

would create a 16-bit file.  If they want increased percision then they
could do:

sox infile.mp3 -3 outfile.wav

Opinions?

Chris

>
> OK, I just committed that patch to CVS.  Makes code read much nicer having
> HAVE_LAME_398 type #defines.
>
> I have only one minor concern.  For mad precision, we may want to keep it
> at 16-bits even though we read in higher.  The reason is pretty simple.
> Users expect the following command line:
>
> sox infile.mp3 outfile.wav
>
> to create a 16-bit WAV file instead of a 24-bit... Similar to how:
>
> madplay -o outfile.wav infile.mp3
>
That sounds reasonable to me. I'm convinced.