-
Notifications
You must be signed in to change notification settings - Fork 4
ManifestBuilder API rebased and updates #7
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #7 +/- ##
===========================================
- Coverage 53.65% 27.61% -26.04%
===========================================
Files 13 16 +3
Lines 863 1503 +640
Branches 116 186 +70
===========================================
- Hits 463 415 -48
- Misses 327 1015 +688
Partials 73 73 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
ok-nick
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, I have a few comments.
| const val OPENED = "c2pa.opened" | ||
| const val PLACED = "c2pa.placed" | ||
| const val DRAWING = "c2pa.drawing" | ||
| const val COLOR_ADJUSTMENTS = "c2pa.color_adjustments" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are new actions here such as c2pa.adjustedColor and perhaps a few more defined here. We should probably add them upstream in the Rust SDK too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We kind of went overboard in adding possible actions. We were also considering paring back, to just focus on the typical ones for a mobile app in this release. For now, we can just leave them, and then review and plan after the merge.
| data class Ingredient( | ||
| val title: String? = null, | ||
| val format: String, | ||
| val instanceId: String = UUID.randomUUID().toString(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd prefer to stick to the default instance ID generation that happens in the Rust SDK. Are there any downsides in doing that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No I don't think so. We can rely on that. Let me make sure we don't have any other reason to generate it here.
| val documentId: String? = null, | ||
| val provenance: String? = null, | ||
| val hash: String? = null, | ||
| val relationship: String = "parentOf", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There can only be 1 parent ingredient in a manifest, thus the Rust SDK defaults to "componentOf." I have the same question if we need to default here or in the Rust SDK.
| data class Thumbnail( | ||
| val format: String, | ||
| val identifier: String, | ||
| val contentType: String = "image/jpeg" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How come the format and contentType are separate fields? Should we be defaulting to JPEG? The SDK will infer the format from the identifier and may auto-generate PNG, JPEG, or other formats depending on the circumstance.
| private var claimGenerator: ClaimGenerator? = null | ||
| private var format: String? = null | ||
| private var title: String? = null | ||
| private var instanceId: String = UUID.randomUUID().toString() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same question about the UUID default here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
workong on removing these now, and will update with a push soon, once testing is complete.
| ) | ||
|
|
||
| class ManifestBuilder { | ||
| private var claimGenerator: ClaimGenerator? = null |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There can be multiple claim generators.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the case of this generator though, there is only 1 instance active within our builder.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
claimGenerator is now an array, and multiple can be added
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ok-nick this was true in C2PA version 1.x (and for that reason, may still be exposed in the core SDK). In C2PA 2.2, there can only be one claim generator. (See https://spec.c2pa.org/specifications/specifications/2.1/specs/C2PA_Specification.html#_schema.)
| claimGeneratorName: String = "Android C2PA SDK", | ||
| claimGeneratorVersion: String = "1.0.0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the Rust SDK we inject our Rust SDK specific claim generator into the user-specified list. Maybe we should consider doing that here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So the Rust SDK specific one would be in addition or included in the name of what the user provides?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@scouten-adobe clarified in #7 (comment) that there's only one claim generator in v2 claims. I agree with what you have implemented here already then.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another approach to consider is generating these type definitions automatically from the JSON schemas exported in the Rust SDK. I believe the Node and JS SDK take a similar approach, not sure if that's possible here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. We did some variation on that at first. I will review and consider. This code is not just type definition however, as there is some additional logic of the builder, I but I understand what you mean. We are trying to keep this Kotlin/Android like while also making it easy to maintain.
| const val DATA_HASH = "c2pa.data_hash" | ||
| const val HASH_DATA = "c2pa.hash.data" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
c2pa.hash.data is the correct label here, I'm not sure if the other one exists.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed
| } | ||
| } | ||
|
|
||
| object C2PAAssertionTypes { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure if all of these assertion types are defined in the spec.
Changes in this pull request
Adds new package to Kotlin code for providing a ManifestBuilder interface for creating valid C2PA JSON Manifests
Types of changes
Checklist
TO DOitems (or similar) have been entered as GitHub issues and the link to that issue has been included in a comment