Skip to content

Fix SaizBox truncate sizing for default sample info entries#1704

Open
p-bond wants to merge 4 commits into
dev_sprint_25_2from
feature/VPAAMP_739_fix_SaizBox_Truncate
Open

Fix SaizBox truncate sizing for default sample info entries#1704
p-bond wants to merge 4 commits into
dev_sprint_25_2from
feature/VPAAMP_739_fix_SaizBox_Truncate

Conversation

@p-bond

@p-bond p-bond commented Jul 8, 2026

Copy link
Copy Markdown
Contributor

Fix SaizBox truncate sizing for default sample info layouts

SaizBox::truncate assumed every SAIZ box had a per-sample info table and always reduced the box size by sample_count - 1 when truncating to one sample. That is only valid when default_sample_info_size is zero. For boxes that use a non-zero default sample info size, there is no per-sample table to shrink, so the old logic could write an invalid smaller size and corrupt the rewritten buffer.

This change preserves whether a per-sample table is present when parsing SAIZ and only shrinks the box when that table exists. It also returns early for sample counts of 0 or 1 to avoid invalid size arithmetic.

The L1 IsoBmffBoxTests suite now covers the table-present path, the default-size path where size must stay unchanged, and the low-sample guard behavior.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Fixes SaizBox::truncate() sizing when SAIZ uses a non-zero default_sample_info_size (i.e., no per-sample info table to shrink), preventing invalid box-size writes that can corrupt rewritten ISO BMFF buffers.

Changes:

  • Track whether SAIZ parsing encountered a per-sample info size table and only shrink the SAIZ box when that table exists.
  • Add an early return in SaizBox::truncate() for numSamples <= 1 to avoid invalid size arithmetic.
  • Expand IsoBmffBoxTests coverage for SAIZ: table-present vs default-size layouts and low-sample guard behavior.

Reviewed changes

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

File Description
isobmff/isobmffbox.cpp Preserve SAIZ layout details during parsing and conditionally shrink size during truncate.
isobmff/isobmffbox.h Extend SaizBox state and constructor signature to carry “table present” info.
test/utests/tests/IsoBmffBoxTests/IsoBmffBoxTests.cpp Add L1 tests validating SAIZ truncation behavior across SAIZ layouts and sample counts.

Comment thread isobmff/isobmffbox.h
Comment thread isobmff/isobmffbox.cpp

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

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

Comments suppressed due to low confidence (1)

isobmff/isobmffbox.cpp:1690

  • SaizBox::truncate() now falls into the else branch (and logs "No room for a skip box") whenever hasPerSampleInfoTable is false, because oldSize == newSize makes (oldSize - newSize) >= SIZEOF_SIZE_AND_TAG false. For default-sample-info-size SAIZ boxes, this log is misleading (there was no truncation attempt and room is irrelevant) and could add noise in normal playback.
	// Need min 8 bytes to insert a skip box
	if ((oldSize - newSize) >= SIZEOF_SIZE_AND_TAG)
	{
		WRITE_U32(getBase(), newSize);
		SkipBox skip{oldSize - newSize, getBase() + newSize};
	}
	else
	{
		AAMPLOG_INFO("No room for a skip box");
		// Not truncating the table, just setting the sample count to 1
	}

@p-bond p-bond marked this pull request as ready for review July 9, 2026 16:06
@p-bond p-bond requested a review from a team as a code owner July 9, 2026 16:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants