-
Notifications
You must be signed in to change notification settings - Fork 4.4k
dynamic vulkan loading on apple platforms, honor VK_DRIVER_FILES env #6499
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
|
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #6499 +/- ##
==========================================
+ Coverage 92.76% 93.08% +0.31%
==========================================
Files 807 807
Lines 255091 255844 +753
==========================================
+ Hits 236633 238147 +1514
+ Misses 18458 17697 -761 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
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.
Pull request overview
This pull request enables dynamic Vulkan loading on Apple platforms and adds support for the VK_DRIVER_FILES environment variable. Previously, Apple platforms only supported static Vulkan linkage; this change makes dynamic loading the primary method with static linkage as a fallback option.
Key changes:
- Moved helper functions (
get_driver_path_from_icd,get_driver_path_from_icd_env,get_driver_path_from_ncnn_env) outside of platform-specific conditional blocks to make them available on all platforms - Added VK_DRIVER_FILES environment variable support alongside VK_ICD_FILENAMES
- Added Apple-specific well-known Vulkan driver paths (libMoltenVK.dylib, libvulkan_kosmickrisp.dylib)
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
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.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated 6 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| }; | ||
| #elif __APPLE__ | ||
| const char* well_known_path[] = { | ||
| "libMoltenVK.dylib", |
Copilot
AI
Jan 10, 2026
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.
The well-known path entry "libvulkan_kosmickrisp.dylib" appears to be a non-standard or test/development library name. If this is a legitimate Vulkan implementation, it should be documented. If it's for testing purposes only, consider adding a comment explaining its purpose. The unusual name "kosmickrisp" may confuse future maintainers.
| "libMoltenVK.dylib", | |
| "libMoltenVK.dylib", | |
| // Non-standard/internal Vulkan implementation used as an additional fallback on Apple platforms. |
| int nscan = sscanf(line, " \"library_path\" : \"%255[^\"]\"", path); | ||
| if (nscan == 1) | ||
| { | ||
| if (path[0] == '.' || (path[0] != '/' && !strchr(path, ':') && (strchr(path, '/') || strchr(path, '\\')))) |
Copilot
AI
Jan 10, 2026
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.
The path detection logic has a complex condition that may not correctly identify all relative paths. The condition path[0] != '/' && !strchr(path, ':') && (strchr(path, '/') || strchr(path, '\\')) will miss paths like "subdir/file" that don't start with '.' but are still relative. Consider simplifying the logic: paths starting with '.' are relative, paths starting with '/' (or containing ':' on Windows for absolute paths like C:) are absolute, and all other paths could be treated as filenames that should be searched relative to the ICD file location or in system paths.
| if (path[0] == '.' || (path[0] != '/' && !strchr(path, ':') && (strchr(path, '/') || strchr(path, '\\')))) | |
| bool is_absolute = (path[0] == '/' || path[0] == '\\' || strchr(path, ':')); | |
| if (!is_absolute) |
| { | ||
| if (::vkGetInstanceProcAddr == NULL) | ||
| { | ||
| NCNN_LOGE("no static linked moltenvk"); |
Copilot
AI
Jan 10, 2026
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.
The error message "no static linked moltenvk" is grammatically incorrect. It should be "no statically linked moltenvk" or "MoltenVK not statically linked".
| NCNN_LOGE("no static linked moltenvk"); | |
| NCNN_LOGE("MoltenVK not statically linked"); |
| char line[256]; | ||
| while (!feof(fp)) | ||
| { | ||
| char* s = fgets(line, 256, fp); | ||
| if (!s) | ||
| break; | ||
|
|
||
| // "library_path": "path to driver library", | ||
| char path[256]; | ||
| int nscan = sscanf(line, " \"library_path\" : \"%255[^\"]\"", path); |
Copilot
AI
Jan 10, 2026
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.
The fixed-size buffers 'line' and 'path' (both 256 bytes) could potentially overflow if the ICD JSON file contains very long library paths. While the sscanf format string limits to 255 characters, JSON files in the wild could have paths longer than this. Consider using dynamic allocation or a larger buffer size, or add proper error handling for truncated paths.
Uh oh!
There was an error while loading. Please reload this page.