Invalid PNG metadata when ICONDIRENTRY has a bitCount of zero

[rosasurfer]

Thank you for this little libray. I face an issue with a specific icon which has mixed BMP and PNG images. In this case it's a 256x256 PNG contained in ResourceHacker's main executable: http://angusj.com/resourcehacker/

If an icon contains a PNG image and in ICONDIRENTRY the PNG's bitCount is set to zero the value in IconImage is not updated to the PNG's real bit depth.

How to reproduce:

$service = new IcoFileService();
$icon = $service->from(file_get_contents('http://rosasurfer.com/.intern/mixed.ico'));
/** @var IconImage $image */
foreach ($icon as $image) {
    echo $image->getDescription().PHP_EOL;
}

Expected result:

256x256 pixel PNG @ 32 bits/pixel
64x64 pixel BMP @ 32 bits/pixel
48x48 pixel BMP @ 32 bits/pixel
40x40 pixel BMP @ 32 bits/pixel
32x32 pixel BMP @ 32 bits/pixel
24x24 pixel BMP @ 32 bits/pixel
20x20 pixel BMP @ 32 bits/pixel
16x16 pixel BMP @ 32 bits/pixel

Actual result:

256x256 pixel PNG @ 0 bits/pixel
64x64 pixel BMP @ 32 bits/pixel
48x48 pixel BMP @ 32 bits/pixel
40x40 pixel BMP @ 32 bits/pixel
32x32 pixel BMP @ 32 bits/pixel
24x24 pixel BMP @ 32 bits/pixel
20x20 pixel BMP @ 32 bits/pixel
16x16 pixel BMP @ 32 bits/pixel

I worked around it by adding the following snippet to IconParser::parseIconDirEntries() but I don't know the header format good enough and suspect I break cases where a bitCount of zero has a different valid meaning.

if ($icoDirEntry['bitCount'] == 0) {
    $icoDirEntry['bitCount'] = 32;
}

Wouldn't it be better to parse the actual PNG header instead of using possibly conflicting ICONDIRENTRY values? I use this little library which only needs to read the first 29 bytes of a PNG: https://github.com/ktomk/Miscellaneous/tree/master/get_png_imageinfo

Another note: All internal parser methods are private and there is no way to re-use your class. Can you consider making them protected?

Thank you again.

[lordelph]

Thanks for the detailed report - much appreciated. It looks like what should happen is that if the bit depth is 0, the image data is inspected to determine the bit depth. I'll come up with a fix for that.

Re the private methods, what were you hoping to override?

[rosasurfer]

Re the private methods: As I didn't want to modify your code I intended to fix the issue by passing an extended version of your parser into the IcoFileService constructor, until I realized that I can't and that I have to duplicate and rewrite your class. In my experience it's best OOP practice to make any non-public api protected instead of private, especially in open source. You never know what strange ideas other people come up with. :-)