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

Re-add optional OIDN denoise as an external dynamic library. #82831

Closed
wants to merge 1 commit into from

Conversation

bruvzg
Copy link
Member

@bruvzg bruvzg commented Oct 5, 2023

Implements godotengine/godot-proposals#7640

Alternative variant #82832 (uses executable).

Pros: more stable API, no writing/reading to disk.
Cons: library should be the same architecture as executable (and there's no ARM64 prebuild library for macOS in official releases), might have issues with dependency loading paths.

TODO: ensure correct library lookup paths are set on all platforms.

@akien-mga
Copy link
Member

Superseded by #82832.

@atirut-w
Copy link
Contributor

Bummer. Using OIDN as an external lib sounded mor reasonable for integrations.

@akien-mga
Copy link
Member

Well this can be reconsidered later if need be, it doesn't change much in the implementation. But not having macOS ARM support would be the bigger bummer IMO.

@bruvzg
Copy link
Member Author

bruvzg commented Oct 12, 2023

But not having macOS ARM support would be the bigger bummer IMO.

It seems to be possible to build it for ARM macOS, but for some reason it's not part of official releases.

@AThousandShips AThousandShips removed this from the 4.x milestone Oct 12, 2023
@atafra
Copy link

atafra commented Oct 13, 2023

We're planning to include ARM macOS builds for OIDN starting with the next major release.

@bruvzg
Copy link
Member Author

bruvzg commented Oct 13, 2023

TODO: ensure correct library lookup paths are set on all platforms.

Seems like on Windows, OIDN is unusable as a dynamic library unless all dependency libs are in the same folder as executable.

Main dll is loading fine, and it's finding module dll, but unable to load its dependencies (tbb dlls), and since it's using LoadLibraryW we can't override library search path with AddDllDirectory on the engine side.

But in should be fixable in the OIDN module loader with something like this:

diff --git a/core/module.cpp b/core/module.cpp
index 14ac590..db74014 100644
--- a/core/module.cpp
+++ b/core/module.cpp
@@ -50,7 +50,7 @@ OIDN_NAMESPACE_BEGIN
     // Prevent the system from displaying a message box when the module fails to load
     UINT prevErrorMode = GetErrorMode();
     SetErrorMode(prevErrorMode | SEM_FAILCRITICALERRORS);
-    void* module = LoadLibraryW(path.c_str());
+    void* module = LoadLibraryExW(path.c_str(), nullptr, LOAD_LIBRARY_SEARCH_DLL_LOAD_DIR);
     SetErrorMode(prevErrorMode);
   #else
     void* module = dlopen(path.c_str(), RTLD_NOW | RTLD_LOCAL);

I'll check it on Linux and macOS a bit later.
Edit: macOS libs seems to have correctly set @rpath, so it's not an issue.
Edit: seems to be working on Linux as well, so it's a Windows specific issue.
Edit: upstream PR - RenderKit/oidn#179

@bruvzg
Copy link
Member Author

bruvzg commented Oct 14, 2023

A new PR version - #83314

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

Successfully merging this pull request may close these issues.

8 participants