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

Add zune-jpeg as an alternative backend for JPEG decoding #1876

Closed
wants to merge 9 commits into from

Conversation

Shnatsel
Copy link
Contributor

@Shnatsel Shnatsel commented Mar 9, 2023

This is an initial implementation to get the ball rolling. I've implemented a decoder but it's not yet wired up to decoder selection based on format, or any other machinery. Haven't tested it either.

Right now this buffers the entire input in memory, but this requirement may be relaxed soon..

It also parses headers twice, because zune_jpeg::JpegDecoder accepts a lifetime parameter and putting it into the same struct as the input data would create a self-referential struct. This should not be a big deal because it is offset by the efficiency gains over jpeg-decoder.

Fixes #1845 once complete


I license past and future contributions under the dual MIT/Apache-2.0 license, allowing licensees to choose either at their option.

@Shnatsel
Copy link
Contributor Author

Shnatsel commented Mar 9, 2023

Depends on etemesi254/zune-image#98 upstream, that must be merged and released first

@Shnatsel
Copy link
Contributor Author

Shnatsel commented Mar 9, 2023

Question to maintainers: what should this be integrated into on top of just providing a Decoder struct, if anything?

@HeroicKatora
Copy link
Member

Question to maintainers: what should this be integrated into on top of just providing a Decoder struct, if anything?

I'd say the important part is choosing between the decoder implementations in load_decoder: https://github.com/image-rs/image/blob/master/src/io/free_functions.rs#L45

That should be pretty much it, in terms of decoder, that's the central translation from a file format to its decoder. One possible concept for dynamically choosing this would be to pass a preference hash map for each format and falling back to the current choice when no preference is given.

@fintelia
Copy link
Contributor

fintelia commented Mar 9, 2023

I haven't thought it through fully, but my initial reaction is that if we use zune-jpeg it should probably replace our current use of jpeg-decoder for decoding JPEGs.

@Shnatsel
Copy link
Contributor Author

Shnatsel commented Mar 9, 2023

I haven't thought it through fully, but my initial reaction is that if we use zune-jpeg it should probably replace our current use of jpeg-decoder for decoding JPEGs.

Do you mean it should be set as the decoder to be used by default, but keep jpeg_decoder around to avoid an API break by removing it? Or do you mean remove jpeg_decoder entirely and try to contort the zune-jpeg wrapper into the same API (can probably be arranged with liberal use of PhantomData)? Or do you mean an API break that removes jpeg_decoder entirely and switch to the API proposed in this PR?

@fintelia
Copy link
Contributor

fintelia commented Mar 9, 2023

I haven't thought it through fully, but my initial reaction is that if we use zune-jpeg it should probably replace our current use of jpeg-decoder for decoding JPEGs.

Do you mean it should be set as the decoder to be used by default, but keep jpeg_decoder around to avoid an API break by removing it? Or do you mean remove jpeg_decoder entirely and try to contort the zune-jpeg wrapper into the same API (can probably be arranged with liberal use of PhantomData)? Or do you mean an API break that removes jpeg_decoder entirely and switch to the API proposed in this PR?

I mean removing jpeg_decoder entirely while exposing the same API if possible, or doing an API break if not.

@Shnatsel
Copy link
Contributor Author

It should be possible to stuff zune-jpeg behind a compatible API. The jpeg_decoder wrapper is strictly more restrictive. I'm going to continue working in a separate Decoder struct for now, but I'll switch over to a hostile takeover of the JpegDecoder struct once everything settles, provided @HeroicKatora is OK with it.

@Shnatsel
Copy link
Contributor Author

I've opened #1877 to show what outright replacing jpeg_decoder with zune-jpeg looks like.

@HeroicKatora
Copy link
Member

I'll switch over to a hostile takeover of the JpegDecoder struct once everything settles, provided @HeroicKatora is OK with it.

Sounds good to me, once the dust settles to have a period in which unanticipated feedback can arrive.

emilk added a commit to rerun-io/rerun that referenced this pull request 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 marked this pull request as ready for review September 29, 2023 13:43
@Shnatsel Shnatsel changed the title WIP integration of zune-jpeg Switch from jpeg-decoder to zune-jpeg as JPEG decoding backend Sep 29, 2023
@Shnatsel Shnatsel changed the title Switch from jpeg-decoder to zune-jpeg as JPEG decoding backend Add zune-jpeg as an alternative backend for JPEG decoding Sep 29, 2023
@Shnatsel
Copy link
Contributor Author

I'm going to go ahead and close this in favor of #1877

@Shnatsel Shnatsel closed this Sep 29, 2023
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.

Allow using zune-jpeg for JPEG decoding
3 participants