-
Notifications
You must be signed in to change notification settings - Fork 22
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
Link librav1d
against C dav1d
and seek_stress
instead of Rust versions
#825
Conversation
90fe29a
to
61deaef
Compare
61deaef
to
9350967
Compare
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.
Could you clarify, does this preserve the ability to build librav1d
linked against Rust dav1d
and seek_stress
binaries? If not, I'd like to keep that, as that's type checked, unlike linking against C. It might suffice just to compile this to get the type checking, as opposed to running tests with both the C and Rust CLI binaries.
Also, with this change, how do I build and test things and also run cargo
independently so I get normal errors that aren't swallowed? Is it intended that I run cargo
independently first to see errors, then run meson
which runs cargo
, whose compilation is cached?
Also, could we use cargo
's --profile
option instead of --release
?
dav1d
and seek_stress
against librav1d
librav1d
against C dav1d
and seek_stress
instead of Rust versions
Yes,
I think that would be confusing. The idea is to match build types such that if we configure meson for a debug build, it links against libravid.a with debug symbols. |
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.
Looks good to me.
tools/dav1d_cli_parse.rs
Outdated
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.
Are these CPU flag changes necessary as part of this, or was this you fixing another issue in the same PR? It's fine either way, I just don't see the obvious connection with the build system changes.
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 think this is to fix #803.
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 think this is to fix #803.
Correct.
Are these CPU flag changes necessary as part of this, or was this you fixing another issue in the same PR?
No, this could have been split into two easier-to-review PRs. Sorry for the confusion.
When building build/tools/dav1d
with meson we now link against target/release/librav1d.a
(if release build). That incidentally fixes #803 when using meson. To solve #803 fully, I also fixed the transpiled version so that cpumask
works correctly when using cargo build
.
Closes #803. Can be toggled on off via the test_rust meson configuration option.
This is the easiest way to let us link this tool against libdav1d or librav1d for testing.
9350967
to
e7a8152
Compare
Closes #807 and #803.
When the
test_rust
meson option (on by default) is set, use a custom meson command to buildlibrav1d.a
and linkdav1d
andseek_stress
with it instead oflibdav1d
.Notes: