search

memorysafety / rav1d

An AV1 decoder in Rust.
BSD 2-Clause "Simplified" License
242 stars 15 forks source link

Switch to stable Rust toolchain #592

Open rinon opened 7 months ago

rinon commented 7 months ago

Ideally rav1d should build with a stable toolchain at some point. It looks like we ended up with a few nightly-only features from the initial c2rust transpile that we should eventually remove. I currently find three nightly-only features that we use:

The intrinsics we can take care of by using stabilized Rust atomic types, the variadic usage appears to just be vfprintf which would be easy to remove, and for extern types we can probably do opaque pointers in another way.

kkysen commented 7 months ago

The extern types might be all gone now after we deduplicated everything. At least the rav1d library; the few binaries still have a bunch that are hard to unravel due to type erasure generics.

kkysen commented 7 months ago

I've been meaning to fix all the atomic intrinsics to use atomic types soon; that'll be part of making the core data types safe.

kkysen commented 7 months ago

For the variadics, I'm not sure we can fully get rid of those because there are callbacks the user can set and we need to keep that ABI. Eventually we can put it behind a legacy C API feature flag, as we shouldn't need it for pure Rust usage.

rinon commented 7 months ago

The extern types might be all gone now after we deduplicated everything. At least the rav1d library; the few binaries still have a bunch that are hard to unravel due to type erasure generics.

Could we do the same thing as bindgen and make the type unconstructable? We don't care what it's purported layout is, only that we can't construct it.

I've been meaning to fix all the atomic intrinsics to use atomic types soon; that'll be part of making the core data types safe.

Do we have an issue on this? I was just looking at that, will simplify some things nicely.

For the variadics, I'm not sure we can fully get rid of those because there are callbacks the user can set and we need to keep that ABI. Eventually we can put it behind a legacy C API feature flag, as we shouldn't need it for pure Rust usage.

Eek ok, yeah. This whole issue is super low priority, I just wanted to create an issue on it while I was thinking about it.

kkysen commented 7 months ago

Could we do the same thing as bindgen and make the type unconstructable? We don't care what it's purported layout is, only that we can't construct it.

I'm not sure, but the binaries I don't think are very important compared to the rav1d library that others will use. And there are Rust libraries we can use for the demuxing in those binaries instead if we want to make them safe, sidestepping the issue. But I don't think people depend on these binaries (except our tests I guess), so it's a very low priority.

I've been meaning to fix all the atomic intrinsics to use atomic types soon; that'll be part of making the core data types safe.

Do we have an issue on this? I was just looking at that, will simplify some things nicely.

No, I don't think so. Feel free to open one.

Eek ok, yeah. This whole issue is super low priority, I just wanted to create an issue on it while I was thinking about it.

👍

thedataking commented 7 months ago

But I don't think people depend on these binaries (except our tests I guess), so it's a very low priority.

Exactly. If the Rust binaries give us any trouble, I think we can delete them and build the C sources with meson. I have a patch somewhere to do that already – that's one way to test that we remain C compatible.

kkysen commented 7 months ago

Exactly. If the Rust binaries give us any trouble, I think we can delete them and build the C sources with meson. I have a patch somewhere to do that already – that's one way to test that we remain C compatible.

I'm not sure if we should fully delete them in case someone finds them useful for a rust version of them, but I'd be in favor of testing with the C versions of them (though it does complicate the build system, unfortunately).

kkysen commented 5 months ago

With #688 fixing #638, we can remove #![feature(core_intrinsics)] now.

negge commented 4 months ago

Is the minimum rust version documented anywhere in the project? I could not get rav1d main to compile with 1.76.0-nightly on aarch64.

kkysen commented 4 months ago

Is the minimum rust version documented anywhere in the project? I could not get rav1d main to compile with 1.76.0-nightly on aarch64.

Hi! Yes, we have our nightly pinned in rust-toolchain.toml. cargo should use it automatically. How are you trying to build rav1d?

We can remove the last nightly feature, but it wasn't a priority, so we haven't done it quite yet. Once we do, we'll switch to stable.

negge commented 4 months ago

Hi! Yes, we have our nightly pinned in rust-toolchain.toml. cargo should use it automatically. How are you trying to build rav1d?

I filed issue #773 with the exact build command. Just a simple cargo build --release.

kkysen commented 2 months ago

With #620, we'll be able to build rav1d on stable, with the following exceptions:

For #![feature(stdarch_arm_feature_detection)] and #![feature(stdarch_riscv_feature_detection)], I'm not sure if those will be stabilized soon, but it still seems better to do it with this officially supported way than the dav1d way of manually detecting CPU features.

It would be nice if we can get rust_toolchain.toml to use channel = "stable" only for rav1d and for x86, x86_64, and aarch64.

rinon commented 2 weeks ago

I'm gonna do a quick fix up on the tools so we can be on stable.

kkysen commented 2 weeks ago

I'm gonna do a quick fix up on the tools so we can be on stable.

I'm not sure it's so simple to get rid of #![feature(extern_types)] since the types are different in different modules.