search

rust-av / dav1d-rs

libdav1d rust bindings
MIT License
40 stars 20 forks source link

Stop using bindgen and instead provide a platform-independent static … #68

Closed sdroege closed 2 years ago

sdroege commented 2 years ago

…version of the FFI definitions

bindgen requires libclang to be available and on various platforms providing a compatible version of libclang is not that easy and unnecessarily complicates the build.

The dav1d API is relatively stable nowadays so manual updates to the FFI definitions shouldn't cause too much additional work here.

Context of this change is https://gitlab.freedesktop.org/gstreamer/cerbero/-/merge_requests/954

sdroege commented 2 years ago

The dav1d API is relatively stable nowadays so manual updates to the FFI definitions shouldn't cause too much additional work here.

Basically whenever a new dav1d release is made, you would diff the headers and then add/change whatever has changed. Which should be manageable.

It would also allow you to make the APIs available conditionally based on the dav1d version it is actually compiled with, providing more control to users about the dav1d version they're targetting (compare: gtk-rs version feature flags that allow a user to opt-in explicitly to having e.g glib 2.72 APIs available)

lu-zero commented 2 years ago

I like the idea, and I thank you a lot for the effort since it will take quite a bit of time.

Some random notes

sdroege commented 2 years ago

Ideally https://doc.rust-lang.org/core/ffi/index.html has the types we should use, not sure if it is better to wait and see if a newer bindgen will do that for us or it is better to do a replacement pass on the current code.

That's just a matter of search&replace, doesn't really matter what bindgen does here. I can change it to use those types if you prefer but see https://github.com/rust-av/dav1d-rs/pull/68#discussion_r988809966 (we'd have to depend on Rust >= 1.64 just for that and it's not like dav1d-the-C-library will work in no-std environments).

I'm not totally convinced we want to remove the ability to use bindgen to freshen the binding, if core::ffi is enough to take care of the platform-specific details, probably it would be fine anyway though.

The only platform-specific details in dav1d are the errnos, which are statically covered by the libc crate already (which is maintained by the Rust team). The dav1d C ABI is fortunately simple.

The additional complications that bindgen adds here doesn't really seem worth it, and by not using it we get the ability to actually give API-stability guarantees for dav1d-sys which is IMHO a huge improvement.

sdroege commented 2 years ago

Before continuing here, I'd need a decision from you whether to keep bindgen in some form (and not give any API guarantees about dav1d-sys) or if you're OK with dropping it. FWIW, I'm fine making sure that dav1d-sys stays in sync with the dav1d C API in the future so that's no additional work for you at least.

lu-zero commented 2 years ago

I think we can drop it, hide the libc dependency in the -sys crate and document in the readme that bindgen is not used.

sdroege commented 2 years ago

Sounds good, I'll update this accordingly later. Thanks!

sdroege commented 2 years ago

document in the readme that bindgen is not used

How/why would you document that btw? We're also not using many other things :)

sdroege commented 2 years ago

Updated that all in easily to review separate commits. From my side this would be ready, please let me know what you would like to have changed :)

sdroege commented 2 years ago

Should I squash all commits into one or do you want to merge it like this?

Also it would be great if we could make a release after this to unblock GStreamer shipping this :) dav1d could become 0.9.1 (no API changes) and dav1d-sys 0.7.0 (API changes).

lu-zero commented 2 years ago

Up to you, and yes once it lands I'll cut a release.

sdroege commented 2 years ago

I prefer like this because it makes the individual changes easier to follow for anybody who wants to follow what changed