From c5ea13bb34a9781fc40444b5d5ea964440a5283b Mon Sep 17 00:00:00 2001 From: Ryan Francesconi Date: Tue, 14 Apr 2026 14:23:35 -0700 Subject: [PATCH] overloads for read, write, remove MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Changes made mp4chapterlist.h • Added (​MP4::​File*) overloads for read, write, remove • Replaced broken class ​File; forward declaration with #include "mp4file​.h" (fixed a subtle C++ name-resolution linker bug where Atoms(​File*) resolved to MP4::​File* instead of Tag​Lib::​File*) mp4chapterlist.cpp • Refactored: path-based overloads are now thin wrappers that delegate to file-based overloads • File-based overloads construct Atoms locally — no Atoms* in the public API • Removed chpl​Header​Size = 9 constant; replaced the minimum-size guard in parse​Chpl​Data with a correct 5-byte check (the old constant was version-1 specific and would reject valid version-0 atoms) mp4qtchapterlist.h • Added (​MP4::​File*) overloads for read, write, remove • Removed Atoms* parameters entirely from the public API mp4qtchapterlist.cpp • Same refactor: path-based overloads delegate; file-based overloads construct Atoms locally • Added empty-chapter guard: write(​MP4::​File*, {}) delegates to remove(file) instead of writing a 0-sample chapter track tests/test_mp4.cpp • Added test​Chapter​List​File​API and test​QTChapter​List​File​API — exercise the full write/read/remove cycle via the file-based API • Updated test bodies to use the simplified (​MP4::​File*) API (no MP4::​Atoms construction in test code) --- taglib/mp4/mp4chapterlist.cpp | 58 +++++++++++------- taglib/mp4/mp4chapterlist.h | 22 +++++++ taglib/mp4/mp4qtchapterlist.cpp | 99 ++++++++++++++++++------------ taglib/mp4/mp4qtchapterlist.h | 22 +++++++ tests/test_mp4.cpp | 105 ++++++++++++++++++++++++++++++++ 5 files changed, 246 insertions(+), 60 deletions(-) diff --git a/taglib/mp4/mp4chapterlist.cpp b/taglib/mp4/mp4chapterlist.cpp index 22184435..dca06cda 100644 --- a/taglib/mp4/mp4chapterlist.cpp +++ b/taglib/mp4/mp4chapterlist.cpp @@ -34,9 +34,6 @@ using namespace TagLib; namespace { - // Nero chpl version 1 header: version(1) + flags(3) + reserved(4) + count(1) = 9 bytes - constexpr int chplHeaderSize = 9; - ByteVector renderAtom(const ByteVector &name, const ByteVector &data) { return ByteVector::fromUInt(static_cast(data.size() + 8)) + name + data; @@ -171,7 +168,8 @@ namespace { MP4::ChapterList chapters; - if(data.size() < static_cast(chplHeaderSize)) + // Minimum: version(1) + flags(3) + count(1) = 5 bytes (version 0 layout) + if(data.size() < 5) return chapters; unsigned int pos = 0; @@ -225,15 +223,21 @@ MP4::MP4ChapterList::read(const char *path) return ChapterList(); } - Atoms atoms(&file); + return read(&file); +} + +MP4::ChapterList +MP4::MP4ChapterList::read(MP4::File *file) +{ + Atoms atoms(file); Atom *chpl = atoms.find("moov", "udta", "chpl"); if(!chpl) return ChapterList(); // Read the atom content (skip 8-byte atom header) - file.seek(chpl->offset() + 8); - ByteVector data = file.readBlock(chpl->length() - 8); + file->seek(chpl->offset() + 8); + ByteVector data = file->readBlock(chpl->length() - 8); return parseChplData(data); } @@ -247,7 +251,13 @@ MP4::MP4ChapterList::write(const char *path, const ChapterList &chapters) return false; } - Atoms atoms(&file); + return write(&file, chapters); +} + +bool +MP4::MP4ChapterList::write(MP4::File *file, const ChapterList &chapters) +{ + Atoms atoms(file); if(!atoms.find("moov")) { debug("MP4ChapterList::write() -- No moov atom found"); @@ -265,13 +275,13 @@ MP4::MP4ChapterList::write(const char *path, const ChapterList &chapters) offset_t oldLength = existingChpl->length(); offset_t delta = static_cast(chplAtom.size()) - oldLength; - file.insert(chplAtom, offset, oldLength); + file->insert(chplAtom, offset, oldLength); if(delta != 0) { // Update parent sizes: moov and udta AtomList parentPath = atoms.path("moov", "udta", "chpl"); - updateParentSizes(&file, parentPath, delta, 1); // ignore chpl itself - updateChunkOffsets(&file, &atoms, delta, offset); + updateParentSizes(file, parentPath, delta, 1); // ignore chpl itself + updateChunkOffsets(file, &atoms, delta, offset); } } else { @@ -281,10 +291,10 @@ MP4::MP4ChapterList::write(const char *path, const ChapterList &chapters) if(udtaPath.size() == 2) { // udta exists -- insert chpl at the beginning of udta's content offset_t insertOffset = udtaPath.back()->offset() + 8; - file.insert(chplAtom, insertOffset, 0); + file->insert(chplAtom, insertOffset, 0); - updateParentSizes(&file, udtaPath, chplAtom.size()); - updateChunkOffsets(&file, &atoms, chplAtom.size(), insertOffset); + updateParentSizes(file, udtaPath, chplAtom.size()); + updateChunkOffsets(file, &atoms, chplAtom.size(), insertOffset); } else { // No udta -- insert udta + chpl at the beginning of moov's content @@ -297,10 +307,10 @@ MP4::MP4ChapterList::write(const char *path, const ChapterList &chapters) } offset_t insertOffset = moovPath.back()->offset() + 8; - file.insert(udtaAtom, insertOffset, 0); + file->insert(udtaAtom, insertOffset, 0); - updateParentSizes(&file, moovPath, udtaAtom.size()); - updateChunkOffsets(&file, &atoms, udtaAtom.size(), insertOffset); + updateParentSizes(file, moovPath, udtaAtom.size()); + updateChunkOffsets(file, &atoms, udtaAtom.size(), insertOffset); } } @@ -316,7 +326,13 @@ MP4::MP4ChapterList::remove(const char *path) return false; } - Atoms atoms(&file); + return remove(&file); +} + +bool +MP4::MP4ChapterList::remove(MP4::File *file) +{ + Atoms atoms(file); Atom *chpl = atoms.find("moov", "udta", "chpl"); if(!chpl) { @@ -327,12 +343,12 @@ MP4::MP4ChapterList::remove(const char *path) offset_t offset = chpl->offset(); offset_t length = chpl->length(); - file.removeBlock(offset, length); + file->removeBlock(offset, length); // Update parent sizes with negative delta AtomList parentPath = atoms.path("moov", "udta", "chpl"); - updateParentSizes(&file, parentPath, -length, 1); // ignore chpl itself - updateChunkOffsets(&file, &atoms, -length, offset); + updateParentSizes(file, parentPath, -length, 1); // ignore chpl itself + updateChunkOffsets(file, &atoms, -length, offset); return true; } diff --git a/taglib/mp4/mp4chapterlist.h b/taglib/mp4/mp4chapterlist.h index 37cb723f..3f739fb2 100644 --- a/taglib/mp4/mp4chapterlist.h +++ b/taglib/mp4/mp4chapterlist.h @@ -28,6 +28,7 @@ #include "tlist.h" #include "tstring.h" #include "taglib_export.h" +#include "mp4file.h" namespace TagLib { namespace MP4 { @@ -56,6 +57,13 @@ namespace TagLib { */ static ChapterList read(const char *path); + /*! + * Reads chapter markers from the already-opened \a file. + * Avoids a second open when the caller already has the file open. + * Returns an empty list if the file has no chpl atom. + */ + static ChapterList read(MP4::File *file); + /*! * Writes chapter markers to the MP4 file at \a path, * replacing any existing chpl atom. The chapter count is @@ -64,11 +72,25 @@ namespace TagLib { */ static bool write(const char *path, const ChapterList &chapters); + /*! + * Writes chapter markers to the already-opened \a file, + * replacing any existing chpl atom. + * The chapter count is capped at 255 (Nero format limit). + * Returns \c true on success. + */ + static bool write(MP4::File *file, const ChapterList &chapters); + /*! * Removes the chpl atom from the MP4 file at \a path. * Returns \c true on success, or if no chpl atom exists. */ static bool remove(const char *path); + + /*! + * Removes the chpl atom from the already-opened \a file. + * Returns \c true on success, or if no chpl atom exists. + */ + static bool remove(MP4::File *file); }; } // namespace MP4 diff --git a/taglib/mp4/mp4qtchapterlist.cpp b/taglib/mp4/mp4qtchapterlist.cpp index c9d5ef88..a726c748 100644 --- a/taglib/mp4/mp4qtchapterlist.cpp +++ b/taglib/mp4/mp4qtchapterlist.cpp @@ -950,23 +950,29 @@ MP4::MP4QTChapterList::read(const char *path) if(!file.isOpen() || !file.isValid()) return ChapterList(); - Atoms atoms(&file); + return read(&file); +} - TrackInfo audio = findAudioTrack(&file, &atoms); +MP4::ChapterList +MP4::MP4QTChapterList::read(MP4::File *file) +{ + Atoms atoms(file); + + TrackInfo audio = findAudioTrack(file, &atoms); if(!audio.trak) return ChapterList(); - Atom *chapterTrak = findChapterTrak(&file, &atoms, audio.trak); + Atom *chapterTrak = findChapterTrak(file, &atoms, audio.trak); if(!chapterTrak) return ChapterList(); - ChapterTrackInfo trackInfo = readChapterTrackInfo(&file, chapterTrak); + ChapterTrackInfo trackInfo = readChapterTrackInfo(file, chapterTrak); if(trackInfo.timescale == 0) return ChapterList(); - std::vector sttsEntries = readStts(&file, chapterTrak); - SampleSizeInfo sizeInfo = readStsz(&file, chapterTrak); - std::vector offsets = resolveSampleOffsets(&file, chapterTrak, sizeInfo); + std::vector sttsEntries = readStts(file, chapterTrak); + SampleSizeInfo sizeInfo = readStsz(file, chapterTrak); + std::vector offsets = resolveSampleOffsets(file, chapterTrak, sizeInfo); if(offsets.empty()) return ChapterList(); @@ -984,7 +990,7 @@ MP4::MP4QTChapterList::read(const char *path) if(sampleSize == 0 && sampleIndex < sizeInfo.perSampleSizes.size()) sampleSize = sizeInfo.perSampleSizes[sampleIndex]; - String title = readTextSample(&file, offsets[sampleIndex], sampleSize); + String title = readTextSample(file, offsets[sampleIndex], sampleSize); long long startTime100ns = static_cast( static_cast(currentTime) * 10000000.0 / @@ -1021,24 +1027,33 @@ MP4::MP4QTChapterList::write(const char *path, const ChapterList &chapters) return false; } + return write(&file, chapters); +} + +bool +MP4::MP4QTChapterList::write(MP4::File *file, const ChapterList &chapters) +{ + // Writing an empty list is equivalent to removing the chapter track. + if(chapters.isEmpty()) + return remove(file); + // ---- Phase 1: Parse and gather info ---- - Atoms atoms(&file); - + Atoms atoms(file); Atom *moov = atoms.find("moov"); if(!moov) { debug("MP4QTChapterList::write() -- No moov atom found"); return false; } - MovieInfo movieInfo = readMovieInfo(&file, &atoms); + MovieInfo movieInfo = readMovieInfo(file, &atoms); if(movieInfo.durationMs <= 0) { debug("MP4QTChapterList::write() -- Could not determine file duration"); return false; } long long durationMs = movieInfo.durationMs; - TrackInfo audio = findAudioTrack(&file, &atoms); + TrackInfo audio = findAudioTrack(file, &atoms); if(!audio.trak) { debug("MP4QTChapterList::write() -- No audio track found"); return false; @@ -1052,7 +1067,7 @@ MP4::MP4QTChapterList::write(const char *path, const ChapterList &chapters) // Optional second parse -- only constructed when replacing existing chapters. std::unique_ptr cleanAtoms; - Atom *existingChapter = findChapterTrak(&file, &atoms, audio.trak); + Atom *existingChapter = findChapterTrak(file, &atoms, audio.trak); if(existingChapter) { // Remove chapter trak FIRST (higher offset in file). offset_t chapterOff = existingChapter->offset(); @@ -1062,17 +1077,17 @@ MP4::MP4QTChapterList::write(const char *path, const ChapterList &chapters) moov->removeChild(existingChapter); delete existingChapter; - file.removeBlock(chapterOff, chapterLen); + file->removeBlock(chapterOff, chapterLen); AtomList moovPath = atoms.path("moov"); - updateParentSizes(&file, moovPath, -chapterLen); - updateChunkOffsets(&file, &atoms, -chapterLen, chapterOff); + updateParentSizes(file, moovPath, -chapterLen); + updateChunkOffsets(file, &atoms, -chapterLen, chapterOff); // Remove tref from audio trak (lower offset, still valid). - removeAudioTref(&file, &atoms, audio.trak); + removeAudioTref(file, &atoms, audio.trak); // Re-parse to get clean state after removals. - cleanAtoms = std::make_unique(&file); + cleanAtoms = std::make_unique(file); activeAtoms = cleanAtoms.get(); moov = activeAtoms->find("moov"); @@ -1080,7 +1095,7 @@ MP4::MP4QTChapterList::write(const char *path, const ChapterList &chapters) debug("MP4QTChapterList::write() -- moov disappeared after cleanup"); return false; } - audio = findAudioTrack(&file, activeAtoms); + audio = findAudioTrack(file, activeAtoms); if(!audio.trak) { debug("MP4QTChapterList::write() -- No audio track after cleanup"); return false; @@ -1100,7 +1115,7 @@ MP4::MP4QTChapterList::write(const char *path, const ChapterList &chapters) workingChapters.prepend(dummy); } - unsigned int nextId = getNextTrackId(&file, activeAtoms); + unsigned int nextId = getNextTrackId(file, activeAtoms); unsigned int chapterTrackId = nextId > 0 ? nextId : audio.trackId + 1; constexpr unsigned int timescale = 1000; std::vector sampleSizes = calculateSampleSizes(workingChapters); @@ -1114,7 +1129,7 @@ MP4::MP4QTChapterList::write(const char *path, const ChapterList &chapters) movieInfo.duration); offset_t totalInsert = static_cast(trefAtom.size() + trakMeasure.size()); // Text samples go inside an mdat atom at EOF. stco offsets point past the 8-byte mdat header. - offset_t textDataOffset = file.length() + totalInsert + 8; + offset_t textDataOffset = file->length() + totalInsert + 8; // Build final trak with correct stco offsets pointing to where text data will land. ByteVector trakAtom = buildChapterTrak( @@ -1129,22 +1144,22 @@ MP4::MP4QTChapterList::write(const char *path, const ChapterList &chapters) // tref is logically inside audio trak; chapter trak is logically after it. offset_t insertOffset = audio.trak->offset() + audio.trak->length(); - file.insert(combinedPayload, insertOffset, 0); + file->insert(combinedPayload, insertOffset, 0); // Fix audio trak size on disk -- only tref goes inside - file.seek(audio.trak->offset()); - unsigned int audioTrakSize = file.readBlock(4).toUInt(); + file->seek(audio.trak->offset()); + unsigned int audioTrakSize = file->readBlock(4).toUInt(); unsigned int newAudioTrakSize = static_cast(audioTrakSize + trefAtom.size()); - file.seek(audio.trak->offset()); - file.writeBlock(ByteVector::fromUInt(newAudioTrakSize)); + file->seek(audio.trak->offset()); + file->writeBlock(ByteVector::fromUInt(newAudioTrakSize)); // Fix moov size -- both tref and chapter trak are inside moov AtomList moovPath = activeAtoms->path("moov"); - updateParentSizes(&file, moovPath, combinedPayload.size()); + updateParentSizes(file, moovPath, combinedPayload.size()); // Fix existing chunk offsets -- only the ORIGINAL atom tree is iterated, // so the new chapter trak's stco (which already has correct offsets) is untouched. - updateChunkOffsets(&file, activeAtoms, combinedPayload.size(), insertOffset); + updateChunkOffsets(file, activeAtoms, combinedPayload.size(), insertOffset); // ---- Phase 4: Append text samples in mdat at EOF ---- @@ -1155,15 +1170,15 @@ MP4::MP4QTChapterList::write(const char *path, const ChapterList &chapters) // Wrap text samples in an mdat atom so players can find them. ByteVector mdatAtom = renderAtom("mdat", textSamples); - file.seek(0, TagLib::File::End); - file.writeBlock(mdatAtom); + file->seek(0, TagLib::File::End); + file->writeBlock(mdatAtom); // ---- Phase 5: Update mvhd next_track_ID ---- // mvhd is before insertOffset, so its offset is unchanged. - unsigned int currentNextId = getNextTrackId(&file, activeAtoms); + unsigned int currentNextId = getNextTrackId(file, activeAtoms); if(chapterTrackId >= currentNextId) { - setNextTrackId(&file, activeAtoms, chapterTrackId + 1); + setNextTrackId(file, activeAtoms, chapterTrackId + 1); } return true; @@ -1178,13 +1193,19 @@ MP4::MP4QTChapterList::remove(const char *path) return false; } - Atoms atoms(&file); + return remove(&file); +} - TrackInfo audio = findAudioTrack(&file, &atoms); +bool +MP4::MP4QTChapterList::remove(MP4::File *file) +{ + Atoms atoms(file); + + TrackInfo audio = findAudioTrack(file, &atoms); if(!audio.trak) return true; // No audio track -- nothing to do - Atom *chapterTrak = findChapterTrak(&file, &atoms, audio.trak); + Atom *chapterTrak = findChapterTrak(file, &atoms, audio.trak); if(!chapterTrak) return true; // No chapter track -- nothing to do @@ -1200,14 +1221,14 @@ MP4::MP4QTChapterList::remove(const char *path) moov->removeChild(chapterTrak); delete chapterTrak; - file.removeBlock(chapterOff, chapterLen); + file->removeBlock(chapterOff, chapterLen); AtomList moovPath = atoms.path("moov"); - updateParentSizes(&file, moovPath, -chapterLen); - updateChunkOffsets(&file, &atoms, -chapterLen, chapterOff); + updateParentSizes(file, moovPath, -chapterLen); + updateChunkOffsets(file, &atoms, -chapterLen, chapterOff); // Remove tref from audio trak (lower offset, still valid after chapter trak removal). - removeAudioTref(&file, &atoms, audio.trak); + removeAudioTref(file, &atoms, audio.trak); return true; } diff --git a/taglib/mp4/mp4qtchapterlist.h b/taglib/mp4/mp4qtchapterlist.h index e20abf2a..af0d6b4c 100644 --- a/taglib/mp4/mp4qtchapterlist.h +++ b/taglib/mp4/mp4qtchapterlist.h @@ -56,6 +56,14 @@ namespace TagLib { */ static ChapterList read(const char *path); + /*! + * Reads chapter markers from the QuickTime chapter track in the + * already-opened \a file. Avoids a second open when the caller + * already has the file open. + * Returns an empty list if the file has no chapter track. + */ + static ChapterList read(MP4::File *file); + /*! * Writes chapter markers as a QuickTime chapter track to the MP4 * file at \a path, replacing any existing chapter track. The @@ -64,12 +72,26 @@ namespace TagLib { */ static bool write(const char *path, const ChapterList &chapters); + /*! + * Writes chapter markers as a QuickTime chapter track to the + * already-opened \a file, replacing any existing chapter track. + * Returns \c true on success. + */ + static bool write(MP4::File *file, const ChapterList &chapters); + /*! * Removes the QuickTime chapter track and its \c tref/chap * reference from the MP4 file at \a path. * Returns \c true on success, or if no chapter track exists. */ static bool remove(const char *path); + + /*! + * Removes the QuickTime chapter track and its \c tref/chap + * reference from the already-opened \a file. + * Returns \c true on success, or if no chapter track exists. + */ + static bool remove(MP4::File *file); }; } // namespace MP4 diff --git a/tests/test_mp4.cpp b/tests/test_mp4.cpp index 5c21fd01..b2be6734 100644 --- a/tests/test_mp4.cpp +++ b/tests/test_mp4.cpp @@ -115,6 +115,8 @@ class TestMP4 : public CppUnit::TestFixture CPPUNIT_TEST(testQTChapterListOverwrite); CPPUNIT_TEST(testQTChapterListTimestampPrecision); CPPUNIT_TEST(testQTChapterListNonZeroFirstChapter); + CPPUNIT_TEST(testChapterListFileAPI); + CPPUNIT_TEST(testQTChapterListFileAPI); CPPUNIT_TEST_SUITE_END(); public: @@ -1313,6 +1315,109 @@ public: CPPUNIT_ASSERT_EQUAL(String("Three"), chapters[2].title); } } + void testChapterListFileAPI() + { + ScopedFileCopy copy("no-tags", ".m4a"); + string filename = copy.fileName(); + + // Write chapters via the file-based API + { + MP4::File file(filename.c_str(), false); + CPPUNIT_ASSERT(file.isOpen() && file.isValid() && !file.readOnly()); + + MP4::ChapterList chapters; + MP4::Chapter ch1; + ch1.startTime = 0; + ch1.title = "Alpha"; + chapters.append(ch1); + + MP4::Chapter ch2; + ch2.startTime = 200000000LL; // 20 seconds + ch2.title = "Beta"; + chapters.append(ch2); + + CPPUNIT_ASSERT(MP4::MP4ChapterList::write(&file, chapters)); + } + + // Read back via the file-based API + { + MP4::File file(filename.c_str(), false); + CPPUNIT_ASSERT(file.isOpen() && file.isValid()); + + MP4::ChapterList chapters = MP4::MP4ChapterList::read(&file); + CPPUNIT_ASSERT_EQUAL(2U, chapters.size()); + CPPUNIT_ASSERT_EQUAL(0LL, chapters[0].startTime); + CPPUNIT_ASSERT_EQUAL(String("Alpha"), chapters[0].title); + CPPUNIT_ASSERT_EQUAL(200000000LL, chapters[1].startTime); + CPPUNIT_ASSERT_EQUAL(String("Beta"), chapters[1].title); + } + + // Remove via the file-based API + { + MP4::File file(filename.c_str(), false); + CPPUNIT_ASSERT(file.isOpen() && file.isValid() && !file.readOnly()); + + CPPUNIT_ASSERT(MP4::MP4ChapterList::remove(&file)); + } + + // Verify removed + { + MP4::ChapterList chapters = MP4::MP4ChapterList::read(filename.c_str()); + CPPUNIT_ASSERT(chapters.isEmpty()); + } + } + + void testQTChapterListFileAPI() + { + ScopedFileCopy copy("no-tags", ".m4a"); + string filename = copy.fileName(); + + // Write chapters via the file-based API + { + MP4::File file(filename.c_str(), false); + CPPUNIT_ASSERT(file.isOpen() && file.isValid() && !file.readOnly()); + + MP4::ChapterList chapters; + MP4::Chapter ch1; + ch1.startTime = 0; + ch1.title = "Alpha"; + chapters.append(ch1); + + MP4::Chapter ch2; + ch2.startTime = 200000000LL; // 20 seconds + ch2.title = "Beta"; + chapters.append(ch2); + + CPPUNIT_ASSERT(MP4::MP4QTChapterList::write(&file, chapters)); + } + + // Read back via the file-based API + { + MP4::File file(filename.c_str(), false); + CPPUNIT_ASSERT(file.isOpen() && file.isValid()); + + MP4::ChapterList chapters = MP4::MP4QTChapterList::read(&file); + CPPUNIT_ASSERT_EQUAL(2U, chapters.size()); + CPPUNIT_ASSERT_EQUAL(0LL, chapters[0].startTime); + CPPUNIT_ASSERT_EQUAL(String("Alpha"), chapters[0].title); + CPPUNIT_ASSERT_EQUAL(200000000LL, chapters[1].startTime); + CPPUNIT_ASSERT_EQUAL(String("Beta"), chapters[1].title); + } + + // Remove via the file-based API + { + MP4::File file(filename.c_str(), false); + CPPUNIT_ASSERT(file.isOpen() && file.isValid() && !file.readOnly()); + + CPPUNIT_ASSERT(MP4::MP4QTChapterList::remove(&file)); + } + + // Verify removed + { + MP4::ChapterList chapters = MP4::MP4QTChapterList::read(filename.c_str()); + CPPUNIT_ASSERT(chapters.isEmpty()); + } + } }; CPPUNIT_TEST_SUITE_REGISTRATION(TestMP4);