Expose track_in_movie and track_in_preview flags on Tkhd#146
Expose track_in_movie and track_in_preview flags on Tkhd#146iameli wants to merge 1 commit intokixelated:mainfrom
Conversation
The tkhd box has three flag bits: track_enabled (bit 0), track_in_movie (bit 1), and track_in_preview (bit 2). Previously only track_enabled was exposed as a public field. The other two were decoded but discarded, and always written as false on encode. Add `in_movie` and `in_preview` fields to Tkhd so callers can read and write all three flags. Update all test expectations to match the actual flag values in their respective fixtures. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
WalkthroughTwo new public boolean fields, 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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. Comment Tip You can validate your CodeRabbit configuration file in your editor.If your editor has YAML language server, you can enable auto-completion and validation by adding |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/moov/trak/tkhd.rs (1)
198-199: Consider adding a direct serialized-flags assertion in unit tests.Current tests are round-trip based; adding one direct assertion on the version+flags word would make bit-position regressions easier to catch.
Suggested test hardening
#[test] fn test_tkhd32() { let expected = Tkhd { creation_time: 100, modification_time: 200, track_id: 1, duration: 634634, layer: 0, alternate_group: 0, volume: 1.into(), matrix: Matrix::default(), width: 512.into(), height: 288.into(), enabled: true, in_movie: true, in_preview: false, }; let mut buf = Vec::new(); expected.encode(&mut buf).unwrap(); + // size(4) + kind(4) + version/flags(4) + assert_eq!(&buf[8..12], &[0x01, 0x00, 0x00, 0x03]); let mut buf = buf.as_ref(); let decoded = Tkhd::decode(&mut buf).unwrap(); assert_eq!(decoded, expected); }Also applies to: 223-224
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/moov/trak/tkhd.rs` around lines 198 - 199, Tests currently only round-trip the TrackHeaderBox; add a direct assertion that the packed version+flags 32-bit word matches the expected literal to catch bit-position regressions. In the unit test for TrackHeaderBox (the case that sets in_movie: true and in_preview: false) call the serializer used (e.g., TrackHeaderBox::to_bytes / write_box / serialize) and extract the first 4 bytes (version+flags) or use the helper that builds the version_and_flags word, then assert equality against the expected u32 constant (computed from version and the flag bits). Reference the fields in_movie and in_preview and the TrackHeaderBox serialization path so the test fails if flag bit positions change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/moov/trak/tkhd.rs`:
- Around line 198-199: Tests currently only round-trip the TrackHeaderBox; add a
direct assertion that the packed version+flags 32-bit word matches the expected
literal to catch bit-position regressions. In the unit test for TrackHeaderBox
(the case that sets in_movie: true and in_preview: false) call the serializer
used (e.g., TrackHeaderBox::to_bytes / write_box / serialize) and extract the
first 4 bytes (version+flags) or use the helper that builds the
version_and_flags word, then assert equality against the expected u32 constant
(computed from version and the flag bits). Reference the fields in_movie and
in_preview and the TrackHeaderBox serialization path so the test fails if flag
bit positions change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 790684be-dab9-4ba2-ac19-d75021888af0
📒 Files selected for processing (11)
src/moov/mod.rssrc/moov/trak/tkhd.rssrc/test/av1.rssrc/test/bbb.rssrc/test/esds.rssrc/test/flac.rssrc/test/h264.rssrc/test/hevc.rssrc/test/libavif_anim.rssrc/test/uncompressed.rssrc/test/vp9.rs
bradh
left a comment
There was a problem hiding this comment.
LGTM.
A round-trip test would be nice, but I can try to find some time for that later.
Ran into this while working on the MUXL spec; I think having
track_in_movieset is probably a sensible default. Now we can do that!From Claude: