Skip to content

Audit MPEG-4 box types #125

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
benrr101 wants to merge 3 commits into
base: develop
Choose a base branch
from
Open

Audit MPEG-4 box types #125

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
70eb13f
Auditing mpeg4 box types
benrr101 Apr 2, 2026
6f2d497
An letter
benrr101 Apr 3, 2026
929067c
Backwards compat and more tests
benrr101 Apr 4, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 10 additions & 4 deletions src/mpeg4/appleTag.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,9 +55,12 @@ export default class AppleTag extends Tag {
public set title(v: string) { this.setQuickTimeString(Mpeg4BoxType.NAM, v); }

/** @inheritDoc */
public get subtitle(): string { return this.getFirstQuickTimeString(Mpeg4BoxType.SUBT); }
public get subtitle(): string {
return this.getFirstQuickTimeString(Mpeg4BoxType.SUBT) // @TODO: Backwards compat for pre-6.0.2 releases
|| this.getFirstQuickTimeString(Mpeg4BoxType.ST3);
Comment on lines +59 to +60
Copy link

Copilot AI Apr 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 uses AI. Check for mistakes.
}
/** @inheritDoc */
public set subtitle(v: string) { this.setQuickTimeString(Mpeg4BoxType.SUBT, v); }
public set subtitle(v: string) { this.setQuickTimeString(Mpeg4BoxType.ST3, v); }

/** @inheritDoc */
public get description(): string { return this.getFirstQuickTimeString(Mpeg4BoxType.DESC); }
Expand Down Expand Up @@ -209,9 +212,12 @@ export default class AppleTag extends Tag {
}

/** @inheritDoc */
public get conductor(): string { return this.getFirstQuickTimeString(Mpeg4BoxType.COND); }
public get conductor(): string {
return this.getFirstQuickTimeString(Mpeg4BoxType.COND) // @TODO: Backwards compat for pre-6.0.2 releases
|| this.getFirstQuickTimeString(Mpeg4BoxType.CON);
Comment on lines +216 to +217
Copy link

Copilot AI Apr 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 uses AI. Check for mistakes.
}
/** @inheritDoc */
public set conductor(v: string) { this.setQuickTimeString(Mpeg4BoxType.COND, v); }
public set conductor(v: string) { this.setQuickTimeString(Mpeg4BoxType.CON, v); }

/** @inheritDoc */
public get copyright(): string { return this.getFirstQuickTimeString(Mpeg4BoxType.CPRT); }
Expand Down
34 changes: 28 additions & 6 deletions src/mpeg4/mpeg4BoxType.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,11 @@
import {ByteVector, StringType} from "../byteVector";

/**
* Provides references to different box types used by the library. This class is used to severely reduce the number
* of times these types are created in {@link AppleTag,} greatly improving the speed at which warm files are read.
* Provides references to different box types used by the library. This class is used to severely
* reduce the number of times these types are created in {@link AppleTag,} greatly improving the
* speed at which warm files are read.
*
* These box types were cross-referenced with FFMPEG source and Exiftool database.
*/
export default class Mpeg4BoxType {
/** QuickTime album artist box */
Expand All @@ -13,7 +16,15 @@ export default class Mpeg4BoxType {
public static readonly ART = this.getType("©ART");
/** QuickTime comment box */
public static readonly CMT = this.getType("©cmt");
/** QuickTime conductor box? @TODO: Verify this works should not be ©con */
/**
* QuickTime conductor box. This is listed in the FFMPEG source and Exiftool.
*/
public static readonly CON = this.getType("©con");
/**
* Conductor box from original .NET source. This is not listed anywhere in the Exiftool or
* FFMPEG docs.
* @TODO: Remove this when backwards compat time has ended.
*/
public static readonly COND = this.getType("cond");
/** QuickTime cover art box */
public static readonly COVR = this.getType("covr");
Expand Down Expand Up @@ -91,19 +102,30 @@ export default class Mpeg4BoxType {
public static readonly STCO = this.getType("stco");
/** ISO sample description box */
public static readonly STSD = this.getType("stsd");
/** Subtitle box? @TODO: There's no record of this one */
/**
* QuickTime subtitle box. This is listed in the FFMPEG source and Exiftool.
*/
public static readonly ST3 = this.getType("©st3")
Copy link

Copilot AI Apr 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 uses AI. Check for mistakes.
/**
* Subtitle box from original .NET source. This is not listed anywhere in the Exiftool or
* FFMPEG docs.
* @TODO: Remove this when backwards compat time has ended.
*/
public static readonly SUBT = this.getType("Subt");
/** Alias text box? @TODO: There's no record of this one */
public static readonly TEXT = this.getType("text");
/** QuickTime BPM box */
public static readonly TMPO = this.getType("tmpo");
/** ISO track container box */
public static readonly TRAK = this.getType("trak");
/** QuickTime track number box */
/** QuickTime track number box @TODO: What about ©TRK as per FFMPEG source? */
public static readonly TRKN = this.getType("trkn");
/** ISO User data box */
public static readonly UDTA = this.getType("udta");
/** Alias URL box? @TODO: There's no record of this one */
/**
* Alias URL box?
* @remarks Specified in FFMPEG source but not in Exiftool.
*/
public static readonly URL = this.getType("©url");
/** ISO user extension box */
public static readonly UUID = this.getType("uuid");
Expand Down
75 changes: 42 additions & 33 deletions test-unit/mpeg4/appleTagTests.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,17 @@ import {TagTypes} from "../../src/tag";

@test
public subtitle() {
this.testQuickTimeString((t, v) => t.subtitle = v, (t) => t.subtitle, Mpeg4BoxType.SUBT);
this.testQuickTimeString((t, v) => t.subtitle = v, (t) => t.subtitle, Mpeg4BoxType.ST3);
}

@test
public subtitle_existingSubt() {
// Arrange
const box1 = this.getQuickTimeBox(Mpeg4BoxType.SUBT, "foobarbaz");
const testTag = this.getEmptyTag([box1.box]);

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

@test
Copy link

Copilot AI Apr 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 uses AI. Check for mistakes.
Expand Down Expand Up @@ -169,7 +179,7 @@ import {TagTypes} from "../../src/tag";
ByteVector.fromShort(0),
AppleDataBoxFlagType.ContainsData
);
const box4 = this.getQuickTimeBox(Mpeg4BoxType.GNRE, ByteVector.fromString("foo", StringType.UTF8));
const box4 = this.getQuickTimeBox(Mpeg4BoxType.GNRE, "foo");
const tag = this.getEmptyTag([box3.box, box4.box, box1.box, box2.box]);

// Act
Expand Down Expand Up @@ -210,7 +220,7 @@ import {TagTypes} from "../../src/tag";
// Arrange
const box1 = this.getQuickTimeBox(Mpeg4BoxType.GNRE, ByteVector.fromShort(1));
const value2 = "foo; bar; baz";
const box2 = this.getQuickTimeBox(Mpeg4BoxType.GEN, ByteVector.fromString(value2, StringType.UTF8));
const box2 = this.getQuickTimeBox(Mpeg4BoxType.GEN, value2);
const tag = this.getEmptyTag([box1.box, box2.box]);

// Act
Expand Down Expand Up @@ -265,9 +275,9 @@ import {TagTypes} from "../../src/tag";
@test
public year_multipleBoxes() {
// Arrange
const box1 = this.getQuickTimeBox(Mpeg4BoxType.DAY, ByteVector.fromString("asdf", StringType.UTF8));
const box2 = this.getQuickTimeBox(Mpeg4BoxType.DAY, ByteVector.fromString("123456", StringType.UTF8));
const box3 = this.getQuickTimeBox(Mpeg4BoxType.DAY, ByteVector.fromString("234", StringType.UTF8));
const box1 = this.getQuickTimeBox(Mpeg4BoxType.DAY, "asdf");
const box2 = this.getQuickTimeBox(Mpeg4BoxType.DAY, "123456");
const box3 = this.getQuickTimeBox(Mpeg4BoxType.DAY, "234");
const testTag = this.getEmptyTag([box1.box, box2.box, box3.box]);

// Act / Assert
Expand All @@ -277,7 +287,7 @@ import {TagTypes} from "../../src/tag";
@test
public year_setMultipleBoxes() {
// Arrange
const box1 = this.getQuickTimeBox(Mpeg4BoxType.DAY, ByteVector.fromString("1234", StringType.UTF8));
const box1 = this.getQuickTimeBox(Mpeg4BoxType.DAY, "1234");
const testTag = this.getEmptyTag([box1.box, box1.box, box1.box]);

// Act / Assert
Expand All @@ -289,7 +299,7 @@ import {TagTypes} from "../../src/tag";
@test
public year_setZero_clearsMultiple() {
// Arrange
const box1 = this.getQuickTimeBox(Mpeg4BoxType.DAY, ByteVector.fromString("1234", StringType.UTF8));
const box1 = this.getQuickTimeBox(Mpeg4BoxType.DAY, "1234");
const testTag = this.getEmptyTag([box1.box, box1.box, box1.box]);

// Act / Assert
Expand Down Expand Up @@ -429,7 +439,17 @@ import {TagTypes} from "../../src/tag";

@test
public conductor() {
this.testQuickTimeString((t, v) => t.conductor = v, (t) => t.conductor, Mpeg4BoxType.COND);
this.testQuickTimeString((t, v) => t.conductor = v, (t) => t.conductor, Mpeg4BoxType.CON);
}

@test
public conductor_existingCondBox() {
// Arrange
const box1 = this.getQuickTimeBox(Mpeg4BoxType.COND, "foobarbaz");
const testTag = this.getEmptyTag([box1.box]);

// Act / Assert
assert.strictEqual(testTag.tag.conductor, "foobarbaz");
Comment on lines +451 to +452
Copy link

Copilot AI Apr 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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"
);

Copilot uses AI. Check for mistakes.
}

@test
Expand Down Expand Up @@ -464,18 +484,9 @@ import {TagTypes} from "../../src/tag";
@test
public dateTagged_multipleBoxes() {
// Arrange
const box1 = this.getQuickTimeBox(
Mpeg4BoxType.DTAG,
ByteVector.fromString("asdf", StringType.UTF8)
);
const box2 = this.getQuickTimeBox(
Mpeg4BoxType.DTAG,
ByteVector.fromString("2023-10-21T10:46:00", StringType.UTF8)
);
const box3 = this.getQuickTimeBox(
Mpeg4BoxType.DTAG,
ByteVector.fromString("2023-10-21", StringType.UTF8)
);
const box1 = this.getQuickTimeBox(Mpeg4BoxType.DTAG, "asdf");
const box2 = this.getQuickTimeBox(Mpeg4BoxType.DTAG, "2023-10-21T10:46:00");
const box3 = this.getQuickTimeBox(Mpeg4BoxType.DTAG, "2023-10-21");
const testTag = this.getEmptyTag([box1.box, box2.box, box3.box]);

// Act / Assert
Expand All @@ -485,10 +496,7 @@ import {TagTypes} from "../../src/tag";
@test
public dateTagged_setMultipleBoxes() {
// Arrange
const box1 = this.getQuickTimeBox(
Mpeg4BoxType.DTAG,
ByteVector.fromString("1900-10-21T10:56:00", StringType.UTF8)
);
const box1 = this.getQuickTimeBox(Mpeg4BoxType.DTAG, "1900-10-21T10:56:00");
const testTag = this.getEmptyTag([box1.box, box1.box, box1.box]);
const testValue = new Date("2023-10-21 10:59:00");

Expand All @@ -504,10 +512,7 @@ import {TagTypes} from "../../src/tag";
@test
public dateTagged_setZero_clearsMultiple() {
// Arrange
const box1 = this.getQuickTimeBox(
Mpeg4BoxType.DTAG,
ByteVector.fromString("2021-10-21T11:04:00", StringType.UTF8)
);
const box1 = this.getQuickTimeBox(Mpeg4BoxType.DTAG, "2021-10-21T11:04:00");
const testTag = this.getEmptyTag([box1.box, box1.box, box1.box]);

// Act / Assert
Expand Down Expand Up @@ -1187,9 +1192,9 @@ import {TagTypes} from "../../src/tag";

// TEST CASE 3: Multiple boxes return all valid instances ----------
// Valid box type and flags
const box1 = this.getQuickTimeBox(boxType, ByteVector.fromString("foo", StringType.UTF8));
const box1 = this.getQuickTimeBox(boxType, "foo");
// Valid box type and flags
const box2 = this.getQuickTimeBox(boxType, ByteVector.fromString("bar", StringType.UTF8));
const box2 = this.getQuickTimeBox(boxType, "bar");
// Valid box type, invalid flags
const box3 = this.getQuickTimeBox(
boxType,
Expand All @@ -1202,7 +1207,7 @@ import {TagTypes} from "../../src/tag";
box4.addChild(box2.dataBox);
box4.addChild(box3.dataBox);
// Multiple values in single data box
const box5 = this.getQuickTimeBox(boxType, ByteVector.fromString("fux; bux; quxx", StringType.UTF8));
const box5 = this.getQuickTimeBox(boxType, "fux; bux; quxx");

const testTag2 = this.getEmptyTag([box1.box, box2.box, box3.box, box4, box5.box]);

Expand Down Expand Up @@ -1454,10 +1459,14 @@ import {TagTypes} from "../../src/tag";

private getQuickTimeBox(
boxType: ByteVector,
value: ByteVector,
value: ByteVector|string,
flags: AppleDataBoxFlagType = AppleDataBoxFlagType.ContainsText
): {box: AppleAnnotationBox, dataBox: AppleDataBox} {
value = typeof(value) === "string"
? ByteVector.fromString(value, StringType.UTF8)
: value;
const dataBox = AppleDataBox.fromDataAndFlags(value, flags);

const box = AppleAnnotationBox.fromType(boxType);
box.addChild(dataBox);

Expand Down