Make remaining bytes validation conditional on strict mode

[libark]

This PR makes non-strict builds more tolerant of leftover bytes in MP4 atom bodies, improving robustness when decoding real-world files. Strict validation remains unchanged for tests and strict-mode builds.

[coderabbitai]

Walkthrough

The change conditions remaining-body checks during atom decoding on build configuration. In src/any.rs, src/atom.rs, and src/tokio/atom.rs, checks that previously returned UnderDecode when leftover bytes remained are now executed only when the "strict" feature is enabled or under cfg(test). In non-strict, non-test builds leftover bytes in atom bodies are no longer treated as UnderDecode. No public signatures were changed.

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title accurately and concisely describes the main change: making remaining bytes validation conditional on strict mode, which is reflected across all modified files.
Description check	✅ Passed	The description directly relates to the changeset, explaining that non-strict builds are now more tolerant of leftover bytes in MP4 atom bodies while strict validation remains unchanged.

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

✨ Finishing touches

• 📝 Generate docstrings

---

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

🤖 Fix all issues with AI agents

In `@src/atom.rs`:
- Around line 66-68: The trailing-bytes rejection in the ReadFrom<Option<T>>
implementation is currently unconditional and inconsistent with the earlier
check that only errors when cfg!(feature = "strict") || cfg!(test); update the
Option<T> ReadFrom impl (and the similar occurrence around the other block at
lines referenced in the review) to gate the UnderDecode(T::KIND) return behind
the same condition (cfg!(feature = "strict") || cfg!(test)) so non-strict builds
tolerate trailing bytes like the other path; locate the check inside the impl
ReadFrom for Option<T> and the other matching block and wrap or replace the
unconditional Err(...) with the conditional check used earlier.

[bradh]

Apart from the issue flagged by the review bot, it'd be good to test this. A bit tricky with the test == strict case.

And from my review on #111:

Did this behaviour get mandated in ISOBMFF? I recall some discussion, but not the outcome.

[libark]

And from my review on #111:

Did this behaviour get mandated in ISOBMFF? I recall some discussion, but not the outcome.

ISOBMFF does not mandate tail padding or alignment for boxes. The only strict requirement is that the box size correctly covers all bytes.

MOV writers often pad atoms with zeros to align to 4 or 8 byte boundaries. This is a legacy convention, not required by ISOBMFF, and parsers should handle it.

[bradh]

The modified behaviour requires tests. Unsure of the best way to do this - probably turning off strict mode whenever we're testing, and checking with and without the feature flag enabled.

This is a legacy convention, not required by ISOBMFF, and parsers should handle it.

OK. Can you add the quicktime reference for this as an explanatory comment in the code?

[kixelated]

Should we log when the entire box isn't processed, similar to when unknown atoms are parsed?

The reason I added UnderDecode was primarily to catch implementation bugs and it's saved me a bunch of times. We can explicitly skip boxes that often have padding if that's true @libark, double checking any specs and maybe validating that it's zeroed or something.

[libark]

Should we log when the entire box isn't processed, similar to when unknown atoms are parsed?

The reason I added UnderDecode was primarily to catch implementation bugs and it's saved me a bunch of times. We can explicitly skip boxes that often have padding if that's true @libark, double checking any specs and maybe validating that it's zeroed or something.

Checking whether the remaining bytes are zeroed makes sense from a correctness perspective, but it does come with a performance cost.

Using a strict mode here seems like a good compromise: it allows catching potential issues during development, while preserving optimal performance and compatibility for regular crate users.

[kixelated]

Checking whether the remaining bytes are zeroed makes sense from a correctness perspective, but it does come with a performance cost..

I doubt there would be any measurable performance cost.

Could you make a helper method similar to the one for unknown boxes? Return under-decode when strict, log a warning otherwise?

To avoid spurious logging, we could also check for zeroed padding and only when the logger is enabled?

[bradh]

Is the "residual" padding always to a 4 byte or 8 byte boundary? Maybe that is a simpler (or additional) check.