Skip to content

Conversation

@ok-nick
Copy link
Contributor

@ok-nick ok-nick commented Oct 27, 2025

Adds extra validation before loading the JUMBF to confirm the input format type matches the given stream, otherwise outputting a more specific error.

Some format types such as MP3 and SVG may require a more in-depth structural check.

Copy link
Collaborator

@mauricefisher64 mauricefisher64 left a comment

Choose a reason for hiding this comment

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

Approved with some sugguestions

@ok-nick ok-nick force-pushed the ok-nick/file-signature-check branch from 3a94319 to 04cb96e Compare October 27, 2025 20:52
@ok-nick ok-nick marked this pull request as ready for review October 28, 2025 13:55
@codspeed-hq
Copy link

codspeed-hq bot commented Oct 28, 2025

CodSpeed Performance Report

Merging #1528 will not alter performance

Comparing ok-nick/file-signature-check (b04c214) with main (6a28095)

Summary

✅ 16 untouched
⏩ 2 skipped1

Footnotes

  1. 2 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports.

Copy link
Contributor

@tmathern tmathern left a comment

Choose a reason for hiding this comment

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

If we are unsure about mp3 and svg and need more work/verifications, I'd rather we split it out. To avoid taking the risk of breaking (currently working) edge cases.
The corollary is to add good test coverage using real files to raise confidence we covered all cases (is it even possible with mp3?).

@tmathern tmathern self-requested a review October 29, 2025 01:52
Copy link
Contributor

@tmathern tmathern left a comment

Choose a reason for hiding this comment

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

Approving assuming the SVG related question has the same answer as MP3 (please confirm).

@ok-nick
Copy link
Contributor Author

ok-nick commented Nov 14, 2025

I will be changing this PR to perform the support checks as it's being read. This will be much more reliable since we can use our normal parsing methods (like XML for SVG) and it can be done in one pass. We also do this already for some formats, like GIF.

@ok-nick ok-nick added the check-release Add this label to any PR to invoke a larger suite of tests. label Dec 3, 2025
@ok-nick
Copy link
Contributor Author

ok-nick commented Dec 3, 2025

The reason for adding per-parser errors:

Yes it is odd right now, I agree. Eventually I imagine that all error types returned by (for example) BMFF would be contained in the BmffError enum. Any error returned would then include the error parsing BMFF: some more specific error context. That would make it much easier to reason about adding more granular errors to the asset handlers without appending it onto the already giant c2pa::Error enum.

For more context, the idea is that eventually when are errors are more structured, libraries such as anyhow can output much more detailed errors such as:

Error: Decoding: reading asset at "foo.jpg"
Caused by:
    0: XMP parse error: missing rdf:Description
    1: XML error at 14:23: unexpected token '>'
    2: I/O error: early EOF

Backtrace:
   0: c2pa_rs::xmp::parse at crates/xmp/src/parse.rs:42:9
   1: c2pa_rs::decode::from_jpeg at crates/core/src/decode.rs:118:23
   2: app::main at src/main.rs:12:5
   ...

@ok-nick ok-nick merged commit 247c3a1 into main Dec 3, 2025
62 checks passed
@ok-nick ok-nick deleted the ok-nick/file-signature-check branch December 3, 2025 20:26
@caiopensrc caiopensrc mentioned this pull request Dec 2, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

check-release Add this label to any PR to invoke a larger suite of tests. safe to test

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants