Skip to content

Conversation

gpeacock
Copy link
Contributor

@gpeacock gpeacock commented Oct 1, 2024

No description provided.

def __init__(self, format, stream, manifest_data=None):
super().__init__()
self.from_stream(format, C2paStream(stream))
if manifest_data is not None:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there any chance that the manifest_data could be blank (e.g. or any variation of whatever could be considered blank). Do we need special care around that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This option is allow verifying a sidecar or remote manifest. If we have retrieved a .c2pa manifest store from our cloud or a sidecar, we can pass it in along with an associated asset to see if it verifies. If the manifest_data is not a correctly structured binary .c2pa we will return errors reporting it.
I should add a ticket somewhere to do more type checking at a higher level, If you pass the wrong type of parameter, it will get caught, but the error messages are obscure ones from the binding layer.

if let Ok(mut builder) = self.builder.try_write() {
builder.set_no_embed(true);
} else {
return Err(Error::RwLock);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Question: Is this to poison the lock or... ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is here because the uniffi tool we use to generate python bindings does not support mutable parameters. So we add an internal check to ensure there is only one mutable access at a time.

Copy link
Contributor

Choose a reason for hiding this comment

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

We could use atomics here in Buidler to get around this eventually, if we end up having to have these binding layers hold on to a Mutex because of the Uniffi requirements of no mut vars as params. Not something that needs to be done for this PR, but it may make our lives easier down the road.

use std::sync::atomic::{AtomicBool, Ordering};

struct Builder {
    no_embed: AtomicBool
}

impl Builder {
    pub fn set_no_embed(&self, value: bool) {
        self.no_embed.store(value, Ordering::Relaxed)
    }
}


fn main() {
    let b = Builder {
        no_embed: AtomicBool::new(true)
    };
    
    
    b.set_no_embed(true);
    
    // note how b, defined above, is not mutable. 
    std::thread::scope(|s| {
        let _ = s.spawn(|| b.set_no_embed(false));
        let _ = s.spawn(|| b.set_no_embed(true));
        let _ = s.spawn(|| b.set_no_embed(false));
    });
}

@crandmck
Copy link
Collaborator

crandmck commented Oct 1, 2024

Should we add something to the docs (i.e. README) about these new functions?

I'm not sure how much of the API we need to document for each language. These functions are pretty much the same as the ones in Rust. We should spend some time thinking about that.

Copy link
Contributor

@dyro dyro left a comment

Choose a reason for hiding this comment

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

Looks good! Posted an idea now how to prevent the Mutex in bindings layers, but definitely not required to get this PR in.

if let Ok(mut builder) = self.builder.try_write() {
builder.set_no_embed(true);
} else {
return Err(Error::RwLock);
Copy link
Contributor

Choose a reason for hiding this comment

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

We could use atomics here in Buidler to get around this eventually, if we end up having to have these binding layers hold on to a Mutex because of the Uniffi requirements of no mut vars as params. Not something that needs to be done for this PR, but it may make our lives easier down the road.

use std::sync::atomic::{AtomicBool, Ordering};

struct Builder {
    no_embed: AtomicBool
}

impl Builder {
    pub fn set_no_embed(&self, value: bool) {
        self.no_embed.store(value, Ordering::Relaxed)
    }
}


fn main() {
    let b = Builder {
        no_embed: AtomicBool::new(true)
    };
    
    
    b.set_no_embed(true);
    
    // note how b, defined above, is not mutable. 
    std::thread::scope(|s| {
        let _ = s.spawn(|| b.set_no_embed(false));
        let _ = s.spawn(|| b.set_no_embed(true));
        let _ = s.spawn(|| b.set_no_embed(false));
    });
}

Copy link
Contributor

@scouten-adobe scouten-adobe left a comment

Choose a reason for hiding this comment

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

No concerns on the Rust side. I don't know enough Python to review that side.

@gpeacock
Copy link
Contributor Author

gpeacock commented Oct 1, 2024

Looks good! Posted an idea now how to prevent the Mutex in bindings layers, but definitely not required to get this PR in.

Thanks for the suggestion. I'd rather not redefine the Builder structure for the Python bindings. But we could look into alternatives if this locking turns out to be an issue.

@gpeacock gpeacock merged commit d6d3fab into main Oct 1, 2024
11 checks passed
@gpeacock gpeacock deleted the gpeacock/manifest_and_stream branch October 1, 2024 23:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants