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

make the jpeg encoder work with non-memory and non RGB8 images #1223

Merged
merged 4 commits into from
May 31, 2020

Conversation

lovasoa
Copy link
Contributor

@lovasoa lovasoa commented May 24, 2020

Change the JPEG encoder to work on a GenericImageView instead of a byte slice

This change allows users to encode jpeg images from other sources
than memory, thus finally allowing the encoding of images that
do not fit in memory.

This also allows users to make the encoder work in a streaming fashion.
(see #1219)

This also finally allows encoding images from other pixel formats than
8-bit RGB. The conversion to 8bit YCbCr (used in JPEG) is done on the fly
during encoding.

I need this change for dezoomify-rs.

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

…te slice

This change allows users to encode jpeg images from other sources
than memory, thus finally allowing the encoding of images that
do not fit in memory.

This also allows users to make the encoder work in a streaming fashion.
(see image-rs#1219)

This also finally allows encoding images from other pixel formats than
8-bit RGB. The conversion to 8bit YCbCr (used in JPEG) is done on the fly
during encoding.
@HeroicKatora
Copy link
Member

HeroicKatora commented May 24, 2020

I'm not sure how being generic over GenericImageView solves the problem of not holding all of the image in memory. It seems to me that loading from an external memory such as the file system is inherently fallible and the interface for accessing pixels can not properly express this. Can you expand on how the interface is intended to be used for this purpose?

@lovasoa
Copy link
Contributor Author

lovasoa commented May 24, 2020

Hi @HeroicKatora !

In dezoomify-rs, I handle missing image parts by writing black pixels in them. But this is up to the application developer to choose what to do. Anyway, I really don't think it's excessive to ask for image pixels to be infaillibly accessible when encoding an image. What do you think about the PR overall?

@fintelia
Copy link
Contributor

Do we know whether this change leads to any performance regression?

@HeroicKatora
Copy link
Member

HeroicKatora commented May 24, 2020

Anyway, I really don't think it's excessive to ask for image pixels to be infaillibly accessible when encoding an image.

Any I/O operation is fallible be it disk failure, network failure, or data corruption. The strategy of using replacement pixels doesn't sound too bad for your application, it seems like a reasonable fallback. It only doesn't seem generically applicable.

On the PR itself I'm fine with the interface. It even seems to add support for bgr8 which is awesome. There's only the performance question raised by @fintelia, then you got a 👍 from my side.

@lovasoa

This comment has been minimized.

src/jpeg/encoder.rs Outdated Show resolved Hide resolved
src/jpeg/encoder.rs Outdated Show resolved Hide resolved
Copy link
Member

@HeroicKatora HeroicKatora left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The performance doesn't seem too bad overall, and we have a likely fix in the pipeline. Overall, this unlocks many interesting possibilities for the jpeg encoder. Besides images stored externally (for which I don't consider this interface perfect but for which it is better than nothing) this includes lazily computed images. That's definitely note-worthy.

This also only adds little additional API: a single method. The only problem is that of color conversion but we already require the methods from Pixel alone so it should not be the concern of this PR. Any replacement is likely to have an alternative trait bound available and requires a major version anyways.

@HeroicKatora HeroicKatora merged commit 0430da6 into image-rs:master May 31, 2020
@lovasoa
Copy link
Contributor Author

lovasoa commented Jun 1, 2020

Awesome! Thank you for merging. Would you be interested in a pull request adding the YCbCr pixel type to image-rs, allowing to avoid unnecessary back-and-forth colorspace conversions when the original image is already in YCbCr ?

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.

None yet

4 participants