diff --git a/ConfigureChecks.cmake b/ConfigureChecks.cmake index 4f0e1cd6..93815cba 100644 --- a/ConfigureChecks.cmake +++ b/ConfigureChecks.cmake @@ -243,7 +243,7 @@ check_cxx_source_compiles(" int main() { _strdup(0); return 0; -} + } " HAVE_ISO_STRDUP) # Determine whether zlib is installed. diff --git a/NEWS b/NEWS index 6d7c485c..89839d83 100644 --- a/NEWS +++ b/NEWS @@ -10,11 +10,16 @@ * Better handling of duplicate ID3v2 tags in all kinds of files. * Better handling of duplicate tag chunks in WAV files. * Better handling of duplicate tag chunks in AIFF files. + * Better handling of duplicate Vorbis comment blocks in FLAC files. * Fixed crash when calling File::properties() after strip(). * Fixed crash when parsing certain MPEG files. * Fixed possible file corruptions when saving ASF files. * Fixed possible file corruptions when saving FLAC files. * Fixed possible file corruptions when saving MP4 files. + * Fixed possible file corruptions when saving MPEG files. + * Fixed possible file corruptions when saving APE files. + * Fixed possible file corruptions when saving Musepack files. + * Fixed possible file corruptions when saving WavPack files. * Fixed updating the comment field of Vorbis comments. * Fixed reading date and time in ID3v2.3 tags. * Replaced ENABLE_STATIC build option with BUILD_SHARED_LIBS. diff --git a/config.h.cmake b/config.h.cmake index 482673db..bd8763c5 100644 --- a/config.h.cmake +++ b/config.h.cmake @@ -29,10 +29,10 @@ #cmakedefine HAVE_VSNPRINTF 1 #cmakedefine HAVE_VSPRINTF_S 1 -/* Defined if your compiler supports ISO _strdup. */ +/* Defined if your compiler supports ISO _strdup */ #cmakedefine HAVE_ISO_STRDUP 1 -/* Defined if you have libz */ +/* Defined if zlib is installed */ #cmakedefine HAVE_ZLIB 1 /* Indicates whether debug messages are shown even in release mode */ diff --git a/taglib/ape/apefile.cpp b/taglib/ape/apefile.cpp index 10d701f2..8fd8a50a 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() { @@ -72,25 +69,18 @@ public: delete properties; } - long long APELocation; - unsigned int APESize; + long long APELocation; + long long APESize; long long ID3v1Location; ID3v2::Header *ID3v2Header; - long long ID3v2Location; - unsigned int ID3v2Size; + long long ID3v2Location; + long long ID3v2Size; DoubleTagUnion tag; AudioProperties *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; }; //////////////////////////////////////////////////////////////////////////////// @@ -145,64 +135,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, static_cast(d->APESize)); + + if(d->ID3v1Location >= 0) + d->ID3v1Location += (static_cast(data.size()) - d->APESize); + + d->APESize = data.size(); } else { - if(d->hasAPE) { - removeBlock(d->APELocation, d->APESize); - d->hasAPE = false; - if(d->hasID3v1) { - if(d->ID3v1Location > d->APELocation) { - d->ID3v1Location -= d->APESize; - } - } + + // APE tag is empty. Remove the old one. + + if(d->APELocation >= 0) { + removeBlock(d->APELocation, static_cast(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) @@ -217,27 +210,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); } //////////////////////////////////////////////////////////////////////////////// @@ -254,17 +244,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 @@ -274,10 +261,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 @@ -286,14 +272,14 @@ void APE::File::read(bool readProperties) long 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 c9c728ea..1275b196 100644 --- a/taglib/ape/apefile.h +++ b/taglib/ape/apefile.h @@ -133,9 +133,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/taglib/flac/flacfile.cpp b/taglib/flac/flacfile.cpp index deadf825..9d668b40 100644 --- a/taglib/flac/flacfile.cpp +++ b/taglib/flac/flacfile.cpp @@ -82,8 +82,8 @@ public: } const ID3v2::FrameFactory *ID3v2FrameFactory; - long long ID3v2Location; - unsigned int ID3v2OriginalSize; + long long ID3v2Location; + long long ID3v2OriginalSize; long long ID3v1Location; @@ -160,21 +160,16 @@ bool FLAC::File::save() // Replace metadata blocks - bool foundVorbisCommentBlock = false; - for(BlockIterator it = d->blocks.begin(); it != d->blocks.end(); ++it) { if((*it)->code() == MetadataBlock::VorbisComment) { // Set the new Vorbis Comment block delete *it; - *it = new UnknownMetadataBlock(MetadataBlock::VorbisComment, d->xiphCommentData); - foundVorbisCommentBlock = true; + d->blocks.erase(it); + break; } } - if(!foundVorbisCommentBlock) { - d->blocks.append(new UnknownMetadataBlock(MetadataBlock::VorbisComment, d->xiphCommentData)); - foundVorbisCommentBlock = true; - } + d->blocks.append(new UnknownMetadataBlock(MetadataBlock::VorbisComment, d->xiphCommentData)); // Render data for the metadata blocks @@ -216,10 +211,10 @@ bool FLAC::File::save() insert(data, d->flacStart, static_cast(originalLength)); - d->streamStart += (data.size() - originalLength); + d->streamStart += (static_cast(data.size()) - originalLength); if(d->ID3v1Location >= 0) - d->ID3v1Location += (data.size() - originalLength); + d->ID3v1Location += (static_cast(data.size()) - originalLength); // Update ID3 tags @@ -231,13 +226,13 @@ bool FLAC::File::save() d->ID3v2Location = 0; data = ID3v2Tag()->render(); - insert(data, d->ID3v2Location, d->ID3v2OriginalSize); + insert(data, d->ID3v2Location, static_cast(d->ID3v2OriginalSize)); - d->flacStart += (data.size() - d->ID3v2OriginalSize); - d->streamStart += (data.size() - d->ID3v2OriginalSize); + d->flacStart += (static_cast(data.size()) - d->ID3v2OriginalSize); + d->streamStart += (static_cast(data.size()) - d->ID3v2OriginalSize); if(d->ID3v1Location >= 0) - d->ID3v1Location += (data.size() - d->ID3v2OriginalSize); + d->ID3v1Location += (static_cast(data.size()) - d->ID3v2OriginalSize); d->ID3v2OriginalSize = data.size(); } @@ -246,7 +241,7 @@ bool FLAC::File::save() // ID3v2 tag is empty. Remove the old one. if(d->ID3v2Location >= 0) { - removeBlock(d->ID3v2Location, d->ID3v2OriginalSize); + removeBlock(d->ID3v2Location, static_cast(d->ID3v2OriginalSize)); d->flacStart -= d->ID3v2OriginalSize; d->streamStart -= d->ID3v2OriginalSize; @@ -438,51 +433,43 @@ void FLAC::File::scan() nextBlockOffset += 4; d->flacStart = nextBlockOffset; - seek(nextBlockOffset); + while(true) { - ByteVector header = readBlock(4); + seek(nextBlockOffset); + const ByteVector header = readBlock(4); - // Header format (from spec): - // <1> Last-metadata-block flag - // <7> BLOCK_TYPE - // 0 : STREAMINFO - // 1 : PADDING - // .. - // 4 : VORBIS_COMMENT - // .. - // <24> Length of metadata to follow + // Header format (from spec): + // <1> Last-metadata-block flag + // <7> BLOCK_TYPE + // 0 : STREAMINFO + // 1 : PADDING + // .. + // 4 : VORBIS_COMMENT + // .. + // 6 : PICTURE + // .. + // <24> Length of metadata to follow - char blockType = header[0] & 0x7f; - bool isLastBlock = (header[0] & 0x80) != 0; - size_t length = header.toUInt24BE(1); + const char blockType = header[0] & ~LastBlockFlag; + const bool isLastBlock = (header[0] & LastBlockFlag) != 0; + const size_t blockLength = header.toUInt24BE(1); - // First block should be the stream_info metadata + // First block should be the stream_info metadata - if(blockType != MetadataBlock::StreamInfo) { - debug("FLAC::File::scan() -- invalid FLAC stream"); - setValid(false); - return; - } + if(d->blocks.isEmpty() && blockType != MetadataBlock::StreamInfo) { + debug("FLAC::File::scan() -- First block should be the stream_info metadata"); + setValid(false); + return; + } - d->blocks.append(new UnknownMetadataBlock(blockType, readBlock(length))); - nextBlockOffset += length + 4; - - // Search through the remaining metadata - while(!isLastBlock) { - - header = readBlock(4); - blockType = header[0] & 0x7f; - isLastBlock = (header[0] & 0x80) != 0; - length = header.toUInt24BE(1); - - if(length == 0 && blockType != MetadataBlock::Padding) { + if(blockLength == 0 && blockType != MetadataBlock::Padding) { debug("FLAC::File::scan() -- Zero-sized metadata block found"); setValid(false); return; } - const ByteVector data = readBlock(length); - if(data.size() != length) { + const ByteVector data = readBlock(blockLength); + if(data.size() != blockLength) { debug("FLAC::File::scan() -- Failed to read a metadata block"); setValid(false); return; @@ -494,9 +481,10 @@ void FLAC::File::scan() if(blockType == MetadataBlock::VorbisComment) { if(d->xiphCommentData.isEmpty()) { d->xiphCommentData = data; + block = new UnknownMetadataBlock(MetadataBlock::VorbisComment, data); } else { - debug("FLAC::File::scan() -- multiple Vorbis Comment blocks found, using the first one"); + debug("FLAC::File::scan() -- multiple Vorbis Comment blocks found, discarding"); } } else if(blockType == MetadataBlock::Picture) { @@ -509,25 +497,20 @@ void FLAC::File::scan() delete picture; } } - - if(!block) { - block = new UnknownMetadataBlock(blockType, data); - } - if(block->code() != MetadataBlock::Padding) { - d->blocks.append(block); + else if(blockType == MetadataBlock::Padding) { + // Skip all padding blocks. } else { - delete block; + block = new UnknownMetadataBlock(blockType, data); } - nextBlockOffset += length + 4; + if(block) + d->blocks.append(block); - if(nextBlockOffset >= File::length()) { - debug("FLAC::File::scan() -- FLAC stream corrupted"); - setValid(false); - return; - } - seek(nextBlockOffset); + nextBlockOffset += blockLength + 4; + + if(isLastBlock) + break; } // End of metadata, now comes the datastream diff --git a/taglib/mpc/mpcfile.cpp b/taglib/mpc/mpcfile.cpp index 13a7f9df..7db6afdf 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() { @@ -64,25 +61,18 @@ public: delete properties; } - long long APELocation; - unsigned int APESize; + long long APELocation; + long long APESize; long long ID3v1Location; ID3v2::Header *ID3v2Header; - long long ID3v2Location; - unsigned int ID3v2Size; + long long ID3v2Location; + long long ID3v2Size; DoubleTagUnion tag; AudioProperties *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; }; //////////////////////////////////////////////////////////////////////////////// @@ -137,69 +127,80 @@ bool MPC::File::save() // Possibly strip ID3v2 tag - if(d->hasID3v2 && !d->ID3v2Header) { - removeBlock(d->ID3v2Location, d->ID3v2Size); - d->hasID3v2 = false; - if(d->hasID3v1) - d->ID3v1Location -= d->ID3v2Size; - if(d->hasAPE) + if(!d->ID3v2Header && d->ID3v2Location >= 0) { + removeBlock(d->ID3v2Location, static_cast(d->ID3v2Size)); + + 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, static_cast(d->APESize)); + + if(d->ID3v1Location >= 0) + d->ID3v1Location += (static_cast(data.size()) - d->APESize); + + d->APESize = data.size(); + } + else { + + // APE tag is empty. Remove the old one. + + if(d->APELocation >= 0) { + removeBlock(d->APELocation, static_cast(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; } @@ -216,22 +217,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) @@ -241,12 +239,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); } //////////////////////////////////////////////////////////////////////////////// @@ -255,30 +253,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); @@ -287,23 +261,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 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 5ffdf0f3..796a8ef3 100644 --- a/taglib/mpc/mpcfile.h +++ b/taglib/mpc/mpcfile.h @@ -132,9 +132,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/taglib/mpeg/id3v2/id3v2frame.cpp b/taglib/mpeg/id3v2/id3v2frame.cpp index 29ed421f..8bb3035b 100644 --- a/taglib/mpeg/id3v2/id3v2frame.cpp +++ b/taglib/mpeg/id3v2/id3v2frame.cpp @@ -265,17 +265,19 @@ ByteVector Frame::fieldData(const ByteVector &frameData) const if(inflateInit(&stream) != Z_OK) return ByteVector(); - stream.avail_in = (uLongf) frameData.size() - frameDataOffset; - stream.next_in = (Bytef *) frameData.data() + frameDataOffset; + ByteVector inData = frameData; + + stream.avail_in = static_cast(inData.size() - frameDataOffset); + stream.next_in = reinterpret_cast(inData.data() + frameDataOffset); static const size_t chunkSize = 1024; - ByteVector data; + ByteVector outData; ByteVector chunk(chunkSize); do { - stream.avail_out = (uLongf) chunk.size(); - stream.next_out = (Bytef *) chunk.data(); + stream.avail_out = static_cast(chunk.size()); + stream.next_out = reinterpret_cast(chunk.data()); int result = inflate(&stream, Z_NO_FLUSH); @@ -290,15 +292,15 @@ ByteVector Frame::fieldData(const ByteVector &frameData) const return ByteVector(); } - data.append(stream.avail_out == 0 ? chunk : chunk.mid(0, chunk.size() - stream.avail_out)); + outData.append(stream.avail_out == 0 ? chunk : chunk.mid(0, chunk.size() - stream.avail_out)); } while(stream.avail_out == 0); inflateEnd(&stream); - if(frameDataLength != data.size()) + if(frameDataLength != outData.size()) debug("frameDataLength does not match the data length returned by zlib"); - return data; + return outData; } else #endif diff --git a/taglib/mpeg/id3v2/id3v2framefactory.h b/taglib/mpeg/id3v2/id3v2framefactory.h index 1ccc77ea..53e64910 100644 --- a/taglib/mpeg/id3v2/id3v2framefactory.h +++ b/taglib/mpeg/id3v2/id3v2framefactory.h @@ -47,9 +47,9 @@ namespace TagLib { * Reimplementing this factory is the key to adding support for frame types * not directly supported by TagLib to your application. To do so you would * subclass this factory reimplement createFrame(). Then by setting your - * factory to be the default factory in ID3v2::Tag constructor or with - * MPEG::File::setID3v2FrameFactory() you can implement behavior that will - * allow for new ID3v2::Frame subclasses (also provided by you) to be used. + * factory to be the default factory in ID3v2::Tag constructor you can + * implement behavior that will allow for new ID3v2::Frame subclasses (also + * provided by you) to be used. * * This implements both abstract factory and singleton patterns * of which more information is available on the web and in software design diff --git a/taglib/mpeg/mpegfile.cpp b/taglib/mpeg/mpegfile.cpp index def26b5f..ca2af56a 100644 --- a/taglib/mpeg/mpegfile.cpp +++ b/taglib/mpeg/mpegfile.cpp @@ -24,6 +24,7 @@ ***************************************************************************/ #include +#include #include #include #include @@ -64,20 +65,14 @@ namespace class MPEG::File::FilePrivate { public: - FilePrivate(ID3v2::FrameFactory *frameFactory = ID3v2::FrameFactory::instance()) : + FilePrivate(const ID3v2::FrameFactory *frameFactory = ID3v2::FrameFactory::instance()) : ID3v2FrameFactory(frameFactory), ID3v2Location(-1), ID3v2OriginalSize(0), APELocation(-1), - APEFooterLocation(-1), APEOriginalSize(0), ID3v1Location(-1), - hasID3v2(false), - hasID3v1(false), - hasAPE(false), - properties(0) - { - } + properties(0) {} ~FilePrivate() { @@ -86,24 +81,16 @@ public: const ID3v2::FrameFactory *ID3v2FrameFactory; - long long ID3v2Location; - unsigned int ID3v2OriginalSize; + long long ID3v2Location; + long long ID3v2OriginalSize; - long long APELocation; - long long APEFooterLocation; - unsigned int APEOriginalSize; + long long APELocation; + long long APEOriginalSize; long long ID3v1Location; TripleTagUnion tag; - // These indicate whether the file *on disk* has these tags, not if - // this data structure does. This is used in computing offsets. - - bool hasID3v2; - bool hasID3v1; - bool hasAPE; - AudioProperties *properties; }; @@ -169,17 +156,6 @@ bool MPEG::File::save() bool MPEG::File::save(int tags, bool stripOthers, int id3v2Version, bool duplicateTags) { - if(tags == NoTags && stripOthers) - return strip(AllTags); - - if(!ID3v2Tag() && !ID3v1Tag() && !APETag()) { - - if((d->hasID3v1 || d->hasID3v2 || d->hasAPE) && stripOthers) - return strip(AllTags); - - return true; - } - if(readOnly()) { debug("MPEG::File::save() -- File is read only."); return false; @@ -187,7 +163,7 @@ bool MPEG::File::save(int tags, bool stripOthers, int id3v2Version, bool duplica // Create the tags if we've been asked to. - if (duplicateTags) { + if(duplicateTags) { // Copy the values from the tag that does exist into the new tag, // except if the existing tag is to be stripped. @@ -199,79 +175,93 @@ bool MPEG::File::save(int tags, bool stripOthers, int id3v2Version, bool duplica Tag::duplicate(ID3v2Tag(), ID3v1Tag(true), false); } - bool success = true; + // Remove all the tags not going to be saved. + + if(stripOthers) + strip(~tags, false); if(ID3v2 & tags) { if(ID3v2Tag() && !ID3v2Tag()->isEmpty()) { - if(!d->hasID3v2) + // ID3v2 tag is not empty. Update the old one or create a new one. + + if(d->ID3v2Location < 0) d->ID3v2Location = 0; - insert(ID3v2Tag()->render(id3v2Version), d->ID3v2Location, d->ID3v2OriginalSize); + const ByteVector data = ID3v2Tag()->render(id3v2Version); + insert(data, d->ID3v2Location, static_cast(d->ID3v2OriginalSize)); - d->hasID3v2 = true; + if(d->APELocation >= 0) + d->APELocation += (static_cast(data.size()) - d->ID3v2OriginalSize); - // v1 tag location has changed, update if it exists + if(d->ID3v1Location >= 0) + d->ID3v1Location += (static_cast(data.size()) - d->ID3v2OriginalSize); - if(ID3v1Tag()) - d->ID3v1Location = findID3v1(); - - // APE tag location has changed, update if it exists - - if(APETag()) - findAPE(); + d->ID3v2OriginalSize = data.size(); + } + else { + + // ID3v2 tag is empty. Remove the old one. + + strip(ID3v2, false); } - else if(stripOthers) - success = strip(ID3v2, false) && success; } - else if(d->hasID3v2 && stripOthers) - success = strip(ID3v2) && success; if(ID3v1 & tags) { + if(ID3v1Tag() && !ID3v1Tag()->isEmpty()) { - int offset = d->hasID3v1 ? -128 : 0; - seek(offset, End); - writeBlock(ID3v1Tag()->render()); - d->hasID3v1 = true; - d->ID3v1Location = findID3v1(); - } - else if(stripOthers) - success = strip(ID3v1) && success; - } - else if(d->hasID3v1 && stripOthers) - success = strip(ID3v1, false) && success; - // Dont save an APE-tag unless one has been created + // ID3v1 tag is not empty. Update the old one or create a new one. - if((APE & tags) && APETag()) { - if(d->hasAPE) - insert(APETag()->render(), d->APELocation, d->APEOriginalSize); - else { - if(d->hasID3v1) { - insert(APETag()->render(), d->ID3v1Location, 0); - d->APEOriginalSize = APETag()->footer()->completeTagSize(); - d->hasAPE = true; - d->APELocation = d->ID3v1Location; - d->ID3v1Location += d->APEOriginalSize; + if(d->ID3v1Location >= 0) { + seek(d->ID3v1Location); } else { seek(0, End); - d->APELocation = tell(); - APE::Tag *apeTag = d->tag.access(APEIndex, false); - d->APEFooterLocation = d->APELocation - + apeTag->footer()->completeTagSize() - - APE::Footer::size(); - writeBlock(APETag()->render()); - d->APEOriginalSize = APETag()->footer()->completeTagSize(); - d->hasAPE = true; + d->ID3v1Location = tell(); } + + writeBlock(ID3v1Tag()->render()); + } + else { + + // ID3v1 tag is empty. Remove the old one. + + strip(ID3v1, false); } } - else if(d->hasAPE && stripOthers) - success = strip(APE, false) && success; - return success; + if(APE & tags) { + + 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; + else + d->APELocation = length(); + } + + const ByteVector data = APETag()->render(); + insert(data, d->APELocation, static_cast(d->APEOriginalSize)); + + if(d->ID3v1Location >= 0) + d->ID3v1Location += (static_cast(data.size()) - d->APEOriginalSize); + + d->APEOriginalSize = data.size(); + } + else { + + // APE tag is empty. Remove the old one. + + strip(APE, false); + } + } + + return true; } ID3v2::Tag *MPEG::File::ID3v2Tag(bool create) @@ -301,44 +291,39 @@ bool MPEG::File::strip(int tags, bool freeMemory) return false; } - if((tags & ID3v2) && d->hasID3v2) { - removeBlock(d->ID3v2Location, d->ID3v2OriginalSize); + if((tags & ID3v2) && d->ID3v2Location >= 0) { + removeBlock(d->ID3v2Location, static_cast(d->ID3v2OriginalSize)); + + if(d->APELocation >= 0) + d->APELocation -= d->ID3v2OriginalSize; + + if(d->ID3v1Location >= 0) + d->ID3v1Location -= d->ID3v2OriginalSize; + d->ID3v2Location = -1; d->ID3v2OriginalSize = 0; - d->hasID3v2 = false; if(freeMemory) d->tag.set(ID3v2Index, 0); - - // v1 tag location has changed, update if it exists - - if(ID3v1Tag()) - d->ID3v1Location = findID3v1(); - - // APE tag location has changed, update if it exists - - if(APETag()) - findAPE(); } - if((tags & ID3v1) && d->hasID3v1) { + if((tags & ID3v1) && d->ID3v1Location >= 0) { truncate(d->ID3v1Location); + d->ID3v1Location = -1; - d->hasID3v1 = false; if(freeMemory) d->tag.set(ID3v1Index, 0); } - if((tags & APE) && d->hasAPE) { - removeBlock(d->APELocation, d->APEOriginalSize); + if((tags & APE) && d->APELocation >= 0) { + removeBlock(d->APELocation, static_cast(d->APEOriginalSize)); + + if(d->ID3v1Location >= 0) + d->ID3v1Location -= d->APEOriginalSize; + d->APELocation = -1; - d->APEFooterLocation = -1; - d->hasAPE = false; - if(d->hasID3v1) { - if(d->ID3v1Location > d->APELocation) - d->ID3v1Location -= d->APEOriginalSize; - } + d->APEOriginalSize = 0; if(freeMemory) d->tag.set(APEIndex, 0); @@ -432,17 +417,17 @@ long long MPEG::File::lastFrameOffset() bool MPEG::File::hasID3v1Tag() const { - return d->hasID3v1; + return (d->ID3v1Location >= 0); } bool MPEG::File::hasID3v2Tag() const { - return d->hasID3v2; + return (d->ID3v2Location >= 0); } bool MPEG::File::hasAPETag() const { - return d->hasAPE; + return (d->APELocation >= 0); } //////////////////////////////////////////////////////////////////////////////// @@ -456,35 +441,25 @@ void MPEG::File::read(bool readProperties) d->ID3v2Location = findID3v2(); if(d->ID3v2Location >= 0) { - d->tag.set(ID3v2Index, new ID3v2::Tag(this, d->ID3v2Location, d->ID3v2FrameFactory)); - d->ID3v2OriginalSize = ID3v2Tag()->header()->completeTagSize(); - - if(ID3v2Tag()->header()->tagSize() <= 0) - d->tag.set(ID3v2Index, 0); - else - d->hasID3v2 = true; } // Look for an ID3v1 tag - d->ID3v1Location = findID3v1(); + d->ID3v1Location = Utils::findID3v1(this); - if(d->ID3v1Location >= 0) { + if(d->ID3v1Location >= 0) d->tag.set(ID3v1Index, new ID3v1::Tag(this, d->ID3v1Location)); - d->hasID3v1 = true; - } // Look for an APE tag - findAPE(); + d->APELocation = Utils::findAPE(this, d->ID3v1Location); if(d->APELocation >= 0) { - - d->tag.set(APEIndex, new APE::Tag(this, d->APEFooterLocation)); + d->tag.set(APEIndex, new APE::Tag(this, d->APELocation)); d->APEOriginalSize = APETag()->footer()->completeTagSize(); - d->hasAPE = true; + d->APELocation = d->APELocation + APE::Footer::size() - d->APEOriginalSize; } if(readProperties) @@ -531,36 +506,3 @@ long long MPEG::File::findID3v2() return tagOffset; } - -long long MPEG::File::findID3v1() -{ - if(isValid()) { - seek(-128, End); - long long p = tell(); - - if(readBlock(3) == ID3v1::Tag::fileIdentifier()) - return p; - } - return -1; -} - -void MPEG::File::findAPE() -{ - if(isValid()) { - seek(d->hasID3v1 ? -160 : -32, End); - - long long p = tell(); - - if(readBlock(8) == APE::Tag::fileIdentifier()) { - d->APEFooterLocation = p; - seek(d->APEFooterLocation); - APE::Footer footer(readBlock(APE::Footer::size())); - d->APELocation = d->APEFooterLocation - footer.completeTagSize() - + APE::Footer::size(); - return; - } - } - - d->APELocation = -1; - d->APEFooterLocation = -1; -} diff --git a/taglib/mpeg/mpegfile.h b/taglib/mpeg/mpegfile.h index 7b7310d2..3f2f3e7b 100644 --- a/taglib/mpeg/mpegfile.h +++ b/taglib/mpeg/mpegfile.h @@ -165,9 +165,6 @@ namespace TagLib { * If you would like more granular control over the content of the tags, * with the concession of generality, use parameterized save call below. * - * \warning In the current implementation, it's dangerous to call save() - * repeatedly. At worst it will corrupt the file. - * * \see save(int tags) */ virtual bool save(); @@ -184,8 +181,8 @@ namespace TagLib { * The \a id3v2Version parameter specifies the version of the saved * ID3v2 tag. It can be either 4 or 3. * - * \warning In the current implementation, it's dangerous to call save() - * repeatedly. At worst it will corrupt the file. + * If \a duplicateTags is true and at least one tag -- ID3v1 or ID3v2 -- + * exists this will duplicate its content into the other tag. */ bool save(int tags, bool stripOthers = true, int id3v2Version = 4, bool duplicateTags = true); @@ -330,8 +327,6 @@ namespace TagLib { void read(bool readProperties); long long findID3v2(); - long long findID3v1(); - void findAPE(); class FilePrivate; FilePrivate *d; diff --git a/taglib/trueaudio/trueaudiofile.cpp b/taglib/trueaudio/trueaudiofile.cpp index 7a5b0e32..0328e20a 100644 --- a/taglib/trueaudio/trueaudiofile.cpp +++ b/taglib/trueaudio/trueaudiofile.cpp @@ -57,9 +57,7 @@ public: ID3v2Location(-1), ID3v2OriginalSize(0), ID3v1Location(-1), - properties(0), - hasID3v1(false), - hasID3v2(false) {} + properties(0) {} ~FilePrivate() { @@ -67,20 +65,14 @@ public: } const ID3v2::FrameFactory *ID3v2FrameFactory; - long long ID3v2Location; - unsigned int ID3v2OriginalSize; + long long ID3v2Location; + long long ID3v2OriginalSize; long long ID3v1Location; DoubleTagUnion tag; AudioProperties *properties; - - // These indicate whether the file *on disk* has these tags, not if - // this data structure does. This is used in computing offsets. - - bool hasID3v1; - bool hasID3v2; }; //////////////////////////////////////////////////////////////////////////////// @@ -159,40 +151,59 @@ bool TrueAudio::File::save() // Update ID3v2 tag if(ID3v2Tag() && !ID3v2Tag()->isEmpty()) { - if(!d->hasID3v2) { + + // ID3v2 tag is not empty. Update the old one or create a new one. + + if(d->ID3v2Location < 0) d->ID3v2Location = 0; + + const ByteVector data = ID3v2Tag()->render(); + insert(data, d->ID3v2Location, static_cast(d->ID3v2OriginalSize)); + + if(d->ID3v1Location >= 0) + d->ID3v1Location += (static_cast(data.size()) - d->ID3v2OriginalSize); + + d->ID3v2OriginalSize = data.size(); + } + else { + + // ID3v2 tag is empty. Remove the old one. + + if(d->ID3v2Location >= 0) { + removeBlock(d->ID3v2Location, static_cast(d->ID3v2OriginalSize)); + + if(d->ID3v1Location >= 0) + d->ID3v1Location -= d->ID3v2OriginalSize; + + d->ID3v2Location = -1; d->ID3v2OriginalSize = 0; } - ByteVector data = ID3v2Tag()->render(); - insert(data, d->ID3v2Location, d->ID3v2OriginalSize); - d->ID3v1Location -= d->ID3v2OriginalSize - data.size(); - d->ID3v2OriginalSize = static_cast(data.size()); - d->hasID3v2 = true; - } - else if(d->hasID3v2) { - removeBlock(d->ID3v2Location, d->ID3v2OriginalSize); - d->ID3v1Location -= d->ID3v2OriginalSize; - d->ID3v2Location = -1; - d->ID3v2OriginalSize = 0; - d->hasID3v2 = false; } // Update ID3v1 tag if(ID3v1Tag() && !ID3v1Tag()->isEmpty()) { - if(!d->hasID3v1) { + + // ID3v1 tag is not empty. Update the old one or create a new one. + + if(d->ID3v1Location >= 0) { + seek(d->ID3v1Location); + } + else { seek(0, End); d->ID3v1Location = tell(); } - else - seek(d->ID3v1Location); + writeBlock(ID3v1Tag()->render()); - d->hasID3v1 = true; } - else if(d->hasID3v1) { - removeBlock(d->ID3v1Location, 128); - d->ID3v1Location = -1; - d->hasID3v1 = false; + else { + + // ID3v1 tag is empty. Remove the old one. + + if(d->ID3v1Location >= 0) { + truncate(d->ID3v1Location); + d->ID3v1Location = -1; + } } return true; @@ -210,27 +221,24 @@ ID3v2::Tag *TrueAudio::File::ID3v2Tag(bool create) void TrueAudio::File::strip(int tags) { - if(tags & ID3v1) { + if(tags & ID3v1) d->tag.set(TrueAudioID3v1Index, 0); - ID3v2Tag(true); - } - if(tags & ID3v2) { + if(tags & ID3v2) d->tag.set(TrueAudioID3v2Index, 0); - if(!ID3v1Tag()) - ID3v2Tag(true); - } + if(!ID3v1Tag()) + ID3v2Tag(true); } bool TrueAudio::File::hasID3v1Tag() const { - return d->hasID3v1; + return (d->ID3v1Location >= 0); } bool TrueAudio::File::hasID3v2Tag() const { - return d->hasID3v2; + return (d->ID3v2Location >= 0); } //////////////////////////////////////////////////////////////////////////////// @@ -244,27 +252,18 @@ void TrueAudio::File::read(bool readProperties) d->ID3v2Location = Utils::findID3v2(this); if(d->ID3v2Location >= 0) { - d->tag.set(TrueAudioID3v2Index, new ID3v2::Tag(this, d->ID3v2Location, d->ID3v2FrameFactory)); - d->ID3v2OriginalSize = ID3v2Tag()->header()->completeTagSize(); - - if(ID3v2Tag()->header()->tagSize() <= 0) - d->tag.set(TrueAudioID3v2Index, 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(TrueAudioID3v1Index, new ID3v1::Tag(this, d->ID3v1Location)); - d->hasID3v1 = true; - } - if(!d->hasID3v1) + if(d->ID3v1Location < 0) ID3v2Tag(true); // Look for TrueAudio metadata @@ -273,12 +272,12 @@ void TrueAudio::File::read(bool readProperties) long long streamLength; - if(d->hasID3v1) + if(d->ID3v1Location >= 0) streamLength = d->ID3v1Location; else streamLength = length(); - if(d->hasID3v2) { + if(d->ID3v2Location >= 0) { seek(d->ID3v2Location + d->ID3v2OriginalSize); streamLength -= (d->ID3v2Location + d->ID3v2OriginalSize); } diff --git a/taglib/wavpack/wavpackfile.cpp b/taglib/wavpack/wavpackfile.cpp index 049dc72d..59cce8b8 100644 --- a/taglib/wavpack/wavpackfile.cpp +++ b/taglib/wavpack/wavpackfile.cpp @@ -54,29 +54,21 @@ public: APELocation(-1), APESize(0), ID3v1Location(-1), - properties(0), - hasAPE(false), - hasID3v1(false) {} + properties(0) {} ~FilePrivate() { delete properties; } - long long APELocation; - unsigned int APESize; + long long APELocation; + long long APESize; long long ID3v1Location; DoubleTagUnion tag; AudioProperties *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; }; //////////////////////////////////////////////////////////////////////////////// @@ -131,64 +123,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, static_cast(d->APESize)); + + if(d->ID3v1Location >= 0) + d->ID3v1Location += (static_cast(data.size()) - d->APESize); + + d->APESize = data.size(); } else { - if(d->hasAPE) { - removeBlock(d->APELocation, d->APESize); - d->hasAPE = false; - if(d->hasID3v1) { - if(d->ID3v1Location > d->APELocation) { - d->ID3v1Location -= d->APESize; - } - } + + // APE tag is empty. Remove the old one. + + if(d->APELocation >= 0) { + removeBlock(d->APELocation, static_cast(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) @@ -203,27 +198,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); } //////////////////////////////////////////////////////////////////////////////// @@ -236,10 +228,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 @@ -249,10 +239,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 @@ -261,9 +250,9 @@ void WavPack::File::read(bool readProperties) long 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 88a83d0a..440ff372 100644 --- a/taglib/wavpack/wavpackfile.h +++ b/taglib/wavpack/wavpackfile.h @@ -126,9 +126,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_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); diff --git a/tests/test_flac.cpp b/tests/test_flac.cpp index e4a2c559..996dc10c 100644 --- a/tests/test_flac.cpp +++ b/tests/test_flac.cpp @@ -60,6 +60,8 @@ public: { FLAC::File f(newname.c_str()); CPPUNIT_ASSERT_EQUAL(String("The Artist"), f.tag()->artist()); + CPPUNIT_ASSERT_EQUAL(69LL, f.find("Artist")); + CPPUNIT_ASSERT_EQUAL(-1LL, f.find("Artist", 70)); } } diff --git a/tests/test_id3v2.cpp b/tests/test_id3v2.cpp index 7f0af903..41345f93 100644 --- a/tests/test_id3v2.cpp +++ b/tests/test_id3v2.cpp @@ -1073,29 +1073,20 @@ public: { MPEG::File f(newname.c_str()); - ID3v2::Tag *tag = f.ID3v2Tag(true); - - ID3v2::TextIdentificationFrame *frame1 = new ID3v2::TextIdentificationFrame("TIT2"); - frame1->setText("Title"); - tag->addFrame(frame1); - - ID3v2::AttachedPictureFrame *frame2 = new ID3v2::AttachedPictureFrame(); - frame2->setPicture(ByteVector(100 * 1024, '\xff')); - tag->addFrame(frame2); - - f.save(); - CPPUNIT_ASSERT(f.length() > 100 * 1024); + f.ID3v2Tag()->setTitle(std::wstring(64 * 1024, L'X').c_str()); + f.save(MPEG::File::ID3v2, true); } - { MPEG::File f(newname.c_str()); - CPPUNIT_ASSERT_EQUAL(true, f.hasID3v2Tag()); - - ID3v2::Tag *tag = f.ID3v2Tag(); - tag->removeFrames("APIC"); - - f.save(); - CPPUNIT_ASSERT(f.length() < 10 * 1024); + CPPUNIT_ASSERT(f.hasID3v2Tag()); + CPPUNIT_ASSERT_EQUAL(74789LL, f.length()); + f.ID3v2Tag()->setTitle("ABCDEFGHIJ"); + f.save(MPEG::File::ID3v2, true); + } + { + MPEG::File f(newname.c_str()); + CPPUNIT_ASSERT(f.hasID3v2Tag()); + CPPUNIT_ASSERT_EQUAL(9263LL, f.length()); } } 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); diff --git a/tests/test_mpeg.cpp b/tests/test_mpeg.cpp index 522c0d4f..a5003c52 100644 --- a/tests/test_mpeg.cpp +++ b/tests/test_mpeg.cpp @@ -31,6 +31,12 @@ class TestMPEG : public CppUnit::TestFixture CPPUNIT_TEST(testFuzzedFile); CPPUNIT_TEST(testFrameOffset); CPPUNIT_TEST(testStripAndProperties); + CPPUNIT_TEST(testRepeatedSave1); + CPPUNIT_TEST(testRepeatedSave2); + CPPUNIT_TEST(testRepeatedSave3); + CPPUNIT_TEST(testEmptyID3v2); + CPPUNIT_TEST(testEmptyID3v1); + CPPUNIT_TEST(testEmptyAPE); CPPUNIT_TEST_SUITE_END(); public: @@ -253,6 +259,120 @@ public: } } + void testRepeatedSave1() + { + ScopedFileCopy copy("xing", ".mp3"); + + { + MPEG::File f(copy.fileName().c_str()); + f.ID3v2Tag(true)->setTitle(std::string(4096, 'X').c_str()); + f.save(); + } + { + MPEG::File f(copy.fileName().c_str()); + f.ID3v2Tag(true)->setTitle(""); + f.save(); + f.ID3v2Tag(true)->setTitle(std::string(4096, 'X').c_str()); + f.save(); + CPPUNIT_ASSERT_EQUAL(5141LL, f.firstFrameOffset()); + } + } + + void testRepeatedSave2() + { + ScopedFileCopy copy("xing", ".mp3"); + + MPEG::File f(copy.fileName().c_str()); + f.ID3v2Tag(true)->setTitle("0123456789"); + f.save(); + f.save(); + CPPUNIT_ASSERT_EQUAL(-1LL, f.find("ID3", 3)); + } + + void testRepeatedSave3() + { + ScopedFileCopy copy("xing", ".mp3"); + + { + MPEG::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(); + } + { + MPEG::File f(copy.fileName().c_str()); + CPPUNIT_ASSERT(f.hasAPETag()); + CPPUNIT_ASSERT(f.hasID3v1Tag()); + } + } + + void testEmptyID3v2() + { + ScopedFileCopy copy("xing", ".mp3"); + + { + MPEG::File f(copy.fileName().c_str()); + f.ID3v2Tag(true)->setTitle("0123456789"); + f.save(MPEG::File::ID3v2); + } + { + MPEG::File f(copy.fileName().c_str()); + f.ID3v2Tag(true)->setTitle(""); + f.save(MPEG::File::ID3v2, false); + } + { + MPEG::File f(copy.fileName().c_str()); + CPPUNIT_ASSERT(!f.hasID3v2Tag()); + } + } + + void testEmptyID3v1() + { + ScopedFileCopy copy("xing", ".mp3"); + + { + MPEG::File f(copy.fileName().c_str()); + f.ID3v1Tag(true)->setTitle("0123456789"); + f.save(MPEG::File::ID3v1); + } + { + MPEG::File f(copy.fileName().c_str()); + f.ID3v1Tag(true)->setTitle(""); + f.save(MPEG::File::ID3v1, false); + } + { + MPEG::File f(copy.fileName().c_str()); + CPPUNIT_ASSERT(!f.hasID3v1Tag()); + } + } + + void testEmptyAPE() + { + ScopedFileCopy copy("xing", ".mp3"); + + { + MPEG::File f(copy.fileName().c_str()); + f.APETag(true)->setTitle("0123456789"); + f.save(MPEG::File::APE); + } + { + MPEG::File f(copy.fileName().c_str()); + f.APETag(true)->setTitle(""); + f.save(MPEG::File::APE, false); + } + { + MPEG::File f(copy.fileName().c_str()); + CPPUNIT_ASSERT(!f.hasAPETag()); + } + } + }; CPPUNIT_TEST_SUITE_REGISTRATION(TestMPEG); diff --git a/tests/test_trueaudio.cpp b/tests/test_trueaudio.cpp index 8468e0ec..c960f5f7 100644 --- a/tests/test_trueaudio.cpp +++ b/tests/test_trueaudio.cpp @@ -16,6 +16,7 @@ class TestTrueAudio : public CppUnit::TestFixture CPPUNIT_TEST(testReadPropertiesWithoutID3v2); CPPUNIT_TEST(testReadPropertiesWithTags); CPPUNIT_TEST(testStripAndProperties); + CPPUNIT_TEST(testRepeatedSave); CPPUNIT_TEST_SUITE_END(); public: @@ -70,6 +71,32 @@ public: } } + void testRepeatedSave() + { + ScopedFileCopy copy("empty", ".tta"); + + { + TrueAudio::File f(copy.fileName().c_str()); + CPPUNIT_ASSERT(!f.hasID3v2Tag()); + CPPUNIT_ASSERT(!f.hasID3v1Tag()); + + f.ID3v2Tag(true)->setTitle("01234 56789 ABCDE FGHIJ"); + f.save(); + + f.ID3v2Tag()->setTitle("0"); + f.save(); + + f.ID3v1Tag(true)->setTitle("01234 56789 ABCDE FGHIJ"); + f.ID3v2Tag()->setTitle("01234 56789 ABCDE FGHIJ 01234 56789 ABCDE FGHIJ 01234 56789"); + f.save(); + } + { + TrueAudio::File f(copy.fileName().c_str()); + CPPUNIT_ASSERT(f.hasID3v2Tag()); + CPPUNIT_ASSERT(f.hasID3v1Tag()); + } + } + }; CPPUNIT_TEST_SUITE_REGISTRATION(TestTrueAudio); 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);