Audit MPEG-4 box types by benrr101 · Pull Request #125 · benrr101/node-taglib-sharp

benrr101 · 2026-04-03T04:54:27Z

Description

As per #119, subtitle tags for MPEG4 files were being stored incorrectly - they were being stored in the "Subt" box. FFMPEG source reads subtitle tags from the "©st3" box. I've gone through the list of boxes used for MPEG4 and cross referenced them with either FFMPEG source or the Exiftool website.

Remaining open questions:

There's still a few box types that I don't know where they came from and there's no equivalent (eg. role).
- Should they be removed?
There's a few box types that have multiple possible boxes to store a tag (eg, album -> albm or ©alb)
- Should these be exposed via settings and user can select?

Testing

Existing unit and integration tests pass

Copilot

Pull request overview

This PR audits and updates the MPEG-4/QuickTime atom (box) type mappings used by the Apple/MPEG4 tagging implementation, aiming to align where tags are stored with external tools like FFmpeg and ExifTool (notably subtitle metadata per #119).

Changes:

Update the QuickTime conductor atom mapping to use ©con (instead of cond).
Update the subtitle atom mapping to use ©st3 (instead of Subt) and clarify related comments.
Reformat/expand documentation comments around the box type list and a few specific atoms.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

src/mpeg4/mpeg4BoxType.ts

Copilot

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-04-06T06:21:34Z

src/mpeg4/appleTag.ts

+        return this.getFirstQuickTimeString(Mpeg4BoxType.SUBT) // @TODO: Backwards compat for pre-6.0.2 releases
+            || this.getFirstQuickTimeString(Mpeg4BoxType.ST3);


subtitle getter checks the legacy Subt box first, but the setter now writes to ©st3 only. If a file already contains Subt, calling tag.subtitle = ... will leave the old Subt box in place and subsequent reads will still return the stale legacy value. Prefer reading ©st3 first (and/or remove the legacy Subt boxes when writing) so the updated value is observed and the file doesn't accumulate conflicting boxes.

Suggested change

return this.getFirstQuickTimeString(Mpeg4BoxType.SUBT) // @TODO: Backwards compat for pre-6.0.2 releases

|| this.getFirstQuickTimeString(Mpeg4BoxType.ST3);

return this.getFirstQuickTimeString(Mpeg4BoxType.ST3)

|| this.getFirstQuickTimeString(Mpeg4BoxType.SUBT); // @TODO: Backwards compat for pre-6.0.2 releases

Copilot · 2026-04-06T06:21:35Z

src/mpeg4/appleTag.ts

+        return this.getFirstQuickTimeString(Mpeg4BoxType.COND) // @TODO: Backwards compat for pre-6.0.2 releases
+            || this.getFirstQuickTimeString(Mpeg4BoxType.CON);


Same issue as subtitle: the conductor getter checks legacy cond first while the setter writes only ©con. If a tag already has cond, setting conductor will not update what the getter returns (and will leave two conflicting boxes). Read ©con first and/or remove legacy cond when writing.

Suggested change

return this.getFirstQuickTimeString(Mpeg4BoxType.COND) // @TODO: Backwards compat for pre-6.0.2 releases

|| this.getFirstQuickTimeString(Mpeg4BoxType.CON);

return this.getFirstQuickTimeString(Mpeg4BoxType.CON)

|| this.getFirstQuickTimeString(Mpeg4BoxType.COND); // @TODO: Backwards compat for pre-6.0.2 releases

Copilot · 2026-04-06T06:21:35Z

src/mpeg4/mpeg4BoxType.ts

+    /**
+     * QuickTime subtitle box. This is listed in the FFMPEG source and Exiftool.
+     */
+    public static readonly ST3 = this.getType("©st3")


ST3 static field is missing the trailing semicolon, while the rest of the constants in this file use semicolons. Keeping the delimiter consistent avoids style churn and potential lint/format discrepancies.

Suggested change

public static readonly ST3 = this.getType("©st3")

public static readonly ST3 = this.getType("©st3");

Copilot · 2026-04-06T06:21:35Z

test-unit/mpeg4/appleTagTests.ts

+        assert.strictEqual(testTag.tag.subtitle, "foobarbaz");
    }

    @test


The new backwards-compat read test covers reading an existing Subt box, but it doesn't cover the important scenario where a file already has Subt and then subtitle is updated. Given the current getter/setter behavior, this can regress (stale legacy value winning). Add a unit test that starts with a Subt box, sets subtitle, and asserts the returned value and resulting boxes are correct.

Suggested change

@test

@test

public subtitle_setClearsExistingSubt() {

// Arrange

const box1 = this.getQuickTimeBox(Mpeg4BoxType.SUBT, "foobarbaz");

const testTag = this.getEmptyTag([box1.box]);

// Act

testTag.tag.subtitle = "fizzbuzz";

// Assert

assert.strictEqual(testTag.tag.subtitle, "fizzbuzz");

const st3Boxes = testTag.ilst.getQuickTimeDataBoxes(Mpeg4BoxType.ST3);

assert.strictEqual(st3Boxes.length, 1);

Testers.bvEqual(st3Boxes[0].data, ByteVector.fromString("fizzbuzz", StringType.UTF8));

const subtBoxes = testTag.ilst.getQuickTimeDataBoxes(Mpeg4BoxType.SUBT);

assert.isEmpty(subtBoxes);

}

@test

Copilot · 2026-04-06T06:21:35Z

test-unit/mpeg4/appleTagTests.ts

+        // Act / Assert
+        assert.strictEqual(testTag.tag.conductor, "foobarbaz");


Similarly for conductor, consider adding a test that starts with a legacy cond box, updates conductor, and asserts the getter returns the new value and legacy boxes are handled as intended (removed or ignored). This would prevent the legacy box from masking newly written data.

Suggested change

// Act / Assert

assert.strictEqual(testTag.tag.conductor, "foobarbaz");

// Assert legacy value is readable before update

assert.strictEqual(testTag.tag.conductor, "foobarbaz");

// Act

testTag.tag.conductor = "updated conductor";

// Assert

assert.strictEqual(testTag.tag.conductor, "updated conductor");

assert.strictEqual(

testTag.ilst.children.filter((c) => c.boxType === Mpeg4BoxType.COND).length,

0,

"legacy COND boxes should not remain after updating conductor"

);

assert.strictEqual(

testTag.ilst.children.filter((c) => c.boxType === Mpeg4BoxType.CON).length,

1,

"updating conductor should write a single CON box"

);

benrr101 added 2 commits April 2, 2026 14:35

Auditing mpeg4 box types

70eb13f

An letter

6f2d497

benrr101 added this to the v6.0.2 milestone Apr 3, 2026

benrr101 requested a review from Copilot April 3, 2026 04:54

Copilot started reviewing on behalf of benrr101 April 3, 2026 04:55 View session

Copilot AI reviewed Apr 3, 2026

View reviewed changes

src/mpeg4/mpeg4BoxType.ts Show resolved Hide resolved

src/mpeg4/mpeg4BoxType.ts Outdated Show resolved Hide resolved

src/mpeg4/mpeg4BoxType.ts Outdated Show resolved Hide resolved

src/mpeg4/mpeg4BoxType.ts Outdated Show resolved Hide resolved

Backwards compat and more tests

929067c

benrr101 requested a review from Copilot April 6, 2026 06:18

Copilot started reviewing on behalf of benrr101 April 6, 2026 06:19 View session

Copilot AI reviewed Apr 6, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Audit MPEG-4 box types#125

Audit MPEG-4 box types#125
benrr101 wants to merge 3 commits intodevelopfrom
fix/mpeg4-subtitle-metadata

benrr101 commented Apr 3, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Apr 6, 2026

Uh oh!

Copilot AI Apr 6, 2026

Uh oh!

Copilot AI Apr 6, 2026

Uh oh!

Copilot AI Apr 6, 2026

Uh oh!

Copilot AI Apr 6, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

		return this.getFirstQuickTimeString(Mpeg4BoxType.SUBT) // @TODO: Backwards compat for pre-6.0.2 releases
		\|\| this.getFirstQuickTimeString(Mpeg4BoxType.ST3);

	public static readonly ST3 = this.getType("©st3")
	public static readonly ST3 = this.getType("©st3");

-    @test
+    @test
+    public subtitle_setClearsExistingSubt() {
+        // Arrange
+        const box1 = this.getQuickTimeBox(Mpeg4BoxType.SUBT, "foobarbaz");
+        const testTag = this.getEmptyTag([box1.box]);
+        // Act
+        testTag.tag.subtitle = "fizzbuzz";
+        // Assert
+        assert.strictEqual(testTag.tag.subtitle, "fizzbuzz");
+        const st3Boxes = testTag.ilst.getQuickTimeDataBoxes(Mpeg4BoxType.ST3);
+        assert.strictEqual(st3Boxes.length, 1);
+        Testers.bvEqual(st3Boxes[0].data, ByteVector.fromString("fizzbuzz", StringType.UTF8));
+        const subtBoxes = testTag.ilst.getQuickTimeDataBoxes(Mpeg4BoxType.SUBT);
+        assert.isEmpty(subtBoxes);
+    }
+    @test

		// Act / Assert
		assert.strictEqual(testTag.tag.conductor, "foobarbaz");

-        // Act / Assert
-        assert.strictEqual(testTag.tag.conductor, "foobarbaz");
+        // Assert legacy value is readable before update
+        assert.strictEqual(testTag.tag.conductor, "foobarbaz");
+        // Act
+        testTag.tag.conductor = "updated conductor";
+        // Assert
+        assert.strictEqual(testTag.tag.conductor, "updated conductor");
+        assert.strictEqual(
+            testTag.ilst.children.filter((c) => c.boxType === Mpeg4BoxType.COND).length,
+,
+            "legacy COND boxes should not remain after updating conductor"
+        );
+        assert.strictEqual(
+            testTag.ilst.children.filter((c) => c.boxType === Mpeg4BoxType.CON).length,
+,
+            "updating conductor should write a single CON box"
+        );

Conversation

benrr101 commented Apr 3, 2026

Description

Testing

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Apr 6, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 6, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 6, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 6, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 6, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants