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

feature: Implement load_lossy #1753

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

demurgos
Copy link

@demurgos demurgos commented Jul 5, 2022

Add support for lossy image loading.

The new load_lossy function adds error recovery.

If loading reaches the step where it allocates the pixel buffer, then it always succeeds: any error following this step will result in a partially filled buffer where the missing pixels will use their default "zero" value.

This allows for example to read truncated files or other minor issues.


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

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.

Succinct and little surface changes, very neat. I'd be in favor. The only concern that comes to mind would be that completely discarding the error information may be suboptimal for few cases. But we can take a look later, I'd generally treat them as warnings and defer this to warning reports in either case.

Comment on lines 227 to 230
pub fn decode(mut self) -> ImageResult<DynamicImage> {
let format = self.require_format()?;
free_functions::load_inner(self.inner, self.limits, format)
free_functions::load_inner(self.inner, self.limits, format, LoadErrorHandling::Strict)
}
Copy link
Member

Choose a reason for hiding this comment

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

Preferrably you'd add fn decode_lossy for Reader instead of image::load_lossy. Use of the module functions is somewhat discouraged due to lack of limit handling and imprecise error reporting.

@fintelia
Copy link
Contributor

fintelia commented Jul 6, 2022

This PR assumes that ImageDecoder::read_image already implements the desired behavior of reading as much pixel data as possible and leaving any corrupt pixels in the output buffer untouched. However, this isn't actually specified anywhere. And AFAIK for many decoders this isn't actually true at all: they try to bail with an error as soon as they discover that any pixel data is corrupt, and some even use the output buffer as scratch memory while they do decoding!

I like the overall API/feature, but I wouldn't be comfortable merging without both (a) changing the spec for ImageDecoder to provide the needed guarantees, and (b) someone going through and auditing all our existing decoders to make sure they either already implement lossy decoding or updating them so they do.

@demurgos
Copy link
Author

demurgos commented Jul 6, 2022

Thank you for your quick feedback.
First of all, I'm happy that the feature is deemed interesting, but I agree that the PR needs more work.

  1. I'll rework the error type so it's possible to return more context when a partial error occurs (so we don't just discard the error). It may also be useful to return the count of decoded pixels (to differentiate between corrupted pixels ands really black/transparent pixels).
  2. I'll add a Reader method.
  3. I'll add tests and check the behavior of the various decoders (this may actually take some time 😅) .

Add support for lossy image loading.

The new `load_lossy` function adds error recovery.

If loading reaches the step where it allocates the pixel buffer, then it always succeeds: any error following this step will result in a partially filled buffer where the missing pixels will use their default "zero" value.

This allows for example to read truncated files or other minor issues.

- Closes image-rs#1752
@HeroicKatora
Copy link
Member

And AFAIK for many decoders this isn't actually true at all: they try to bail with an error as soon as they discover that any pixel data is corrupt, and some even use the output buffer as scratch memory while they do decoding!

I believe perfect is the enemy of good, in this case, as long as the error interface is available (i.e. it's possible for the caller to know that only partial information is available). The corruptness of the image is part of the intent afterall ;) It may be interesting if the decoder could indicate a region of the image as being correct regardless but by no means would I personally see this as a requirement. (It can be added as a defaulted extension method of ImageDecoder at a later date anyways).

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.

Lossy image loading
3 participants