diff --git a/taglib/flac/flacfile.cpp b/taglib/flac/flacfile.cpp index 252907d0..90df397a 100644 --- 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(MetadataBlock::Padding | LastBlockFlag); + data.append(paddingHeader); + data.resize(static_cast(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 6cbcb5a0..cf8c15c0 100644 --- 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 9fdd51e1..8b2e1f5b 100644 --- a/tests/test_flac.cpp +++ b/tests/test_flac.cpp @@ -7,6 +7,7 @@ #include #include #include +#include #include #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);