feat: Allow emscripten users to define HTTP handler#1936
feat: Allow emscripten users to define HTTP handler#1936
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1936 +/- ##
==========================================
+ Coverage 76.50% 76.72% +0.21%
==========================================
Files 172 175 +3
Lines 41142 42254 +1112
==========================================
+ Hits 31475 32418 +943
- Misses 9667 9836 +169 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Merging this PR will not alter performance
Comparing Footnotes
|
7f658f3 to
bafb849
Compare
There was a problem hiding this comment.
Do we want to add a cpp examples in this repo? This feels more like a c2pa-c example? (cc @gpeacock)
There was a problem hiding this comment.
It seems like we might also be able to reuse a lot of structs defined there.
c2pa_c_ffi/src/c_api.rs
Outdated
|
|
||
| use c2pa::http::HttpResolverError; | ||
|
|
||
| let uri_cstr = CString::new(request.uri().to_string()) |
There was a problem hiding this comment.
Not a blocker here, but it would be clean if impl TryFrom<Request> for C2paHttpRequest and same for response.
There was a problem hiding this comment.
I'll give it a shot.
c2pa_c_ffi/src/c_api.rs
Outdated
| let response_body = if c_response.body.is_null() || c_response.body_len == 0 { | ||
| Vec::new() | ||
| } else { | ||
| unsafe { std::slice::from_raw_parts(c_response.body, c_response.body_len).to_vec() } | ||
| }; | ||
|
|
||
| // Release the callback-owned buffer. | ||
| if let Some(free_fn) = c_response.body_free { | ||
| if !c_response.body.is_null() { | ||
| unsafe { free_fn(c_response.body) }; | ||
| } | ||
| } | ||
|
|
||
| if rc != 0 { | ||
| let msg = CimplError::last_message() | ||
| .unwrap_or_else(|| "HTTP callback returned error".to_string()); | ||
| return Err(HttpResolverError::Other(msg.into())); | ||
| } | ||
|
|
||
| let response = c2pa::http::http::Response::builder() | ||
| .status(c_response.status as u16) | ||
| .body(Box::new(std::io::Cursor::new(response_body)) as Box<dyn std::io::Read>) | ||
| .map_err(HttpResolverError::Http)?; | ||
| Ok(response) | ||
| } |
There was a problem hiding this comment.
Would it be possible to return a Cursor over c_response.body rather than cloning it into a vec? We can free it through impl Drop for C2paHttpResponse.
* use libc::free instead of custom deallocator * impl TryFrom<Request/Response>
5155fbe to
22317c4
Compare
ok-nick
left a comment
There was a problem hiding this comment.
Nice work, looks great. Approving with the exception that we consider where the C++ example belongs.
c2pa_c_ffi/src/c_api.rs
Outdated
| /// | ||
| /// # Safety | ||
| /// `self.body` must have been allocated with `malloc()` (or be null). | ||
| unsafe fn into_http_response( |
There was a problem hiding this comment.
Can also implement TryFrom here too.
| /// | ||
| /// Use [`as_ffi`](OwnedC2paHttpRequest::as_ffi) to obtain the `#[repr(C)]` view | ||
| /// whose pointers borrow from this struct. | ||
| struct OwnedC2paHttpRequest { |
There was a problem hiding this comment.
How come you split OwnedC2paHttpRequest and C2paHttpRequest into separate structs?
There was a problem hiding this comment.
C2paHttpRequest needs *const c_char fields so that cbindgen can generate the bindings.
OwnedC2paHttpRequest owns the CString data (Box<[u8]>).
There was a problem hiding this comment.
It seems like we might also be able to reuse a lot of structs defined there.
Changes in this pull request
Follow-up to the Emscripten build support added in c2pa-rs.
Emscripten does not support reqwest or ureq, the standard http handlers in the Rust SDK. The Rust SDK supports users defining their own http handlers through the context. We need to expose this in the C Bindings.
Checklist
TO DOitems (or similar) have been entered as GitHub issues and the link to that issue has been included in a comment.