We are already trying to do this, but still failing in some cases. A few testcases that return a shade of gray instead of a color: http://www.cnn.com/favicon.ico http://boingboing.net/favicon.ico http://www.amazon.com/favicon.ico The first two are very red, so I wonder if we're doing something wrong with red hues. The third one is more about ignoring gray tones to return the dominant non-gray color. There's some JS code in toolkit that looks a lot more complicated than what we're currently doing, maybe we can steal some ideas: http://mxr.mozilla.org/mozilla-central/source/toolkit/components/places/ColorAnalyzer_worker.js See also bug 634139.

Bug 826780 also has some improvements we should take.

So the problem with the red favicons is actually a bug with red. If maxH is 0 (red), we never end up updating maxS and maxV to return the correct color. Looking through this code, I noticed that we don't actually do anything with sat or val, and we only keep track of maxS and maxV to put a color back together, so I decided to streamline the logic. We may want to actually compute S and V values for the dominant color in the future, but this just clarifies our current behavior. I also found that the logic to skip black and white values was slightly off. We should be using an || instead of an &&. Fixing this returns yellow for the amazon.com logo.

Oops, most recent changes didn't make it in here.

Comment on attachment 743869 [details] [diff] [review] Clean up getDominantColor logic Review of attachment 743869 [details] [diff] [review]: ----------------------------------------------------------------- This makes me nervous (but I know the old algorythm was prone to problems). Can you test a few webapps and see how we do (this should fix some bugs there too!)

This patch does what the patch in bug 826780 was trying to do, keeping track of the average hue/saturation/value per bin, then using that to compute the dominant color that we return. This patch also still fixes the bugs I mentioned in previous comments.

Comment on attachment 744306 [details] [diff] [review] Even more improved patch Review of attachment 744306 [details] [diff] [review]: ----------------------------------------------------------------- Nice fixes.