#237 ICO plugin: PNG-format pages are incorrectly detected

[Tanner Helland]

Hello FreeImage team,

At present, FreeImage uses an incorrect mechanism for detecting the presence of PNG-format pages inside ICO files. The current code is:

if((icon_list[page].bWidth == 0) && (icon_list[page].bHeight == 0)) {
// Vista icon support
}
else {
// standard icon support
}

There are two problems with this approach:

1) A width/height of 0 doesn't necessarily indicate a PNG stream. It can also indicate a standard bitmap page with width/height 256. See this link.

2) Some icon authoring software places actual width/height values in the ICONDIRENTRY for PNG pages, causing FreeImage to incorrectly assume the pages are in bitmap format.

A better way to check for PNG pages is to actually look for the PNG signature at dwImageOffset. If the PNG signature is found (instead of the first 8 bytes of a BITMAPINFOHEADER), load the stream as a PNG; otherwise, load it as an old-style Windows icon.

Attached is a modified PluginICO.cpp file with this change. My modifications are prefaced by

//CHANGE:

I've also attached a helpful test icon. It contains two 256x256 icons: one in standard ICO format, and one in PNG format. To facilitate testing, the two pages contain different images. (The ICO page is a palette+paintbrush, while the PNG format is a folder+trophy).

Anyway, this simple fix should improve the compatibility of the ICO plugin with many ICO files.

[Hervé Drolon]

Hi,

thanks for the patch, I've added it to the CVS with some modifications so as to be compatible with big endian systems.
About the width/height at 0, your linked blog says that it concerns the ICONDIRENTRY structure. Did you encountered ICO files using a BITMAPINFOHEADER structure with width/height at 0 ?

Hervé

[Tanner Helland]

Hi Hervé. Thank you for your quick response, and sorry for not thinking about big endian systems in my original patch. (It was pretty "quick and dirty", so I'm happy you cleaned it up.)

I have never encountered ICO files with width/height 0 in the BITMAPINFOHEADER. Since that struct has 4 bytes each for width/height, I would assume there was never any need to use 0 to indicate 256. (I say this, but icon files have surprised me before, so who can be certain??)

But in my experience, I have never seen 0 used for width/height in the BITMAPINFOHEADER. I would consider that a faulty icon.

[Hervé Drolon]

OK, so I will remove the width/height patch regarding the LoadStandardIcon function.

[Tanner Helland]

Good idea, and sorry for going overboard with that. Just to be safe, I ran a quick check on a bunch of old-style Windows icons that used 0/0 in ICONDIRENTRY. In each case, actual dimensions (e.g. 256/256) were used in BITMAPINFOHEADER. So I think it is safe to remove the LoadStandardIcon "if (width == 0)" check.

Thanks again for your quick work!

[Hervé Drolon]

fixed in release 3.17.0