#69 mp3 tag problem fix
My previous VBR patch used LAME's lame_mp3_tags_fid function which is bad when dynamically linked. I should have known better, but I hadn't planned on writing any tag support. It was required to get VBR files to be reasonable functional in a player (correct duration, working slider, etc.)
This patch started out as a rewrite using LAME's lame_get_lametag_frame function which is reliable even with dynamic linking. Looking at a few functions in the LAME sources showed what was required. Specifically, I referred to lame_mp3_tags_fid, PutVbrTag, skipId3v2. As usual what I thought I needed didn't take that long, but the unexpected did. Testing showed idigdoug's cool patch, "Write ID3 tags to newly-created MP3 files", was causing files to show the wrong duration. Apparently when using the tempo effect (or any effect that changes time) the wrong value (0xFFFFFFFF) is used to calculate the TLEN tag and in Windows Media Player this takes precedence over a correct VBR header.
Now with this patch, if possible, when stopwrite is called both the ID3 and VBR headers are rewritten with correct durations for mp3 file output.
Over on sox-devel Douglas Cook commented:
>>>
The TLEN issue should be fixed in a later patch, but it hasn't been
committed yet.
The main reason why the later patch doesn't use lame_get_lametag is
that I'm not sure that function is consistently available on common
builds of lame. It might need to be detected at build time.
<<<
sox-devel is readonly so I'm responding here at the patch tracker.
I understand you are concerned about lame_get_lametag. I'm not sure how broadly you target sox, but I've researched when these newer calls showed up in LAME. Lame's ChangeLog:
http://lame.cvs.sourceforge.net/viewvc/lame/lame/ChangeLog?view=markup
Shows that lame_get_lametag and related functions were added 2008-02-10. LAME's History file:
http://lame.cvs.sourceforge.net/viewvc/\*checkout*/lame/lame/doc/html/history.html
shows the first release to include support was LAME 3.98 beta 7 in April 6 2008 which includes this item:
libmp3lame API: allow frontends to separately retrieve LAME/Xing and ID3 data, because the old library automatism makes it impossible to make fully buffered encodes.
The first (non-Beta) release with the new functions was LAME 3.98 on July 4 2008.
So the patch requires LAME 3.98.
Maybe users who are need to run older versions of LAME could also use older versions of SoX.
I'm not as familiar with the Linux side, but for Windows the idea that users could download binaries for the current version of SoX and the current version of LAME and it would just work is compelling. Last year I'd say it was impossible for a Windows user to get SoX to process MP3 files (you had to build SoX). Now that SoX supports dynamic linking to an MP3 encoder it should be easy for a user to get it working.
I see the trade-off between supporting the old LAME API for widest possible compatibility, or using the new API to better take advantage of available functionality and future updates to LAME and to avoid adding redundant functionality to SoX. For me the new API is the right choice, but don't pretend to know what's best for the SoX user base as a whole.
> It might need to be detected at build time.
For statically linked LAME support that's right. But if you're dynamically linking to LAME (specifically using what Windows would call explicit dynamic linking, with calls to LoadLibrary and GetProcAddress), you could detect specific API availability at run-time and only work around the missing API's when needed.
I think a version of this patch is currently in CVS and so closing patch tracker. Pleaes re-open or create new patch if there are outstanding code you have. Thanks.