From f180445be510e95f22067dd5cd2411508d3658b1 Mon Sep 17 00:00:00 2001 From: Tsuda Kageyu Date: Fri, 27 Nov 2015 00:12:24 +0900 Subject: [PATCH] Fix saving APE files. This fixes the issue reported at #624. --- taglib/ape/apefile.cpp | 118 ++++++++++++++++++----------------------- taglib/ape/apefile.h | 3 -- tests/test_ape.cpp | 27 ++++++++++ 3 files changed, 79 insertions(+), 69 deletions(-) diff --git a/taglib/ape/apefile.cpp b/taglib/ape/apefile.cpp index 24171145..17d97668 100644 --- a/taglib/ape/apefile.cpp +++ b/taglib/ape/apefile.cpp @@ -61,10 +61,7 @@ public: ID3v2Header(0), ID3v2Location(-1), ID3v2Size(0), - properties(0), - hasAPE(false), - hasID3v1(false), - hasID3v2(false) {} + properties(0) {} ~FilePrivate() { @@ -84,13 +81,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; - bool hasID3v2; }; //////////////////////////////////////////////////////////////////////////////// @@ -155,64 +145,67 @@ bool APE::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 *APE::File::ID3v1Tag(bool create) @@ -227,27 +220,24 @@ APE::Tag *APE::File::APETag(bool create) void APE::File::strip(int tags) { - if(tags & ID3v1) { + if(tags & ID3v1) d->tag.set(ApeID3v1Index, 0); - APETag(true); - } - if(tags & APE) { + if(tags & APE) d->tag.set(ApeAPEIndex, 0); - if(!ID3v1Tag()) - APETag(true); - } + if(!ID3v1Tag()) + APETag(true); } bool APE::File::hasAPETag() const { - return d->hasAPE; + return (d->APELocation >= 0); } bool APE::File::hasID3v1Tag() const { - return d->hasID3v1; + return (d->ID3v1Location >= 0); } //////////////////////////////////////////////////////////////////////////////// @@ -264,17 +254,14 @@ void APE::File::read(bool readProperties) seek(d->ID3v2Location); d->ID3v2Header = new ID3v2::Header(readBlock(ID3v2::Header::size())); d->ID3v2Size = d->ID3v2Header->completeTagSize(); - d->hasID3v2 = true; } // Look for an ID3v1 tag d->ID3v1Location = Utils::findID3v1(this); - if(d->ID3v1Location >= 0) { + if(d->ID3v1Location >= 0) d->tag.set(ApeID3v1Index, new ID3v1::Tag(this, d->ID3v1Location)); - d->hasID3v1 = true; - } // Look for an APE tag @@ -284,10 +271,9 @@ void APE::File::read(bool readProperties) d->tag.set(ApeAPEIndex, 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 APE audio properties @@ -296,14 +282,14 @@ void APE::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(); - if(d->hasID3v2) { + if(d->ID3v2Location >= 0) { seek(d->ID3v2Location + d->ID3v2Size); streamLength -= (d->ID3v2Location + d->ID3v2Size); } diff --git a/taglib/ape/apefile.h b/taglib/ape/apefile.h index e638d865..cfb19ff7 100644 --- a/taglib/ape/apefile.h +++ b/taglib/ape/apefile.h @@ -146,9 +146,6 @@ namespace TagLib { * * \note According to the official Monkey's Audio SDK, an APE file * can only have either ID3V1 or APE tags, so a parameter is used here. - * - * \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_ape.cpp b/tests/test_ape.cpp index 64869c4c..12ca2db8 100644 --- a/tests/test_ape.cpp +++ b/tests/test_ape.cpp @@ -23,6 +23,7 @@ class TestAPE : public CppUnit::TestFixture CPPUNIT_TEST(testFuzzedFile1); CPPUNIT_TEST(testFuzzedFile2); CPPUNIT_TEST(testStripAndProperties); + CPPUNIT_TEST(testRepeatedSave); CPPUNIT_TEST_SUITE_END(); public: @@ -134,6 +135,32 @@ public: } } + void testRepeatedSave() + { + ScopedFileCopy copy("mac-399", ".ape"); + + { + APE::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(); + } + { + APE::File f(copy.fileName().c_str()); + CPPUNIT_ASSERT(f.hasAPETag()); + CPPUNIT_ASSERT(f.hasID3v1Tag()); + } + } + }; CPPUNIT_TEST_SUITE_REGISTRATION(TestAPE);