Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

(MINOR) Remove previously embedded manifests for remote manifests #136

Merged
merged 7 commits into from
Sep 23, 2022
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 14 additions & 0 deletions sdk/src/jumbf_io.rs
Original file line number Diff line number Diff line change
Expand Up @@ -215,3 +215,17 @@ pub fn object_locations(in_path: &Path) -> Result<Vec<HashObjectPositions>> {
_ => Err(Error::UnsupportedType),
}
}

/// removes the C2PA JUMBF from an asset
/// Note: Use with caution since this deletes C2PA data
/// It is useful when creating remote manifests from embedded manifests
///
/// path - path to file to be updated
/// returns Unsupported type or errors from remove_cai_store
pub fn remove_jumbf_from_file(path: &Path) -> Result<()> {
let ext = get_file_extension(path).ok_or(Error::UnsupportedType)?;
match get_assetio_handler(&ext) {
Some(asset_handler) => asset_handler.remove_cai_store(path),
_ => Err(Error::UnsupportedType),
}
}
26 changes: 26 additions & 0 deletions sdk/src/manifest.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1158,4 +1158,30 @@ pub(crate) mod tests {
TEST_SMALL_JPEG
);
}

#[cfg(all(feature = "file_io", feature = "xmp_write"))]
#[actix::test]
async fn test_embed_sidecar_with_parent_manifest() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why is this an async test? I don't see any await or async inside the function.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

good point, I removed async from some of the other tests that didn't need it.

let temp_dir = tempdir().expect("temp dir");
let source = fixture_path("XCA.jpg");
let output = temp_dir.path().join("XCAplus.jpg");
let sidecar = output.with_extension("c2pa");
let fp = format!("file:/{}", sidecar.to_str().unwrap());
let url = url::Url::parse(&fp).unwrap();

let signer = temp_signer();

let parent = Ingredient::from_file(fixture_path("XCA.jpg")).expect("getting parent");
let mut manifest = test_manifest();
manifest.set_parent(parent).expect("setting parent");
manifest.set_remote_manifest(url);
let _c2pa_data = manifest.embed(&source, &output, &signer).expect("embed");

//let manifest_store = crate::ManifestStore::from_file(&sidecar).expect("from_file");
let manifest_store = crate::ManifestStore::from_file(&output).expect("from_file");
assert_eq!(
manifest_store.get_active().unwrap().title().unwrap(),
"XCAplus.jpg"
);
}
}
8 changes: 7 additions & 1 deletion sdk/src/store.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I can add that in a different PR as to not hold this up.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

object_locations, save_jumbf_to_file,
object_locations, remove_jumbf_from_file, save_jumbf_to_file,
},
utils::{
hash_utils::{hash256, Exclusion},
Expand Down Expand Up @@ -1610,10 +1610,14 @@ impl Store {
dest_path.to_path_buf()
}
crate::claim::RemoteManifest::SideCar => {
// remove any previous c2pa manifest from the asset
remove_jumbf_from_file(dest_path)?;
dest_path.with_extension(MANIFEST_STORE_EXT)
}
crate::claim::RemoteManifest::Remote(_url) => {
let d = dest_path.with_extension(MANIFEST_STORE_EXT);
// remove any previous c2pa manifest from the asset
remove_jumbf_from_file(dest_path)?;
// even though this block is protected by the outer cfg!(feature = "xmp_write")
// the class embedded_xmp is not defined so we have to explicitly exclude it from the build
#[cfg(feature = "xmp_write")]
Expand All @@ -1634,6 +1638,8 @@ impl Store {
match pc.remote_manifest() {
crate::claim::RemoteManifest::NoRemote => dest_path.to_path_buf(),
crate::claim::RemoteManifest::SideCar => {
// remove any previous c2pa manifest from the asset
remove_jumbf_from_file(dest_path)?;
dest_path.with_extension(MANIFEST_STORE_EXT)
}
crate::claim::RemoteManifest::Remote(_)
Expand Down