ImageOptim / mozjpeg-rust

Safe Rust wrapper for the MozJPEG library
https://lib.rs/mozjpeg
Other
74 stars 19 forks source link

Added in-buffer scanline decoding API #25

Closed OtaK closed 2 years ago

OtaK commented 2 years ago

Hi!

I needed this personally for a project, so I'm submitting a (draft) PR for this feature.

I'm still a bit unsure on how sound my code is, but it does work on my machine™️ for hours on end (it's used in a webcam capture + tensorflow ML inference project).

Most of the diff is rustfmt doing its thing anyway. I'll make a cleanup pass before marking the PR as reviewable :)

Thanks!

kornelski commented 2 years ago

Thanks for the PR. The functionality seems useful

OtaK commented 2 years ago

Okay, this PR took a bit of scale;

I separated the methods I needed with the ones that are in line with the crate's original API. The read_scanlines_flat* functions are meant to work with/produce flat buffers like the ones consumed by the image crate without needing to do post-processing or any kind of funky reordering/flattening.

There are now 2 big categories:

I'm aware this does increase the API surface by a bit, but it's the best I could come up with as the existing API makes it extremely painful to produce data that is readily consumable by image.

kornelski commented 2 years ago

Yeah, that looks okay

kornelski commented 2 years ago

I've noticed the code has already been making slices from uninitialized data, and that's UB, so I'll have to fix that anyway.

OtaK commented 2 years ago

Maybe you noticed, I added a bit more tests in general to test the different ways to decode images (rgb, rgba, packed rgb, packed rgba formats). I didn't test the in-buffer methods directly since those methods are called internally by the allocating ones.

Those UB would be residing only in the non-flat functions right? Since the packed format requires a &mut [u8] this assumes that memory is initialized.

If you want I might take a shot at solving those UBs, but I'm concerned this would cause API breakage anyway.

kornelski commented 2 years ago

Thanks