Bug 122698

Summary:	Saving APE tags in mp3 file adds unwanted padding
Product:	[Unmaintained] taglib	Reporter:	Matej Vadnjal <matej>
Component:	general	Assignee:	Scott Wheeler <wheeler>
Status:	RESOLVED FIXED
Severity:	normal	CC:	kde
Priority:	NOR
Version First Reported In:	unspecified
Target Milestone:	---
Platform:	Gentoo Packages
OS:	Linux
Latest Commit:		Version Fixed/Implemented In:
Sentry Crash Report:
Attachments:	Proposed patch

Description Matej Vadnjal 2006-02-25 15:27:56 UTC

Version:           1.4 (using KDE KDE 3.5.1)
Installed from:    Gentoo Packages
OS:                Linux

When saving an APE tag to an mp3 file, which allready has an ID3v1 tag, taglib adds 896 empty bytes to the end of the file. However the APE tag is correctly inserted right in front if the ID3v1 tag. 

It looks like this is a problem in the insert() function which uses bufferSize() to insert 1024 bytes of padding after the APE tag. This padding should not be necessary for APE tags, because they are stored near the end of the file.

Comment 1 Matej Vadnjal 2006-02-26 13:48:29 UTC

Created attachment 14886 [details]
Proposed patch

Ok, I took a look at the insert() function in file toolkit/tfile.cpp. Turns
out, that ti doesn't actually check if the end of file was reached on first
read (before the "while(bytesRead != 0)" loop, but just assumes that bytesRead
equals bufferLength.

This patch eliminates this bug. And hopefully doesn't introduce new ones.

Comment 2 Lukáš Lalinský 2006-04-20 01:20:58 UTC

Same problem with MPC files (http://bugs.musicbrainz.org/ticket/1304). The patch from #1 fixes it.

Any change this will get into svn soon?

Comment 3 Scott Wheeler 2006-04-20 01:26:53 UTC

Well, for some definition of "soon".  I usually work on projects in batch, especially TagLib.  Usually when there are certain level of bugs I go through and fix almost all of them and before a release I check in fixes for almost all of them.  So, a more accurate answer is that they'll definitely be looked at before the next release.

Comment 4 Scott Wheeler 2007-07-18 18:41:14 UTC

SVN commit 689596 by wheeler:

Make sure that we only write the number of bytes that we read.

BUG:122698


 M  +8 -3      tfile.cpp  


--- trunk/kdesupport/taglib/taglib/toolkit/tfile.cpp #689595:689596
@@ -325,6 +325,7 @@
   // that aren't yet in memory, so this is necessary.
 
   ulong bufferLength = bufferSize();
+
   while(data.size() - replace > bufferLength)
     bufferLength += bufferSize();
 
@@ -352,10 +353,14 @@
 
   buffer = aboutToOverwrite;
 
+  // In case we've already reached the end of file...
+
+  buffer.resize(bytesRead);
+
   // Ok, here's the main loop.  We want to loop until the read fails, which
   // means that we hit the end of the file.
 
-  while(bytesRead != 0) {
+  while(!buffer.isEmpty()) {
 
     // Seek to the current read position and read the data that we're about
     // to overwrite.  Appropriately increment the readPosition.
@@ -375,8 +380,8 @@
     // writePosition.
 
     seek(writePosition);
-    fwrite(buffer.data(), sizeof(char), bufferLength, d->file);
-    writePosition += bufferLength;
+    fwrite(buffer.data(), sizeof(char), buffer.size(), d->file);
+    writePosition += buffer.size();
 
     // Make the current buffer the data that we read in the beginning.