fix: preserve per-sample fields on trun encode roundtrip

[ksletmoe-aws]

Problem

The trun encoder uses all-or-nothing semantics for per-sample fields (duration, size, flags, cts). If any entry has None for a field, the encoder does not write that field for any entry.

After decode, entries that inherited defaults from tfhd (default_sample_duration, default_sample_size, default_sample_flags) have None in the corresponding TrunEntry field. A decode→encode roundtrip silently drops those fields entirely, producing invalid output.

This manifests as:

• first_sample_flags lost: entry[0] gets Some(keyframe_flags) from first_sample_flags, entries[1..N] get None. Encoder sees not-all-Some, drops flags for all entries. Keyframe signaling is lost.
• sample_duration dropped: With frag_every_frame, the first entry may have an explicit duration while the rest inherit from tfhd. Encoder drops duration entirely, producing duration=0 in re-encoded output.
• sample_size dropped: Same pattern as duration.

Fix

Change from all() to any() semantics: if at least one entry has a value for a field, set the trun flag and write the field for all entries, backfilling None entries with 0.

Special case preserved: when only entry[0] has flags and the rest are None, use first_sample_flags (compact encoding matching the original).

Testing

• first_sample_flags_roundtrip — first-only flags pattern preserved
• mixed_flags_uses_per_sample — mixed Some/None flags backfilled
• all_flags_roundtrip — all-explicit flags unchanged
• duration_backfill_roundtrip — None duration backfilled to 0
• size_backfill_roundtrip — None size backfilled to 0
• all_none_fields_omitted — all-None fields still omitted (no wasted bytes)
• Full test suite (212 tests) passes with no regressions

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: de15d51b-737d-4b91-a9d7-ab0a29163fe8

📥 Commits

Reviewing files that changed from the base of the PR and between d51b778 and 23db21a.

📒 Files selected for processing (1)

• src/moof/traf/trun.rs

✅ Files skipped from review due to trivial changes (1)

• src/moof/traf/trun.rs

---

Walkthrough

The trun encoder now detects a strict “first-only” flags pattern (first entry’s flags is Some, all others None) and sets first_sample_flags, emitting the first entry’s flags. If any entry has flags but the pattern is not first-only, it enables sample_flags and encodes a flag value for every sample, backfilling None as 0. The same “any-entry-set → enable field” rule and None→0 backfill were applied to sample_duration, sample_size, and sample_cts. Unit tests were added for these behaviors and for omission when all entries are None.

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title accurately describes the main fix: preserving per-sample fields during trun encode roundtrips, which is the core issue addressed in this changeset.
Description check	✅ Passed	The description is detailed and directly related to the changeset, clearly explaining the problem, fix, and testing approach for the trun encoder roundtrip issue.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@src/moof/traf/trun.rs`:
- Around line 99-102: The current first_only_flags detection (using all_flags
and first().is_some_and(...)) is too broad and treats cases like [Some, Some,
None] as "first-only", causing later explicit flags to be dropped; change the
check to true only when the first entry has Some flags and every subsequent
entry has None (e.g., first_has = self.entries.first().map(|s|
s.flags.is_some()).unwrap_or(false) && self.entries.iter().skip(1).all(|s|
s.flags.is_none())); update usages of first_only_flags (the variable computed in
trun.rs and the encoding branches that write only the first flag vs per-entry
flags) so they rely on this stricter predicate and thus preserve any later
explicit flags.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: bbaa06aa-cb02-4ed7-a4bb-d3b003d7615f

📥 Commits

Reviewing files that changed from the base of the PR and between b81f0c4 and d7360f8.

📒 Files selected for processing (1)

• src/moof/traf/trun.rs

[kixelated]

This seems like a bug? Could we fix it?

[ksletmoe-aws]

Good catch — fixed. The encoder now backfills None flags with 0 and emits per-sample sample_flags, so the [Some, Some, None] case roundtrips losslessly instead of silently dropping everything. Updated the test to match.

[coderabbitai]

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@src/moof/traf/trun.rs`:
- Around line 113-141: The encoder must not backfill tfhd-inherited None values
with zeroes; update trun.rs so that before encoding you detect mixed presence
for sample_duration, sample_size, and sample_flags (and ensure
first_sample_flags' entry exists) and return a Result::Err using the crate's
custom error type (defined in error.rs) when some entries have Some(...) and
others have None instead of silently writing 0. Concretely: instead of using
any(|s| s.field.is_some()) + unwrap_or(0), compute whether the field is
uniformly present (all entries have Some) or uniformly absent (all None); if
uniformly present, encode the values as now; if uniformly absent, do not mark
the field in the trun header; if mixed, return an appropriate error (e.g.
Error::TrunMixedInheritedField(...) or similar). Also ensure when
ext.first_sample_flags is set you verify entries.get(0).and_then(|e|
e.flags).is_some() else return an error rather than unwrap. Use the existing
encode(buf) call paths but only write values when the field is actually present
for all entries.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 300f6f2c-f358-4ab7-acf0-9d567362778e

📥 Commits

Reviewing files that changed from the base of the PR and between e792465 and d51b778.

📒 Files selected for processing (1)

• src/moof/traf/trun.rs

[bradh]

Thanks for the PR. I haven't forgotten this, just need to get back to check what Ed. 8 says.

[bradh]

Thanks. Sorry this took so long.