Fix saving FLAC files.

This fixes all the issues reported at #622.

diff --git a/taglib/flac/flacfile.cpp b/taglib/flac/flacfile.cpp
index 252907d062f3c38656ab171d9e1bfdd6946d32a5..90df397aa2bafc6ae06ecfb5fa7404fafe3749d1 100644 (file)
--- a/taglib/flac/flacfile.cpp
+++ b/taglib/flac/flacfile.cpp
@@ -46,27 +46,25 @@ using namespace TagLib;
 namespace
 {
   enum { FlacXiphIndex = 0, FlacID3v2Index = 1, FlacID3v1Index = 2 };
-  enum { MinPaddingLength = 4096 };
-  enum { LastBlockFlag = 0x80 };
+
+  const long MinPaddingLength = 4096;
+  const long MaxPaddingLegnth = 1024 * 1024;
+
+  const char LastBlockFlag = '\x80';
 }
 
 class FLAC::File::FilePrivate
 {
 public:
-  FilePrivate() :
-    ID3v2FrameFactory(ID3v2::FrameFactory::instance()),
+  FilePrivate(const ID3v2::FrameFactory *frameFactory = ID3v2::FrameFactory::instance()) :
+    ID3v2FrameFactory(frameFactory),
     ID3v2Location(-1),
     ID3v2OriginalSize(0),
     ID3v1Location(-1),
     properties(0),
     flacStart(0),
     streamStart(0),
-    scanned(false),
-    hasXiphComment(false),
-    hasID3v2(false),
-    hasID3v1(false)
-  {
-  }
+    scanned(false) {}
 
   ~FilePrivate()
   {
@@ -92,10 +90,6 @@ public:
   long flacStart;
   long streamStart;
   bool scanned;
-
-  bool hasXiphComment;
-  bool hasID3v2;
-  bool hasID3v1;
 };
 
 ////////////////////////////////////////////////////////////////////////////////
@@ -113,9 +107,8 @@ FLAC::File::File(FileName file, bool readProperties, Properties::ReadStyle) :
 FLAC::File::File(FileName file, ID3v2::FrameFactory *frameFactory,
                  bool readProperties, Properties::ReadStyle) :
   TagLib::File(file),
-  d(new FilePrivate())
+  d(new FilePrivate(frameFactory))
 {
-  d->ID3v2FrameFactory = frameFactory;
   if(isOpen())
     read(readProperties);
 }
@@ -123,9 +116,8 @@ FLAC::File::File(FileName file, ID3v2::FrameFactory *frameFactory,
 FLAC::File::File(IOStream *stream, ID3v2::FrameFactory *frameFactory,
                  bool readProperties, Properties::ReadStyle) :
   TagLib::File(stream),
-  d(new FilePrivate())
+  d(new FilePrivate(frameFactory))
 {
-  d->ID3v2FrameFactory = frameFactory;
   if(isOpen())
     read(readProperties);
 }
@@ -214,39 +206,83 @@ bool FLAC::File::save()
     data.append(blockData);
   }
 
-  // Adjust the padding block(s)
+  // Compute the amount of padding, and append that to data.
+  // TODO: Should be calculated in offset_t in taglib2.
 
   long originalLength = d->streamStart - d->flacStart;
-  int paddingLength = originalLength - data.size() - 4;
+  long paddingLength = originalLength - data.size() - 4;
+
   if(paddingLength <= 0) {
     paddingLength = MinPaddingLength;
   }
-  ByteVector padding = ByteVector::fromUInt(paddingLength);
-  padding.resize(paddingLength + 4);
-  padding[0] = (char)(FLAC::MetadataBlock::Padding | LastBlockFlag);
-  data.append(padding);
+  else {
+    // Padding won't increase beyond 1% of the file size or 1MB.
+
+    long threshold = length() / 100;
+    threshold = std::max(threshold, MinPaddingLength);
+    threshold = std::min(threshold, MaxPaddingLegnth);
+
+    if(paddingLength > threshold)
+      paddingLength = MinPaddingLength;
+  }
+
+  ByteVector paddingHeader = ByteVector::fromUInt(paddingLength);
+  paddingHeader[0] = static_cast<char>(MetadataBlock::Padding | LastBlockFlag);
+  data.append(paddingHeader);
+  data.resize(static_cast<TagLib::uint>(data.size() + paddingLength));
 
   // Write the data to the file
 
   insert(data, d->flacStart, originalLength);
-  d->hasXiphComment = true;
+
+  d->streamStart += (data.size() - originalLength);
+
+  if(d->ID3v1Location >= 0)
+    d->ID3v1Location += (data.size() - originalLength);
 
   // Update ID3 tags
 
-  if(ID3v2Tag()) {
-    if(d->hasID3v2) {
-      if(d->ID3v2Location < d->flacStart)
-        debug("FLAC::File::save() -- This can't be right -- an ID3v2 tag after the "
-              "start of the FLAC bytestream?  Not writing the ID3v2 tag.");
-      else
-        insert(ID3v2Tag()->render(), d->ID3v2Location, d->ID3v2OriginalSize);
+  if(ID3v2Tag() && !ID3v2Tag()->isEmpty()) {
+
+    // ID3v2 tag is not empty. Update the old one or create a new one.
+
+    if(d->ID3v2Location < 0)
+      d->ID3v2Location = 0;
+
+    data = ID3v2Tag()->render();
+    insert(data, d->ID3v2Location, d->ID3v2OriginalSize);
+
+    d->flacStart   += (data.size() - d->ID3v2OriginalSize);
+    d->streamStart += (data.size() - d->ID3v2OriginalSize);
+
+    if(d->ID3v1Location >= 0)
+      d->ID3v1Location += (data.size() - d->ID3v2OriginalSize);
+
+    d->ID3v2OriginalSize = data.size();
+  }
+  else {
+
+    // ID3v2 tag is empty. Remove the old one.
+
+    if(d->ID3v2Location >= 0) {
+      removeBlock(d->ID3v2Location, d->ID3v2OriginalSize);
+
+      d->flacStart   -= d->ID3v2OriginalSize;
+      d->streamStart -= d->ID3v2OriginalSize;
+
+      if(d->ID3v1Location >= 0)
+        d->ID3v1Location -= d->ID3v2OriginalSize;
+
+      d->ID3v2Location = -1;
+      d->ID3v2OriginalSize = 0;
     }
-    else
-      insert(ID3v2Tag()->render(), 0, 0);
   }
 
-  if(ID3v1Tag()) {
-    if(d->hasID3v1) {
+  if(ID3v1Tag() && !ID3v1Tag()->isEmpty()) {
+
+    // ID3v1 tag is not empty. Update the old one or create a new one.
+
+    if(d->ID3v1Location >= 0) {
       seek(d->ID3v1Location);
     }
     else {
@@ -255,7 +291,15 @@ bool FLAC::File::save()
     }
 
     writeBlock(ID3v1Tag()->render());
-    d->hasID3v1 = true;
+  }
+  else {
+
+    // ID3v1 tag is empty. Remove the old one.
+
+    if(d->ID3v1Location >= 0) {
+      truncate(d->ID3v1Location);
+      d->ID3v1Location = -1;
+    }
   }
 
   return true;
@@ -342,17 +386,17 @@ void FLAC::File::removePictures()
 
 bool FLAC::File::hasXiphComment() const
 {
-  return d->hasXiphComment;
+  return !d->xiphCommentData.isEmpty();
 }
 
 bool FLAC::File::hasID3v1Tag() const
 {
-  return d->hasID3v1;
+  return (d->ID3v1Location >= 0);
 }
 
 bool FLAC::File::hasID3v2Tag() const
 {
-  return d->hasID3v2;
+  return (d->ID3v2Location >= 0);
 }
 
 ////////////////////////////////////////////////////////////////////////////////
@@ -366,25 +410,16 @@ void FLAC::File::read(bool readProperties)
   d->ID3v2Location = Utils::findID3v2(this);
 
   if(d->ID3v2Location >= 0) {
-
     d->tag.set(FlacID3v2Index, new ID3v2::Tag(this, d->ID3v2Location, d->ID3v2FrameFactory));
-
     d->ID3v2OriginalSize = ID3v2Tag()->header()->completeTagSize();
-
-    if(ID3v2Tag()->header()->tagSize() <= 0)
-      d->tag.set(FlacID3v2Index, 0);
-    else
-      d->hasID3v2 = true;
   }
 
   // Look for an ID3v1 tag
 
   d->ID3v1Location = Utils::findID3v1(this);
 
-  if(d->ID3v1Location >= 0) {
+  if(d->ID3v1Location >= 0)
     d->tag.set(FlacID3v1Index, new ID3v1::Tag(this, d->ID3v1Location));
-    d->hasID3v1 = true;
-  }
 
   // Look for FLAC metadata, including vorbis comments
 
@@ -393,10 +428,10 @@ void FLAC::File::read(bool readProperties)
   if(!isValid())
     return;
 
-  if(d->hasXiphComment)
+  if(!d->xiphCommentData.isEmpty())
     d->tag.set(FlacXiphIndex, new Ogg::XiphComment(d->xiphCommentData));
   else
-    d->tag.set(FlacXiphIndex, new Ogg::XiphComment);
+    d->tag.set(FlacXiphIndex, new Ogg::XiphComment());
 
   if(readProperties) {
 
@@ -406,10 +441,10 @@ void FLAC::File::read(bool readProperties)
 
     long streamLength;
 
-    if(d->hasID3v1)
+    if(d->ID3v1Location >= 0)
       streamLength = d->ID3v1Location - d->streamStart;
     else
-      streamLength = File::length() - d->streamStart;
+      streamLength = length() - d->streamStart;
 
     d->properties = new Properties(infoData, streamLength);
   }
@@ -427,7 +462,7 @@ void FLAC::File::scan()
 
   long nextBlockOffset;
 
-  if(d->hasID3v2)
+  if(d->ID3v2Location >= 0)
     nextBlockOffset = find("fLaC", d->ID3v2Location + d->ID3v2OriginalSize);
   else
     nextBlockOffset = find("fLaC");
@@ -495,9 +530,8 @@ void FLAC::File::scan()
 
     // Found the vorbis-comment
     if(blockType == MetadataBlock::VorbisComment) {
-      if(!d->hasXiphComment) {
+      if(d->xiphCommentData.isEmpty()) {
         d->xiphCommentData = data;
-        d->hasXiphComment = true;
       }
       else {
         debug("FLAC::File::scan() -- multiple Vorbis Comment blocks found, using the first one");

diff --git a/taglib/flac/flacfile.h b/taglib/flac/flacfile.h
index 6cbcb5a0aecb8ee461b2dd929f1aaac1b729d367..cf8c15c0f09220cb9d9570efad44eba68daba104 100644 (file)
--- a/taglib/flac/flacfile.h
+++ b/taglib/flac/flacfile.h
@@ -155,9 +155,6 @@ namespace TagLib {
        * has no XiphComment, one will be constructed from the ID3-tags.
        *
        * This returns true if the save was successful.
-       *
-       * \warning In the current implementation, it's dangerous to call save()
-       * repeatedly.  At worst it will corrupt the file.
        */
       virtual bool save();
 

diff --git a/tests/test_flac.cpp b/tests/test_flac.cpp
index 9fdd51e159e84bc4b2c56ce19c1c9b92dc5ff363..8b2e1f5b0833331d9c9de9469b0b9c3951958a68 100644 (file)
--- a/tests/test_flac.cpp
+++ b/tests/test_flac.cpp
@@ -7,6 +7,7 @@
 #include <flacfile.h>
 #include <xiphcomment.h>
 #include <id3v1tag.h>
+#include <id3v2tag.h>
 #include <cppunit/extensions/HelperMacros.h>
 #include "utils.h"
 
@@ -22,13 +23,19 @@ class TestFLAC : public CppUnit::TestFixture
   CPPUNIT_TEST(testAddPicture);
   CPPUNIT_TEST(testReplacePicture);
   CPPUNIT_TEST(testRemoveAllPictures);
-  CPPUNIT_TEST(testRepeatedSave);
+  CPPUNIT_TEST(testRepeatedSave1);
+  CPPUNIT_TEST(testRepeatedSave2);
+  CPPUNIT_TEST(testRepeatedSave3);
   CPPUNIT_TEST(testSaveMultipleValues);
   CPPUNIT_TEST(testDict);
   CPPUNIT_TEST(testInvalid);
   CPPUNIT_TEST(testAudioProperties);
-  CPPUNIT_TEST(testZeroSizedPadding);
+  CPPUNIT_TEST(testZeroSizedPadding1);
+  CPPUNIT_TEST(testZeroSizedPadding2);
+  CPPUNIT_TEST(testShrinkPadding);
   CPPUNIT_TEST(testSaveID3v1);
+  CPPUNIT_TEST(testUpdateID3v2);
+  CPPUNIT_TEST(testEmptyID3v2);
   CPPUNIT_TEST_SUITE_END();
 
 public:
@@ -185,7 +192,7 @@ public:
     }
   }
 
-  void testRepeatedSave()
+  void testRepeatedSave1()
   {
     ScopedFileCopy copy("silence-44-s", ".flac");
     string newname = copy.fileName();
@@ -206,6 +213,31 @@ public:
     }
   }
 
+  void testRepeatedSave2()
+  {
+    ScopedFileCopy copy("no-tags", ".flac");
+
+    FLAC::File f(copy.fileName().c_str());
+    f.ID3v2Tag(true)->setTitle("0123456789");
+    f.save();
+    CPPUNIT_ASSERT_EQUAL(5735L, f.length());
+    f.save();
+    CPPUNIT_ASSERT_EQUAL(5735L, f.length());
+    CPPUNIT_ASSERT(f.find("fLaC") >= 0);
+  }
+
+  void testRepeatedSave3()
+  {
+    ScopedFileCopy copy("no-tags", ".flac");
+
+    FLAC::File f(copy.fileName().c_str());
+    f.xiphComment()->setTitle(std::string(8 * 1024, 'X').c_str());
+    f.save();
+    CPPUNIT_ASSERT_EQUAL(12862L, f.length());
+    f.save();
+    CPPUNIT_ASSERT_EQUAL(12862L, f.length());
+  }
+
   void testSaveMultipleValues()
   {
     ScopedFileCopy copy("silence-44-s", ".flac");
@@ -278,7 +310,7 @@ public:
       f.audioProperties()->signature());
   }
 
-  void testZeroSizedPadding()
+  void testZeroSizedPadding1()
   {
     ScopedFileCopy copy("zero-sized-padding", ".flac");
 
@@ -286,6 +318,44 @@ public:
     CPPUNIT_ASSERT(f.isValid());
   }
 
+  void testZeroSizedPadding2()
+  {
+    ScopedFileCopy copy("silence-44-s", ".flac");
+
+    {
+      FLAC::File f(copy.fileName().c_str());
+      f.xiphComment()->setTitle("ABC");
+      f.save();
+    }
+    {
+      FLAC::File f(copy.fileName().c_str());
+      f.xiphComment()->setTitle(std::string(3067, 'X').c_str());
+      f.save();
+    }
+    {
+      FLAC::File f(copy.fileName().c_str());
+      CPPUNIT_ASSERT(f.isValid());
+    }
+  }
+
+  void testShrinkPadding()
+  {
+    ScopedFileCopy copy("no-tags", ".flac");
+
+    {
+      FLAC::File f(copy.fileName().c_str());
+      f.xiphComment()->setTitle(std::wstring(128 * 1024, L'X').c_str());
+      f.save();
+      CPPUNIT_ASSERT(f.length() > 128 * 1024);
+    }
+    {
+      FLAC::File f(copy.fileName().c_str());
+      f.xiphComment()->setTitle("0123456789");
+      f.save();
+      CPPUNIT_ASSERT(f.length() < 8 * 1024);
+    }
+  }
+
   void testSaveID3v1()
   {
     ScopedFileCopy copy("no-tags", ".flac");
@@ -309,6 +379,41 @@ public:
     }
   }
 
+  void testUpdateID3v2()
+  {
+    ScopedFileCopy copy("no-tags", ".flac");
+
+    {
+      FLAC::File f(copy.fileName().c_str());
+      f.ID3v2Tag(true)->setTitle("0123456789");
+      f.save();
+    }
+    {
+      FLAC::File f(copy.fileName().c_str());
+      f.ID3v2Tag()->setTitle("ABCDEFGHIJ");
+      f.save();
+    }
+    {
+      FLAC::File f(copy.fileName().c_str());
+      CPPUNIT_ASSERT_EQUAL(String("ABCDEFGHIJ"), f.ID3v2Tag()->title());
+    }
+  }
+
+  void testEmptyID3v2()
+  {
+    ScopedFileCopy copy("no-tags", ".flac");
+
+    {
+      FLAC::File f(copy.fileName().c_str());
+      f.ID3v2Tag(true);
+      f.save();
+    }
+    {
+      FLAC::File f(copy.fileName().c_str());
+      CPPUNIT_ASSERT(!f.hasID3v2Tag());
+    }
+  }
+
 };
 
 CPPUNIT_TEST_SUITE_REGISTRATION(TestFLAC);