MP4: Reuse parsed atoms when lazily reading chapters (#1460)

neroChapters() and qtChapters() parsed the whole atom tree again on first
use, although the file already holds it from opening. They now use the
parsed atoms, and parse them again after a save or strip, which can move
atoms.

Co-Authored-By: Claude Opus 5.5 <[email protected]>

taglib/mp4/mp4chapterholder.h

@@ -31,6 +31,7 @@
 namespace TagLib {
   class File;
   namespace MP4 {
+    class Atoms;
     /*!
      * Base class to hold chapters and store modified state.
      */
@@ -71,13 +72,32 @@ namespace TagLib {
     template <typename T>
     ChapterList getChaptersLazy(std::unique_ptr<T> &holder, TagLib::File *file)
     {
-      if (!holder) {
+      if(!holder) {
         holder = std::make_unique<T>();
         holder->read(file);
       }
       return holder->chapters();
     }
 
+    /*!
+     * Lazily fetch list of chapters from the already parsed \a atoms of \a file,
+     * instead of parsing them again.
+     * @tparam T class derived from ChapterHolder and implementing read(File *, const Atoms *)
+     * @param holder unique pointer to holder, initially null
+     * @param file file with chapters
+     * @param atoms parsed atoms of \a file, must be up to date
+     * @return list of chapters, empty if no chapters found.
+     */
+    template <typename T>
+    ChapterList getChaptersLazy(std::unique_ptr<T> &holder, TagLib::File *file, const Atoms *atoms)
+    {
+      if(!holder) {
+        holder = std::make_unique<T>();
+        holder->read(file, atoms);
+      }
+      return holder->chapters();
+    }
+
     /*!
      * Lazily set a list of chapters.
      * @tparam T class derived from ChapterHolder
@@ -87,7 +107,7 @@ namespace TagLib {
     template <typename T>
     void setChaptersLazy(std::unique_ptr<T> &holder, const ChapterList& chapters)
     {
-      if (!holder) {
+      if(!holder) {
         holder = std::make_unique<T>();
         // The chapters have not been read before, so we do not know their
         // current state and mark them as modified. Otherwise, the check below

taglib/mp4/mp4file.cpp

@@ -52,6 +52,8 @@ public:
   std::unique_ptr<MP4::Properties> properties;
   std::unique_ptr<MP4::NeroChapterList> neroChapterList;
   std::unique_ptr<MP4::QtChapterList> qtChapterList;
+  // Saving can move atoms without updating all offsets of the parsed tree.
+  bool atomsStale = false;
 };
 
 ////////////////////////////////////////////////////////////////////////////////
@@ -117,7 +119,9 @@ MP4::Properties *MP4::File::audioProperties() const
 
 MP4::ChapterList MP4::File::neroChapters()
 {
-  return getChaptersLazy(d->neroChapterList, this);
+  if(d->atomsStale)
+    return getChaptersLazy(d->neroChapterList, this);
+  return getChaptersLazy(d->neroChapterList, this, d->atoms.get());
 }
 
 void MP4::File::setNeroChapters(const ChapterList& chapters)
@@ -127,7 +131,9 @@ void MP4::File::setNeroChapters(const ChapterList& chapters)
 
 MP4::ChapterList MP4::File::qtChapters()
 {
-  return getChaptersLazy(d->qtChapterList, this);
+  if(d->atomsStale)
+    return getChaptersLazy(d->qtChapterList, this);
+  return getChaptersLazy(d->qtChapterList, this, d->atoms.get());
 }
 
 void MP4::File::setQtChapters(const ChapterList& chapters)
@@ -172,6 +178,7 @@ MP4::File::save()
     return false;
   }
 
+  d->atomsStale = true;
   return d->tag->save() &&
     saveChaptersIfModified(d->neroChapterList, this) &&
     saveChaptersIfModified(d->qtChapterList, this);
@@ -191,6 +198,7 @@ MP4::File::strip(int tags)
   }
 
   if(tags & MP4) {
+    d->atomsStale = true;
     return d->tag->strip();
   }
 

taglib/mp4/mp4nerochapterlist.cpp

@@ -221,8 +221,12 @@ namespace
 bool MP4::NeroChapterList::read(TagLib::File *file)
 {
   const Atoms atoms(file);
+  return read(file, &atoms);
+}
 
-  const Atom *chpl = atoms.find("moov", "udta", "chpl");
+bool MP4::NeroChapterList::read(TagLib::File *file, const Atoms *atoms)
+{
+  const Atom *chpl = atoms->find("moov", "udta", "chpl");
   modified = false;
   chapterList.clear();
   if(chpl) {

taglib/mp4/mp4nerochapterlist.h

@@ -30,6 +30,7 @@
 namespace TagLib {
   class File;
   namespace MP4 {
+    class Atoms;
 
     /*!
      * Reads, writes, and removes Nero-style chapter markers (chpl atom)
@@ -45,6 +46,12 @@ namespace TagLib {
        */
       bool read(TagLib::File *file);
 
+      /*!
+       * Same as read(TagLib::File *), using the already parsed \a atoms
+       * of \a file instead of parsing them again.
+       */
+      bool read(TagLib::File *file, const Atoms *atoms);
+
       /*!
        * Writes chapter markers to the already-opened \a file,
        * replacing any existing chpl atom.

taglib/mp4/mp4qtchapterlist.cpp

@@ -1231,14 +1231,19 @@ namespace
 bool MP4::QtChapterList::read(TagLib::File *file)
 {
   const Atoms atoms(file);
+  return read(file, &atoms);
+}
+
+bool MP4::QtChapterList::read(TagLib::File *file, const Atoms *atoms)
+{
   modified = false;
   chapterList.clear();
 
-  TrackInfo audio = findAudioTrack(file, &atoms);
+  TrackInfo audio = findAudioTrack(file, atoms);
   if(!audio.trak)
     return false;
 
-  Atom *chapterTrak = findChapterTrak(file, &atoms, audio.trak);
+  Atom *chapterTrak = findChapterTrak(file, atoms, audio.trak);
   if(!chapterTrak)
     return false;
 

taglib/mp4/mp4qtchapterlist.h

@@ -30,6 +30,7 @@
 namespace TagLib {
   class File;
   namespace MP4 {
+    class Atoms;
 
     /*!
      * Reads, writes, and removes QuickTime-style chapter tracks from MP4
@@ -57,6 +58,12 @@ namespace TagLib {
        */
       bool read(TagLib::File *file);
 
+      /*!
+       * Same as read(TagLib::File *), using the already parsed \a atoms
+       * of \a file instead of parsing them again.
+       */
+      bool read(TagLib::File *file, const Atoms *atoms);
+
       /*!
        * Writes chapter markers as a QuickTime chapter track to the
        * already-opened \a file, replacing any existing chapter track.

tests/test_mp4.cpp

@@ -179,6 +179,7 @@ class TestMP4 : public CppUnit::TestFixture
   CPPUNIT_TEST(testNeroAndQTChaptersAreIndependent);
   CPPUNIT_TEST(testNeroChaptersAloneWhenNoQT);
   CPPUNIT_TEST(testLazyReadingAndWritingChapters);
+  CPPUNIT_TEST(testReadingChaptersAfterSave);
   CPPUNIT_TEST_SUITE_END();
 
 public:
@@ -1992,6 +1993,37 @@ public:
     }
   }
 
+  void testReadingChaptersAfterSave()
+  {
+    ScopedFileCopy copy("has-tags", ".m4a");
+    string filename = copy.fileName();
+
+    {
+      MP4::File f(filename.c_str());
+      f.setNeroChapters(MP4::ChapterList{
+        MP4::Chapter("Nero 1", 0),
+        MP4::Chapter("Nero 2", 10000LL)
+      });
+      CPPUNIT_ASSERT(f.save());
+    }
+
+    // Writing QT chapters moves atoms, chapters first read afterwards must
+    // not use the atoms parsed when the file was opened.
+    {
+      MP4::File f(filename.c_str());
+      f.setQtChapters(MP4::ChapterList{
+        MP4::Chapter("QT 1", 0),
+        MP4::Chapter("QT 2", 20000LL)
+      });
+      CPPUNIT_ASSERT(f.save());
+
+      const MP4::ChapterList nero = f.neroChapters();
+      CPPUNIT_ASSERT_EQUAL(2U, nero.size());
+      CPPUNIT_ASSERT_EQUAL(String("Nero 2"), nero[1].title());
+      CPPUNIT_ASSERT_EQUAL(10000LL, nero[1].startTime());
+    }
+  }
+
 };
 
 CPPUNIT_TEST_SUITE_REGISTRATION(TestMP4);