Error Tolerance?

[nathaniel-daniel]

Hello, I have a question about the error tolerance goals of this library. I have a png with a corrupt icc profile that this library fails to load. However, zune-png loads the file and logs a warning via log instead, ignoring the icc section and pretending it does not exist. How much priority does this crate put on loading invalid files/recovering from errors? Is zune-png's behavior within the scope of this library?

[fintelia]

Some sorts of errors can be ignored via DecodeOptions. If the issue is a bad CRC, we should already be defaulting to ignoring section and pretending it isn't there. Though that behavior was added pretty recently, so do make sure you're using the latest version

[nathaniel-daniel]

Unfortunately, I'm fairly certain that the issue is with a corrupt/truncated deflate stream, I'll check the CRC when I get time, but from my testing the issue is still present on master, so I think the CRC is probably valid. The error I get is Format(FormatError { inner: CorruptFlateStream { err: InsufficientInput } }). I've tested with a few other decoders, like libpng, and most seem to all reject or ignore the icc profile as well. Strangely, PIL seems to load the profile just fine (via Image.open(path).info.get('icc_profile')), but I'm not sure if I am using it correctly; I'm fairly certain PIL uses libpng to load pngs, which I have confirmed rejects the iccp section via the C api.

Is an option in DecodeOptions for ignoring the profile if it is invalid something within this library's scope? If so, I'm willing to attempt a PR.

Also, I was wondering about the policy in general with regards to error tolerance. Is there one? Is it desirable to match the behavior of other decoders? Should output be given even on failure, like for truncated image files?

[fintelia]

There isn't a totally decided error tolerance policy at the moment. Generally what we've aimed for is matching defaults for libpng with options to be stricter or weaker. At the moment there isn't any way to indicate any issues without fully stopping the decoding process by triggering an error.

The InsufficientInput error indicates that the compressed stream within the ICC chunk was incomplete. It is possible that PIL just returns the bytes that could be decompressed and silently ignores the rest.

I think it would be reasonable to ignore the chunk if it cannot be decompressed, and it would be great to get a PR to the effect!

[nathaniel-daniel]

There isn't a totally decided error tolerance policy at the moment. Generally what we've aimed for is matching defaults for libpng with options to be stricter or weaker. At the moment there isn't any way to indicate any issues without fully stopping the decoding process by triggering an error.

I see. Are there any plans to add some form of warning reporting? It might be useful in cases where you need to be tolerant of errors, but also want to know if the image decompressed correctly. Otherwise, you would need to run decompression twice, with different DecoderOptions.

I think it would be reasonable to ignore the chunk if it cannot be decompressed, and it would be great to get a PR to the effect!

Great! I have a few questions regarding what that PR should look like. Should this new behavior be gated behind a new option in DecodeOptions, or simply done by default? Also, would changing the icc_profile field on the Info struct to be an Option<Result<_, _>> be acceptable? That seems like the cleanest solution wrt reporting issues with icc decoding and handling ignoring duplicate iccp chunks, but is also a breaking change. The alternative would be to keep that field as an option but add an extra flag somewhere to let the decoder know it already encountered an iccp chunk, to reject a second iccp chunk if the first is invalid.

[fintelia]

I see. Are there any plans to add some form of warning reporting? It might be useful in cases where you need to be tolerant of errors, but also want to know if the image decompressed correctly. Otherwise, you would need to run decompression twice, with different DecoderOptions.

No plans at the moment. I'm not opposed, but there's unresolved design questions and it would take a bunch of implementation work as well.