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

Basis plugins: support basis_universal 1.50 #119

Closed
wants to merge 30 commits into from

Conversation

pezcode
Copy link
Contributor

@pezcode pezcode commented Jan 31, 2022

Just a few minor changes to make BasisImporter and BasisImageConverter work with the recent 1.16 release 1.50 release.

Nothing major changed, just a few small API differences. Since 1.15 basis has version defines we can use to #ifdef around this stuff, makes things pretty simple 👍

OpenCL works surprisingly well with the automatic fallback to CPU, with nice speedups at very minimal error increase (had to adapt the test error margins for it). What I don't like is the hard dependency, making portability an issue. I would've preferred it being loaded dynamically with dlopen() or similar.

The global selector palette code options were removed completely since apparently they've been a no-op since at least version 1.15 (the minimum version we support). See https://github.com/BinomialLLC/basis_universal/wiki/Release-Notes#12822-v116-release-notes.

I'm leaving this as a draft because I assume there'll still be a few fixes and merged PRs in basis_universal in the near future that could affect some of the workarounds we currently need. But maybe somebody else has a use for this (and could try and report back! 😊). It's been over two years heh

Todos:

  • Figure out how to make OpenCL dependency optional
  • Use all supported versions across different CIs
  • GltfSceneConverterTest compares against a .glb with an embedded Basis-encoded .ktx2, byte-by-byte. Needs some fuzzy comparison to make up for different output with different Basis versions/settings.

@mosra mosra added this to the 2021.0a milestone Jan 31, 2022
@mosra
Copy link
Owner

mosra commented Jan 31, 2022

Thank you!

being loaded dynamically with dlopen()

Crazy thinking -- since the plugin itself is dlopen()'d, it should be technically possible to build a BasisOpenCLImageConverter variant of the plugin that has OpenCL enabled, have BasisImageConverter the CPU-only variant (with both sharing the same source, just passing a different define and resulting in a differently named module). And then somehow make the OpenCL variant the preferred one if it manages to work on the target machine, or maybe not, and leave it up to the user to select it explicitly the same way as multithreading has to be enabled explicitly.

The global selector palette code options were removed completely

I suppose I could merge this bit already and left the rest of the PR for later -- can you separate it into a dedicated commit?

Interesting bit from the changelog (for 1.15, though):

KTX2 1D textures are not supported yet

I suppose if/when that gets done, we could revive the code from #118 for that? As in, keep the PR branch in your repo for later, don't delete it :)

The problem described in #115 still persists, no change to #include "../zstd/zstd.h". I should probably open an issue for that, then. Alongside with an ability to cut out the image loading/saving code we don't use.

@pezcode
Copy link
Contributor Author

pezcode commented Jan 31, 2022

can you separate it into a dedicated commit?

Will do! Pushed, feel free to cherrypick.

keep the PR branch in your repo for later, don't delete it :)

I don't ever delete branches for exactly this reason 😄

The problem described in #115 still persists, no change to #include "../zstd/zstd.h". I should probably open an issue for that, then. Alongside with an ability to cut out the image loading/saving code we don't use.

They already seem to be on the roadmap:
zstd: BinomialLLC/basis_universal#228
images: BinomialLLC/basis_universal#269

@mosra
Copy link
Owner

mosra commented Jan 31, 2022

The first commit is now in master (as 90e3b3d), thanks.

Thanks for looking up the issues, apparently I stopped following the repo quite a while back and wasn't aware of the new stuff happening in there.

@codecov
Copy link

codecov bot commented Jan 31, 2022

Codecov Report

Attention: Patch coverage is 83.33333% with 4 lines in your changes missing coverage. Please review.

Project coverage is 96.99%. Comparing base (8e2cf9a) to head (0f11752).
Report is 12 commits behind head on master.

Files with missing lines Patch % Lines
...lugins/BasisImageConverter/BasisImageConverter.cpp 73.33% 4 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #119      +/-   ##
==========================================
- Coverage   97.01%   96.99%   -0.03%     
==========================================
  Files         147      147              
  Lines       16529    16552      +23     
==========================================
+ Hits        16035    16054      +19     
- Misses        494      498       +4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@pezcode
Copy link
Contributor Author

pezcode commented Sep 11, 2024

Soooo, Basis just got updated to 1.50 and the changeset to get it compiling again is minimal, so I'll push that into this MR as well. Neither of the two issues above (zstd + useless image loading code) got fixed, unfortunately.

The only open question here is how to handle the OpenCL dependency. Do you still want to pursue the idea of compiling two versions? If yes, how would that be exposed in CMake? Two separate targets? Two separate MAGNUM_WITH_ options?

(I also started working on adding support for the new UASTC HDR, but that'll be a different MR. Got it all compiling but still needs proper tests 🐱)

@pezcode pezcode changed the title Basis plugins: support basis_universal 1.16 Basis plugins: support basis_universal 1.50 Sep 11, 2024
@pezcode pezcode mentioned this pull request Sep 11, 2024
4 tasks
Copy link
Owner

@mosra mosra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I didn't even know this was still open 😅

I fixed the zstd thing in 7f29410 back in September 2023, so that's just OpenCL left. And I think what you did here (i.e., compiling with it supported if available, and not using it if not) is the way to go. One can then always call cmake with -DCMAKE_DISABLE_FIND_PACKAGE_OpenCL=ON. Can you add that bit to the docs?

I'd like to ensure that at least one of the CI jobs runs w/o OpenCL, with original error margins. I think that'd be the AppVeyor jobs, or if not then Emscripten at least (so, some extra ifdef in the test...).

The image importer stubs could be done in a similar way as I "fixed" zstd. Feel free to try that, or I can do this myself. (I spent the last 3 days fighting doxygen, so this library is quite manageable in comparison.)

# Added in 1.50
basisu_astc_hdr_enc.cpp
3rdparty/android_astc_decomp.cpp
3rdparty/tinyexr.cpp)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Haha oh god, tinyexr. At this point I should really start shipping my own dummy stubs for all these...

@pezcode
Copy link
Contributor Author

pezcode commented Sep 14, 2024

I reverted the generic comparison threshold bumps, and added a separate test case for converting with OpenCL enabled. Since this requires no real extra setup in the plugin code, I'd say it's OK to only test one file here?

Also added the stubs, was easier than expected. I didn't test with a pre-compiled Basis, but I assume the duplicate symbols are simply ignored during linking. Or does that only hold true for static libs? Then it might need some extra indirection, rather than just adding it to the plugin CMake target source set.

All CI builds now use the latest version, I figured it's best to test that. Or do you want mixed versions?

MSVC 2017 now compiles with CMAKE_DISABLE_FIND_PACKAGE_OpenCL although I'm not even sure if OpenCL is available there to begin with 🐵

Copy link
Owner

@mosra mosra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't test with a pre-compiled Basis

I don't think anybody uses a pre-compiled Basis anymore. There used to be a vcpkg package that I used as a dependency, but due to general upstream unwilligness to do anything about the buildsystem it got abandoned. So it's just the sources directly compiled into the plugin now (OTOH, for example ImGui still has a pre-compiled vcpkg package, and that one I do use.)

Or do you want mixed versions?

Yes, I'd like at least one build to stay on the previous version, certain projects (such as habitat-sim) are on the older version and I want them to be able to update Magnum but keep old Basis, without breakages if I accidentally do something that doesn't work with 1.15 anymore. Basically what I commented on the other PR:

I think e.g. the MSVC 2015/2017 could be at 1.15, 2019 and MinGW at 1.16, 2022 and clang-cl at 1.50 and then linux + mac at 1.16 / 1.50, because those are so fragmented that we want to verify that OpenCL works on all.

CMAKE_DISABLE_FIND_PACKAGE_OpenCL although I'm not even sure if OpenCL is available there to begin with 🐵

Yeah, better do that on some job where you can install OpenCL... There's no best option where to do that, but I think the ARM64 build could be abused for this (install the same opencl packages as on other linux jobs, but then use this option). OpenCL on ARM will then (or might) be verified on the Apple jobs at least, unless they axed the support completely now.

For the CI failures you'll need to add -D__is_trivially_copyable=__has_trivial_copy to compiler flags for all the Basis files on GCC 4.8. That's just Binomial having insufficient testing yet again.

src/MagnumPlugins/BasisImageConverter/CMakeLists.txt Outdated Show resolved Hide resolved
modules/FindBasisUniversal.cmake Show resolved Hide resolved
doc/building-plugins.dox Outdated Show resolved Hide resolved
@pezcode pezcode force-pushed the basis-1_16 branch 3 times, most recently from f93afc5 to dcda265 Compare September 14, 2024 23:42
@pezcode
Copy link
Contributor Author

pezcode commented Sep 15, 2024

On the finish line 🐱

GltfSceneConverterTest seems to be failing because it's comparing the .glb output of KTX+Basis byte-by-byte, which is not very stable across versions (or with different options) I assume.

And then just missing the different basis versions across CIs.

@mosra
Copy link
Owner

mosra commented Sep 15, 2024

Thank you! Yeah, not really sure what to do with that... what the test wants to verify is that the correct plugin got called, and correct MIME type etc was used.

I can look at that tomorrow, maybe an option would be to export a *.gltf instead and do just some fuzzy check on the referenced image.

@mosra
Copy link
Owner

mosra commented Sep 15, 2024

tomorrow

which is actually today 🙈 Done in 8e2cf9a, if you rebase the tests should pass now.

Fully backwards compatible thanks to the new BASISU_LIB_VERSION/BASISD_LIB_VERSION.

The OpenCL dependency is kind of brutal since it hard-links to the shared library. Is there a nicer way to configure this other than CMAKE_DISABLE_FIND_PACKAGE_OpenCL?
Copy link
Owner

@mosra mosra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two remaining minor things, and I think that's it 🥇

Thanks for the massive amount of work done on these plugins.

package/ci/circleci.yml Outdated Show resolved Hide resolved
package/ci/circleci.yml Show resolved Hide resolved
package/ci/unix-desktop.sh Show resolved Hide resolved
Copy link
Owner

@mosra mosra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was looking at the MSVC build failures, found some more bits.

The clang build failing is usual flakiness that happens from time to time. No need to worry with that.

package/ci/appveyor-desktop.bat Outdated Show resolved Hide resolved
modules/FindBasisUniversal.cmake Outdated Show resolved Hide resolved
- IF "%TARGET%" == "desktop" IF "%APPVEYOR_BUILD_WORKER_IMAGE%" == "Visual Studio 2022" IF "%COMPILER%" == "msvc" set BASIS_VERSION=v1_50_0_2
- IF "%TARGET%" == "desktop" IF "%APPVEYOR_BUILD_WORKER_IMAGE%" == "Visual Studio 2019" IF "%COMPILER%" == "msvc" set BASIS_VERSION=1.16.4
- IF "%TARGET%" == "desktop" IF "%APPVEYOR_BUILD_WORKER_IMAGE%" == "Visual Studio 2017" IF "%COMPILER%" == "msvc" set BASIS_VERSION=v1_15_update2
- IF "%TARGET%" == "desktop" IF "%APPVEYOR_BUILD_WORKER_IMAGE%" == "Visual Studio 2015" IF "%COMPILER%" == "msvc" set BASIS_VERSION=v1_15_update2
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MSVC 2015 doesn't have Basis built at all, IIRC because they didn't even support that compiler in the first place.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I removed the exhaustive version selection (and made the latest version the default + a few specific overrides), but as before it still downloads Basis even for MSVC 2015. Making the whole unzipping portion + the string manipulation optional seemed like too much of a struggle.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alright, that's fine then. Given it's cmd.exe it's better to not try to do too much with it :D

}

/* If built without OpenCL, converting falls back to CPU and still succeeds */
#ifndef OpenCL_FOUND
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess this needs another branch with #if BASISU_VERSION < 115, with CORRADE_INFO("OpenCL not used in version 1.15 yet.");.

That's what is making the MSVC 2017 build to fail.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I went the "easy" route and just added BasisUniversalEncoder_INCLUDE_DIR to the test so it has access to the version define in one of the Basis headers. Of course this broke on GCC < 5 because that needs the __is_trivially_copyable=__has_trivial_copy fix on the actual Encoder target. I fixed that by copying the INTERFACE_COMPILE_DEFINITIONS from the Encoder, but I'm wondering if this is already too ugly.

Is just linking to the Encoder target from the test the better way? I'm just a bit afraid of weird side effects this might have on coverage etc.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Linking Encoder target could do quite a mess on static builds, suddenly everything being twice in there etc.

Copying the compile definitions is fine, it's quite clean actually :)

So there's some helpful debug output
This still downloads and extracts Basis even if not built on MSVC 2015
but it did that before as well, and making the preparation plus
unzipping optional would be really annoying.
@pezcode
Copy link
Contributor Author

pezcode commented Sep 18, 2024

Note that Linux jobs now all install PoCL, a CPU OpenCL implementation. But that drags in a bunch of stuff and may prolong the CI package install step. Could you just double-check if that's OK or if we need another solution?

@pezcode
Copy link
Contributor Author

pezcode commented Sep 18, 2024

Not sure why I thought this would be easy 😹

Basis, nice as it is, tries to fall back to a CPU OpenCL device, but then does this:

if (ret == CL_DEVICE_NOT_FOUND)
{
    // ASSERTS IN HERE
    ocl_error_printf("ocl::init: Couldn't get any GPU device ID's, trying CL_DEVICE_TYPE_CPU\n");

    ret = clGetDeviceIDs(platforms[0], CL_DEVICE_TYPE_CPU, 1, &m_device_id, &num_devices);
}
// THIS CAN'T BE TURNED OFF!!!!!!
// WHAT
#define BASISU_OPENCL_ASSERT_ON_ANY_ERRORS (1)

static void ocl_error_printf(const char* pFmt, ...)
{
    va_list args;
    va_start(args, pFmt);
    error_vprintf(pFmt, args);
    va_end(args);

#if BASISU_OPENCL_ASSERT_ON_ANY_ERRORS
    assert(0);
#endif
}

Just causing an assert + crash in debug builds with no way to turn it off 🌵

@mosra
Copy link
Owner

mosra commented Sep 18, 2024

... yeah, was just writing a comment on that. FFS.

I guess pass -DNDEBUG to this file alone? Because we're otherwise crashing with no fucking message whatsoever every time something slightly goes wrong.

@mosra
Copy link
Owner

mosra commented Sep 18, 2024

Could you just double-check if that's OK

As far as I can see on Linux it's okay. APT isn't the fastest package manager, but it spends most of the time during some initial warmup step independently of how much stuff is installed. Looking at the CI times, it fluctuates anywhere between 40 seconds and three minutes, I guess depending on external factors like mirror download speed.

It'll be much worse with Homebrew tho, any extra package there is extra painful due to how is that damn thing reliant on Git and Ruby ... can't you use the regular (deprecated) Apple OpenCL there? The ARM Macs should have even the GPU accessible from the CI VM, or so I heard at least.

When it doesn't find a GPU device, it falls back to a CPU device and
prints a nice error along the way. Unfortunately, all OpenCL-related
errors are accompanied by an assert that can't be turned off. There's
BASISU_OPENCL_ASSERT_ON_ANY_ERRORS but that's always defined to 1 at the
top of the file...

So disable asserts via NDEBUG to actually get to use (and test) the
OpenCL code in debug builds.
…rectly

Apple's OpenCL implementation doesn't seem to support being used through
the ICD, so you have to compile against it directly or use a wrapper.

See also:
https://portablecl.org/docs/html/using.html#using-pocl-on-macosx
https://github.com/jrprice/ocl_icd_wrapper
@pezcode
Copy link
Contributor Author

pezcode commented Sep 18, 2024

I reverted the homebrew changes, I'm just not sure if a blank system has the headers and libraries required. Might still need the opencl-headers formula at least.

@pezcode
Copy link
Contributor Author

pezcode commented Sep 18, 2024

OK, it finds OpenCL on Mac, but fails at runtime with:

7: ERROR: ocl::init: Unable to get any device ID's
7: ERROR: opencl_init: Failed initializing OpenCL

Not sure what's missing... I can't find any relevant issues on the Basis repo.

The ASan failures seem to be coming from PoCL 🙄 Is there a way to selectively disable checks for specific libraries?

@mosra
Copy link
Owner

mosra commented Sep 19, 2024

Ugh, fuck all this.

  1. Make the two sanitizer builds with CMAKE_DISABLE_FIND_PACKAGE_OpenCL, and enable it on the ARM build instead. If this would mean something leaks and gets undetected, it's not our problem, it's either Basis (which we have no way of fixing) or something inside OpenCL (which we also have no way of fixing, or it's likely false positives due to libLLVM-6.so not being ASan-aware). There isn't any OpenCL-specific code path in the plugin itself so it's enough to test w/o OpenCL enabled.

  2. For Mac I'd keep building against the Apple implementation, because that's the likely default scenario for anyone there. I was googling a bit, and found various hints that OpenCL.framework is still there, but isn't really working anymore. I'd do something like

    if(CORRADE_TARGET_APPLE AND OpenCL_LIBRARY MATCHES "System/Library/Frameworks/OpenCL\\.framework")
        set(_BASISIMAGECONVERTER_EXPECT_OPENCL_FRAMEWORK_FAILURE ON)
    endif()

    in CMake, propagate that variable to the test configure.h, and then do this in the test. Assuming it'll still fall back to the CPU implementation and produce a valid output, it's fine like this.

    {
        #ifdef _BASISIMAGECONVERTER_EXPECT_OPENCL_FRAMEWORK_FAILURE
        CORRADE_EXPECT_FAIL("Apple OpenCL implementation is used, which likely doesn't work anymore.");
        #endif
        CORRADE_COMPARE(out.str(), "");
    }

    And if people become annoyed at the warnings and the Apple OpenCL implementation is indeed unsalvageable, we could subsequently move this check directly to FindBasisUniversal.cmake and not even enable OpenCL on Apple at all if it's the system framework being found.

@pezcode
Copy link
Contributor Author

pezcode commented Sep 19, 2024

OK so current state:

  • linux tests 1.16 + disabled OpenCL with CMAKE_DISABLE_FIND_PACKAGE_OpenCL (mainly for testing this interaction)
  • linux-arm64 tests 1.16 + CPU OpenCL
  • linux-static tests 1.15
  • linux-nondeprecated tests 1.50 + CPU OpenCL
  • linux-sanitizers/linux-threadsanitizer test 1.50 + disabled OpenCL
  • macos tests 1.50 + disabled OpenCL (testing)
  • macos-static tests 1.16 + built-in OpenCL
  • MSVC 2015 tests nothing, no Basis there
  • MSVC 2017 tests 1.15
  • MSVC 2019 tests 1.16
  • MSVC 2022 tests 1.50
  • MSVC rt tests 1.16
  • mingw tests 1.50
  • clang-cl tests 1.50
  • none of the Windows builds test OpenCL

@pezcode
Copy link
Contributor Author

pezcode commented Sep 19, 2024

I opened an issue for the OpenCL assert during CPU fallback: BinomialLLC/basis_universal#378

@mosra mosra marked this pull request as ready for review September 20, 2024 08:04
@mosra
Copy link
Owner

mosra commented Sep 20, 2024

Not sure what is codecov reporting and why it thinks that OpenCL warning line isn't covered, but it looks good to me! Going to merge this today.

@mosra
Copy link
Owner

mosra commented Sep 20, 2024

Squashed the commit series a bit and merged as 8e2cf9a...be3ce68. Thank you!

@mosra mosra closed this Sep 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

2 participants