-
Notifications
You must be signed in to change notification settings - Fork 10
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
capi: Expose frame buffer API #94
base: master
Are you sure you want to change the base?
Conversation
87fd5be
to
eb6d7d3
Compare
Signed-off-by: Derek Buitenhuis <derek.buitenhuis@gmail.com>
Since we hit a cursed Rust -> C ABI issues, I'm attaching a simple test program and files that triggers the madness, at @Luni-4's request. |
/// Returns the correct `ssim` value or `NULL` on errors | ||
#[no_mangle] | ||
pub unsafe extern fn avm_calculate_frame_buf_ssim( | ||
frame1: [*const u8; 3], |
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.
What cbindgen produces for those?
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.
It produces exactly the API I wanted:
/**
* Calculate the `msssim` metric between two frame buffers
*
* Returns the correct `msssim` value or `NULL` on errors
*/
const AVMContext *avm_calculate_frame_buf_msssim(const uint8_t *frame1[3],
ptrdiff_t frame1_strides[3],
const uint8_t *frame2[3],
ptrdiff_t frame2_strides[3],
uint32_t width,
uint32_t height,
uint8_t bitdepth,
AVMChromaSamplePosition chroma_pos,
AVMChromaSampling subsampling,
AVMRational pixel_aspect_ratio);
But it doesn't matter, since Rust can't generate a confirming C ABI for it, as discussed on IRC.
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.
This is a bug in cbindgen. ptrdiff_t frame2_strides[3]
is *mut frame2_strides
.
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.
What would be the correct way to write the rust declaration to generate ptrdiff_t frame2_strides[3]
? And should we file a cbindgen bug?
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.
Let me poke upstream to rustc confirm, but from what you are telling me it seems so and a good idea to tell cbindgen upstream.
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.
improper_ctypes
is on by default only in extern blocks.
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.
improper_ctypes
is on by default only in extern blocks.
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 mean... not's not really a good answer. Why wouldn't it be on for all extern "C"
functions? It makes it almost useless.
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.
That's what I had been told today :) I hadn't tested it yet btw.
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.
upstream cbindgen would welcome a patch to prevent this and one to add annotations to use the style you'd use here.
Now that C-APIs have been removed, are you still interested in re-adding them @dwbuiten? Or can we close this PR? |
Notes
Currently untested - I will get to testing this tonight after dinner, and will update the PR text after. It's very likely to explode on 4:0:0.
Comments welcome - it's kinda ugly, but such is C interop.
Also of note is that
Cargo.toml
in the main directory is sitll pointing to av-metrics 0.4.0 for some reason.