Bug 28073 - Cross-platform ICO decoder gets wrong bit depth for icons w/o color count
Summary: Cross-platform ICO decoder gets wrong bit depth for icons w/o color count
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Images (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC All
: P2 Normal
Assignee: Peter Kasting
URL: http://games.tiscali.cz/favicon.ico
Keywords:
Depends on:
Blocks:
 
Reported: 2009-08-07 10:45 PDT by Peter Kasting
Modified: 2009-08-11 15:44 PDT (History)
1 user (show)

See Also:


Attachments
patch (1.69 KB, patch)
2009-08-07 10:51 PDT, Peter Kasting
eric: review-
Details | Formatted Diff | Diff
patch w/test, no expected results (6.84 KB, patch)
2009-08-07 16:43 PDT, Peter Kasting
eric: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Peter Kasting 2009-08-07 10:45:38 PDT
The ICO decoder ranks icons by quality, which is calculated first by size, then by bit depth.  To find the bit depth to use, we read the directory entry's wBitCount and bColorCount entries.  If the bit count is nonzero, it is used; otherwise, the bit depth is calculated from the color count.

Apparently, real-world icons exist which use wBitCount == 0, bColorCount == 0 to mean "256 colors".  The URL above is one such example; imdb.com/favicon.ico is another.  I already knew 0 meant "256" for bWidth and bHeight, but I didn't know that for bColorCount.  http://msdn.microsoft.com/en-us/library/ms997538.aspx implies this is legitimate (although it's not clear).

This is a simple one-liner.
Comment 1 Peter Kasting 2009-08-07 10:51:00 PDT
Created attachment 34294 [details]
patch

OK, not quite one line, but pretty simple.
Comment 2 Eric Seidel (no email) 2009-08-07 10:55:26 PDT
Comment on attachment 34294 [details]
patch

You and your wrapping!

Test case?

Our style is a bit lame, but this is technically stylistically wrong:
+        if (colorCount == 0)

r- for lack of test.
Comment 3 Peter Kasting 2009-08-07 16:43:45 PDT
Created attachment 34342 [details]
patch w/test, no expected results

Here's the patch with the style violation fixed and a layout test added.  I don't seem to be able to generate expected pixel results on my Windows box.  Expected results should be identical for all platforms, though.  Not sure if there's some way to have the buildbot do this for me or something?
Comment 4 Eric Seidel (no email) 2009-08-07 21:45:55 PDT
Comment on attachment 34342 [details]
patch w/test, no expected results

Fantastic!  Will need pixel test results but those can be added in separate commits.
Comment 5 Peter Kasting 2009-08-11 12:00:39 PDT
Fixed in r47042.
Comment 6 Eric Seidel (no email) 2009-08-11 15:44:17 PDT
Adding results:
Committing to http://svn.webkit.org/repository/webkit/trunk ...
	M	LayoutTests/ChangeLog
	A	LayoutTests/fast/images/icon-0colors-expected.checksum
	A	LayoutTests/fast/images/icon-0colors-expected.png
	A	LayoutTests/fast/images/icon-0colors-expected.txt
Committed r47070