-
Notifications
You must be signed in to change notification settings - Fork 302
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
feat: WASM png support #370
Conversation
e.g. Failed to decode png image. image size: -1
Thanks. I'm assuming spng and miniz are unmodified. If that is the case, can you download them (and check their sha256 checksum) as part of the build script rather than including their source code directly? |
Note: Best way to download them is probably to do it as part of the |
I can give that a shot. I assumed you might want me to modify spng to remove the encoder part though to shrink the WASM size. My colleague Ran mentioned that maybe I should experiment with webp too which claims on its website to generate even smaller lossless images https://developers.google.com/speed/webp/. Maybe I should check that out before we merge? |
As far as removing the encoder, I think it will make no difference, the webassembly optimizer should remove code that it can statically determine to be unused. There are other codecs to explore --- webp, avif, jpeg2k, jpeg-xl. However, I can imagine that for different situations, different codecs will be preferred. It isn't ideal to add a bunch of codecs to the neuroglancer precomputed format, since it means every implementation needs to support them, but I imagine if we add them to neuroglancer they will also be useful for other formats like zarr. The imagecodecs Python package already semi-officially adds support for them to zarr under the names |
I guess that makes sense, though it's possible WebP might mostly dominate PNG according to this study (on data that is pretty different): https://developers.google.com/speed/webp/docs/webp_lossless_alpha_study Do you anticipate neuroglancer (and Precomputed) eventually supporting all the codecs you listed? Based on the denoising paper I've been expecting people to want to at least support JPEG-XL or AVIF. |
It seems plausible that all would be supported by Neuroglancer, though I would just assume wait until there is a specific use case for each one. It sounds like the webp compression study you linked (https://developers.google.com/speed/webp/docs/webp_lossless_alpha_study) may have been inaccurate as far as webp decoding speed: https://groups.google.com/a/webmproject.org/g/webp-discuss/c/FPOfZs2cCS4/m/2F0vs7qGiKIJ Also it would be nice to get code splitting working in Neuroglancer before adding too many codecs, so that users don't have to download all codecs even if they aren't using them. Unfortunately that is currently blocked by the fact that esbuild only supports code splitting for the esm bundle format (evanw/esbuild#16) and Firefox does not support esm for workers (https://bugzilla.mozilla.org/show_bug.cgi?id=1247687). Though we could actually split out the wasm files even if we can't split out the javascript --- originally I had the wasm bundled into the javascript, and then split the javascript, but now that splitting the javascript can't be done, just splitting the wasm could be a reasonable alternative. |
Interesting, it looks like that groups discussion is from 2013 and the study says it was last updated in 2017 (but doesn't say what was updated). They may have improved the codec implementation in the intervening years, so I guess we'll have to test it ourselves (we have different data than they tested anyway). I was thinking similarly regarding splitting out the WASM. So far the burden isn't too high (similar to a about two big chunks).
jpegjs is 39.3 KB unminified for the decoder. In any case, I'll finish up this PR later today. |
I think this PR is ready for review. I moved the libraries to the docker. |
Okay, I moved the header logic into a JS function. |
Sorry, I think I wasn't clear. Currently the javascript |
Okay! I validated the chunk size and added support for |
The jpeg library only supports 8-bit so there is no need to check data type. The data type can be obtained from |
Thanks! That worked. I think that takes care of the comments so far. |
const magicSpec = [ 137, 80, 78, 71, 13, 10, 26, 10 ]; | ||
const validHeaderCode = [ 'I', 'H', 'D', 'R' ]; | ||
|
||
// not a full implementation of read header, just the parts we need |
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 know you put in some export to implement this header parsing in javascript, but is there any advantage over just relying on spng to do that?
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 main advantage I can think of is that the error message is easier to write in an informative way. I can revert it back if you'd like.
One other refinement I could make is to set |
I think it is better not to convert to grayscale for the precomputed format --- that will just encourage laxness and force every implementation to handle that case. |
Okay! I fixed up those items. |
Thanks! |
Thank you Jeremy! I'll release PNG support in CloudVolume today. |
Hi Jeremy,
I added PNG support for Neuroglancer and tested it with grayscale images using a newly compiled libpng.wasm module. I modeled this PR after #318
The important differences are:
I have a working PR for CloudVolume that can write PNGs.
I ran PNG on CREMI aplus, which was a bigger volume than my previous small tests and again saw a ~25% improvement in storage size, so I think this is real.
I'm a bit unhappy with how I had to use a macro in order to safely free ctx at each if statement. If you know a better C idiom, I'd be happy to update it.
Thanks!