(MINOR) Remove previously embedded manifests for remote manifests #136
Conversation
Codecov Report
@@ Coverage Diff @@
## main #136 +/- ##
==========================================
+ Coverage 78.22% 78.37% +0.15%
==========================================
Files 66 66
Lines 14495 14565 +70
==========================================
+ Hits 11338 11415 +77
+ Misses 3157 3150 -7
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
| @@ -32,7 +32,7 @@ use crate::{ | |||
| cose_validator::verify_cose, | |||
| jumbf_io::{ | |||
| get_file_extension, get_supported_file_extension, is_bmff_format, load_jumbf_from_file, | |||
There was a problem hiding this comment.
We should probably do one more thing in this function. If we remove the manifest we should clear the XMP. It will be cleared for the remote case but not the sidecar case.
There was a problem hiding this comment.
I can add that in a different PR as to not hold this up.
There was a problem hiding this comment.
good point. We might need to check if there was something we needed to delete in the case where xmp_write is disabled, otherwise we could return an error when there's nothing wrong.
We also need to figure out how to remove c2pa jumbf from in memory buffers. But that can also wait for later.
sdk/src/manifest.rs
Outdated
|
|
||
| #[cfg(all(feature = "file_io", feature = "xmp_write"))] | ||
| #[actix::test] | ||
| async fn test_embed_sidecar_with_parent_manifest() { |
There was a problem hiding this comment.
Why is this an async test? I don't see any await or async inside the function.
There was a problem hiding this comment.
good point, I removed async from some of the other tests that didn't need it.
gpeacock
changed the title
…ff_support * 'main' of https://github.com/contentauth/c2pa-rs: (78 commits) Add Exif Assertion support (#140) (IGNORE) Disregard grep non-zero exit status (#139) Prepare 0.14.0 release (IGNORE) Fix GH Actions logic error that is causing CI failure now (#137) (MINOR) Remove previously embedded manifests for remote manifests (#136) (MINOR) Add support for manifest removal (#123) Prepare 0.13.2 release manifest_data was missing for remote manifests (#135) (IGNORE) Restore cargo-semver-checks step (#129) Prepare 0.13.1 release Add ManifestStore::from_manifest_and_asset_bytes_async (#130) (IGNORE) Disable cargo-semver-checks step (#126) (IGNORE) Enforce semantic versioning requirements during PR validation and publish workflows (#124) Prepare 0.13.0 release Add RemoteManifestUrl Error, returning url (#120) Convert status_log error val to a string so that we can return full errors (#121) Report failures from remote manifest fetch (#116) Fast XMP extraction from PNG (#117) Bump MSRV to 1.59.0 (#118) Make sure there is a single manifest store in the asset (#114) ... # Conflicts: # sdk/src/error.rs # sdk/src/jumbf_io.rs
Changes in this pull request
Assets with remote manifests should remove previously embedded manifests
This fixes a bug when writing remote manifests and not updating an embedded manifest. The embedded manifest will now be removed from the output asset.
Checklist
TO DOitems (or similar) have been entered as GitHub issues and the link to that issue has been included in a comment.