Skip to content
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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

dwbuiten
Copy link
Contributor

@dwbuiten dwbuiten commented May 14, 2020

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.

@dwbuiten dwbuiten requested review from lu-zero and Luni-4 May 14, 2020 15:47
@dwbuiten dwbuiten force-pushed the buf branch 2 times, most recently from 87fd5be to eb6d7d3 Compare May 14, 2020 18:05
Signed-off-by: Derek Buitenhuis <derek.buitenhuis@gmail.com>
@dwbuiten
Copy link
Contributor Author

Since we hit a cursed Rust -> C ABI issues, I'm attaching a simple test program and files
quick_c_buf_test.zip

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],
Copy link
Member

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?

Copy link
Contributor Author

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.

Copy link
Member

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.

Copy link
Contributor Author

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?

Copy link
Member

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.

Copy link
Member

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.

Copy link
Member

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.

Copy link
Contributor Author

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.

Copy link
Member

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.

Copy link
Member

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.

@Luni-4
Copy link
Member

Luni-4 commented Dec 21, 2020

Now that C-APIs have been removed, are you still interested in re-adding them @dwbuiten? Or can we close this PR?

@Luni-4 Luni-4 removed their request for review June 16, 2022 07:04
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.

3 participants