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

Link librav1d against C dav1d and seek_stress instead of Rust versions #825

Merged
merged 4 commits into from
Mar 20, 2024

Conversation

thedataking
Copy link
Collaborator

@thedataking thedataking commented Mar 16, 2024

Closes #807 and #803.

When the test_rust meson option (on by default) is set, use a custom meson command to build librav1d.a and link dav1d and seek_stress with it instead of libdav1d.

Notes:

  • Using the subprojects feature of meson doesn't seem to be an option since it requires a specific directory layout.
  • Cross-compiling isn't supported.
  • The macOS build behaves differently than local macOS, possibly due to meson version differences. Didn't root cause.

@thedataking thedataking force-pushed the perl/issue-803 branch 5 times, most recently from 90fe29a to 61deaef Compare March 16, 2024 10:32
@thedataking thedataking requested a review from rinon March 16, 2024 10:36
@thedataking thedataking marked this pull request as ready for review March 16, 2024 10:37
meson.build Show resolved Hide resolved
Copy link
Collaborator

@kkysen kkysen left a 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?

@kkysen kkysen changed the title Link dav1d and seek_stress against librav1d Link librav1d against C dav1d and seek_stress instead of Rust versions Mar 19, 2024
@thedataking
Copy link
Collaborator Author

Could you clarify, does this preserve the ability to build librav1d linked against Rust dav1d and seek_stress binaries?

Yes, cargo build works as usual this only affects how things are built when using meson. When you cargo build, it links against the Rust version of the dav1d CLI tools which are also fixed in this PR.

Also, could we use cargo's --profile option instead of --release?

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.

Copy link
Collaborator

@randomPoison randomPoison left a 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.

Copy link
Collaborator

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.

Copy link
Collaborator

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.

Copy link
Collaborator Author

@thedataking thedataking Mar 20, 2024

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.
@thedataking thedataking merged commit 302e9b4 into main Mar 20, 2024
36 checks passed
@thedataking thedataking deleted the perl/issue-803 branch March 20, 2024 02:30
@thedataking thedataking mentioned this pull request Jun 26, 2024
2 tasks
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.

Use original C code for dav1d CLI
3 participants