-
Notifications
You must be signed in to change notification settings - Fork 263
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
Make basis_transcoder.{js,wasm} substantially smaller #7
Conversation
basis_transcoder.js 141235 basis_transcoder.wasm 833219
basis_transcoder.js 140999 basis_transcoder.wasm 832985
The impact on the file size is enormous, and it's used to output one line into console which isn't exposed through the API anyhow. basis_transcoder.js 58707 basis_transcoder.wasm 705079
This change makes it possible to exclude formats using compile time options. BC7 is *never* exposed on the web and it has an absolutely huge coding table which means we're just wasting space. basis_transcoder.js 58707 basis_transcoder.wasm 224404
basis_transcoder.js 58707 basis_transcoder.wasm 217809
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 @zeux for looking into the file size of the wasm build! These are tremendous improvements!
CC @austinEng , @shrekshao , @kainino0x
@@ -39,7 +37,6 @@ struct basis_file | |||
|
|||
if (!m_transcoder.validate_header(m_file.data(), m_file.size())) { | |||
m_file.clear(); | |||
std::cerr << "Invalid Basis header" << std::endl; |
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.
Some sort of error reporting is desirable - at least a bool isValid() method returning (m_magic == MAGIC).
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.
Note that error handling can/should be performed by checking the result of startTranscoding()
call - the extra console output may be valuable for debugging but not as part of the production .wasm binary.
For debugging, you can build the transcoder with BASISU_DEVEL_MESSAGES option which will emit detailed error messages indicating what specifically went wrong with the header.
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.
Separately, I think m_magic
variable and all associated code can be removed based on the fact that every method of transcoder is doing quick header validation which should be ~as strong as the code in this file; not that big of a deal but that seems cleaner. It will also make it so that JS code can call startTranscoding and other functions without hitting assertions in debug builds for malformed files.
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 agree we can remove the magic. This is old code when we weren't using Embind and had a more C-like api. It was possible to pass a raw pointer to any of the functions. Now that we have a proper BasisFile
constructor, we could just have the constructor throw a Javascript exception (or have a createBasisFile
function return undefined
). That way none of the member functions are accessible unless you first make a valid BasisFile
. This plus the quick header validation should be sufficient.
Thank you, this is awesome. Transcoder download size is very, very important so I will test and merge this in tomorrow (Thursday). For BC7, I've been planning on reducing the size of the tables, so eventually when BC7 is supported more widely on the Web it can be enabled. BC7 doesn't add much extra value now, but it will once I get higher quality support in there. |
It's in, thanks! |
This change substantially reduces the size of Basis transcoder when built for Web; this means that the transcoder itself can download and compile faster.
The changes are as follows:
This change also cleans up the use of assert(x) by adding NDEBUG - this change isn't very significant on the code size, but removes a hack that's not necessary
The cumulative impact of the changes is as follows (individual impact is recorded in each commit message):
Before:
basis_transcoder.js 139216
basis_transcoder.wasm 886294
basis_transcoder.js.gz + basis_transcoder.wasm.gz 366383
After:
basis_transcoder.js 58707
basis_transcoder.wasm 217809
basis_transcoder.js.gz + basis_transcoder.wasm.gz 124984
There are some more gains to be had in wasm and JS code but they seem to come with performance tradeoffs or more complex build pipeline, so I've settled on this for now.
Re: BC7: it technically can be supported via EXT_texture_compression_bptc extension that's currently "community approved". However, neither Chrome nor Firefox implement it on Windows, and the compatibility table is uninspiring: https://developer.mozilla.org/en-US/docs/Web/API/EXT_texture_compression_bptc. Since BC7 tables take ~470 KB of wasm, I think it makes sense to compile this out for now, and add BC7 if and when it becomes a viable option in WebGL.