diff --git a/CMakeLists.txt b/CMakeLists.txt index e217c44c..5a29841e 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -133,7 +133,7 @@ if(BUILD_BINDINGS) add_subdirectory(bindings) endif() -if(BUILD_TESTS) +if(BUILD_TESTS AND NOT BUILD_SHARED_LIBS) enable_testing() add_subdirectory(tests) endif() diff --git a/ConfigureChecks.cmake b/ConfigureChecks.cmake index 452ce84a..4f0e1cd6 100644 --- a/ConfigureChecks.cmake +++ b/ConfigureChecks.cmake @@ -259,7 +259,7 @@ endif() # Determine whether CppUnit is installed. -if(BUILD_TESTS) +if(BUILD_TESTS AND NOT BUILD_SHARED_LIBS) find_package(CppUnit) if(NOT CppUnit_FOUND) message(STATUS "CppUnit not found, disabling tests.") diff --git a/NEWS b/NEWS index bec7d290..6d7c485c 100644 --- a/NEWS +++ b/NEWS @@ -11,12 +11,17 @@ * Better handling of duplicate tag chunks in WAV files. * Better handling of duplicate tag chunks in AIFF 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 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. * Marked ByteVector::null and ByteVector::isNull() deprecated. * Marked String::null and ByteVector::isNull() deprecated. * Marked XiphComment::removeField() deprecated. + * Marked custom integer types deprecated. * Many smaller bug fixes and performance improvements. TagLib 1.10 (Nov 11, 2015) diff --git a/taglib/flac/flacfile.cpp b/taglib/flac/flacfile.cpp index 8e5cd3a6..deadf825 100644 --- a/taglib/flac/flacfile.cpp +++ b/taglib/flac/flacfile.cpp @@ -45,15 +45,22 @@ using namespace TagLib; namespace { + typedef List BlockList; + typedef BlockList::Iterator BlockIterator; + typedef BlockList::Iterator BlockConstIterator; + enum { FlacXiphIndex = 0, FlacID3v2Index = 1, FlacID3v1Index = 2 }; - enum { MinPaddingLength = 4096 }; - enum { LastBlockFlag = 0x80 }; + + const long long MinPaddingLength = 4096; + const long long MaxPaddingLegnth = 1024 * 1024; + + const char LastBlockFlag = '\x80'; } class FLAC::File::FilePrivate { public: - FilePrivate() : + FilePrivate(const ID3v2::FrameFactory *frameFactory) : ID3v2FrameFactory(ID3v2::FrameFactory::instance()), ID3v2Location(-1), ID3v2OriginalSize(0), @@ -61,19 +68,16 @@ public: properties(0), flacStart(0), streamStart(0), - scanned(false), - hasXiphComment(false), - hasID3v2(false), - hasID3v1(false) + scanned(false) { + if(frameFactory) + ID3v2FrameFactory = frameFactory; + + blocks.setAutoDelete(true); } ~FilePrivate() { - size_t size = blocks.size(); - for(size_t i = 0; i < size; i++) { - delete blocks[i]; - } delete properties; } @@ -87,15 +91,11 @@ public: AudioProperties *properties; ByteVector xiphCommentData; - List blocks; + BlockList blocks; long long flacStart; long long streamStart; bool scanned; - - bool hasXiphComment; - bool hasID3v2; - bool hasID3v1; }; //////////////////////////////////////////////////////////////////////////////// @@ -105,10 +105,8 @@ public: FLAC::File::File(FileName file, bool readProperties, AudioProperties::ReadStyle, ID3v2::FrameFactory *frameFactory) : TagLib::File(file), - d(new FilePrivate()) + d(new FilePrivate(frameFactory)) { - if(frameFactory) - d->ID3v2FrameFactory = frameFactory; if(isOpen()) read(readProperties); } @@ -116,10 +114,8 @@ FLAC::File::File(FileName file, bool readProperties, AudioProperties::ReadStyle, FLAC::File::File(IOStream *stream, bool readProperties, AudioProperties::ReadStyle, ID3v2::FrameFactory *frameFactory) : TagLib::File(stream), - d(new FilePrivate()) + d(new FilePrivate(frameFactory)) { - if(frameFactory) - d->ID3v2FrameFactory = frameFactory; if(isOpen()) read(readProperties); } @@ -165,72 +161,109 @@ bool FLAC::File::save() // Replace metadata blocks bool foundVorbisCommentBlock = false; - List newBlocks; - for(unsigned int i = 0; i < d->blocks.size(); i++) { - MetadataBlock *block = d->blocks[i]; - if(block->code() == MetadataBlock::VorbisComment) { + + for(BlockIterator it = d->blocks.begin(); it != d->blocks.end(); ++it) { + if((*it)->code() == MetadataBlock::VorbisComment) { // Set the new Vorbis Comment block - delete block; - block = new UnknownMetadataBlock(MetadataBlock::VorbisComment, d->xiphCommentData); + delete *it; + *it = new UnknownMetadataBlock(MetadataBlock::VorbisComment, d->xiphCommentData); foundVorbisCommentBlock = true; } - if(block->code() == MetadataBlock::Padding) { - delete block; - continue; - } - newBlocks.append(block); } + if(!foundVorbisCommentBlock) { - newBlocks.append(new UnknownMetadataBlock(MetadataBlock::VorbisComment, d->xiphCommentData)); + d->blocks.append(new UnknownMetadataBlock(MetadataBlock::VorbisComment, d->xiphCommentData)); foundVorbisCommentBlock = true; } - d->blocks = newBlocks; // Render data for the metadata blocks ByteVector data; - for(unsigned int i = 0; i < newBlocks.size(); i++) { - FLAC::MetadataBlock *block = newBlocks[i]; - ByteVector blockData = block->render(); + for(BlockConstIterator it = d->blocks.begin(); it != d->blocks.end(); ++it) { + ByteVector blockData = (*it)->render(); ByteVector blockHeader = ByteVector::fromUInt32BE(blockData.size()); - blockHeader[0] = block->code(); + blockHeader[0] = (*it)->code(); data.append(blockHeader); 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 long originalLength = d->streamStart - d->flacStart; + long long paddingLength = originalLength - data.size() - 4; - long originalLength = static_cast(d->streamStart - d->flacStart); - int paddingLength = originalLength - data.size() - 4; if(paddingLength <= 0) { paddingLength = MinPaddingLength; } - ByteVector padding = ByteVector::fromUInt32BE(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 long threshold = length() / 100; + threshold = std::max(threshold, MinPaddingLength); + threshold = std::min(threshold, MaxPaddingLegnth); + + if(paddingLength > threshold) + paddingLength = MinPaddingLength; + } + + ByteVector paddingHeader = ByteVector::fromUInt32BE(static_cast(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; + insert(data, d->flacStart, static_cast(originalLength)); + + 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 { @@ -239,7 +272,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; @@ -268,8 +309,8 @@ void FLAC::File::setID3v2FrameFactory(const ID3v2::FrameFactory *factory) List FLAC::File::pictureList() { List pictures; - for(unsigned int i = 0; i < d->blocks.size(); i++) { - Picture *picture = dynamic_cast(d->blocks[i]); + for(BlockConstIterator it = d->blocks.begin(); it != d->blocks.end(); ++it) { + Picture *picture = dynamic_cast(*it); if(picture) { pictures.append(picture); } @@ -284,8 +325,7 @@ void FLAC::File::addPicture(Picture *picture) void FLAC::File::removePicture(Picture *picture, bool del) { - MetadataBlock *block = picture; - List::Iterator it = d->blocks.find(block); + BlockIterator it = d->blocks.find(picture); if(it != d->blocks.end()) d->blocks.erase(it); @@ -295,32 +335,30 @@ void FLAC::File::removePicture(Picture *picture, bool del) void FLAC::File::removePictures() { - List newBlocks; - for(unsigned int i = 0; i < d->blocks.size(); i++) { - Picture *picture = dynamic_cast(d->blocks[i]); - if(picture) { - delete picture; + for(BlockIterator it = d->blocks.begin(); it != d->blocks.end(); ) { + if(dynamic_cast(*it)) { + delete *it; + it = d->blocks.erase(it); } else { - newBlocks.append(d->blocks[i]); + ++it; } } - d->blocks = newBlocks; } 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); } //////////////////////////////////////////////////////////////////////////////// @@ -334,25 +372,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 @@ -361,10 +390,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) { @@ -374,10 +403,10 @@ void FLAC::File::read(bool readProperties) long 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 AudioProperties(infoData, streamLength); } @@ -395,7 +424,7 @@ void FLAC::File::scan() long long nextBlockOffset; - if(d->hasID3v2) + if(d->ID3v2Location >= 0) nextBlockOffset = find("fLaC", d->ID3v2Location + d->ID3v2OriginalSize); else nextBlockOffset = find("fLaC"); @@ -463,9 +492,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 f17df67e..31b6de67 100644 --- a/taglib/flac/flacfile.h +++ b/taglib/flac/flacfile.h @@ -133,9 +133,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/taglib/mp4/mp4file.cpp b/taglib/mp4/mp4file.cpp index 0ea68ff6..8a33f8f4 100644 --- a/taglib/mp4/mp4file.cpp +++ b/taglib/mp4/mp4file.cpp @@ -55,8 +55,7 @@ public: FilePrivate() : tag(0), atoms(0), - properties(0), - hasMP4Tag(false) {} + properties(0) {} ~FilePrivate() { @@ -68,8 +67,6 @@ public: MP4::Tag *tag; MP4::Atoms *atoms; MP4::AudioProperties *properties; - - bool hasMP4Tag; }; MP4::File::File(FileName file, bool readProperties, AudioProperties::ReadStyle) : @@ -123,10 +120,6 @@ MP4::File::read(bool readProperties) return; } - if(d->atoms->find("moov", "udta", "meta", "ilst")) { - d->hasMP4Tag = true; - } - d->tag = new Tag(this, d->atoms); if(readProperties) { d->properties = new AudioProperties(this, d->atoms); @@ -146,16 +139,11 @@ MP4::File::save() return false; } - const bool success = d->tag->save(); - if(success) { - d->hasMP4Tag = true; - } - - return success; + return d->tag->save(); } bool MP4::File::hasMP4Tag() const { - return d->hasMP4Tag; + return (d->atoms->find("moov", "udta", "meta", "ilst") != 0); } diff --git a/taglib/mp4/mp4file.h b/taglib/mp4/mp4file.h index f9a8502f..0475d42d 100644 --- a/taglib/mp4/mp4file.h +++ b/taglib/mp4/mp4file.h @@ -95,9 +95,6 @@ namespace TagLib { * Save 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. */ bool save(); diff --git a/taglib/mp4/mp4tag.cpp b/taglib/mp4/mp4tag.cpp index 549f9f1a..52b7c73b 100644 --- a/taglib/mp4/mp4tag.cpp +++ b/taglib/mp4/mp4tag.cpp @@ -630,6 +630,11 @@ MP4::Tag::saveNew(ByteVector data) updateParents(path, data.size()); updateOffsets(data.size(), offset); + + // Insert the newly created atoms into the tree to keep it up-to-date. + + d->file->seek(offset); + path.back()->children.prepend(new Atom(d->file)); } void diff --git a/taglib/mpeg/mpegheader.cpp b/taglib/mpeg/mpegheader.cpp index 8260a4df..9efb456b 100644 --- a/taglib/mpeg/mpegheader.cpp +++ b/taglib/mpeg/mpegheader.cpp @@ -23,11 +23,11 @@ * http://www.mozilla.org/MPL/ * ***************************************************************************/ -#include +#include +#include +#include +#include -#include "tstring.h" -#include "tdebug.h" -#include "tsmartptr.h" #include "mpegheader.h" using namespace TagLib; @@ -164,39 +164,54 @@ MPEG::Header &MPEG::Header::operator=(const Header &h) void MPEG::Header::parse(const ByteVector &data) { - if(data.size() < 4 || static_cast(data[0]) != 0xff) { + if(data.size() < 4) { + debug("MPEG::Header::parse() -- data is too short for an MPEG frame header."); + return; + } + + // Check for the MPEG synch bytes. + + if(static_cast(data[0]) != 0xFF) { debug("MPEG::Header::parse() -- First byte did not match MPEG synch."); return; } - std::bitset<32> flags(TAGLIB_CONSTRUCT_BITSET(data.toUInt32BE(0))); - - // Check for the second byte's part of the MPEG synch - - if(!flags[23] || !flags[22] || !flags[21]) { + if((static_cast(data[1]) & 0xE0) != 0xE0) { debug("MPEG::Header::parse() -- Second byte did not match MPEG synch."); return; } // Set the MPEG version - if(!flags[20] && !flags[19]) + const int versionBits = (static_cast(data[1]) >> 3) & 0x03; + + if(versionBits == 0) d->data->version = Version2_5; - else if(flags[20] && !flags[19]) + else if(versionBits == 2) d->data->version = Version2; - else if(flags[20] && flags[19]) + else if(versionBits == 3) d->data->version = Version1; + else { + debug("MPEG::Header::parse() -- Invalid MPEG version bits."); + return; + } // Set the MPEG layer - if(!flags[18] && flags[17]) - d->data->layer = 3; - else if(flags[18] && !flags[17]) - d->data->layer = 2; - else if(flags[18] && flags[17]) - d->data->layer = 1; + const int layerBits = (static_cast(data[1]) >> 1) & 0x03; - d->data->protectionEnabled = !flags[16]; + if(layerBits == 1) + d->data->layer = 3; + else if(layerBits == 2) + d->data->layer = 2; + else if(layerBits == 3) + d->data->layer = 1; + else { + debug("MPEG::Header::parse() -- Invalid MPEG layer bits."); + return; + } + + d->data->protectionEnabled = (static_cast(data[1] & 0x01) == 0); // Set the bitrate @@ -219,9 +234,14 @@ void MPEG::Header::parse(const ByteVector &data) // The bitrate index is encoded as the first 4 bits of the 3rd byte, // i.e. 1111xxxx - int i = static_cast(data[2]) >> 4; + const int bitrateIndex = (static_cast(data[2]) >> 4) & 0x0F; - d->data->bitrate = bitrates[versionIndex][layerIndex][i]; + d->data->bitrate = bitrates[versionIndex][layerIndex][bitrateIndex]; + + if(d->data->bitrate == 0) { + debug("MPEG::Header::parse() -- Invalid bit rate."); + return; + } // Set the sample rate @@ -233,9 +253,9 @@ void MPEG::Header::parse(const ByteVector &data) // The sample rate index is encoded as two bits in the 3nd byte, i.e. xxxx11xx - i = static_cast(data[2]) >> 2 & 0x03; + const int samplerateIndex = (static_cast(data[2]) >> 2) & 0x03; - d->data->sampleRate = sampleRates[d->data->version][i]; + d->data->sampleRate = sampleRates[d->data->version][samplerateIndex]; if(d->data->sampleRate == 0) { debug("MPEG::Header::parse() -- Invalid sample rate."); @@ -245,13 +265,13 @@ void MPEG::Header::parse(const ByteVector &data) // The channel mode is encoded as a 2 bit value at the end of the 3nd byte, // i.e. xxxxxx11 - d->data->channelMode = static_cast((static_cast(data[3]) & 0xC0) >> 6); + d->data->channelMode = static_cast((static_cast(data[3]) >> 6) & 0x03); // TODO: Add mode extension for completeness - d->data->isOriginal = flags[2]; - d->data->isCopyrighted = flags[3]; - d->data->isPadded = flags[9]; + d->data->isOriginal = ((static_cast(data[3]) & 0x04) != 0); + d->data->isCopyrighted = ((static_cast(data[3]) & 0x08) != 0); + d->data->isPadded = ((static_cast(data[2]) & 0x02) != 0); // Samples per frame diff --git a/taglib/mpeg/mpegproperties.cpp b/taglib/mpeg/mpegproperties.cpp index da22cfa3..c58229af 100644 --- a/taglib/mpeg/mpegproperties.cpp +++ b/taglib/mpeg/mpegproperties.cpp @@ -150,27 +150,33 @@ bool MPEG::AudioProperties::isOriginal() const void MPEG::AudioProperties::read(File *file) { - // Only the first frame is required if we have a VBR header. + // Only the first valid frame is required if we have a VBR header. - const long long first = file->firstFrameOffset(); - if(first < 0) { - debug("MPEG::AudioProperties::read() -- Could not find a valid first MPEG frame in the stream."); + long long firstFrameOffset = file->firstFrameOffset(); + if(firstFrameOffset < 0) { + debug("MPEG::Properties::read() -- Could not find an MPEG frame in the stream."); return; } - file->seek(first); - const Header firstHeader(file->readBlock(4)); + file->seek(firstFrameOffset); + Header firstHeader(file->readBlock(4)); - if(!firstHeader.isValid()) { - debug("MPEG::AudioProperties::read() -- The first page header is invalid."); - return; + while(!firstHeader.isValid()) { + firstFrameOffset = file->nextFrameOffset(firstFrameOffset + 1); + if(firstFrameOffset < 0) { + debug("MPEG::Properties::read() -- Could not find a valid first MPEG frame in the stream."); + return; + } + + file->seek(firstFrameOffset); + firstHeader = Header(file->readBlock(4)); } // Check for a VBR header that will help us in gathering information about a // VBR stream. - file->seek(first + 4); - d->xingHeader.reset(new XingHeader(file->readBlock(firstHeader.frameLength() - 4))); + file->seek(firstFrameOffset); + d->xingHeader.reset(new XingHeader(file->readBlock(firstHeader.frameLength()))); if(!d->xingHeader->isValid()) d->xingHeader.reset(); @@ -194,7 +200,7 @@ void MPEG::AudioProperties::read(File *file) d->bitrate = firstHeader.bitrate(); - long long streamLength = file->length() - first; + long long streamLength = file->length() - firstFrameOffset; if(file->hasID3v1Tag()) streamLength -= 128; diff --git a/taglib/ogg/xiphcomment.cpp b/taglib/ogg/xiphcomment.cpp index bf6e5be2..a3ee83e8 100644 --- a/taglib/ogg/xiphcomment.cpp +++ b/taglib/ogg/xiphcomment.cpp @@ -328,7 +328,7 @@ void Ogg::XiphComment::removePicture(FLAC::Picture *picture, bool del) delete picture; } -void Ogg::XiphComment::removePictures() +void Ogg::XiphComment::removeAllPictures() { d->pictureList.clear(); } diff --git a/taglib/ogg/xiphcomment.h b/taglib/ogg/xiphcomment.h index a9fcb165..8b433b01 100644 --- a/taglib/ogg/xiphcomment.h +++ b/taglib/ogg/xiphcomment.h @@ -234,7 +234,7 @@ namespace TagLib { /*! * Remove all pictures. */ - void removePictures(); + void removeAllPictures(); /*! * Add a new picture to the comment block. The comment block takes ownership of the diff --git a/taglib/riff/rifffile.cpp b/taglib/riff/rifffile.cpp index 513ae169..51a8a5ab 100644 --- a/taglib/riff/rifffile.cpp +++ b/taglib/riff/rifffile.cpp @@ -54,10 +54,8 @@ public: size(0) {} const ByteOrder endianness; - ByteVector type; - unsigned int size; - ByteVector format; + unsigned int size; std::vector chunks; }; @@ -248,14 +246,16 @@ void RIFF::File::removeChunk(const ByteVector &name) void RIFF::File::read() { - d->type = readBlock(4); + const long long baseOffset = tell(); + + seek(baseOffset + 4); if(d->endianness == BigEndian) d->size = readBlock(4).toUInt32BE(0); else d->size = readBlock(4).toUInt32LE(0); - d->format = readBlock(4); + seek(baseOffset + 12); // + 8: chunk header at least, fix for additional junk bytes while(tell() + 8 <= length()) { diff --git a/taglib/riff/riffutils.h b/taglib/riff/riffutils.h index 42977efe..14b8508e 100644 --- a/taglib/riff/riffutils.h +++ b/taglib/riff/riffutils.h @@ -34,7 +34,7 @@ namespace TagLib { namespace RIFF { - static bool isValidChunkName(const ByteVector &name) + inline bool isValidChunkName(const ByteVector &name) { if(name.size() != 4) return false; diff --git a/tests/data/invalid-frames.mp3 b/tests/data/invalid-frames.mp3 new file mode 100644 index 00000000..a6dc1117 Binary files /dev/null and b/tests/data/invalid-frames.mp3 differ diff --git a/tests/test_flac.cpp b/tests/test_flac.cpp index 4162f222..e4a2c559 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(5735LL, f.length()); + f.save(); + CPPUNIT_ASSERT_EQUAL(5735LL, 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(12862LL, f.length()); + f.save(); + CPPUNIT_ASSERT_EQUAL(12862LL, 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); diff --git a/tests/test_mp4.cpp b/tests/test_mp4.cpp index 0e12c2e1..3c7ebc56 100644 --- a/tests/test_mp4.cpp +++ b/tests/test_mp4.cpp @@ -31,6 +31,7 @@ class TestMP4 : public CppUnit::TestFixture CPPUNIT_TEST(testCovrRead2); CPPUNIT_TEST(testProperties); CPPUNIT_TEST(testFuzzedFile); + CPPUNIT_TEST(testRepeatedSave); CPPUNIT_TEST_SUITE_END(); public: @@ -358,6 +359,17 @@ public: CPPUNIT_ASSERT(f.isValid()); } + void testRepeatedSave() + { + ScopedFileCopy copy("no-tags", ".m4a"); + + MP4::File f(copy.fileName().c_str()); + f.tag()->setTitle("0123456789"); + f.save(); + f.save(); + CPPUNIT_ASSERT_EQUAL(2862LL, f.find("0123456789")); + CPPUNIT_ASSERT_EQUAL(-1LL, f.find("0123456789", 2863)); + } }; CPPUNIT_TEST_SUITE_REGISTRATION(TestMP4); diff --git a/tests/test_mpeg.cpp b/tests/test_mpeg.cpp index 9daf391c..522c0d4f 100644 --- a/tests/test_mpeg.cpp +++ b/tests/test_mpeg.cpp @@ -22,6 +22,7 @@ class TestMPEG : public CppUnit::TestFixture CPPUNIT_TEST(testAudioPropertiesXingHeaderVBR); CPPUNIT_TEST(testAudioPropertiesVBRIHeader); CPPUNIT_TEST(testAudioPropertiesNoVBRHeaders); + CPPUNIT_TEST(testSkipInvalidFrames); CPPUNIT_TEST(testVersion2DurationWithXingHeader); CPPUNIT_TEST(testSaveID3v24); CPPUNIT_TEST(testSaveID3v24WrongParam); @@ -102,6 +103,19 @@ public: CPPUNIT_ASSERT_EQUAL(209, lastHeader.frameLength()); } + void testSkipInvalidFrames() + { + MPEG::File f(TEST_FILE_PATH_C("invalid-frames.mp3")); + CPPUNIT_ASSERT(f.audioProperties()); + CPPUNIT_ASSERT_EQUAL(0, f.audioProperties()->length()); + CPPUNIT_ASSERT_EQUAL(0, f.audioProperties()->lengthInSeconds()); + CPPUNIT_ASSERT_EQUAL(393, f.audioProperties()->lengthInMilliseconds()); + CPPUNIT_ASSERT_EQUAL(160, f.audioProperties()->bitrate()); + CPPUNIT_ASSERT_EQUAL(2, f.audioProperties()->channels()); + CPPUNIT_ASSERT_EQUAL(44100, f.audioProperties()->sampleRate()); + CPPUNIT_ASSERT(!f.audioProperties()->xingHeader()); + } + void testVersion2DurationWithXingHeader() { MPEG::File f(TEST_FILE_PATH_C("mpeg2.mp3"));