From 083eadec609da79de3bcba6d18e3e479a1fa3025 Mon Sep 17 00:00:00 2001 From: Tsuda Kageyu Date: Fri, 27 Nov 2015 00:52:44 +0900 Subject: [PATCH] Fix saving MPC files. This fixes the issue reported at #624. --- taglib/mpc/mpcfile.cpp | 181 ++++++++++++++++++++--------------------- taglib/mpc/mpcfile.h | 3 - tests/test_mpc.cpp | 27 ++++++ 3 files changed, 114 insertions(+), 97 deletions(-) diff --git a/taglib/mpc/mpcfile.cpp b/taglib/mpc/mpcfile.cpp index 015bc0e0..83a9be82 100644 --- a/taglib/mpc/mpcfile.cpp +++ b/taglib/mpc/mpcfile.cpp @@ -53,10 +53,7 @@ public: ID3v2Header(0), ID3v2Location(-1), ID3v2Size(0), - properties(0), - hasAPE(false), - hasID3v1(false), - hasID3v2(false) {} + properties(0) {} ~FilePrivate() { @@ -76,13 +73,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; }; //////////////////////////////////////////////////////////////////////////////// @@ -147,69 +137,80 @@ bool MPC::File::save() // Possibly strip ID3v2 tag - if(d->hasID3v2 && !d->ID3v2Header) { + if(!d->ID3v2Header && d->ID3v2Location >= 0) { removeBlock(d->ID3v2Location, d->ID3v2Size); - d->hasID3v2 = false; - if(d->hasID3v1) - d->ID3v1Location -= d->ID3v2Size; - if(d->hasAPE) + + if(d->APELocation >= 0) d->APELocation -= d->ID3v2Size; + + if(d->ID3v1Location >= 0) + d->ID3v1Location -= d->ID3v2Size; + + d->ID3v2Location = -1; + d->ID3v2Size = 0; } // 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; } - } else - if(d->hasID3v1) { - removeBlock(d->ID3v1Location, 128); - d->hasID3v1 = false; - if(d->hasAPE) { - if(d->APELocation > d->ID3v1Location) - d->APELocation -= 128; - } + + writeBlock(ID3v1Tag()->render()); + } + else { + + // 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 { + + // APE tag is empty. Remove the old one. + + if(d->APELocation >= 0) { + removeBlock(d->APELocation, d->APESize); + + if(d->ID3v1Location >= 0) + d->ID3v1Location -= d->APESize; + + d->APELocation = -1; + d->APESize = 0; } } - else - if(d->hasAPE) { - removeBlock(d->APELocation, d->APESize); - d->hasAPE = false; - if(d->hasID3v1) { - if(d->ID3v1Location > d->APELocation) - d->ID3v1Location -= d->APESize; - } - } return true; } @@ -226,22 +227,19 @@ APE::Tag *MPC::File::APETag(bool create) void MPC::File::strip(int tags) { - if(tags & ID3v1) { + if(tags & ID3v1) d->tag.set(MPCID3v1Index, 0); + + if(tags & APE) + d->tag.set(MPCAPEIndex, 0); + + if(!ID3v1Tag()) APETag(true); - } if(tags & ID3v2) { delete d->ID3v2Header; d->ID3v2Header = 0; } - - if(tags & APE) { - d->tag.set(MPCAPEIndex, 0); - - if(!ID3v1Tag()) - APETag(true); - } } void MPC::File::remove(int tags) @@ -251,12 +249,12 @@ void MPC::File::remove(int tags) bool MPC::File::hasID3v1Tag() const { - return d->hasID3v1; + return (d->ID3v1Location >= 0); } bool MPC::File::hasAPETag() const { - return d->hasAPE; + return (d->APELocation >= 0); } //////////////////////////////////////////////////////////////////////////////// @@ -265,30 +263,6 @@ bool MPC::File::hasAPETag() const void MPC::File::read(bool readProperties) { - // Look for an ID3v1 tag - - d->ID3v1Location = Utils::findID3v1(this); - - if(d->ID3v1Location >= 0) { - d->tag.set(MPCID3v1Index, new ID3v1::Tag(this, d->ID3v1Location)); - d->hasID3v1 = true; - } - - // Look for an APE tag - - d->APELocation = Utils::findAPE(this, d->ID3v1Location); - - if(d->APELocation >= 0) { - d->tag.set(MPCAPEIndex, 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) - APETag(true); - // Look for an ID3v2 tag d->ID3v2Location = Utils::findID3v2(this); @@ -297,23 +271,42 @@ void MPC::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) + d->tag.set(MPCID3v1Index, new ID3v1::Tag(this, d->ID3v1Location)); + + // Look for an APE tag + + d->APELocation = Utils::findAPE(this, d->ID3v1Location); + + if(d->APELocation >= 0) { + d->tag.set(MPCAPEIndex, new APE::Tag(this, d->APELocation)); + d->APESize = APETag()->footer()->completeTagSize(); + d->APELocation = d->APELocation + APE::Footer::size() - d->APESize; + } + + if(d->ID3v1Location < 0) + APETag(true); + // Look for MPC metadata if(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/mpc/mpcfile.h b/taglib/mpc/mpcfile.h index 98e7480f..541724dc 100644 --- a/taglib/mpc/mpcfile.h +++ b/taglib/mpc/mpcfile.h @@ -141,9 +141,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_mpc.cpp b/tests/test_mpc.cpp index ec1e9f1c..94613c1e 100644 --- a/tests/test_mpc.cpp +++ b/tests/test_mpc.cpp @@ -24,6 +24,7 @@ class TestMPC : public CppUnit::TestFixture CPPUNIT_TEST(testFuzzedFile3); CPPUNIT_TEST(testFuzzedFile4); CPPUNIT_TEST(testStripAndProperties); + CPPUNIT_TEST(testRepeatedSave); CPPUNIT_TEST_SUITE_END(); public: @@ -132,6 +133,32 @@ public: } } + void testRepeatedSave() + { + ScopedFileCopy copy("click", ".mpc"); + + { + MPC::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(); + } + { + MPC::File f(copy.fileName().c_str()); + CPPUNIT_ASSERT(f.hasAPETag()); + CPPUNIT_ASSERT(f.hasID3v1Tag()); + } + } + }; CPPUNIT_TEST_SUITE_REGISTRATION(TestMPC);