rust-av / dav1d-rs

libdav1d rust bindings
MIT License
39 stars 19 forks source link

Question: would rav1d integration make sense? #101

Open Quackdoc opened 3 months ago

Quackdoc commented 3 months ago

Rav1d being a port of dav1d to rust, is intended to be a drop in replacement for dav1d, and this extends to librav1d vs libdav1d. rav1d does not currently have a rust API and is focused on being a drop in dav1d replacement for now see https://github.com/memorysafety/rav1d/issues/1252

I was wondering if a rav1d-sys would make sense as an optional toggle. Currently rav1d is a still slower then dav1d, but still fast enough to be used.

lu-zero commented 2 months ago

I'd wait till it is completely oxidized since as it is now we'd still need nasm and a C compiler, but it is definitely something to consider.

Sorry for not replying sooner!

kkysen commented 1 month ago

I'd wait till it is completely oxidized since as it is now we'd still need nasm and a C compiler, but it is definitely something to consider.

I'm not sure I understand your point. rav1d only compiles asm not C, but doesn't dav1d-sys already compile dav1d, which is asm and C?

Also, while some of the assembly may be able to rewritten as Rust with portable simd (when stabilized), performance might still be an issue, and there are efforts to verify the assembly and achieve safety that way: https://github.com/scaspin/memory-safe-assembly/pull/31.

lu-zero commented 1 month ago

That I could see you do need a C compiler at least for one target https://github.com/memorysafety/rav1d/blob/main/build.rs#L362

            cc::Build::new()
                .files([&"tools/compat/getopt.c"])
                .include("include/compat")
                .debug(cfg!(debug_assertions))
                .compile(&getopt);

Details aside, is there a compelling reason for me to spend my free time on integrating it and taking the burden to maintain that integration as it is now?

Until cargo gets able to deal with asm in a less brittle way, just that alone would bring quite a bit of headache...

kkysen commented 1 month ago

That I could see you do need a C compiler at least for one target https://github.com/memorysafety/rav1d/blob/main/build.rs#L362

            cc::Build::new()
                .files([&"tools/compat/getopt.c"])
                .include("include/compat")
                .debug(cfg!(debug_assertions))
                .compile(&getopt);

Hmm, I didn't realize we still did that. That's left over, and we don't need that anymore. We'll remove it.

Details aside, is there a compelling reason for me to spend my free time on integrating it and taking the burden to maintain that integration as it is now?

Nope, not as it is now if you don't want to. We'll hopefully have some time to provide a fully safe Rust API.

Until cargo gets able to deal with asm in a less brittle way, just that alone would bring quite a bit of headache...

I wasn't suggesting you expend extra effort on this now, I was just wondering why rav1d depending on compiling asm would be a blocker or more brittle than depending on dav1d, which builds the exact same asm, along with a whole lot of C. I wasn't sure if there was something I missed here.

lu-zero commented 1 month ago

nasm-rs and cc-rs do their work, but they tend to be more brittle than they should in my experience.

I have few ideas on how to address at least some of that, but not enough time since it would require a full RFC and quite a bit of refactoring in cargo.

kkysen commented 1 month ago

nasm-rs and cc-rs do their work, but they tend to be more brittle than they should in my experience.

I'm just confused why meson, ninja, cc, nasm, etc for dav1d don't have all those same brittleness problems and more.

lu-zero commented 1 month ago

So you have either user complaining about nasm or the right gas not being found or taking ages to build or using too much cpu (been there through all of it with rav1e).

Sadly we do not have usable rust replacements for nasm and gas, it would be great to have something able to digest .S or .asm and produce global_asm! so cargo and rustc can consume it and go through the normal process.