-
Notifications
You must be signed in to change notification settings - Fork 85
Integrate Vendor-specific SPZ Extensions, WASM Bindings, and CMake Improvements (Backward-Compatible) #67
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
base: main
Are you sure you want to change the base?
Conversation
|
Seems we might need to update the github workflow to run the pytests? Or am i reading the CI wrong and they ran successfully already |
I think they ran successfully already, which were still done through |
|
The extensibility extensions stuff seems extremely useful for SPZ. How are they backwards compatible? Do you mean older SPZ loaders will be able to load files with these extensions? |
Older SPZ loaders won't load the files with these extensions used/enabled. But they can load the files generated with the new version of SPZ library without these extensions enabled. By default, these extensions are disabled and henceforth one may use this SPZ library as before, without caring about the changes here. |
ProjitB
left a comment
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.
Thanks for adding more tests!
I think the idea of extending the sh bits is great but I do have more questions / concerns on how to make this a bit more modular. Perhaps we can setup a call to discuss a bit further on how the extensions are being added.
My concerns are broadly regarding:
- keeping the core of the library simple, and thus pushing extensions logic further into the extensions code, if this is how we decide to support the behavior. I'd prefer more strict enables around the loading of spzs and a more explicit structure around the enablement of extensions.
- simple, fixed size header format, that can be decoded without reading the rest of the gaussians (even though the header is part of the compression..)
If maintaining backwards compatibility adds a lot of code complexity, i think we should discuss with the larger community, the scope for incrementing the data version, and pushing some of the quantization related features into the core library rather than having it as an extension.
Sorry for the delay in reviewing this!
| include(FetchContent) | ||
| FetchContent_Declare( | ||
| zlib | ||
| URL https://zlib.net/zlib-1.3.1.tar.gz |
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.
i don't think we've so far specified a version of zlib that we pin to. It does seem prudent to do so though..
Thanks for adding this. But perhaps we also make a note of it in our readme?
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.
Sure! Will do!
|
|
||
| ### Header | ||
|
|
||
| **Version 2 (current):** |
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.
This should be Version 3 right?
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.
Yes! Nice catch.
| float shMin = -1.0f, float shMax = 1.0f) const; | ||
| }; | ||
|
|
||
| // Represents a full splat with lower precision. Each splat has at most 64 bytes, although splats |
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.
we probably have to update this comment?
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.
Yes!
| ## File Format | ||
|
|
||
| The .spz format is a gzipped stream of data consisting of a 16-byte header followed by the | ||
| The .spz format is a gzipped stream of data consisting of a variable-size header followed by the |
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.
We may want to more carefully consider the implications of variable header sizes..Is this actually backwards compatible? Any version of spz that changes the header size would break any renderer that tries to decode based on the header bytes right?
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.
This is related to how we support the extensions. Currently we put extensions data immediately after the header. Another option is to use a fixed-size header and an offset to point to any extensions. We may discuss this further in a chat.
|
|
||
| // Quantizes to 8 bits, the round to nearest bucket center. 0 always maps to a bucket center. | ||
| uint8_t quantizeSH(float x, int32_t bucketSize) { | ||
| // Quantizes to 8 bits using min/max scaling, then round to nearest bucket center. 0 always maps to a bucket center. |
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.
is this min/max scaling actually valuable in the average case? Larger splats have millions of gaussians, in which i guess the sh values would vary throughout the standard range. The benefits of this would only be in particular cases. I was wondering if it's worth the added complexity.
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.
It depends on how the splat was created. In both optimization and feedforward models that generate splats, SH values are not always constrained, but are only learned via some render loss. Unless theres a regularizer on the SH values, we've seen model output (mostly from inference on transformer models) have SH ranges well outside of [-1, 1], prompting the minmax scaling feature here.
For what its worth, standard gsplat optimization pipelines tend to have SH in [-1,1], but with more generative 3D models popping up, it might be helpful for those cases?
I'm okay to go either way on this! We can keep it simple and remove minmax, and re-add it in the future if deemed necessary.
| return success; | ||
| } | ||
|
|
||
| void packQuaternionFirstThree(uint8_t r[3], const float rotation[4], const CoordinateConverter& c) { |
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.
Is the reason for having first three that in the less compressed cases (ie more sh bits), we store the quaternions at the highest precision possible?
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.
This is for recovering the compatibility with the older SPZ loaders that can process only V2 files. These loaders assume the quaternions are packed with only the first three components and we want our library to still be able to save such files.
| float minSH = -1.0f; | ||
| float maxSH = 1.0f; | ||
| if (o.sh1Bits != DEFAULT_SH1_BITS || o.shRestBits != DEFAULT_SH_REST_BITS || o.enableSHMinMaxScaling) { | ||
| auto ext = std::make_shared<SpzExtensionSHQuantizationAdobe>(); |
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.
it seems like we're implicitly using this extension. Shouldn't we skip any/all computation if the extension isn't enabled?
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.
Hmm...make sense. We may add an option to explicitly indicate whether to use extensions instead of guessing from the options.
| } else { | ||
| // Backward compatibility for older versions. | ||
| packed.usesQuaternionSmallestThree = false; | ||
| packed.rotations.resize(numPoints * 3); |
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 resize should be gated by the packing condition right? rather than the verison?
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.
For V2, it doesn't seem there's thsupport for the smallest-three packing condition so I assume we'd turn off the usesQuaternionSmallestThree by default. But sure, we can put the force-off of usesQuaternionSmallestThree during initialization of this variable and gate the code here with usesQuaternionSmallestThree.
Summary
This PR merges the
adobe-publicbranch intomainand introduces several non-breaking, opt-in improvements for SPZ users:All changes are fully backward-compatible with existing pipelines and SPZ readers.
What’s New
1. CMake & Build System Improvements
SPZ_BUILD_PYTHON_BINDINGSSPZ_BUILD_TOOLSSPZ_BUILD_WASMSPZ_USE_EMSCRIPTEN_ZLIBZLIB::ZLIBThis makes builds predictable across Linux/macOS/Windows/Emscripten and prevents accidental install breakage.
2. SPZ Core Enhancements
We add an optional, vendor-specific extension system to SPZ, which can be identified through the
flagin the header. Currently, it supports only two extensions. Nevertheless, more vendor-specific extensions can be similarly added in the future. The two new extensions are listed as following:SH Quantization Extension
Optional metadata describing Adobe’s SH coefficient quantization:
sh1BitsshRestBitsshMinshMaxSafe Orbit Camera Extension
Optional metadata describing camera constraints for orbit controls:
safeOrbitElevationMinsafeOrbitElevationMaxsafeOrbitRadiusMinThese extensions attach to the SPZ extension chain and remain invisible to legacy consumers. They are strictly additive and safe to ignore unless explicitly used.
3. WebAssembly / TypeScript Bindings
Introduces a WebAssembly runtime:
EmGaussianCloudwith JSTypedArrayviewsloadSpzFromBufferandsaveSpzToBufferspz.d.tsThese modifications enable browser-based SPZ debugging/visualization with no impact on native runtimes.
4. Python Packaging Updates
pyproject.tomlwith:SPZ_BUILD_PYTHON_BINDINGStestsdependenciesThese modifications simplify Python wheel builds and align with modern scikit-build-core conventions.
Backward Compatibility
✔ Existing SPZ files load without changes
✔ Adobe extensions are optional and ignored by default
✔ No change to baseline SPZ serialization
✔ Build behavior unchanged unless new flags are enabled
Supplementary Info of this PR
Also, here are some details for the new compression options. We found
sh_rest_bits=5(up from 4 in the previous spec) with minmax scaling enabled to be the best setting, though it will slightly inflate spz size compared to the previous version's settings.Configurable SH bits test
adobe_backpack_float32.ply(129.3 MB)Without Min/Max Scaling
sh_rest_bitsWith Min/Max Scaling
sh_rest_bitsMin/Max Scaled Qualitative Comparison
Recommendations:
Caveats and notes: