[pkg-mad-maintainers] Patch to libid3tag0-dev for EasyH10

Wed Aug 24 01:32:04 UTC 2005

Hi all,

My patch used in EasyH10 has three enhancements:
1) id3_file_content_offset() function to tell the offset position where 
ID3v2 tag stored in an MP3 file ends
2) Adding an interface to obtain both ID3v1 and ID3v2 values separately
3) Modifying the behavior when an error occurred

EasyH10 needs to parse the first frame in an MP3 stream to obtain the 
stream information such as bitrate, sample rate, and duration. 
Enhancement 1) tells a hint where the frame syncronization should begin 
and accelerates the parsing a bit.

libid3tag returns a tag value either of ID3v1 or ID3v2. It might be 
enough to read tag information from either ID3v1 or ID3v2 tag. However, 
I got several reports pointing that EasyH10 could not read tag 
information from a few MP3 files. I looked into the files and found that 
they had both ID3v1 and ID3v2 tags, but the number of fields filled in 
ID3v2 was smaller than that of ID3v1. Take an example where a MP3 file 
contains an artist information in ID3v1 but not in ID3v2. Since 
libid3tag prioritizes ID3v2 over ID3v1, libid3tag finds the artist value 
to be empty even if it's described in ID3v1. I wanted to complain about 
the badly tagged files, but iriver plus surprisingly reads them. 
Enhancement 2) enable us to access ID3v1 value independently while an 
MP3 file has an ID3v2 tag.

The default behavior of libid3tag will fail if an error occurs during 
parsing ID3 tag. However, some MP3 files seem to have an improper tag 
generated by a buggy program. After getting several reports, I added 
enhancement 3) and changed the behavior to use the parsed result even if 
an error occurred.

I modified the source code to use the official version of libid3tag as 
well as the patched version. The configure script now detects whether 
libid3tag is patched version or not. The absence of enhancements 2) and 
3) may reduce the number of recognizable MP3 files for some users, but 
most users including me don't have such MP3 files. I will keep using my 
patch for EasyH10 Win32 versions, but it's also enough to use the 
non-patched libid3tag only if MP3 files were tagged properly.

Benjamin:
Could you evaluate this source code?
http://easyh10.sourceforge.net/dev/1.0b6/easyh10-1.0b6.tar.gz
This source code is not released yet but does not require the patched 
version of libid3tag any more. Just type:
$ ./configure
and the build script will detect libid3tag installed in an operating 
system. I think this version should sort out the packaging issue we have 
regarding libid3tag.

Then I will submit two patches for enhancements 2) and 3) to the upstream.

Best regards,
Nyaochi

Benjamin Seidenberg wrote:
> I have gotten in touch with the Easyh10 author (he was on vacation)
> and cc'd my reply (including his email) to this list. He had not yet
> approached  upstream, and will do so when he finishes his vacation.
> Nyaochi: Please keep pkg-mad-maintainers at lists.alioth.org in the CC of
> our messages so they can keep up to date.
> 
> Thanks!
> Benjamin
> 
> On 8/20/05, Sam Clegg <sam at superduper.net> wrote:
> 
>>Kurt Roeckx wrote:
>>
>>>On Sat, Aug 13, 2005 at 09:03:12PM +0200, Clément Stenac wrote:
>>>
>>>
>>>>Hello,
>>>>
>>>>This is of course always possible but requires to modify the SONAME of
>>>>the library, which will not match the upstream anymore. This is
>>>>acceptable, but not really recommended.
>>>
>>>
>>>Actually, just adding function doesn't require an soname change,
>>>this is a backward comptabible API/ABI change, and just requires
>>>us to bump the shlibs.
>>>
>>>But I agree that we should try to get it merged upstream.
>>>
>>>Also, it would be nice to know what the intended use of those 3
>>>added functions are, maybe we can come up with a better/more
>>>general solution to the problem, or some better way to do it.
>>>
>>>It contains this change:
>>>-  file->path    = path ? strdup(path) : 0;
>>>+  if (path) {
>>>+         file->path = (char *)malloc(strlen(path)+1);
>>>+         strcpy(file->path, path);
>>>+  } else {
>>>+         file->path = 0;
>>>+  }
>>>
>>>I don't see why this is changed, both really should do the same.
>>
>>
>>Aggreed.  This part look pointless.
>>
>>The following part means that if there is a problem parsing and Id3
>>frame the library will return you just the frames it could parse.  I'm
>>not sure if this is what we want.
>>
>>--- libid3tag-0.15.1b/tag.c>----2004-02-17 01:04:10.000000000 +0000
>>+++ libid3tag-0.15.1b_easyh10-0.10/tag.c>-------2005-06-05
>>03:33:58.000000000 +0100
>>@@ -605,8 +605,17 @@
>> >------break;  /* padding */
>>-
>>       frame = id3_frame_parse(&ptr, end - ptr, tag->version);
>>-      if (frame == 0 || id3_tag_attachframe(tag, frame) == -1)
>>->------goto fail;
>>+>------  /* Recover from error. */
>>+>------  if (frame) {
>>+>------>-------if (id3_tag_attachframe(tag, frame) == -1) {
>>+>------>-------  goto fail;
>>+>------>-------}
>>+>------  } else {
>>+>------>-------if (tag->nframes == 0) {
>>+>------>-------  goto fail;
>>+>------>-------}
>>+>------>-------break;
>>+>------  }
>>     }
>>
>>
>>The rest of the parch adds id3_file_content_offset, id3_file_gettag,
>>and id3_file_getnumtag.  I'm not sure about these either.  IIRC the
>>id3_file interface is designed to be a simple way to get a tag out of
>>a file.  If you want these extra functions you can use the id3_tag
>>interface which will tell you the size of any tags.
>>
>>Having said that I don't see a problem with adding these features.
>>Whats the debian policy on adding features?   Also, lets at least
>>wait to hear from upstream.
>>
>>
>>--
>>sam clegg
>>:: sam at superduper.net :: http://superduper.net/ :: PGP : D91EE369
>>$superduper: .signature,v 1.14 2005/01/19 13:21:14 sam Exp $
>>
>>
>>
>>
> 
> 