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

Allowdecoder.decodeBlock to be async #172

Closed
manzt opened this issue Sep 2, 2020 · 6 comments
Closed

Allowdecoder.decodeBlock to be async #172

manzt opened this issue Sep 2, 2020 · 6 comments

Comments

@manzt
Copy link
Contributor

manzt commented Sep 2, 2020

I have been experimenting with using a WASM decoder as the current implementation isn't very performant for some of the images we are working with. Specifically I wanted to experiment with fast-lzw, but since all decoding is synchronous in geotiff, I can't just define a new decoder easily since fast-lzw.LZW.decompressAll is an async function.

In general, WASM based modules require some async initialization and since PoolOrDecoder.decode is only called in an async function getTileOrStrip, awaiting the results of this.decodeBlock would allow more flexibility in decoder implementations.

Proposed change

export class BaseDecoder { 
   async decode(fileDirectory, buffer) { // add async here
     const decoded = await this.decodeBlock(buffer); // await the decodeBlock
     /* ... */
     return decoded; 
   } 
 } 
@manzt
Copy link
Contributor Author

manzt commented Sep 2, 2020

Similarly in zarr.js we allow each codec to return a buffer or promise for a buffer and then await the result regardless since decode/encode are always called in an async context.

https://github.com/gzuidhof/zarr.js/blob/f9252cd775d8fbe39bc0c5ab64bd37bdb2106a10/src/core/index.ts#L471

@manzt manzt changed the title AllowPoolOrDecoder.decode to be async Allowdecoder.decodeBlock to be async Sep 2, 2020
@manzt
Copy link
Contributor Author

manzt commented Sep 8, 2020

I created lzw-tiff-decoder which is a wasm-based module written in Rust to perform the tiff decoding and it works fairly well! I just had to fork geotiff.js to add async/await to BaseDecoder.decode as described above.

If you are interested in including lzw-tiff-decoder, I'd be happy to make a PR, but it is a fairly large bundle (~50kb) and I wouldn't want users to need to include the decoder unless completely necessary. Perhaps of more interest would be to enable users to provide their own getDecoder function, allowing the current one to be treeshaken if not used. This would enable users to swap implementations.

@constantinius
Copy link
Member

Hi @manzt

Using async decoders makes more and more sense as WASM gets more traction. I think it makes sense to await the decompressed result. A PR here is welcome!

Regarding the LZW implementation: it would be great to have some performance and byte size comparisons between the options. Do you think you can provide that information?

zarr.js looks like a very interesting project!

@manzt
Copy link
Contributor Author

manzt commented Sep 9, 2020

Using async decoders makes more and more sense as WASM gets more traction. I think it makes sense to await the decompressed result. A PR here is welcome!

Great! I'll make a PR for the async decoders.

Regarding the LZW implementation: it would be great to have some performance and byte size comparisons between the options. Do you think you can provide that information?

Since the current LZW works for some cases, I'd hesitate to suggest adding a ~50kb module unless totally necessary (even if there was a substantial performace benefit). Like I said, it might be more productive to think about enabling users to provide their own decoders and enable treeshaking of the default getDecoder. This isn't possible currently due to this line:

const poolOrDecoder = pool || getDecoder(this.fileDirectory);

For example in our avivator app, we are only dealing with zlib and lzw compressed images tiffs, but our final bundles always end up with all decoders. We actually implement our own Pool with a custom worker, so we can provide our own getDecoder, but because of the line above, a bundler can't throw out the defaults.

it would be great to have some performance

Anecdotaly, the current lzw implementation requires on the order of 10s of seconds to decode a single 1024x1024 tile for some of our images and often results with an OUT OF MEMORY error. The lzw-tiff-decoder wasm module is on the order of milliseconds and seemingly instant.

You can see a GIF of it in action here in Viv: hms-dbmi/viv#277

@constantinius
Copy link
Member

@manzt I think this issue is now resolved with #173, right?

@manzt
Copy link
Contributor Author

manzt commented Mar 4, 2021

Yup!

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

No branches or pull requests

2 participants