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

enable zstddec decode in web worker in KTX2Loader #21984

Merged
merged 11 commits into from
Aug 10, 2021
Merged

Conversation

deepkolos
Copy link
Contributor

@deepkolos deepkolos commented Jun 13, 2021

Description

in my WIP GLTFGPUCompressedTexture, i found that zstddec decode in web worker can have great load time improvement. so i migrate to KTX2Loader too.

图片

@deepkolos deepkolos changed the title enable zstddec decode in web worker in ktx2oader enable zstddec decode in web worker in KTX2Loader Jun 13, 2021
@Mugen87
Copy link
Collaborator

Mugen87 commented Jun 15, 2021

Related #19650.

@mrdoob mrdoob added this to the r130 milestone Jun 15, 2021
@mrdoob
Copy link
Owner

mrdoob commented Jun 15, 2021

@donmccurdy @zeux looks good?

@donmccurdy
Copy link
Collaborator

My understanding is that the files in examples/jsm/libs/* are copies of external libraries, and not meant to be modified locally to avoid diverging from those projects. In this case zstddec.module.js comes from https://github.com/donmccurdy/zstddec. So:

  1. Should the wasm string be exported from https://github.com/donmccurdy/zstddec?
  2. Perhaps the added files should be in examples/jsm/loaders instead?

I'm unsure how (or if) to compare this to #19650. I really like that the WorkerPool abstraction here is very lightweight, and I wouldn't want to see this change blocked on a longer-term project, so other than the question above about file location, this change looks good to me. 👍

@deepkolos
Copy link
Contributor Author

@donmccurdy i think creating another PR to export wasm string will be better, because my next PR will use both zstddec and zstddec worker version, in few imgs cases decoding in UI thread is better then decode in worker. gltf-gpu-compressed-texture

@zeux
Copy link
Contributor

zeux commented Jun 16, 2021

So I don't want to block this PR in any way, and I'm fuzzy on why exactly we ended up with the separate zstd and basis Wasm modules, but an observation is that this is strictly less efficient than it should be:

What I'd expect the flow to be is

  • We parse the headers, let's say in JS via ktx-parse
  • We copy the supercompressed data - whether it's zstd compressed or not and UASTC vs ETC1S - to Wasm module (through a worker boundary)
  • We let the worker decode final texture (potentially in a worker); this includes zstd decompression if needed and transcoding
  • We copy this back to JS (through a worker boundary) and upload the resulting texture to GPU memory

What we have instead after this PR, when zstd is used, is:

  • We parse the headers in JS
  • We copy the zstd compressed data to Wasm module memory (through a worker boundary)
  • We decompress it in wasm
  • We copy it back to JS buffer (through a worker boundary) and pass it to transcoder
  • Transcoder copies it again to another Wasm module memory (through a worker boundary)
  • Worker decodes the texture
  • We copy this back to JS (through a worker boundary) and upload the resulting texture to GPU memory

As a result, we need two thread pools, 2x Wasm instances, more temporary memory, extra latency on passing buffers between worker, and extra redundant copies between Wasm -> JS -> Wasm. This is all suboptimal.

Fixing this requires a single wasm transcoder that supports zstd. I don't remember the history here, but at this point I'd expect basis transcoder to support zstd+uastc inputs without extra configuration, so I feel like the current setup isn't good long term.

edit As a consequence of the "right" fix I'd probably expect zstd decoding to move fully inside Basis loader (unless we need to support non-Basis zstd KTX2 for some reason?), so this change wouldn't be necessary.

@deepkolos
Copy link
Contributor Author

deepkolos commented Jun 16, 2021

@zeux my work in progress project gltf-gpu-compressed-texture is using zstd + (astc|bc7|dxt|etc1|pvrtc), because basis encoder's dependences are not small, and a lite basis transcoder Basis-Universal-Transcoders supports too few format

@donmccurdy
Copy link
Collaborator

donmccurdy commented Jun 16, 2021

I'll try to have a closer look at https://github.com/deepkolos/gltf-gpu-compressed-texture in the future — for the moment Google Translate is not cutting it for me to understand what's going on there sorry! 😅

My hope has been that in some longer-term time horizon, lightweight Khronos transcoders will be available for both ETC1S and UASTC. At the moment they're UASTC-only. When that happens I'd like to switch over (entirely) to those transcoders. In the meantime it seemed like too much hassle to switch between two transcoders depending on the input format.

Agreed with @zeux that separate worker pools for ZSTD and Basis are not optimal... but I don't think it's any worse than what we have now, either. Perhaps we could retain an option of running zstd decoding on the main thread? At least then we know for sure there's no regression...

As a consequence of the "right" fix I'd probably expect zstd decoding to move fully inside Basis loader ...

The reason this isn't easy now is a combination of code history and (my) laziness... the .basis format came along first, so we created BasisTextureLoader. Then .ktx2 became available, and I didn't want to re-implement everything from scratch, so KTX2Loader relies on BasisTextureLoader for most of its processing, including the Web Worker management. But BasisTextureLoader doesn't have ZSTD, and pulling arbitrary external imports into a Web Worker is tough, which is a whole longer rant. Process:

  • GLTFLoader processes .glb
  • GLTFLoader invokes KTX2Loader to transcode embedded .ktx2
  • KTX2Loader invokes ZSTDDecoder to unzip payload.
  • KTX2Loader invokes BasisTextureLoader to transcode payload.
  • BasisTextureLoader spins up some Web Workers and transcodes.
  • KTX2Loader returns result to GLTFLoader.

Maybe it's time to deprecate BasisTextureLoader? 🤔 I don't think there's any remaining reason to use .basis instead of .ktx2, and Binomial's own tools can produce .ktx2 directly now... then we could simplify all of this considerably and just put the relevant code into KTX2Loader, with ZSTD and transcoding in the same worker.

(/cc @lexaknyazev FYI)

@zeux
Copy link
Contributor

zeux commented Jun 16, 2021

Yeah what it looks like is:

a) This fix is intended to be applied to KTX2 loader without the use of transcoding. So it stands on its own - my comment above assumed the transcoding use case but it looks like this is not the case here.
b) The full pipeline for the expected common use, which is KTX2 with transcoding, is long and could be improved if we didn't have BasisTextureLoader

@zeux
Copy link
Contributor

zeux commented Jun 16, 2021

And maybe with this change we can rely on the same web worker infrastructure at least between zstd-only and full zstd+basis decoding in the future?

@deepkolos
Copy link
Contributor Author

if two wasm instance in the same web worker is acceptable,maybe i can do that

@deepkolos
Copy link
Contributor Author

deepkolos commented Jun 16, 2021

the code is not well tested, the example ktx2 is not zstd compressed, i will test it later. sample_uastc.zstd.ktx2 added

@deepkolos
Copy link
Contributor Author

deepkolos commented Jun 16, 2021

@zeux @donmccurdy KTX2Loader's pipeline has changed, but the way to share code between BasisTextureLoader and zstddec.worker.module maybe is not a good way or maybe a prototype to share wasm worker code.

from

js UI -> js worker1 -> wasm -> js worker1 -> js UI -> js worker2 -> wasm -> js worker2 -> js UI 

to

js UI -> js worker1 -> wasm -> js worker1 -> wasm -> js worker1 -> js UI

@donmccurdy
Copy link
Collaborator

It turns out that the latest builds of Binomial's Basis transcoder (https://github.com/BinomialLLC/basis_universal/tree/master/webgl/transcoder/build) include both KTX2 parsing and ZSTD decompression — in the same WASM build — and the size is similar to the {old transcoder + zstd} size anyway. This simplifies things considerably, as we don't need zstddec or ktx-parse ... and should be faster since we can do everything in a single Web Worker without copying across WASM modules.

@deepkolos if you're open to it, my suggestions for this PR would be:

  1. Move WorkerPool.js into examples/jsm/utils
  2. Remove both zstddec.module.js and zstddec.worker.module.js
  3. Probably leave BasisTextureLoader.js as-is for now, we can decide what to do with it later.
  4. Update KTX2Loader.js to use WorkerPool and the latest Binomial transcoder (see this example and existing code in BasisTextureLoader for references).

Is this OK? Thanks again for this PR and for looking into the performance here!

@deepkolos
Copy link
Contributor Author

@donmccurdy ok, i will make the change in this weekend.

@deepkolos
Copy link
Contributor Author

WorkPool’s api is chenged to enbale locating Worker on demand.

@mrdoob mrdoob modified the milestones: r130, r131 Jun 30, 2021
@mrdoob mrdoob modified the milestones: r131, r132 Jul 28, 2021
@deepkolos
Copy link
Contributor Author

@donmccurdy hi, modification is done which needs a code review

Copy link
Collaborator

@donmccurdy donmccurdy left a comment

Choose a reason for hiding this comment

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

This simplifies things nicely and looks more efficient – thanks!

examples/jsm/utils/WorkerPool.js Outdated Show resolved Hide resolved
@mrdoob mrdoob merged commit 9a31750 into mrdoob:dev Aug 10, 2021
@mrdoob
Copy link
Owner

mrdoob commented Aug 10, 2021

Thanks!

@donmccurdy
Copy link
Collaborator

Is there a way to mark this in advance as a 'breaking change' for the release notes? It will require users to update their basis_transcoder.js version to the newer version.

We may be able to detect this and show a warning when BasisModule.KTX2File === undefined as well.

@mrdoob
Copy link
Owner

mrdoob commented Aug 10, 2021

@donmccurdy Feel free to add a note here 👍

@donmccurdy
Copy link
Collaborator

Done, and opened #22314.

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

Successfully merging this pull request may close these issues.

6 participants