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

[FR] Update Vulkan headers in sysroot of NDK r27 to enable FFmpeg's Vulkan integration #2016

Open
Javernaut opened this issue Apr 18, 2024 · 20 comments

Comments

@Javernaut
Copy link

Javernaut commented Apr 18, 2024

Description

The latest NDK r27 beta 1 (27.0.11718014-beta1) has Vulkan headers updated to 275.

FFmpeg - a popular solution for working with media content - has Vulkan integration. However, the latest stable FFmpeg 7.0 requires the Vulkan headers of version 277. Is it possible to bump the Vulkan headers in r27 just a bit before it goes released?

In case the version of Vulkan in NDK is aligned with the Validation Layers, then probably 1.3.280 will be a better option.

@DanAlbert
Copy link
Member

I can't go any faster than the OS, and this is what the OS currently has. Those updates are typically aligned with OS release cycles, so that's probably as new as it gets for r27. r28 may have something newer though (certainly we will eventually, it's just totally out of my hands so I can't say for certain when it will happen).

What specifically does ffmpeg need from the newer version? If it's new APIs that won't help you anyway, because those new APIs won't exist on any devices that you're targeting yet.

@DanAlbert
Copy link
Member

(I'm confirming all that with the guy that actually does these updates, but I'm pretty sure those updates are expensive and so an out-of-cycle upgrade is not likely)

@Javernaut
Copy link
Author

Javernaut commented Apr 18, 2024

A brief check of FFmpeg's history shows that in the commit where they bumped 255 to 277, they introduced the usage of VK_KHR_VIDEO_DECODE_AV1_EXTENSION_NAME extension. This extension isn't available in 275.

Screenshot 2024-04-18 at 22 09 43

However, the extension is considered opitonal. So I believe a simple bump of vulkan in NDK will make the latest FFmpeg compilable.

By the way, I have also checked the previous version of FFmpeg (6.1.1, requires vulkan 255). It is actually compilable with ndk r27-beta1 for arm64 and x86_64, but not for 32 bits.

@DanAlbert
Copy link
Member

I'm waiting to hear back if it's possible, but the window for changes in r27 is closing fast, and I think vulkan upgrades in the OS are tricky.

@Javernaut
Copy link
Author

Javernaut commented Apr 18, 2024

Of course, this is more about compilability of FFmpeg out of the box.

Vulkan versions on end devices fall short from the actual latest version. My private device with Android 14 has Vulkan 1.1.128 (5 y.o.), for example. Not complaining, just saying I understand that simple headers update in NDK won't deliver the updated Vulkan's binaries to end devices.

@DanAlbert
Copy link
Member

Of course, this is more about compilability of FFmpeg out of the box.

Which is a big deal, IMO. We'll do it if we can. If FFmpeg often increases the minimum they require though, we'll always be lagging, especially in the LTS where the header will be 12 months old or more toward the end of the cycle.

There's some ongoing work to make the default path for vulkan to be getting the SDK straight from LunarG (which would mean you're no longer dependent on an old NDK having new tools), but I don't know if that included vulkan.h.

Vulkan versions on end devices fall short from the actual latest version

I actually hadn't seen that data before (not that it's surprising). Thanks!

@DanAlbert
Copy link
Member

Is the vulkan integration a default part of ffmpeg, or is that an extension you're trying to enable (I know they have a lot of config knobs). Apparently none of the vulkan video APIs will work on Android anyway. If it's in the default config for ffmpeg, it's annoying that it won't build out of the box (and I'd suggest that ffmpeg switch to default off for Android), but the runtime behavior with either the fix or explicitly disabling vulkan video during configure would be identical. If it's default off and you're passing an explicit enable flag, there's apparently no point in doing that.

That said, they say it's probably possible to get the header updated in time for r27. They agree with me though that apps requiring latest vulkan headers are always going to run into this sort of problem though :( We can fix it now, but it might break again shortly after release.

@github-project-automation github-project-automation bot moved this to Unconfirmed in NDK r27 Apr 18, 2024
@DanAlbert
Copy link
Member

Being tracked internally at http://b/335709592, I'll forward anything interesting here, but I don't expect anything interesting to happen other than either a fix or a "next release".

@Javernaut
Copy link
Author

Javernaut commented Apr 19, 2024

Thanks @DanAlbert

Is the vulkan integration a default part of ffmpeg

No, FFmpeg can live without Vulkan being enabled. Actually, for a long time it was easier to disable it completely. Here is the statistics of Vulkan in Android from 2021.

As I understand, the top FFmpeg version (with Vulkan enabled) that can be compiled with NDK r26 is 6.0. By bumping Vulkan in NDK r27 to 277+, both FFmpeg 6.1 and 7.0 will be covered.

FFmpeg considers optional features of Vulkan and its integration started with 1.1.97, which is actually supported by many Android devices nowadays. And I think enabling 'at least something' of Vulkan is better than disabling it completely.

Of course, it is possible to stick to older FFmpeg release, it is just 7.0 got Android's content protocol support, attracting more attention.

@DanAlbert
Copy link
Member

Okay, so the default configuration of ffmpeg (that is, no --with-feature flags when you run configure) does require the updated header, even if vulkan won't be used at runtime? I'm trying to confirm whether the latest ffmpeg is incompatible with the current header out of the box, or only in some configurations. Sounds like yes, by default.

@Javernaut
Copy link
Author

From the start Vulkan integration was disabled by default. At some point it was changed to 'autodetect'. The autodetection works like this: the configure script checks the Vulkan’s availability and if it is available, then it will be considered enabled for the build. The autodetection isn’t specific to Vulkan, all external libraries are checked in some way. For Vulkan specifically the configure script initially would check the presence of the vulkan/vulkan.h file and later the check for its version was added.

Frankly speaking up until now I overlooked the fact that Vulkan is autodetected rather than enabled by default.

When Vulkan in NDK passes the check of configure script, then the real compilation of Vulkan’s integration happens. And from the tests I’ve done I can say (as funny as it is) that NDK r27 already cripples FFmpeg 6.1.x default compilation, since Vulkan is autodetected as 'enabled’, but the compilation fails for 32 bit archs. With NDK r26 the FFmpeg 6.1.x doesn’t detect the Vulkan and everything is green. If Vulkan is updated to 277+, then FFmpeg 7.0 will also autodetect it to 'enabled' and potentially fail for some archs.

That being said, I still think updating of Vulkan in NDK is something desirable and the compilation errors are more to FFmpeg itself to fix. And these errors are out of the scope of this particular issue.
Consumers still can use --disable-vulkan as a fix for some time.

@DanAlbert
Copy link
Member

DanAlbert commented Apr 22, 2024

Yes, up to date Vulkan headers are of course something we want :) No argument there.

It does sound like ffmpeg's autodetection is wrong though. Presence of those headers has nothing to do with the availability of the API at runtime. This isn't just pedantic, from what I'm told, those APIs do not work on any Android device.

@enh-google
Copy link
Collaborator

It does sound like ffmpeg's autodetection is wrong though.

at the very least, it seems like they should be looking for the newest constant/function/type they actually need, since they're obviously dependent on a specific version (just to build), rather than just "does the header exist?".

@DanAlbert
Copy link
Member

It looks like it's trying to do that, actually:

if enabled vulkan; then
  check_pkg_config_header_only vulkan "vulkan >= 1.3.277" "vulkan/vulkan.h" "defined VK_VERSION_1_3" ||
      check_cpp_condition vulkan "vulkan/vulkan.h" "defined(VK_VERSION_1_4) || (defined(VK_VERSION_1_3) && VK_HEADER_VERSION >= 277)"
fi

Is that wrongly finding and passing with the host's header?

@Javernaut
Copy link
Author

Is that wrongly finding and passing with the host's header?

When the cross-compilation of FFmpeg for Android is set one must use the --sysroot parameter for the configure script. One should use the sysroot from NDK ${NDK}/toolchains/llvm/prebuilt/${HOST_TAG}/sysroot, right? This is where the Vulkan is found - in NDK. So this is not the host's header, but exatly the NDK's.

@enh-google
Copy link
Collaborator

do you have whatever the cmake equivalent of the configure.log file is? because it doesn't make sense that

if enabled vulkan; then
  check_pkg_config_header_only vulkan "vulkan >= 1.3.277" "vulkan/vulkan.h" "defined VK_VERSION_1_3" ||
      check_cpp_condition vulkan "vulkan/vulkan.h" "defined(VK_VERSION_1_4) || (defined(VK_VERSION_1_3) && VK_HEADER_VERSION >= 277)"
fi

would pass with our current headers...

@Javernaut
Copy link
Author

Javernaut commented Apr 23, 2024

No CMake equivalent, only this explanation:

During the configure script execution the ffbuild/config.log file is generated. For the FFmpeg 6.1.1 (which expects the Vulkan 255) and NDK r27-beta1 (which has Vulkan 275) the config.log file will have such section:

check_pkg_config_cpp vulkan vulkan >= 1.3.255 vulkan/vulkan.h defined VK_VERSION_1_3
test_pkg_config_cpp vulkan vulkan >= 1.3.255 vulkan/vulkan.h defined VK_VERSION_1_3
/opt/homebrew/bin/pkg-config --exists --print-errors vulkan >= 1.3.255
Package vulkan was not found in the pkg-config search path.
Perhaps you should add the directory containing `vulkan.pc'
to the PKG_CONFIG_PATH environment variable
No package 'vulkan' found
check_cpp_condition vulkan vulkan/vulkan.h defined(VK_VERSION_1_4) || (defined(VK_VERSION_1_3) && VK_HEADER_VERSION >= 255)
test_cpp_condition vulkan/vulkan.h defined(VK_VERSION_1_4) || (defined(VK_VERSION_1_3) && VK_HEADER_VERSION >= 255)
test_cpp
BEGIN /var/temp_dir/test.c
    1	#include <vulkan/vulkan.h>
    2	#if !(defined(VK_VERSION_1_4) || (defined(VK_VERSION_1_3) && VK_HEADER_VERSION >= 255))
    3	#error "unsatisfied condition: defined(VK_VERSION_1_4) || (defined(VK_VERSION_1_3) && VK_HEADER_VERSION >= 255)"
    4	#endif
END /var/temp_dir/test.c
/path/to/ndk/27.0.11718014/toolchains/llvm/prebuilt/darwin-x86_64/bin/armv7a-linux-androideabi21-clang --sysroot=/path/to/ndk/27.0.11718014/toolchains/llvm/prebuilt/darwin-x86_64/sysroot -D_ISOC99_SOURCE -D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE -Dstrtod=avpriv_strtod -DPIC -O3 -fPIC -I/path/to/additional/but/empty/include -std=c11 -fPIE -fomit-frame-pointer -fPIC -marm -pthread -E -o /var/temp_dir/test.o /var/temp_dir/test.c

The original check includes check_pkg_config_header_only() and check_cpp_condition(), where the first fails, but the second actually succeeds. And that makes the configure script believe the proper Vulkan sources are available.

Just in case, all the functions like check_pkg_config_header_only(), check_cpp_condition(), test_cpp_condition() and others are defined in the same configure file.

If you try compiling FFmpeg 7.0 (which expects Vulkan 277) with the same NDK r27-beta1, then the second check fails too, as the test.c file contains 277 as the value to check with. No Vulkan support is considered after that.

@DanAlbert DanAlbert moved this from Unconfirmed to Triaged in NDK r27 Jun 25, 2024
@DanAlbert
Copy link
Member

They've been working on it but ran into some issues that slowed down the update, and it looks like it won't happen in time for r27.

@DanAlbert DanAlbert removed this from NDK r27 Jun 25, 2024
@github-project-automation github-project-automation bot moved this to Triaged in NDK r28 Jun 25, 2024
@DanAlbert
Copy link
Member

Looks like Android's still on 275. I'll leave the bug here in case that's updated before we stop taking sysroot updates for r28, but I expect this will have to be punted.

A fix on the ffmpeg side the not require that decl is probably the faster solution. The vulkan header in the NDK belongs to the OS, and the OS doesn't have a need to upgrade more than annually. Unless something goes horribly wrong that would make the OS skip upgrading for a release, it'll certainly be fixed some time in the next year, but I unfortunately can't be more accurate than that because it's out of my hands.

@DanAlbert DanAlbert removed this from NDK r28 Nov 1, 2024
@DanAlbert DanAlbert added this to NDK r29 Nov 1, 2024
@github-project-automation github-project-automation bot moved this to Unconfirmed in NDK r29 Nov 1, 2024
@DanAlbert
Copy link
Member

I just took what is hopefully the last sysroot update for r28, and Android's still on 275. There unfortunately isn't anything I can do to speed this up.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Unconfirmed
Development

No branches or pull requests

3 participants