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

Purpose of DecoderError::PngNotRgba check #1590

Open
filips123 opened this issue Oct 28, 2021 · 1 comment
Open

Purpose of DecoderError::PngNotRgba check #1590

filips123 opened this issue Oct 28, 2021 · 1 comment

Comments

@filips123
Copy link

Decoder for ISO files contains a check that all "embedded PNG images can only be of the 32BPP RGBA format":

// Embedded PNG images can only be of the 32BPP RGBA format.
// https://blogs.msdn.microsoft.com/oldnewthing/20101022-00/?p=12473/
if decoder.color_type() != ColorType::Rgba8 {
    return Err(DecoderError::PngNotRgba.into());
}

If I understand that article from Microsoft from 2010, its purpose is (was?) to prevent various programs from crashing too badly when trying to parse some ICO files with some different formats.

The problem is that I'm trying to parse and re-encode this ICO file, but it fails because of this exact check. It seems this happens because that ICO uses Rgb8 format instead of Rgba8. However, when I completely removed that check, everything was fine and ICO (both original and re-encoded) is correctly displayed on Windows and in programs.

Is that check still needed or can be completely removed? Or maybe it just needs another condition for Rgb8 format?

@HeroicKatora
Copy link
Member

Is that check still needed or can be completely removed? Or maybe it just needs another condition for Rgb8 format?

We need to do a check to find out if we are one of those programs crashing badly but I don't see any technical obstacle to support this as well. And also find out if the mask is supposed to be applied in this case, or to be ignored. Although I suppose this could be worked out incrementally, not supporting this file kind at all is worse to the end-user than ignoring its alpha mask (and a new issue can be opened if this is required).

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

No branches or pull requests

2 participants