From 497c040f04b888ef235c7cae0d59964c2a62bca6 Mon Sep 17 00:00:00 2001 From: Urs Fleisch Date: Sat, 25 Apr 2026 11:46:51 +0200 Subject: [PATCH] Set MP4 chapters only if modified An equality operator is added for the chapters. The chapters are only written to the file if they were really modified, so just reading the chapters without modifying them will not affect the save operation. --- taglib/CMakeLists.txt | 1 - taglib/mp4/mp4chapter.cpp | 10 +++ taglib/mp4/mp4chapter.h | 10 +++ taglib/mp4/mp4chapterholder.cpp | 44 ---------- taglib/mp4/mp4chapterholder.h | 26 ++++-- tests/test_mp4.cpp | 151 ++++++++++++++++++++++++++++++++ 6 files changed, 192 insertions(+), 50 deletions(-) delete mode 100644 taglib/mp4/mp4chapterholder.cpp diff --git a/taglib/CMakeLists.txt b/taglib/CMakeLists.txt index aa304fb3..4c8e5332 100644 --- a/taglib/CMakeLists.txt +++ b/taglib/CMakeLists.txt @@ -377,7 +377,6 @@ if(WITH_MP4) mp4/mp4stem.cpp mp4/mp4itemfactory.cpp mp4/mp4chapter.cpp - mp4/mp4chapterholder.cpp mp4/mp4nerochapterlist.cpp mp4/mp4qtchapterlist.cpp ) diff --git a/taglib/mp4/mp4chapter.cpp b/taglib/mp4/mp4chapter.cpp index baa1a26c..2222748f 100644 --- a/taglib/mp4/mp4chapter.cpp +++ b/taglib/mp4/mp4chapter.cpp @@ -61,6 +61,16 @@ MP4::Chapter &MP4::Chapter::Chapter::operator=(const Chapter &other) MP4::Chapter &MP4::Chapter::Chapter::operator=( Chapter &&other) noexcept = default; +bool MP4::Chapter::operator==(const Chapter &other) const +{ + return title() == other.title() && startTime() == other.startTime(); +} + +bool MP4::Chapter::operator!=(const Chapter &other) const +{ + return !(*this == other); +} + void MP4::Chapter::swap(Chapter &other) noexcept { using std::swap; diff --git a/taglib/mp4/mp4chapter.h b/taglib/mp4/mp4chapter.h index 91627bb2..839f75d8 100644 --- a/taglib/mp4/mp4chapter.h +++ b/taglib/mp4/mp4chapter.h @@ -68,6 +68,16 @@ namespace TagLib { */ Chapter &operator=(Chapter &&other) noexcept; + /*! + * Returns \c true if the chapter and \a other contain the same data. + */ + bool operator==(const Chapter &other) const; + + /*! + * Returns \c true if the chapter and \a other differ in data. + */ + bool operator!=(const Chapter &other) const; + /*! * Exchanges the content of the object with the content of \a other. */ diff --git a/taglib/mp4/mp4chapterholder.cpp b/taglib/mp4/mp4chapterholder.cpp deleted file mode 100644 index 8d9c1ddb..00000000 --- a/taglib/mp4/mp4chapterholder.cpp +++ /dev/null @@ -1,44 +0,0 @@ -/************************************************************************** - copyright : (C) 2006 by Urs Fleisch - email : ufleisch@users.sourceforge.net - **************************************************************************/ - -/*************************************************************************** - * This library is free software; you can redistribute it and/or modify * - * it under the terms of the GNU Lesser General Public License version * - * 2.1 as published by the Free Software Foundation. * - * * - * This library is distributed in the hope that it will be useful, but * - * WITHOUT ANY WARRANTY; without even the implied warranty of * - * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU * - * Lesser General Public License for more details. * - * * - * You should have received a copy of the GNU Lesser General Public * - * License along with this library; if not, write to the Free Software * - * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA * - * 02110-1301 USA * - * * - * Alternatively, this file is available under the Mozilla Public * - * License Version 1.1. You may obtain a copy of the License at * - * http://www.mozilla.org/MPL/ * - ***************************************************************************/ - -#include "mp4chapterholder.h" - -using namespace TagLib; - -MP4::ChapterList MP4::ChapterHolder::chapters() const -{ - return chapterList; -} - -void MP4::ChapterHolder::setChapters(const ChapterList &chapters) -{ - chapterList = chapters; - modified = true; -} - -bool MP4::ChapterHolder::isModified() const -{ - return modified; -} diff --git a/taglib/mp4/mp4chapterholder.h b/taglib/mp4/mp4chapterholder.h index 94e8a52f..72752ba4 100644 --- a/taglib/mp4/mp4chapterholder.h +++ b/taglib/mp4/mp4chapterholder.h @@ -39,17 +39,22 @@ namespace TagLib { /*! * Get list of chapters. */ - ChapterList chapters() const; + ChapterList chapters() const { return chapterList; } /*! * Set list of chapters. */ - void setChapters(const ChapterList &chapters); + void setChapters(const ChapterList &chapters) { chapterList = chapters; } /*! * Returns \c true if the list of chapters has been modified. */ - bool isModified() const; + bool isModified() const { return modified; } + + /*! + * Set if the contained chapters are modified. + */ + void setModified(bool chaptersModified) { modified = chaptersModified; } protected: ChapterList chapterList; @@ -84,8 +89,15 @@ namespace TagLib { { if (!holder) { holder = std::make_unique(); + // The chapters have not been read before, so we do not know their + // current state and mark them as modified. Otherwise, the check below + // would not set the chapters if they are empty. + holder->setModified(true); + } + if(holder->isModified() || holder->chapters() != chapters) { + holder->setChapters(chapters); + holder->setModified(true); } - holder->setChapters(chapters); } /*! @@ -99,7 +111,11 @@ namespace TagLib { bool saveChaptersIfModified(std::unique_ptr &holder, TagLib::File *file) { if(holder && holder->isModified()) { - return holder->write(file); + if(holder->write(file)) { + holder->setModified(false); + return true; + } + return false; } return true; } diff --git a/tests/test_mp4.cpp b/tests/test_mp4.cpp index dc310385..d037a14f 100644 --- a/tests/test_mp4.cpp +++ b/tests/test_mp4.cpp @@ -34,6 +34,7 @@ #include "mp4atom.h" #include "mp4file.h" #include "mp4itemfactory.h" +#include "mp4chapterholder.h" #include "plainfile.h" #include #include "utils.h" @@ -69,6 +70,56 @@ namespace }; CustomItemFactory CustomItemFactory::factory; + + class MockChapterList : public MP4::ChapterHolder { + public: + static const MP4::ChapterList mockChapters; + + bool read(TagLib::File *) + { + chapterList = mockChapters; + ++readCount; + return true; + } + + bool write(TagLib::File *) + { + ++writeCount; + return true; + } + + int readCount = 0; + int writeCount = 0; + }; + + const MP4::ChapterList MockChapterList::mockChapters = { + MP4::Chapter("Mock", 123) + }; + + class MockChapterFile : public PlainFile { + public: + explicit MockChapterFile(FileName name) : PlainFile(name) + { + } + + MP4::ChapterList chapters() + { + return getChaptersLazy(chapterList, this); + } + + void setChapters(const MP4::ChapterList& chapters) + { + setChaptersLazy(chapterList, chapters); + } + + bool save() override + { + return MP4::saveChaptersIfModified(chapterList, this); + } + + std::unique_ptr chapterList; + }; + } // namespace class TestMP4 : public CppUnit::TestFixture @@ -121,6 +172,7 @@ class TestMP4 : public CppUnit::TestFixture CPPUNIT_TEST(testQTChapterListSingleEmptyTitleNotStripped); CPPUNIT_TEST(testNeroAndQTChaptersAreIndependent); CPPUNIT_TEST(testNeroChaptersAloneWhenNoQT); + CPPUNIT_TEST(testLazyReadingAndWritingChapters); CPPUNIT_TEST_SUITE_END(); public: @@ -1634,6 +1686,105 @@ public: } } + void testLazyReadingAndWritingChapters() + { + // No reads or writes if chapters are not used + { + MockChapterFile f(TEST_FILE_PATH_C("no-tags.m4a")); + f.save(); + CPPUNIT_ASSERT(!f.chapterList); + } + // Do not read if already read, do not write if not modified + { + MockChapterFile f(TEST_FILE_PATH_C("no-tags.m4a")); + auto chapters = f.chapters(); + CPPUNIT_ASSERT(chapters == MockChapterList::mockChapters); + CPPUNIT_ASSERT(f.chapterList); + CPPUNIT_ASSERT_EQUAL(1, f.chapterList->readCount); + chapters = f.chapters(); + CPPUNIT_ASSERT(chapters == MockChapterList::mockChapters); + CPPUNIT_ASSERT_EQUAL(1, f.chapterList->readCount); + f.save(); + CPPUNIT_ASSERT_EQUAL(1, f.chapterList->readCount); + CPPUNIT_ASSERT_EQUAL(0, f.chapterList->writeCount); + } + // Do not write if not modified + { + MockChapterFile f(TEST_FILE_PATH_C("no-tags.m4a")); + auto chapters = f.chapters(); + CPPUNIT_ASSERT(chapters == MockChapterList::mockChapters); + CPPUNIT_ASSERT(f.chapterList); + CPPUNIT_ASSERT_EQUAL(1, f.chapterList->readCount); + f.setChapters(MockChapterList::mockChapters); + f.save(); + CPPUNIT_ASSERT_EQUAL(1, f.chapterList->readCount); + CPPUNIT_ASSERT_EQUAL(0, f.chapterList->writeCount); + } + // Write if set without being read before + { + MockChapterFile f(TEST_FILE_PATH_C("no-tags.m4a")); + f.setChapters(MP4::ChapterList()); + f.save(); + CPPUNIT_ASSERT(f.chapterList); + CPPUNIT_ASSERT_EQUAL(0, f.chapterList->readCount); + CPPUNIT_ASSERT_EQUAL(1, f.chapterList->writeCount); + } + // Do write if modified + { + MockChapterFile f(TEST_FILE_PATH_C("no-tags.m4a")); + CPPUNIT_ASSERT(f.chapters() == MockChapterList::mockChapters); + CPPUNIT_ASSERT(f.chapterList); + CPPUNIT_ASSERT_EQUAL(1, f.chapterList->readCount); + const auto chapters1 = MP4::ChapterList({ + MP4::Chapter("Chapter 1", 0), + }); + f.setChapters(chapters1); + CPPUNIT_ASSERT(f.chapters() == chapters1); + f.save(); + CPPUNIT_ASSERT(f.chapters() == chapters1); + CPPUNIT_ASSERT_EQUAL(1, f.chapterList->readCount); + CPPUNIT_ASSERT_EQUAL(1, f.chapterList->writeCount); + f.setChapters(chapters1); + f.save(); + CPPUNIT_ASSERT_EQUAL(1, f.chapterList->writeCount); + auto chapters2 = MP4::ChapterList({ + MP4::Chapter("Chapter 1", 0), + MP4::Chapter("Chapter 2", 1), + }); + f.setChapters(chapters2); + CPPUNIT_ASSERT(f.chapters() == chapters2); + f.save(); + CPPUNIT_ASSERT(f.chapters() == chapters2); + CPPUNIT_ASSERT_EQUAL(2, f.chapterList->writeCount); + chapters2 = MP4::ChapterList({ + MP4::Chapter("Chapter 1", 0), + MP4::Chapter("Chapter 2", 2), + }); + f.setChapters(chapters2); + f.save(); + CPPUNIT_ASSERT_EQUAL(3, f.chapterList->writeCount); + f.setChapters(chapters2); + CPPUNIT_ASSERT(f.chapters() == chapters2); + f.save(); + CPPUNIT_ASSERT(f.chapters() == chapters2); + CPPUNIT_ASSERT_EQUAL(3, f.chapterList->writeCount); + const auto chapters3 = MP4::ChapterList({ + MP4::Chapter("Chapter 1", 0), + MP4::Chapter("Chapter 3", 2), + }); + f.setChapters(chapters3); + CPPUNIT_ASSERT(f.chapters() == chapters3); + f.save(); + CPPUNIT_ASSERT(f.chapters() == chapters3); + CPPUNIT_ASSERT_EQUAL(4, f.chapterList->writeCount); + f.setChapters(MP4::ChapterList()); + CPPUNIT_ASSERT(f.chapters().isEmpty()); + f.save(); + CPPUNIT_ASSERT(f.chapters().isEmpty()); + CPPUNIT_ASSERT_EQUAL(5, f.chapterList->writeCount); + } + } + }; CPPUNIT_TEST_SUITE_REGISTRATION(TestMP4);