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

Allow using zune-jpeg for JPEG decoding #1845

Closed
Shnatsel opened this issue Jan 21, 2023 · 13 comments
Closed

Allow using zune-jpeg for JPEG decoding #1845

Shnatsel opened this issue Jan 21, 2023 · 13 comments

Comments

@Shnatsel
Copy link
Contributor

zune-jpeg is a pure-Rust JPEG decoder with performance on par with libjpeg-turbo, and far exceeding that of jpeg-decoder.

It now passes rigorous correctness tests on real-world images, and shows no issues when fuzzed. I believe it is mature enough to add to image as an optional backend for JPEG decoding, and you know I don't make claims like these lightly.

@HeroicKatora
Copy link
Member

Sounds good with regards to maturity. There's some API details for implementation:

  • multithreading, we need to not spawn threads on wasm targets for instance. Maybe switch to not-zune-jpeg on these targets?
  • the ImageDecoder interfaces in image assumes that the buffer information (size and color space) is available before the main decoding loop is entered. How should the API be used here?
  • we'll can replace (JpegDecoder::scale) with downsampling but it's odd
  • the image interface assumes a Reader, zune-jpeg takes byte slice fully in memory. The buffering shouldn't be too much of a problem itself, but we need to be able to effectively stop reading from the underlying reader at the end-of-stream marker. How best to implement this? Decoding the image to decide what data consistitutes the full encoded buffer is obv. non-sensical.

@Shnatsel
Copy link
Contributor Author

Shnatsel commented Feb 1, 2023

Note that development has moved from https://github.com/etemesi254/zune-jpeg to https://github.com/etemesi254/zune-image repository, and the decoder is now always single-threaded. There have been API changes as well, so please take a look at the zune-image repository.

I'll defer the rest of the questions to the author, @etemesi254

@Shnatsel
Copy link
Contributor Author

Shnatsel commented Feb 1, 2023

I assume some combination of downsampling and plain old resizing will do the trick. For example, downsample only to 2x of the requested size in all dimensions, and let the user then run the final resizing step. This will save a lot of memory on huge images without degrading the output quality much.

  • the image interface assumes a Reader, zune-jpeg takes byte slice fully in memory. The buffering shouldn't be too much of a problem itself, but we need to be able to effectively stop reading from the underlying reader at the end-of-stream marker. How best to implement this?

Unless image already provides an API for decoding multiple image from the same stream without knowing where one ends and the other starts, then simply ignoring the marker and reading till EOF doesn't break anything. It will use more memory when decoding rarJPEGs, but I don't see a way around this without a streaming parser.

@etemesi254
Copy link

etemesi254 commented Feb 1, 2023

the ImageDecoder interfaces in image assumes that the buffer information (size and color space) is available before the main decoding loop is entered. How should the API be used here?

All decoders in zune provide a decode_header interface. These provide colorspace infromation, depth information, width height, etc before decoding, from jpeg to ppm, you can call decode_header to decode this information, and the other get_ methods to get the information.

the image interface assumes a Reader, zune-jpeg takes byte slice fully in memory. The buffering shouldn't be too much of a problem itself, but we need to be able to effectively stop reading from the underlying reader at the end-of-stream marker. How best to implement this? Decoding the image to decide what data consistitutes the full encoded buffer is obv. non-sensical.

This is a bit trickier. When we hit the EOF marker, there are instances where we may request extra bytes from the decoder (aggressive refill), but the underlying bytestream on having no bits usually returns 0, which the huffman decoder treats as a no-op(to match libjpeg-turbo),in case there are extra bytes, then those will be read.
But as @Shnatsel pointed out, I haven't seen multiple images defined within a single reader

@HeroicKatora
Copy link
Member

The concern isn't so much multiple images in a single reader, but that buffering all the available data beyond the image presents a change in semantics that has somewhat unknown consequences. Just not sure about it and if it's cheap I'd rather terminate the reading properly.

@Shnatsel
Copy link
Contributor Author

All remaining panics on fuzzer inputs are fixed and the fuzzer has run for 250,000,000 inputs without any panics. It has also been tested for correctness on a whole imageboard, 300,000 images. It's time to wire it up.

@HeroicKatora
Copy link
Member

Simple plan for png: A reader that only checks the chunking until EOF to create an in-memory buffer of the whole file. Should be doable.

@etemesi254
Copy link

Won't that be expensive?

@Shnatsel
Copy link
Contributor Author

Shnatsel commented Mar 9, 2023

I have a compiling but untested zune-jpeg adapter in #1876

emilk added a commit to rerun-io/rerun that referenced this issue Jun 12, 2023
### Why
It is much faster.

On my Mac, it takes objectron JPEG decoding from 15 to 10 ms. On the web
the difference is smaller: 35ms -> 29ms. See
https://github.com/emilk/image-decode-bench for more.

It can also decode directly to RGBA, which means we don't need to spend
cycles later padding RGB to RGBA. This takes the entire `objectron` demo
`App::update` down from 21 ms to 14 ms.

Sadly, the `gltf` crate still use the `"jpeg"` feature of the `image`
crate, so weneed to pay for the compilation and code bloat of two jpeg
decoders. The `.wasm` of the web-viewer increases by 23kB
(pre-compression) because of this PR.

This PR also adds support for monochrome JPEGs.

### Related
* image-rs/image#1845
* image-rs/image#1876

### Checklist
* [x] I have read and agree to [Contributor
Guide](https://github.com/rerun-io/rerun/blob/main/CONTRIBUTING.md) and
the [Code of
Conduct](https://github.com/rerun-io/rerun/blob/main/CODE_OF_CONDUCT.md)

<!-- This line will get updated when the PR build summary job finishes.
-->
PR Build Summary: https://build.rerun.io/pr/2376

<!-- pr-link-docs:start -->
Docs preview: https://rerun.io/preview/1aae8d4/docs
Examples preview: https://rerun.io/preview/1aae8d4/examples
<!-- pr-link-docs:end -->
@Shnatsel Shnatsel reopened this Sep 29, 2023
@anarcat
Copy link

anarcat commented Nov 13, 2023

It looks like the latest MR for this is #1877.

@fintelia
Copy link
Contributor

Yep. And the plan is to adopt it in the next major release

@Shnatsel
Copy link
Contributor Author

Oh! I see 0.25 has shipped. Do you want any help with writing a release announcement?

@fintelia
Copy link
Contributor

@Shnatsel that would be awesome. I wrote a changelog entry but haven't written up anything beyond that

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 a pull request may close this issue.

5 participants