fix: xmp jpeg write #1156

cdmurph32 · 2025-06-16T21:17:12Z

Changes in this pull request

Adding the manifest url to large XMP segments could cause the XMP data
to exceed the limit of 65503 bytes.

There were 3 causes of this issue:

Extra whitespace from formatting. The quick_xml writer shiftwidth
could be much larger than that of the original. Potentially adding
several KB of whitespace.
Manifest url added multiple times. The manifest url was added to
multiple keys within the XMP data.
Lack of padding awareness. According to the spec, XMP segments should
have 2KB to 4KB of padding. The padding should be adjusted to keep
the XMP data length the same with the addition of the remote manifest
url. This is important for data hashes as it keeps the offsets of the
JPEG metadata the same.

Checklist

This PR represents a single feature, fix, or change.
All applicable changes have been documented.
Any TO DO items (or similar) have been entered as GitHub issues and the link to that issue has been included in a comment.

Adding the manifest url to large XMP segments could cause the XMP data to exceed the limit of 65503 bytes. There were 3 causes of this issue: 1. Extra whitespace from formatting. The quick_xml writer shiftwidth could be much larger than that of the original. Potentially adding several KB of whitespace. 2. Manifest url added multiple times. The manifest url was added to multiple keys within the XMP data. 3. Lack of padding awareness. According to the spec, XMP segments should have 2KB to 4KB of padding. The padding should be adjusted to keep the XMP data length the same with the addition of the remote manifest url. This is important for data hashes as it keeps the offsets of the JPEG metadata the same.

codecov · 2025-06-16T21:20:11Z

Codecov Report

Attention: Patch coverage is 97.91667% with 1 line in your changes missing coverage. Please review.

Project coverage is 79.31%. Comparing base (47df42e) to head (3fa75eb).

Files with missing lines	Patch %	Lines
sdk/src/utils/xmp_inmemory_utils.rs	97.87%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1156      +/-   ##
==========================================
+ Coverage   79.29%   79.31%   +0.02%     
==========================================
  Files         147      147              
  Lines       40701    40743      +42     
==========================================
+ Hits        32274    32316      +42     
  Misses       8427     8427

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

cdmurph32 · 2025-06-16T21:35:03Z

sdk/src/asset_handlers/jpeg_io.rs

@@ -620,7 +620,7 @@ impl RemoteRefEmbed for JpegIO {
                    .unwrap_or((None, MIN_XMP));

                // add provenance and JPEG XMP prefix
-                let xmp = format!("{XMP_SIGNATURE}\0 {}", add_provenance(xmp, &manifest_uri)?);
+                let xmp = format!("{XMP_SIGNATURE}\0{}", add_provenance(xmp, &manifest_uri)?);


This extra space caused there to be a leading space in the XMP data

cdmurph32 · 2025-06-16T21:36:14Z

sdk/src/utils/xmp_inmemory_utils.rs

+    // Minimal padding should be 2 KB to 4 KB. This is used if no XMP packet is found.
+    let mut target_length = 4096;
+    // Remove trailing xpacket if present for easier manipulation
+    let xpacket_end_single = "<?xpacket end='w'?>";


Single quotes are what the spec has, but we specifically test for double quotes. The test images from cameras also use double quotes.
<?xpacket end="w"?>

I think this needs to be more flexible. You may also encounter end='r'.

Or we could be a good citizen and respect the 'r' which means read only.

cdmurph32 · 2025-06-16T21:37:12Z

sdk/src/utils/xmp_inmemory_utils.rs

@@ -130,13 +149,14 @@ fn add_xmp_key(xmp: &str, key: &str, value: &str) -> Result<String> {
                if !added {
                    // didn't exist, so add it
                    elem.push_attribute((key, value));
+                    added = true;


The lack of added = true is why the remote manifests url was being added multiple times.

Thanks for tracking this down!

cdmurph32 · 2025-06-16T21:38:15Z

sdk/src/utils/xmp_inmemory_utils.rs

+    )]
+    #[cfg_attr(target_os = "wasi", wstd::test)]
+    async fn test_broken_xmp_read_write_stream() {
+        let source_bytes = include_bytes!("../../tests/fixtures/broken.jpg");


Should we rename the file?

This is not our file so I don't think we can include it in a unit test.

That's too bad. It would be cool to have a picture from summit in the repo. :-).

You can ask Jen if we can use it.

We try to keep our repo images small, this one is over 6mb.

mauricefisher64 · 2025-06-17T13:26:00Z

sdk/src/utils/xmp_inmemory_utils.rs

+    let xpacket_end_double = "<?xpacket end=\"w\"?>";
+    let xmp_body = if let Some(pos) = xmp.rfind(xpacket_end_single) {
+        target_length = orig_length - xpacket_end_single.len();
+        xmp[..pos].trim_end()


As I commented above, I would just search for "<?xpacket" since the rest of the tag may vary.

We definitely need to be flexible reading here. I've also found cases where the end just wasn't there and other tools think it is fine.

If the end is completely missing then I can't just look for <?xpacket since that could be the beginning. The test add_xmp does this.

mauricefisher64 · 2025-06-17T13:27:21Z

sdk/src/utils/xmp_inmemory_utils.rs

+        xmp[..pos].trim_end()
+    } else if let Some(pos) = xmp.rfind(xpacket_end_double) {
+        target_length = orig_length - xpacket_end_double.len();
+        xmp[..pos].trim_end()


The change I suggested about removes the else if case.

mauricefisher64 · 2025-06-17T13:45:32Z

sdk/src/utils/xmp_inmemory_utils.rs

@@ -177,6 +198,8 @@ fn add_xmp_key(xmp: &str, key: &str, value: &str) -> Result<String> {
            }
        }
    }
+    let padding_length = target_length.saturating_sub(writer.get_ref().get_ref().len());


Shouldn't this take into account the existing padding to keep the length the same if possible? This seems like it would not take into account the existing padding. Is there somewhere you account to the existing padding?

Yes. Line 107

target_length = orig_length - xpacket_end_length;

cdmurph32 added 3 commits June 16, 2025 17:05

fix: remove test debugging

c14e075

fix: rust fmt

4794309

fix: add test asset

c121378

cdmurph32 changed the title ~~Fix xmp jpeg write~~ Fix: xmp jpeg write Jun 16, 2025

fix: fix wasm tests

3037059

cdmurph32 changed the title ~~Fix: xmp jpeg write~~ fix: xmp jpeg write Jun 16, 2025

cdmurph32 commented Jun 16, 2025

View reviewed changes

mauricefisher64 reviewed Jun 17, 2025

View reviewed changes

mauricefisher64 requested a review from gpeacock June 17, 2025 14:02

cdmurph32 added 3 commits June 18, 2025 11:29

Comment incorporation

e9bf11e

fix: Add comment

36cc914

Merge branch 'main' into fix_xmp_jpeg_write

c3710b7

cdmurph32 force-pushed the fix_xmp_jpeg_write branch from 3fa75eb to c3710b7 Compare June 19, 2025 14:46

cdmurph32 requested a review from mauricefisher64 June 19, 2025 22:22

fix: xmp jpeg write #1156

Are you sure you want to change the base?

fix: xmp jpeg write #1156

Uh oh!

Conversation

cdmurph32 commented Jun 16, 2025

Changes in this pull request

Checklist

Uh oh!

codecov bot commented Jun 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cdmurph32 Jun 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cdmurph32 Jun 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mauricefisher64 Jun 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

codecov bot commented Jun 16, 2025 •

edited

Loading

cdmurph32 Jun 16, 2025 •

edited

Loading

cdmurph32 Jun 18, 2025 •

edited

Loading

mauricefisher64 Jun 17, 2025 •

edited

Loading