hello, looks like something broke circa commit 47f05fad43621aa1637427d857d639d0ecfbb7cd #107
Comments
|
Hello, thanks for the report, and I'm sorry to hear that we broke your build. Since these changes are needed for OpenCLOn12 I don't think we can back them out. Can you say more how your cross-compilation toolchain works? Is there an update that might provide the missing header file? Worst-case we could selectively disable this part of the build, but because ICD loaders without this functionality will not enumerate OpenCLOn12 I think we should consider this a last resort. Once we get this fixed I'd also be interested in adding this configuration to our Travis builds. Is this something you could help with? Thanks! |
|
Hi. Thanks for your cool work.
I'm not sure where I don't know anything about Cheers !
|
|
Oh dear. If we copy windows.management.deployment.h" and use that in the cross-compile build process, ICD_LOADER now has specific dependencies for which there are no readily available sources to use in cross-compilation. Is there another way ? |
|
Thanks for a very detailed reply! Regarding this step:
Does your build process only use the static import library I'm guessing this is the case since you are currently building with If so, I think we can add a similar control to skip the section of the ICD loader for OpenCLOn12. Similar to If this sounds good I'll create a PR with this change to try. |
|
AFAICT the missing header is part of the Windows Runtime, hence the loader picks up the Windows SDK as a dependency. The dependency is not only for bringing in types and defines but there is significant new logic for OpenCLOn12, and without calling out the gist of it, it's hard to imagine making it work without COM/WRL/WinRT, so the Win SDK in general. The Windows SDK is installed under Program Files (x86), and said header on my system is found for all installed SDK versions installed: PS C:\Users\mnagy> gci 'C:\Program Files (x86)\Windows Kits\10\' -rec -fil windows.management.deployment.h
Directory: C:\Program Files (x86)\Windows Kits\10\Include\10.0.17763.0\cppwinrt\winrt
Mode LastWriteTime Length Name
---- ------------- ------ ----
-a--- 2018. 10. 23. 0:35 138348 Windows.Management.Deployment.h
Directory: C:\Program Files (x86)\Windows Kits\10\Include\10.0.17763.0\winrt
Mode LastWriteTime Length Name
---- ------------- ------ ----
-a--- 2018. 10. 23. 0:37 379955 windows.management.deployment.h
Directory: C:\Program Files (x86)\Windows Kits\10\Include\10.0.18362.0\cppwinrt\winrt
Mode LastWriteTime Length Name
---- ------------- ------ ----
-a--- 2019. 03. 18. 17:25 138348 Windows.Management.Deployment.h
Directory: C:\Program Files (x86)\Windows Kits\10\Include\10.0.18362.0\winrt
Mode LastWriteTime Length Name
---- ------------- ------ ----
-a--- 2019. 03. 18. 17:26 381033 windows.management.deployment.hThe standalone Windows 10 SDK may be installed from here, although 99% of systems likely install it (like me) via the Visual Studio (Build Tools or IDE) installer. I'm not a minGW user and don't know how CMake behaves when used with minGW. I don't have the capacity to debug this all the way, create an In non-cross compilation scenarios CMake detects the SDK version and stores it in a variable per the docs. |
|
Oof, yeah, MinGW isn't going to support the WinRT headers anytime soon. I think the only two options here are:
@hydra3333: I am very curious about your response to @bashbaug regarding why you need to build the ICD loader. |
|
@hydra3333 Is it not possible to build ffmpeg using Clang on Windows? The LLVM toolchain is pleasantly schizoid in the sense it can behave like GCC/MSVC alike while being able to produce binaries for most platforms. We had a project where the possibility of cross-compiling of the OpenCL parts came up, but the ICD (by definition) is platform specific. It's not the best candidate for cross-compiling. I don't think ffmpeg should have many GNU/POSIX-specific code under the hood that would warrant wanting to compile the entire thing using the GNU toolchain. Or as @jenatali said, taking a step back, what's the reason for trying to build an ICD loader? |
|
Just to add one more thing while waiting for a response... @hydra3333: If you are truly building a "static" ffmpeg.exe like you say, using a static lib of OpenCL, that's a bad idea. One of the primary reasons for dynamic linking is to allow implementations to change/improve out from under you, as long as they support the same API. As a result, your statically-linked ffmpeg wouldn't be able to take advantage of OpenCLOn12 if that's the only OpenCL driver available on the system, either using older OpenCL ICD loaders that predate my changes, or if you end up having to compile without that support. On the other hand, if you're just building the OpenCL ICD loader in order to get an import lib like @bashbaug suggested/hoped, and then discarding the resulting DLL, that's fine, and in that case stripping out OpenCLOn12 support just to get it compiling would be fine as well. You could even use delay-loads so that you can run when the target system doesn't have an OpenCL ICD loader available. It'd be possible to go even further and not build the real ICD loader, and just build some stubs that have the correct import signature to generate the correct imports for ffmpeg.exe. |
This is true. |
I grabbed a temporary copy of Windows.Management.Deployment.h from the development win10 VM MS make available, it still didn't build due to it containing further includes ;) |
OK. I was told the only choices were
|
I'm not sure it is, especially with its tons of extenal dependencies. Perhaps. In my case I use VMs, as I can't "pollute" my main PC with dev environment, and MS charge $ for an ongoing win10 license in a VM. |
yes. |
ah. |
|
So, sorry for your inconvenience. |
|
@hydra3333 Regarding some of your responses:
That way you'll end up importing all the symbols into your executable, in which case it will be ffmpegs responsibility to be updated alongisde GPU drivers and always ship the latest ICD. Since this is very unlikely, vendors highly encourage dynamically linking the ICD loader. For development purposes IMHO it's fine, but not ideal for deployment and distribution.
Having looked at MABS there are ineed a daunting number of dependencies. Having been converting a fair amount of GNU/Linux projects to Windows in the past decade my general remark is that often it's easier than one might think. The biggest part missing is usually the build scripts in something other than GNU Make. However, package repos have improved a lot in the past years, to extents to get ffmepg (and ALL of it's dependencies) to build on Windows is as simple as: I'm a prime time user (and package maintainer) of Vcpkg and the ffmpeg package supports turning on hordes of optional features as fell (simultaneously too, if you so wish): Hope this helps and perhaps even circumvents your issue altogether. All packages and their downstreams in Vcpkg build with MSVC (or Clang if you set it up that way) and patch the sources as needed, so you're not subject to all the hassle of having to cross-compile, once again which is usually a consequence of projects not maintaining CMake(/meson) build scripts. Luckily, community projects like Vcpkg help a lot in hooking up libraries to each other. |
|
@MathiasMagnus I think you're wrong about this:
That'd be an acceptable approach I think, similar to what I had suggested around building a stub ICD loader. That should cause the resulting ffmpeg.exe to be dynamically linked against the ICD loader, not statically linked with the loader or an actual ICD. As I understand it, there does not exist any tool which can "downconvert" a DLL to a static lib for linking, only tools which can generate import libs. |
|
Also, @hydra3333 if your response to this is correct:
Then your ffmpeg.exe isn't actually statically linked against the loader, just against an import lib. You should be able to confirm by looking at the import table of the resulting exe. MSVC's link.exe can show this ( If that ends up being the case, then whatever modifications you need to make to this codebase don't really matter, as long as the OpenCL API entrypoints exist in the resulting DLL, and are present in the .def file, causing the import lib to be created correctly. |
|
@MathiasMagnus thank you very much indeed. I have been following others' recipes and not worrying to much until something breaks to fix, learning that way, and you have clarified very clearly and well.
I am very excited to see this and will certainly take a look at it. |
|
Yes indeed. Thank you once again !
|
|
Hi @hydra3333 , I just created PR #110 that adds a CMake option to disable support for the OpenCLOn12 ICD to simplify build dependencies. Can you give it a try to see if it works with your toolchain? You'll need to explicitly enable the option since the default is to support the OpenCLOn12 ICD. You can do this the same way you were explicitly setting We're looking at some other options to simplify the build process also, but something like this is the most likely short-term solution. Thanks! |
|
@bashbaug OK, thanks I will. In regard to OpenCLOn12, I gather this is "OpenCL 1.2 over DX12" ? per https://www.khronos.org/assets/uploads/apis/OpenCL-3.0-Launch-Apr20.pdf I suppose I'm not missing anything for my use case by the looks anyway. |
|
:) I'm doing something wrong :) Nearly there. patches.zip also of interest |
|
OK, that's encouraging. I just pushed one more commit that may resolve this issue. Would you mind grabbing the latest changes, or making one additional small change to your CMakeLists.txt? If it doesn't, could you check if you have a libruntimeobject.a on your system? See related comment here. The CMake warning about Thanks! |
|
Beaut. Will let you know. |
|
Success ! |
|
I'm a little bit confused about this. I was under the impression that it was safe for applications that want to use OpenCL to ship their own build of the loader (in order to not depend on one being installed on the system), and use it to load ICDs if present. But in this issue, it's being discussed as if applications need to depend on the system-installed loader. Is a copy of the loader guaranteed to be present on all Windows systems (thus making it safe to have a hard runtime dependency on that)? If not, do I need to implement an "ICD loader loader" to dynamically load the OS-installed loader at runtime and call into that? |
Isn't that dangerous ? Overwriting/bypassing what manufacturers/windows may have installed ?
Not that I've found. To be over-ridden by the owners here of course. |
|
I was under the impression that the loader was supposed to be an implementation of a standard interface, and that implementations were supposed to be fairly fungible, with all the device-specific stuff being in the ICD itself. If that's not the case, then… what's the point of the loader as a separate library? |
You need the ICD loader to enable multiple implementations living happily side-by-side in a plug'n'play manner.
It is discouraged because ISVs typically fire and forget and don't do a great job of keeping all their dependencies up to date, not to mention I may install ffmpeg once and leave it at the same version for an entire year. Even if the project did a good job of updating its deps, I may not opt-in for those updates, and implementations may go ahead and update their ICD, but the bundled loader of ffmpeg for eg. may thus be out of sync. Ultimately it all boils down to who owns the loader: is it a system component? Is it part of the driver? The current status quo is that it's shared between all vendors shipping ICDs. They make an honest coorinated effort in not screwing up each others ICD by shipping incompatible loaders. Bypassing the system loader is a bad idea, because projects will likely target building a minimal loader (just make it work), but the additions to loading OpenCLOn12 are opt-in, but in any case an option. Even if I install OpenCLOn12 on my system, if projects choose to bypass the system installed loader, they may not be able to load some implementations, CLOn12 being a prime candidate as it requires extra, novel footwork from the loader. Windows library model incentivies people to bundle their loader, because they need to build it to obtain an import lib, but don't ship libraries / projects that bundle the loader. You essentially commit to updating your product on every channel on the same day a driver comes out from any vendor. It's calling for trouble. In development scenarios it's totally okay to statically link a loader, because "works on my machine" is legit on dev boxes. But don't ship stuff like that. |
|
Adding my two cents as a Windows developer: We bend over backwards to make old apps run on new versions of Windows. If old apps have a dependency on a system OpenCL loader, that means that we're responsible for ensuring that the system loader can find an OpenCL ICD to make the app work. It may mean changes to the ICD loader as the platform evolves, or potentially new changes to find new kinds of ICDs, as is the case for CLOn12. If old apps ship the ICD loader, that means their dependency on the system has changed. Instead of "looking for an ICD" being the operative request to the system, "looking for regkeys" or "querying the kernel" becomes the interface that must remain working. It severely restricts our options for evolving the platform, and makes development/deployment of things like CLOn12 more complex. Something like the ICD loader belongs as a system component. In future versions of Windows, we will start guaranteeing that an up-to-date version of the ICD loader comes included in the OS installation, so that apps can feel more comfortable taking a dependency on it being present. Regarding needing a "loader loader," the cheap option is to just use delay-load linking. The rest of your code doesn't need to care, and then if the loader isn't present, or doesn't have the right entrypoints, you can gracefully handle that case. Though if you want to work with old loaders as well as new loaders, yes you'd need to dynamically retrieve your individual entrypoints from the loader, since on Windows, linking (normal or delay-load) is all-or-nothing. |
|
Thanks @jenatali, I really appreciate the perspective.
On Linux and FreeBSD, that means this package serves a clear purpose: you can bundle it in your package, or add it as a dependency in package managers, to use OpenCL without having a hard dependency on any actual ICD being installed. Excellent. On Windows, it sounds like in the future we'll be in a position similar to that of macOS, where I can just link to the OS-installed copy, but until that ships (and until I stop having to support older Windows versions where it hasn't, which likely won't be for years), that's not an option. So this package existing seemed to imply that it was safe to bundle with an application. If that's not the case, it should be made clear in the README file that nobody trying to ship a userspace application should actually be building this or linking against it. Delay-load linking isn't available in gcc or clang on windows as far as I'm aware, so that's not an option here… so I guess I'm going to have to restructure ffmpeg's OpenCL filtering code to load OpenCL.dll at runtime and call into it. This will probably look something like ffnvcodec, and require maintaining what's essentially a parallel set of OpenCL headers that declare function pointers instead of actual functions. Up to now we'd sort of assumed that wasn't necessary, again because it appeared that this package was supposed to serve that purpose. So… I guess that's what I'll have to do, I'm just kinda frustrated that it's necessary. |
|
My understanding of the reason this project exists for Windows is to provide a common source for drivers to be able to include this DLL in their driver packages. I was shocked to learn that static linking was possible, or that apps could/do include this as part of the app. On Windows, if you have an OpenCL driver, you have a system OpenCL ICD loader. In the future that will be even more broad, to just "you have an OpenCL ICD loader." However, if OpenCL is optional for you, the lack of an ICD loader could prevent you from loading, unless you use dynamic linking or delay loading. Regarding delay-load support, the LLVM linker does support it: https://github.com/llvm/llvm-project/blob/3508c1d8fbe9b053d32330b6995c88c57a82e33f/lld/COFF/Driver.cpp#L1786 . I'm not sure about MinGW/GCC. I've also heard that there's interest in providing a different way of linking to OpenCL, which @bashbaug has been looking into, which would handle dynamically loading the loader and dispatching to ICDs efficiently. |
Which raises the question of why it was ever part of driver packages (instead of installed with the system) to begin with, but I suppose that's immaterial now.
You can see why it'd seem like the right thing to do, given what it allows and how the documentation doesn't tell you not to, right? I'd really like to get things clarified here, so app developers can know how they're meant to interact with this instead of just guessing. Re: guarantees, yeah, OpenCL's optional in my use-case (and probably for most people who ship ffmpeg, whether it's as a standalone static build of the CLI tools or as part of a larger application). Re: lld, ooh, thanks! I couldn't find it in their docs so I assumed it didn't exist; should've known there's be undocumented features like that. Definitely interested in what @bashbaug's up to; if we can use that in ffmpeg instead of coming up with something custom, I'm all for it. |
|
Hello! Thanks for (re-)looping me into this discussion. A POC demonstrating what I've been up to is here. It's (probably) not quite ready for prime time yet, but it is working for my simple test cases: https://github.com/bashbaug/SimpleOpenCLSamples/tree/hybrid-loader-lib/libOpenCLHybrid The basic idea it is to link with this library and not to the ICD loader directly. This library will dynamically load the system ICD loader (or fail gracefully, if it's not found), but only call into the system ICD loader for platform enumeration. All of the other OpenCL APIs are implemented directly within the library itself and linked statically. The nice thing about this approach is that it doesn't bypass the system ICD loader completely, so applications will use the latest mechanisms to enumerate ICDs, but otherwise there are no dependencies on the ICD loader. If this sounds interesting I'll at least move it into its own git repo to make it easier to find. Thanks! |
|
This sounds exactly like the solution to this problem I've been looking for! I'd love to see it in its own repo, and potentially linked from the readme here as a "if you're distributing an application this is probably what you want". |
|
Hmmmm, looking closer at the implementation, though… is it actually guaranteed that the layout of the vtable in the loader won't change? And that the copy of the loader installed on the system will use the same layout as the official loader? |
|
That layout is prescribed in the cl_khr_icd extension. Personally I agree that there should be more runtime negotiation of table layout, but yes that is the layout that all ICDs use. |
|
I know this is a very old thread, but I think we finally fixed the last of the mingw issues. 🎉 @hydra3333 if you're still around would you mind confirming that the latest changes work for you so we can close this issue? Thanks! |
Hello.
Something broke within the last 10 days. It used to work.
Probably 47f05fa
Cross-compiling under ubuntu 18.04.3 for target Win10x64.
The text was updated successfully, but these errors were encountered: