From b7a514886dcfbe879f4c0d4b5ad2b4fa3415012f Mon Sep 17 00:00:00 2001 From: Didier Malenfant Date: Sat, 23 Aug 2014 20:37:32 -0700 Subject: [PATCH 1/5] Ignoring files generated when creating an Xcode project via cmake. --- .gitignore | 2 ++ 1 file changed, 2 insertions(+) diff --git a/.gitignore b/.gitignore index 70912fde..941063c5 100644 --- a/.gitignore +++ b/.gitignore @@ -42,3 +42,5 @@ CMakeFiles/ /taglib/tag.dir/Release /ALL_BUILD.dir /ZERO_CHECK.dir +taglib.xcodeproj +CMakeScripts From 205569c8d271c9446285bdf0caf92d8255bc60bc Mon Sep 17 00:00:00 2001 From: "Uwe L. Korn" Date: Sun, 14 Sep 2014 18:03:27 +0100 Subject: [PATCH 2/5] Fix various memleaks in the tests --- tests/test_aiff.cpp | 1 + tests/test_fileref.cpp | 2 ++ tests/test_flac.cpp | 2 ++ tests/test_id3v2.cpp | 2 ++ tests/test_mp4.cpp | 3 +++ tests/test_riff.cpp | 2 ++ tests/utils.h | 5 ++++- 7 files changed, 16 insertions(+), 1 deletion(-) diff --git a/tests/test_aiff.cpp b/tests/test_aiff.cpp index df1c5ac1..00a12088 100644 --- a/tests/test_aiff.cpp +++ b/tests/test_aiff.cpp @@ -24,6 +24,7 @@ public: RIFF::AIFF::File *f = new RIFF::AIFF::File(filename.c_str()); CPPUNIT_ASSERT_EQUAL(705, f->audioProperties()->bitrate()); + delete f; } }; diff --git a/tests/test_fileref.cpp b/tests/test_fileref.cpp index 197a9213..f13eafaa 100644 --- a/tests/test_fileref.cpp +++ b/tests/test_fileref.cpp @@ -136,6 +136,7 @@ public: FileRef *f = new FileRef(TEST_FILE_PATH_C("empty_flac.oga")); CPPUNIT_ASSERT(dynamic_cast(f->file()) == NULL); CPPUNIT_ASSERT(dynamic_cast(f->file()) != NULL); + delete f; } void testOGA_Vorbis() @@ -143,6 +144,7 @@ public: FileRef *f = new FileRef(TEST_FILE_PATH_C("empty_vorbis.oga")); CPPUNIT_ASSERT(dynamic_cast(f->file()) != NULL); CPPUNIT_ASSERT(dynamic_cast(f->file()) == NULL); + delete f; } void testAPE() diff --git a/tests/test_flac.cpp b/tests/test_flac.cpp index 22bbc854..f99a679a 100644 --- a/tests/test_flac.cpp +++ b/tests/test_flac.cpp @@ -69,6 +69,8 @@ public: CPPUNIT_ASSERT_EQUAL(String("image/png"), pic->mimeType()); CPPUNIT_ASSERT_EQUAL(String("A pixel."), pic->description()); CPPUNIT_ASSERT_EQUAL(TagLib::uint(150), pic->data().size()); + + delete f; } void testAddPicture() diff --git a/tests/test_id3v2.cpp b/tests/test_id3v2.cpp index a77e46af..1f080a1a 100644 --- a/tests/test_id3v2.cpp +++ b/tests/test_id3v2.cpp @@ -207,6 +207,8 @@ public: CPPUNIT_ASSERT_EQUAL(String("image/jpeg"), frame->mimeType()); CPPUNIT_ASSERT_EQUAL(ID3v2::AttachedPictureFrame::FileIcon, frame->type()); CPPUNIT_ASSERT_EQUAL(String("d"), frame->description()); + + delete frame; } void testDontRender22() diff --git a/tests/test_mp4.cpp b/tests/test_mp4.cpp index 9b90eed3..7e25f996 100644 --- a/tests/test_mp4.cpp +++ b/tests/test_mp4.cpp @@ -142,6 +142,7 @@ public: CPPUNIT_ASSERT_EQUAL(String("82,164"), f->tag()->itemListMap()["----:com.apple.iTunes:replaygain_track_minmax"].toStringList()[0]); CPPUNIT_ASSERT_EQUAL(String("Pearl Jam"), f->tag()->artist()); CPPUNIT_ASSERT_EQUAL(String("foo"), f->tag()->comment()); + delete f; } void test64BitAtom() @@ -158,6 +159,7 @@ public: f->tag()->itemListMap()["pgap"] = true; f->save(); + delete atoms; delete f; f = new MP4::File(filename.c_str()); @@ -168,6 +170,7 @@ public: moov = atoms->atoms[0]; // original size + 'pgap' size + padding CPPUNIT_ASSERT_EQUAL(long(77 + 25 + 974), moov->length); + delete atoms; delete f; } diff --git a/tests/test_riff.cpp b/tests/test_riff.cpp index 9b07a7ce..19bff169 100644 --- a/tests/test_riff.cpp +++ b/tests/test_riff.cpp @@ -86,6 +86,8 @@ public: CPPUNIT_ASSERT_EQUAL(ByteVector("TEST"), f->chunkName(2)); CPPUNIT_ASSERT_EQUAL(ByteVector("foo"), f->chunkData(2)); + + delete f; } void testLastChunkAtEvenPosition() diff --git a/tests/utils.h b/tests/utils.h index 5f2f9814..dfe6c4c3 100644 --- a/tests/utils.h +++ b/tests/utils.h @@ -11,6 +11,7 @@ #include #endif #include +#include #include #include #include @@ -26,7 +27,9 @@ inline string testFilePath(const string &filename) inline string copyFile(const string &filename, const string &ext) { - string newname = string(tempnam(NULL, NULL)) + ext; + char *newname_c = tempnam(NULL, NULL); + string newname = string(newname_c) + ext; + free(newname_c); string oldname = testFilePath(filename) + ext; #ifdef _WIN32 CopyFileA(oldname.c_str(), newname.c_str(), FALSE); From e6d7dd08f2fb37c5cf843588b3ba762494dcdb63 Mon Sep 17 00:00:00 2001 From: Scott Wheeler Date: Thu, 18 Sep 2014 10:23:42 +0200 Subject: [PATCH 3/5] No reason to store this in the d-pointer --- taglib/mpeg/id3v2/frames/chapterframe.cpp | 6 +----- taglib/mpeg/id3v2/frames/tableofcontentsframe.cpp | 6 +----- 2 files changed, 2 insertions(+), 10 deletions(-) diff --git a/taglib/mpeg/id3v2/frames/chapterframe.cpp b/taglib/mpeg/id3v2/frames/chapterframe.cpp index 3180e1e0..6bf70504 100644 --- a/taglib/mpeg/id3v2/frames/chapterframe.cpp +++ b/taglib/mpeg/id3v2/frames/chapterframe.cpp @@ -41,7 +41,6 @@ public: uint endTime; uint startOffset; uint endOffset; - const FrameFactory *factory; FrameListMap embeddedFrameListMap; FrameList embeddedFrameList; }; @@ -54,7 +53,6 @@ ChapterFrame::ChapterFrame(const ByteVector &data) : ID3v2::Frame(data) { d = new ChapterFramePrivate; - d->factory = FrameFactory::instance(); setData(data); } @@ -70,7 +68,6 @@ ChapterFrame::ChapterFrame(const ByteVector &eID, const uint &sT, const uint &eT FrameList l = eF; for(FrameList::ConstIterator it = l.begin(); it != l.end(); ++it) addEmbeddedFrame(*it); - d->factory = FrameFactory::instance(); } ChapterFrame::~ChapterFrame() @@ -225,7 +222,7 @@ void ChapterFrame::parseFields(const ByteVector &data) size -= pos; while((uint)embPos < size - Frame::headerSize(4)) { - Frame *frame = d->factory->createFrame(data.mid(pos + embPos)); + Frame *frame = FrameFactory::instance()->createFrame(data.mid(pos + embPos)); if(!frame) return; @@ -261,6 +258,5 @@ ChapterFrame::ChapterFrame(const ByteVector &data, Header *h) : Frame(h) { d = new ChapterFramePrivate; - d->factory = FrameFactory::instance(); parseFields(fieldData(data)); } diff --git a/taglib/mpeg/id3v2/frames/tableofcontentsframe.cpp b/taglib/mpeg/id3v2/frames/tableofcontentsframe.cpp index 9ba00227..72377e0a 100644 --- a/taglib/mpeg/id3v2/frames/tableofcontentsframe.cpp +++ b/taglib/mpeg/id3v2/frames/tableofcontentsframe.cpp @@ -39,7 +39,6 @@ public: bool isTopLevel; bool isOrdered; ByteVectorList childElements; - const FrameFactory *factory; FrameListMap embeddedFrameListMap; FrameList embeddedFrameList; }; @@ -52,7 +51,6 @@ TableOfContentsFrame::TableOfContentsFrame(const ByteVector &data) : ID3v2::Frame(data) { d = new TableOfContentsFramePrivate; - d->factory = FrameFactory::instance(); setData(data); } @@ -65,7 +63,6 @@ TableOfContentsFrame::TableOfContentsFrame(const ByteVector &eID, const ByteVect FrameList l = eF; for(FrameList::ConstIterator it = l.begin(); it != l.end(); ++it) addEmbeddedFrame(*it); - d->factory = FrameFactory::instance(); } TableOfContentsFrame::~TableOfContentsFrame() @@ -244,7 +241,7 @@ void TableOfContentsFrame::parseFields(const ByteVector &data) size -= pos; while((uint)embPos < size - Frame::headerSize(4)) { - Frame *frame = d->factory->createFrame(data.mid(pos + embPos)); + Frame *frame = FrameFactory::instance()->createFrame(data.mid(pos + embPos)); if(!frame) return; @@ -288,6 +285,5 @@ TableOfContentsFrame::TableOfContentsFrame(const ByteVector &data, Header *h) : Frame(h) { d = new TableOfContentsFramePrivate; - d->factory = FrameFactory::instance(); parseFields(fieldData(data)); } From b6289c64dda2308e995ff2820334fd391ceb037b Mon Sep 17 00:00:00 2001 From: Scott Wheeler Date: Thu, 18 Sep 2014 11:16:20 +0200 Subject: [PATCH 4/5] Break up the mega-lines --- taglib/mpeg/id3v2/frames/chapterframe.cpp | 6 +- taglib/mpeg/id3v2/frames/chapterframe.h | 65 ++++++++++--------- .../id3v2/frames/tableofcontentsframe.cpp | 10 ++- 3 files changed, 44 insertions(+), 37 deletions(-) diff --git a/taglib/mpeg/id3v2/frames/chapterframe.cpp b/taglib/mpeg/id3v2/frames/chapterframe.cpp index 6bf70504..136a855a 100644 --- a/taglib/mpeg/id3v2/frames/chapterframe.cpp +++ b/taglib/mpeg/id3v2/frames/chapterframe.cpp @@ -56,7 +56,8 @@ ChapterFrame::ChapterFrame(const ByteVector &data) : setData(data); } -ChapterFrame::ChapterFrame(const ByteVector &eID, const uint &sT, const uint &eT, const uint &sO, const uint &eO, const FrameList &eF) : +ChapterFrame::ChapterFrame(const ByteVector &eID, const uint &sT, const uint &eT, + const uint &sO, const uint &eO, const FrameList &eF) : ID3v2::Frame("CHAP") { d = new ChapterFramePrivate; @@ -204,7 +205,8 @@ void ChapterFrame::parseFields(const ByteVector &data) { uint size = data.size(); if(size < 18) { - debug("A CHAP frame must contain at least 18 bytes (1 byte element ID terminated by null and 4x4 bytes for start and end time and offset)."); + debug("A CHAP frame must contain at least 18 bytes (1 byte element ID " + "terminated by null and 4x4 bytes for start and end time and offset)."); return; } diff --git a/taglib/mpeg/id3v2/frames/chapterframe.h b/taglib/mpeg/id3v2/frames/chapterframe.h index 84b42137..96827e1f 100644 --- a/taglib/mpeg/id3v2/frames/chapterframe.h +++ b/taglib/mpeg/id3v2/frames/chapterframe.h @@ -35,7 +35,7 @@ namespace TagLib { namespace ID3v2 { /*! - * This is an implementation of ID3v2 chapter frames. The purpose of this + * This is an implementation of ID3v2 chapter frames. The purpose of this * frame is to describe a single chapter within an audio file. */ @@ -56,48 +56,49 @@ namespace TagLib { * start time \a sT, end time \a eT, start offset \a sO, * end offset \a eO and embedded frames, that are in \a eF. */ - ChapterFrame(const ByteVector &eID, const uint &sT, const uint &eT, const uint &sO, const uint &eO, const FrameList &eF); + ChapterFrame(const ByteVector &eID, const uint &sT, const uint &eT, const uint &sO, + const uint &eO, const FrameList &eF); /*! * Destroys the frame. */ ~ChapterFrame(); - + /*! * Returns the element ID of the frame. Element ID * is a null terminated string, however it's not human-readable. - * + * * \see setElementID() */ ByteVector elementID() const; - + /*! * Returns time of chapter's start (in miliseconds). - * + * * \see setStartTime() */ uint startTime() const; - + /*! * Returns time of chapter's end (in miliseconds). - * + * * \see setEndTime() */ uint endTime() const; - + /*! * Returns zero based byte offset (count of bytes from the beginning * of the audio file) of chapter's start. - * + * * \note If returned value is 0xFFFFFFFF, start time should be used instead. * \see setStartOffset() */ uint startOffset() const; - + /*! * Returns zero based byte offset (count of bytes from the beginning * of the audio file) of chapter's end. - * + * * \note If returned value is 0xFFFFFFFF, end time should be used instead. * \see setEndOffset() */ @@ -106,48 +107,48 @@ namespace TagLib { /*! * Sets the element ID of the frame to \a eID. If \a eID isn't * null terminated, a null char is appended automatically. - * + * * \see elementID() */ void setElementID(const ByteVector &eID); - + /*! * Sets time of chapter's start (in miliseconds) to \a sT. - * + * * \see startTime() */ void setStartTime(const uint &sT); - + /*! * Sets time of chapter's end (in miliseconds) to \a eT. - * + * * \see endTime() */ void setEndTime(const uint &eT); - + /*! * Sets zero based byte offset (count of bytes from the beginning * of the audio file) of chapter's start to \a sO. - * + * * \see startOffset() */ void setStartOffset(const uint &sO); - + /*! * Sets zero based byte offset (count of bytes from the beginning * of the audio file) of chapter's end to \a eO. - * + * * \see endOffset() */ void setEndOffset(const uint &eO); - + /*! * Returns a reference to the frame list map. This is an FrameListMap of * all of the frames embedded in the CHAP frame. * - * This is the most convenient structure for accessing the CHAP frame's - * embedded frames. Many frame types allow multiple instances of the same - * frame type so this is a map of lists. In most cases however there will + * This is the most convenient structure for accessing the CHAP frame's + * embedded frames. Many frame types allow multiple instances of the same + * frame type so this is a map of lists. In most cases however there will * only be a single frame of a certain type. * * \warning You should not modify this data structure directly, instead @@ -156,13 +157,13 @@ namespace TagLib { * \see embeddedFrameList() */ const FrameListMap &embeddedFrameListMap() const; - + /*! - * Returns a reference to the embedded frame list. This is an FrameList + * Returns a reference to the embedded frame list. This is an FrameList * of all of the frames embedded in the CHAP frame in the order that they * were parsed. * - * This can be useful if for example you want iterate over the CHAP frame's + * This can be useful if for example you want iterate over the CHAP frame's * embedded frames in the order that they occur in the CHAP frame. * * \warning You should not modify this data structure directly, instead @@ -171,7 +172,7 @@ namespace TagLib { const FrameList &embeddedFrameList() const; /*! - * Returns the embedded frame list for frames with the id \a frameID + * Returns the embedded frame list for frames with the id \a frameID * or an empty list if there are no embedded frames of that type. This * is just a convenience and is equivalent to: * @@ -184,7 +185,7 @@ namespace TagLib { const FrameList &embeddedFrameList(const ByteVector &frameID) const; /*! - * Add an embedded frame to the CHAP frame. At this point the CHAP frame + * Add an embedded frame to the CHAP frame. At this point the CHAP frame * takes ownership of the embedded frame and will handle freeing its memory. * * \note Using this method will invalidate any pointers on the list @@ -193,7 +194,7 @@ namespace TagLib { void addEmbeddedFrame(Frame *frame); /*! - * Remove an embedded frame from the CHAP frame. If \a del is true the frame's + * Remove an embedded frame from the CHAP frame. If \a del is true the frame's * memory will be freed; if it is false, it must be deleted by the user. * * \note Using this method will invalidate any pointers on the list @@ -202,7 +203,7 @@ namespace TagLib { void removeEmbeddedFrame(Frame *frame, bool del = true); /*! - * Remove all embedded frames of type \a id from the CHAP frame and free their + * Remove all embedded frames of type \a id from the CHAP frame and free their * memory. * * \note Using this method will invalidate any pointers on the list diff --git a/taglib/mpeg/id3v2/frames/tableofcontentsframe.cpp b/taglib/mpeg/id3v2/frames/tableofcontentsframe.cpp index 72377e0a..f70edd69 100644 --- a/taglib/mpeg/id3v2/frames/tableofcontentsframe.cpp +++ b/taglib/mpeg/id3v2/frames/tableofcontentsframe.cpp @@ -54,7 +54,8 @@ TableOfContentsFrame::TableOfContentsFrame(const ByteVector &data) : setData(data); } -TableOfContentsFrame::TableOfContentsFrame(const ByteVector &eID, const ByteVectorList &ch, const FrameList &eF) : +TableOfContentsFrame::TableOfContentsFrame(const ByteVector &eID, const ByteVectorList &ch, + const FrameList &eF) : ID3v2::Frame("CTOC") { d = new TableOfContentsFramePrivate; @@ -185,7 +186,8 @@ PropertyMap TableOfContentsFrame::asProperties() const return map; } -TableOfContentsFrame *TableOfContentsFrame::findByElementID(const ID3v2::Tag *tag, const ByteVector &eID) // static +TableOfContentsFrame *TableOfContentsFrame::findByElementID(const ID3v2::Tag *tag, + const ByteVector &eID) // static { ID3v2::FrameList tablesOfContents = tag->frameList("CTOC"); @@ -221,7 +223,9 @@ void TableOfContentsFrame::parseFields(const ByteVector &data) { uint size = data.size(); if(size < 6) { - debug("A CTOC frame must contain at least 6 bytes (1 byte element ID terminated by null, 1 byte flags, 1 byte entry count and 1 byte child element ID terminated by null."); + debug("A CTOC frame must contain at least 6 bytes (1 byte element ID terminated by " + "null, 1 byte flags, 1 byte entry count and 1 byte child element ID terminated " + "by null."); return; } From 82315276db1fd4744e5d6c97debd1d50a5f9574f Mon Sep 17 00:00:00 2001 From: Scott Wheeler Date: Thu, 18 Sep 2014 16:23:28 +0200 Subject: [PATCH 5/5] Take ownership of embedded frames, as documented Previously embedded frames that were created automatically were never deleted. Fixes #440 --- taglib/mpeg/id3v2/frames/chapterframe.cpp | 5 ++++ .../id3v2/frames/tableofcontentsframe.cpp | 5 ++++ tests/test_id3v2.cpp | 30 +++++++++---------- 3 files changed, 25 insertions(+), 15 deletions(-) diff --git a/taglib/mpeg/id3v2/frames/chapterframe.cpp b/taglib/mpeg/id3v2/frames/chapterframe.cpp index 136a855a..c8650f9d 100644 --- a/taglib/mpeg/id3v2/frames/chapterframe.cpp +++ b/taglib/mpeg/id3v2/frames/chapterframe.cpp @@ -36,6 +36,11 @@ using namespace ID3v2; class ChapterFrame::ChapterFramePrivate { public: + ChapterFramePrivate() + { + embeddedFrameList.setAutoDelete(true); + } + ByteVector elementID; uint startTime; uint endTime; diff --git a/taglib/mpeg/id3v2/frames/tableofcontentsframe.cpp b/taglib/mpeg/id3v2/frames/tableofcontentsframe.cpp index f70edd69..4698b7ec 100644 --- a/taglib/mpeg/id3v2/frames/tableofcontentsframe.cpp +++ b/taglib/mpeg/id3v2/frames/tableofcontentsframe.cpp @@ -35,6 +35,11 @@ using namespace ID3v2; class TableOfContentsFrame::TableOfContentsFramePrivate { public: + TableOfContentsFramePrivate() + { + embeddedFrameList.setAutoDelete(true); + } + ByteVector elementID; bool isTopLevel; bool isOrdered; diff --git a/tests/test_id3v2.cpp b/tests/test_id3v2.cpp index 1f080a1a..42bfaa38 100644 --- a/tests/test_id3v2.cpp +++ b/tests/test_id3v2.cpp @@ -885,7 +885,7 @@ public: MPEG::File f(newname.c_str()); CPPUNIT_ASSERT(!f.ID3v2Tag()->frameListMap().contains("TPE1")); } - + void testParseChapterFrame() { ID3v2::ChapterFrame f( @@ -901,7 +901,7 @@ public: "\x00\x00\x00\x04" // Embedded frame size "\x00\x00" // Embedded frame flags "\x00" // TIT2 frame text encoding - "CH1", 42)); // Chapter title + "CH1", 42)); // Chapter title CPPUNIT_ASSERT_EQUAL(ByteVector("\x43\x00", 2), f.elementID()); CPPUNIT_ASSERT((uint)0x03 == f.startTime()); @@ -912,7 +912,7 @@ public: CPPUNIT_ASSERT(f.embeddedFrameList("TIT2").size() == 1); CPPUNIT_ASSERT(f.embeddedFrameList("TIT2")[0]->toString() == "CH1"); } - + void testRenderChapterFrame() { ID3v2::ChapterFrame f("CHAP"); @@ -921,9 +921,9 @@ public: f.setEndTime(5); f.setStartOffset(2); f.setEndOffset(3); - ID3v2::TextIdentificationFrame eF("TIT2"); - eF.setText("CH1"); - f.addEmbeddedFrame(&eF); + ID3v2::TextIdentificationFrame *eF = new ID3v2::TextIdentificationFrame("TIT2"); + eF->setText("CH1"); + f.addEmbeddedFrame(eF); CPPUNIT_ASSERT_EQUAL( ByteVector("CHAP" // Frame ID "\x00\x00\x00\x20" // Frame size @@ -937,10 +937,10 @@ public: "\x00\x00\x00\x04" // Embedded frame size "\x00\x00" // Embedded frame flags "\x00" // TIT2 frame text encoding - "CH1", 42), // Chapter title + "CH1", 42), // Chapter title f.render()); } - + void testParseTableOfContentsFrame() { ID3v2::TableOfContentsFrame f( @@ -956,7 +956,7 @@ public: "\x00\x00\x00\x04" // Embedded frame size "\x00\x00" // Embedded frame flags "\x00" // TIT2 frame text encoding - "TC1", 32)); // Table of contents title + "TC1", 32)); // Table of contents title CPPUNIT_ASSERT_EQUAL(ByteVector("\x54\x00", 2), f.elementID()); CPPUNIT_ASSERT(!f.isTopLevel()); @@ -970,7 +970,7 @@ public: CPPUNIT_ASSERT(f.embeddedFrameList("TIT2").size() == 1); CPPUNIT_ASSERT(f.embeddedFrameList("TIT2")[0]->toString() == "TC1"); } - + void testRenderTableOfContentsFrame() { ID3v2::TableOfContentsFrame f("CTOC"); @@ -979,9 +979,9 @@ public: f.setIsOrdered(true); f.addChildElement(ByteVector("\x43\x00", 2)); f.addChildElement(ByteVector("\x44\x00", 2)); - ID3v2::TextIdentificationFrame eF("TIT2"); - eF.setText("TC1"); - f.addEmbeddedFrame(&eF); + ID3v2::TextIdentificationFrame *eF = new ID3v2::TextIdentificationFrame("TIT2"); + eF->setText("TC1"); + f.addEmbeddedFrame(eF); CPPUNIT_ASSERT_EQUAL( ByteVector("CTOC" // Frame ID "\x00\x00\x00\x16" // Frame size @@ -995,10 +995,10 @@ public: "\x00\x00\x00\x04" // Embedded frame size "\x00\x00" // Embedded frame flags "\x00" // TIT2 frame text encoding - "TC1", 32), // Table of contents title + "TC1", 32), // Table of contents title f.render()); } - + }; CPPUNIT_TEST_SUITE_REGISTRATION(TestID3v2);