-
Notifications
You must be signed in to change notification settings - Fork 349
Add ffmpeg as potential external project #188
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
Conversation
This is modelled and set up in exactly the same way as for dav1d. Most of it is the same - the main key differences are:
I've kept the README.md separate from the dav1d one but rewritten the bits that differ - including finding other examples for good/bad autovectorization, and a similar small guide for mapping a checkasm benchmark back to the C source; I think it's more user friendly to have it that way than to have one readme refer to the other, even if there's some duplication in the bulk text in it. There's no tag that contains exactly what we need here, but it is usable on a recent release branch, so the git clone instructions check out that branch (and directions to a recent known-good commit on it). I.e., if there'd be larger changes on the branch, we'd need to update the CMakeLists.txt accordingly (adding/removing source files). Once there's a new tag on that branch (potentially in a month or two), I'll update the instructions to clone and point to that instead. |
This looks a very good idea to me: it's completely optional, and a very lightweight way to optionally pull in a relatively big and real life workload that can increase our test coverage. My very first question about how this is different from dav1d is mostly answered here I think:
This is a good thing, but just out of curiousity I wanted to ask if you can tell if there are other differences? For example, are these codes very similar, or are they different in any way? More testing is good enough on itself, but just trying to get a sense of the added value. I will give this a try to see how this works and will report back soon. |
That's probably the main difference indeed. dav1d is mostly developed by longtime ffmpeg contributors, so the code style and structure, etc, are very similar. dav1d was developed externally, outside of ffmpeg for license/process reasons as a clean-slate project - dav1d is simplified BSD, while ffmpeg is mostly LGPL (with some minor bits being GPL and BSD or similar). When dav1d was started, some minor changes were made, like using meson as the upstream build system, rather than a custom configure shell script+gnu make, but from the point of view of llvm-test-suite it's quite much the same. From the point of view of higher level benchmarking, ffmpeg is much more complex though :-) Outside of microbenchmarking with checkasm, it's kinda easy to benchmark decoding with dav1d; use one single input video clip that exercises most video format features, decode it and measure the time. It's very easy to get a number of e.g. the effect on real-world decoding performance from autovectorization, like measured in https://discourse.llvm.org/t/autovectorization-measurements-from-dav1d-and-ffmpeg/82623. E.g. built with clang without vectorization, decodes at 118 fps, vectorization takes it to 177 fps. Built with GCC without vectorization gives 128 fps, with vectorization 185 fps. And with all handwritten assembly, it's 450 fps. With ffmpeg supporting hundreds of different input formats, and also supporting encoding many dozens of formats, it'd require a quite significant test matrix, where benchmarking usually wouldn't see much differences, unless you targetedly know that changes that were made specifically affect one specific input/output format. One can of course benchmark the performance of one or a couple specific known important cases though, e.g. decoding H264 or HEVC or similar. |
Just had a chance to give this a try, but it looks like it traps in
|
That's odd - this code does run and gets tested very regularly on macOS. I just retested this on macOS, built via the llvm-test-suite wrapping, and ran Does this happen for any run, or only with certain input seeds? If the issue seems to be within I also tested building this with address sanitizer, and it ran |
Forgot to post my results, but I've tried it too and found that it all works OK on our AArch64 linux platforms. |
To add the external source, run "git clone -b release/7.1 https://git.ffmpeg.org/ffmpeg.git" to clone the ffmpeg sources in the llvm-test-suite/test-suite-externals directory, or configure with TEST_SUITE_FFMPEG_ROOT pointing at a directory containing this source. (As this points to a branch, not a tag, it may be required to check out the known matching commit 10aaf84f855dbcedb8ee2e3fce307e9b98320946.) This builds two targets; the ffmpeg command line executable (which isn't executed as a test) and the ffmpeg checkasm test executable (which runs compiler generated functions and compares them with handwritten assembly versions of them). The checkasm execuable can also be run manually to microbenchmark functions, e.g. "External/ffmpeg/ffmpeg_checkasm --test=vp9dsp --bench=vp9_avg_8tap_smooth* 0". It is not very meaningful to benchmark the execution of the whole checkasm executable, as it runs a different numbers of functions depending on the number of SIMD extensions available on the target CPU. (Benchmarking on aarch64 on Linux currently requires direct access to the pmccntr_el0 register. To currently use a different timer register, edit ffmpeg/libavutil/aarch64/timer.h and change it to use cntvct_el0 instead of pmccntr_el0 on Linux too.) This uses a static configuration of the project (when building the upstream project with their own build system, there's a number of build options that can be configured). Assembly is hooked up and enabled on i386, x86_64, arm and aarch64. For architectures other than those, the checkasm test won't have any reference for detecting e.g. miscompilations of functions, but building it can still be meaningful (for testing compilation, or benchmarking the execution of the C version of functions). Instead of having the CMake configuration check for a large number of things and generate a config.h file at configure time, things are either hardcoded, or set up with preprocessor conditions within the hand-tweaked config.h file.
Thanks for checking! BTW, I noticed that I had forgot to actually commit the final amendments, so the cmakelists and readme pointed to the wrong commit to check out (which lacked a recent commit to make sure it prints benchmarks for all functions, also functions without corresponding assembly implementations) - if you checked out a specific commit manually, check that you're on |
Hmm, now I rebuilt with a fresh upstream llvm.org Clang, and I did manage to hit this issue; will look into it! |
Yep I am also hitting it with a Clang built from main. Might be that the object size computation changed somehow and is now incorrect? |
Possibly, maybe... But when building ffmpeg's checkasm with the original upstream ffmpeg build system, with the same version of Clang, I'm not hitting this issue, so it's also possible that it's caused by something in the fixed static configuration used here in the llvm-test-suite integration (which I use rather than running one million configure checks for various OS/toolchain features which we usually don't care about) - or if it is an upstream Clang bug which only happens to get uncovered in specific circumstances. I'm not hitting the same issue when building with a Clang 19.1.x release, so it seems to be related to something that has happened on git main since the last few months, so I can bisect that as well. |
Yes, you were right on the money here. I bisected the regression to llvm/llvm-project@f8c1a22 / llvm/llvm-project#117849.
Yes, this also was what was happening. Upstream ffmpeg only assumes So, apparently a legitimate compiler bug, caught by the integration of ffmpeg into llvm-test-suite, even before it's merged :-) With llvm/llvm-project@f8c1a22 reverted, it builds and runs fine on macOS for me. Alternatively, you can also edit the hardcoded |
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 am happy with this, thanks for doing this.
…generating empty location Fix the regression detected by llvm/llvm-test-suite#188
Seems like this already was merged in 1cea1a9 :-) Closing the PR then. That commit does seem to lack a couple of the later updates to this PR though (a typo fix in the readme, and fixes for the pinned ffmpeg commit), I'll push fixup commits for that. |
Since llvm/llvm-test-suite#182 and llvm/llvm-test-suite#188, these projects can now be added as external projects within llvm-test-suite.
Since llvm/llvm-test-suite#182 and llvm/llvm-test-suite#188, these projects can now be added as external projects within llvm-test-suite.
…ated mode instead of … (#120424) …generating empty location Fix the regression detected by llvm/llvm-test-suite#188
Since llvm/llvm-test-suite#182 and llvm/llvm-test-suite#188, these projects can now be added as external projects within llvm-test-suite.
For reference here, the fix for the crash in checkasm on macos was fixed in llvm/llvm-project#120424 / llvm/llvm-project@e4db3f0 - and now the ffmpeg checkasm test runs fine on macOS when built through llvm-test-suite. |
Great news, thanks! |
…in approximated mode instead of … (#120424) …generating empty location Fix the regression detected by llvm/llvm-test-suite#188
Since llvm/llvm-test-suite#182 and llvm/llvm-test-suite#188, these projects can now be added as external projects within llvm-test-suite.
To add the external source, run
"git clone -b release/7.1 https://git.ffmpeg.org/ffmpeg.git" to clone the ffmpeg sources in the
llvm-test-suite/test-suite-externals directory, or configure with TEST_SUITE_FFMPEG_ROOT pointing at a directory containing this source.
(As this points to a branch, not a tag, it may be required to check out the known matching commit 10aaf84f855dbcedb8ee2e3fce307e9b98320946.)
This builds two targets; the ffmpeg command line executable (which isn't executed as a test) and the ffmpeg checkasm test executable (which runs compiler generated functions and compares them with handwritten assembly versions of them). The checkasm execuable can also be run manually to microbenchmark functions, e.g. "External/ffmpeg/ffmpeg_checkasm --test=vp9dsp --bench=vp9_avg_8tap_smooth* 0".
It is not very meaningful to benchmark the execution of the whole checkasm executable, as it runs a different numbers of functions depending on the number of SIMD extensions available on the target CPU.
(Benchmarking on aarch64 on Linux currently requires direct access to the pmccntr_el0 register. To currently use a different timer register, edit ffmpeg/libavutil/aarch64/timer.h and change it to use cntvct_el0 instead of pmccntr_el0 on Linux too.)
This uses a static configuration of the project (when building the upstream project with their own build system, there's a number of build options that can be configured). Assembly is hooked up and enabled on i386, x86_64, arm and aarch64.
For architectures other than those, the checkasm test won't have any reference for detecting e.g. miscompilations of functions, but building it can still be meaningful (for testing compilation, or benchmarking the execution of the C version of functions).
Instead of having the CMake configuration check for a large number of things and generate a config.h file at configure time, things are either hardcoded, or set up with preprocessor conditions within the hand-tweaked config.h file.