diff --git a/taglib/wavpack/wavpackfile.cpp b/taglib/wavpack/wavpackfile.cpp index f7c97f7e..2ec55716 100644 --- a/taglib/wavpack/wavpackfile.cpp +++ b/taglib/wavpack/wavpackfile.cpp @@ -54,9 +54,7 @@ public: APELocation(-1), APESize(0), ID3v1Location(-1), - properties(0), - hasAPE(false), - hasID3v1(false) {} + properties(0) {} ~FilePrivate() { @@ -71,12 +69,6 @@ public: TagUnion tag; Properties *properties; - - // These indicate whether the file *on disk* has these tags, not if - // this data structure does. This is used in computing offsets. - - bool hasAPE; - bool hasID3v1; }; //////////////////////////////////////////////////////////////////////////////// @@ -141,64 +133,67 @@ bool WavPack::File::save() // Update ID3v1 tag - 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); - writeBlock(ID3v1Tag()->render()); } else { seek(0, End); d->ID3v1Location = tell(); - writeBlock(ID3v1Tag()->render()); - d->hasID3v1 = true; } + + writeBlock(ID3v1Tag()->render()); } else { - if(d->hasID3v1) { - removeBlock(d->ID3v1Location, 128); - d->hasID3v1 = false; - if(d->hasAPE) { - if(d->APELocation > d->ID3v1Location) - d->APELocation -= 128; - } + + // ID3v1 tag is empty. Remove the old one. + + if(d->ID3v1Location >= 0) { + truncate(d->ID3v1Location); + d->ID3v1Location = -1; } } // Update APE tag - if(APETag()) { - if(d->hasAPE) - insert(APETag()->render(), d->APELocation, d->APESize); - else { - if(d->hasID3v1) { - insert(APETag()->render(), d->ID3v1Location, 0); - d->APESize = APETag()->footer()->completeTagSize(); - d->hasAPE = true; + if(APETag() && !APETag()->isEmpty()) { + + // APE tag is not empty. Update the old one or create a new one. + + if(d->APELocation < 0) { + if(d->ID3v1Location >= 0) d->APELocation = d->ID3v1Location; - d->ID3v1Location += d->APESize; - } - else { - seek(0, End); - d->APELocation = tell(); - writeBlock(APETag()->render()); - d->APESize = APETag()->footer()->completeTagSize(); - d->hasAPE = true; - } + else + d->APELocation = length(); } + + const ByteVector data = APETag()->render(); + insert(data, d->APELocation, d->APESize); + + if(d->ID3v1Location >= 0) + d->ID3v1Location += (data.size() - d->APESize); + + d->APESize = data.size(); } else { - if(d->hasAPE) { + + // APE tag is empty. Remove the old one. + + if(d->APELocation >= 0) { removeBlock(d->APELocation, d->APESize); - d->hasAPE = false; - if(d->hasID3v1) { - if(d->ID3v1Location > d->APELocation) { - d->ID3v1Location -= d->APESize; - } - } + + if(d->ID3v1Location >= 0) + d->ID3v1Location -= d->APESize; + + d->APELocation = -1; + d->APESize = 0; } } - return true; + return true; } ID3v1::Tag *WavPack::File::ID3v1Tag(bool create) @@ -213,27 +208,24 @@ APE::Tag *WavPack::File::APETag(bool create) void WavPack::File::strip(int tags) { - if(tags & ID3v1) { + if(tags & ID3v1) d->tag.set(WavID3v1Index, 0); - APETag(true); - } - if(tags & APE) { + if(tags & APE) d->tag.set(WavAPEIndex, 0); - if(!ID3v1Tag()) - APETag(true); - } + if(!ID3v1Tag()) + APETag(true); } bool WavPack::File::hasID3v1Tag() const { - return d->hasID3v1; + return (d->ID3v1Location >= 0); } bool WavPack::File::hasAPETag() const { - return d->hasAPE; + return (d->APELocation >= 0); } //////////////////////////////////////////////////////////////////////////////// @@ -246,10 +238,8 @@ void WavPack::File::read(bool readProperties) d->ID3v1Location = Utils::findID3v1(this); - if(d->ID3v1Location >= 0) { + if(d->ID3v1Location >= 0) d->tag.set(WavID3v1Index, new ID3v1::Tag(this, d->ID3v1Location)); - d->hasID3v1 = true; - } // Look for an APE tag @@ -259,10 +249,9 @@ void WavPack::File::read(bool readProperties) d->tag.set(WavAPEIndex, new APE::Tag(this, d->APELocation)); d->APESize = APETag()->footer()->completeTagSize(); d->APELocation = d->APELocation + APE::Footer::size() - d->APESize; - d->hasAPE = true; } - if(!d->hasID3v1) + if(d->ID3v1Location >= 0) APETag(true); // Look for WavPack audio properties @@ -271,9 +260,9 @@ void WavPack::File::read(bool readProperties) long streamLength; - if(d->hasAPE) + if(d->APELocation >= 0) streamLength = d->APELocation; - else if(d->hasID3v1) + else if(d->ID3v1Location >= 0) streamLength = d->ID3v1Location; else streamLength = length(); diff --git a/taglib/wavpack/wavpackfile.h b/taglib/wavpack/wavpackfile.h index abcabd94..7e0bd27a 100644 --- a/taglib/wavpack/wavpackfile.h +++ b/taglib/wavpack/wavpackfile.h @@ -135,9 +135,6 @@ namespace TagLib { * Saves the file. * * 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_wavpack.cpp b/tests/test_wavpack.cpp index eddc601d..39bc1edd 100644 --- a/tests/test_wavpack.cpp +++ b/tests/test_wavpack.cpp @@ -19,6 +19,7 @@ class TestWavPack : public CppUnit::TestFixture CPPUNIT_TEST(testTaggedProperties); CPPUNIT_TEST(testFuzzedFile); CPPUNIT_TEST(testStripAndProperties); + CPPUNIT_TEST(testRepeatedSave); CPPUNIT_TEST_SUITE_END(); public: @@ -97,6 +98,32 @@ public: } } + void testRepeatedSave() + { + ScopedFileCopy copy("click", ".wv"); + + { + WavPack::File f(copy.fileName().c_str()); + CPPUNIT_ASSERT(!f.hasAPETag()); + CPPUNIT_ASSERT(!f.hasID3v1Tag()); + + f.APETag(true)->setTitle("01234 56789 ABCDE FGHIJ"); + f.save(); + + f.APETag()->setTitle("0"); + f.save(); + + f.ID3v1Tag(true)->setTitle("01234 56789 ABCDE FGHIJ"); + f.APETag()->setTitle("01234 56789 ABCDE FGHIJ 01234 56789 ABCDE FGHIJ 01234 56789"); + f.save(); + } + { + WavPack::File f(copy.fileName().c_str()); + CPPUNIT_ASSERT(f.hasAPETag()); + CPPUNIT_ASSERT(f.hasID3v1Tag()); + } + } + }; CPPUNIT_TEST_SUITE_REGISTRATION(TestWavPack);