Corrupt auxillary chunks should be ignored, not treated as fatal errors

[fintelia]

The PNG spec says:

Unexpected values in fields of known chunks (for example, an unexpected compression method in the IHDR chunk) shall be checked for and treated as errors. However, it is recommended that unexpected field values be treated as fatal errors only in critical chunks. An unexpected value in an ancillary chunk can be handled by ignoring the whole chunk as though it were an unknown chunk type.

Right now, parse_chunk instead treats any error in any known chunk as fatal and sets the decoding state to None.

[anforowicz]

Good catch - thanks for filing the bug.

A few thoughts that I hope are helpful:

• I wonder if the PNG spec's notion of "auxiliary" can be directly applied, because acTL and fcTL chunks are kind of critical to APNG-aware decoders. OTOH, maybe an error parsing these chunks can be indeed treated as-if the chunk was missing.
• One way to fix this would be to tweak fn parse_chunk so that it turns Err(_) into Ok(Decoded::Nothing) for auxiliary chunks. The main risk is whether a fn parse_somechunk leaves the decoded in a bad state when returning an error (i.e. we'd have to "be careful" going forward that fn parse_fctl doesn't do that - we'd have to move self.inflated.reset() and self.ready_for_fdat_chunks = true toward the end of the function - next to self.info.as_mut().unwrap().frame_control = Some(fc)).
• I've been recently reading https://joshlf.com/files/talks/Safety%20in%20an%20Unsafe%20World.pdf and in theory we could make reporting an error for an auxiliary chunk a compilation error. One idea would be to make each chunk a separate type/struct and only allow constructing errors where TChunkType: CriticalChunkTrait or something like that. This seems like quite a big and complicated refactoring - I am not sure if this is worth it...

[kornelski]

APNG chunks started out as a non-standard hack, and it was long too late to fix their names when they got into the spec.

APNG chunks will need special case handling. They could simply be treated as mandatory, or mandatory after decoding past the first frame (iEND).

[fintelia]

For reference, this is how the spec recommends handling APNG errors:

APNG is designed to allow incremental display of frames before the entire datastream has been read. This implies that some errors may not be detected until partway through the animation. It is strongly recommended that when any error is encountered decoders should discard all subsequent frames, stop the animation, and revert to displaying the static image. A decoder which detects an error before the animation has started should display the static image. An error message may be displayed to the user if appropriate.

But really, the top priority should be making sure that adding support for new metadata chunks doesn't cause breaking changes for users due to previously readable PNGs now raising errors about the new chunk being corrupt.