Closed Bug 938165 Opened 11 years ago Closed 3 years ago

Home screen favicon display refinements

Categories

(Firefox for Android Graveyard :: Favicon Handling, defect)

26 Branch
All
Android
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED INCOMPLETE

People

(Reporter: ibarlow, Unassigned)

Details

Attachments

(2 files)

I'm not sure the best way to break this down into smaller pieces, but we have some work to do to improve how we display favicons on the home page. 

There seems to be a lot of inconsistency in how we are displaying them the moment. Our designs specify a few different display options:

-----

1. Display large, sharp favicons where possible.
2. If no large favicon is available, display a smaller (sharp) favicon inside a box made of the icon's dominant colour
3. If no favicon is available at all, show the default globe.

-----

The problem I'm seeing is that while these rules are followed to some extent, we are still ending up with several cases where the icons look very blurry and thus sub-optimal. I've attached some screenshots that represent the following cases:

* Large icons that are blurry -- including *all* of the Search Provider icons
* Small icons within frames are blurry
* Some small favicons don't even have frames drawn around them

Now, there are some instances where we show large, crisp favicons, the screenshot has some of those too -- this gives me hope! How can we get others looking this good, or at least less blurry?
This is essentially the meta-problem my earlier Favicon work aimed to solve. It's a problem of roughly three parts:

Where does the blurryness come from?

The policy used by the new favicon cache for upscaling favicons allows for an icon to be upscaled by at most a factor of 2 to satisfy a request. That is, if a page serves up a 32px favicon and our UI asks for a 64px one, the 32px one will be upscaled to satisfy this request. If the UI were to ask for a 128px favicon, the icon would be scaled to 64px and the border would be shown.
Perhaps we want to reconsider this scaling behaviour? Allow only downscaling? Allow only upscaling by a factor of 1.5? Something else?


Why do we need to upscale so many favicons?

Many sites are serving up large favicons (64px, 128px, even 512px in some places), but we're failing to use them correctly. This is the ICO Decoder bug - Bug 748100. That's essentially solved, just needs a bit of an extra kick to get it past RNewman :P.
In your image, the NYTimes icon being miniscule is an example of this I am certain of - the NYTimes icon is hit by the decoder bug and we only get the 16px version of it. Check out what happens when you load up NYTimes with the ICO Decoder patches applied:
https://www.dropbox.com/s/dd8aj12yektrah3/NvidiaFix.png?m

I am unfamiliar with many of the other pages you have here, but suspect that a large fraction of them are similarly hit by this bug.


Why are search engine favicons different?

The handling of search engine favicons is irritating. We do not fetch the favicon from the website and use it for the search suggestions screen or the search preferences screen. Instead, we fetch the base64 encoded blob from JS land to get the bundled icon.
In my opinion this is a bad idea, but apparently it's not something we're allowed to change. Unless this status changes, we're thus stuck with out of date icons. Also, since the bundled icons are lower resolution than the ones served by the actual pages, we can even end up with an annoying discrepancy between the icons seen for an Amazon page in the history tab vs. the Amazon icon seen in the search suggestions.
The blurryness of search engine favicons is the upscale-by-factor-of-two logic again. Maybe we do want to revisit that.
(In reply to Chris Kitching [:ckitching] from comment #1)
> This is essentially the meta-problem my earlier Favicon work aimed to solve.
> It's a problem of roughly three parts:
> 
> Where does the blurryness come from?
> 
> The policy used by the new favicon cache for upscaling favicons allows for
> an icon to be upscaled by at most a factor of 2 to satisfy a request. That
> is, if a page serves up a 32px favicon and our UI asks for a 64px one, the
> 32px one will be upscaled to satisfy this request. If the UI were to ask for
> a 128px favicon, the icon would be scaled to 64px and the border would be
> shown.
> Perhaps we want to reconsider this scaling behaviour? Allow only
> downscaling? Allow only upscaling by a factor of 1.5? Something else?

I think that we should avoid upscaling, the blurriness does feel like a bug, and I think it would still be noticable if we upscale by a factor of 1.5.

> Why do we need to upscale so many favicons?
> 
> Many sites are serving up large favicons (64px, 128px, even 512px in some
> places), but we're failing to use them correctly. This is the ICO Decoder
> bug - Bug 748100. That's essentially solved, just needs a bit of an extra
> kick to get it past RNewman :P.
> In your image, the NYTimes icon being miniscule is an example of this I am
> certain of - the NYTimes icon is hit by the decoder bug and we only get the
> 16px version of it. Check out what happens when you load up NYTimes with the
> ICO Decoder patches applied:
> https://www.dropbox.com/s/dd8aj12yektrah3/NvidiaFix.png?m
> 
> I am unfamiliar with many of the other pages you have here, but suspect that
> a large fraction of them are similarly hit by this bug.

What would it take to get this bug over the finish line? It sounds like it would be a nice win.

> Why are search engine favicons different?
> 
> The handling of search engine favicons is irritating. We do not fetch the
> favicon from the website and use it for the search suggestions screen or the
> search preferences screen. Instead, we fetch the base64 encoded blob from JS
> land to get the bundled icon.
> In my opinion this is a bad idea, but apparently it's not something we're
> allowed to change. Unless this status changes, we're thus stuck with out of
> date icons. Also, since the bundled icons are lower resolution than the ones
> served by the actual pages, we can even end up with an annoying discrepancy
> between the icons seen for an Amazon page in the history tab vs. the Amazon
> icon seen in the search suggestions.
> The blurryness of search engine favicons is the upscale-by-factor-of-two
> logic again. Maybe we do want to revisit that.

We can address this by including larger icons in the search XML. In fact, we can include multiple icons of different resolutions now that bug 900137 landed, although that would require some more changes on our end to actually make sure we pull out the correct icon.
Flags: needinfo?(chriskitching)
(In reply to :Margaret Leibovic from comment #2)
> I think that we should avoid upscaling, the blurriness does feel like a bug,
> and I think it would still be noticable if we upscale by a factor of 1.5.

Sounds sensible.

> What would it take to get this bug over the finish line? It sounds like it
> would be a nice win.

All that's needed is to address Richard's latest review comments, which are, essentially, "Stop being lazy and writing code in a hurry". Mostly refactoring-shaped things.
I am currently (And likely going to continue to be) extremely short on free time to address this. If you want to steal it then feel free. Otherwise, I'll try harder to find time to finish it up.

> We can address this by including larger icons in the search XML. In fact, we
> can include multiple icons of different resolutions now that bug 900137
> landed, although that would require some more changes on our end to actually
> make sure we pull out the correct icon.

Sounds like a good idea. Wouldn't it be way tidier to just make the ICO decoder bug land, then bundle an ICO with multiple resolutions in the XML, then pull that and hand it to the ICO decoder?
Flags: needinfo?(chriskitching)
Attached image favicon list (updated )
Now that bug 748100 landed, I went ahead and visited the sites in the original favicon issues list. See the screenshot for the result. Some favicons show huge improvements as expected (e.g. nytimes, lifehacker, dpreview) while others show the same issues.
Ah yes, I was meaning to do this myself at some point. Thanks for doing that!

feeds.kottke.org provides no link tag specifying a favicon, so we fallback to favicon.ico, which is a single-resource ICO containing just one 16x16px icon. Since that's the one class of Favicon the skia decoder manages to decode anyway, that was one that worked fine before.
Unfortunately, because the source is a low resolution image, this is the best we can ever get for this site.

Nico-foto.com is really, really strange. They provide one explicit favicon that's an ICO which contains *twelve* 16x16px favions. Unfortunately, as for kottke.org, since the largest available icon is 16x16px, we're stuck.

Yewknee is similar - they provide a png as their favicon, and it's 16x16px. Nothing we can do.

Finally, alwaysriding.co.uk is identical to kottke.org - nothing explicit, and favicon.ico has just one 16x16px favicon in it.


Drat. I was hoping one of these would demonstrate one of the remaining favicon bugs. Everything in your picture is being decoded optimally. Unfortunately, "optimal" happens to be 16x16px. Which is rubbish.

More generally, some sites will not be treated optimally.
If a website provides multiple link tags specifying multiple icons, Fennec will attempt to decode the first one in the page only, and ignore all others. If that one happens to be an SVG (Or, heaven forbid, an ICNS) which we can't decode, we'll just fail and show no icon at all. (Bug 914027)
Worse, if the page provides multiple link tags providing multiple icons with different size attributes, we'll still only check out the very first one - which might not be the best choice of size. (Bug 914952)
Of particular note for that one is how we *should* behave. Hoover up all of them which are smaller than LARGEST_INTERESTING_SIZE and shove them in the cache/db for later use at our whims? Just nab the biggest?

Feel free to steal any of these bugs from me. Bug 914027 in particular has a mostly-finished patch (It worked for me at the time I uploaded it, but will have bitrotted impressively since then.). 

Hopefully that clarifies the current status of favicons in Fennec.
Oh, one thing I forgot to mention...

nico-foto.com. Why doesn't that have a dominant colour background around it? Or does it, but it's showing up as white? (Which seems to show a fault in the average-colour algorithm - although this is perhaps okay for performance reasons).
(In reply to Chris Kitching [:ckitching] from comment #6)
> Or does it, but it's showing up as white? (Which seems to show a fault in
> the average-colour algorithm - although this is perhaps okay for performance
> reasons).

As far as I know, the dominant color code implementation excludes white, meaning it would pick the second most dominant color if the first were white. I'm not 100% sure though, but Margarete should know, I think. Either way something wrong is happening with the nico-foto.com favicon as it's also not upscaled like the other 16x16 favicons. 

Regarding Yewknee: Why does it seem to be so much more blurry than the other 16x16 favicons (e.g. alwaysriding)?
(In reply to Peter Retzer (:pretzer) from comment #7)
> (In reply to Chris Kitching [:ckitching] from comment #6)
> > Or does it, but it's showing up as white? (Which seems to show a fault in
> > the average-colour algorithm - although this is perhaps okay for performance
> > reasons).
> 
> As far as I know, the dominant color code implementation excludes white,
> meaning it would pick the second most dominant color if the first were
> white. I'm not 100% sure though, but Margarete should know, I think.

Correct, we ignore black and white pixels when computing the dominant color:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/gfx/BitmapUtils.java#260
(In reply to :Margaret Leibovic from comment #8)
> (In reply to Peter Retzer (:pretzer) from comment #7)
> > (In reply to Chris Kitching [:ckitching] from comment #6)
> > > Or does it, but it's showing up as white? (Which seems to show a fault in
> > > the average-colour algorithm - although this is perhaps okay for performance
> > > reasons).
> > 
> > As far as I know, the dominant color code implementation excludes white,
> > meaning it would pick the second most dominant color if the first were
> > white. I'm not 100% sure though, but Margarete should know, I think.
> 
> Correct, we ignore black and white pixels when computing the dominant color:
> http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/gfx/
> BitmapUtils.java#260

But if there are NO colors, we fall back to white. We could pick a better fallback. Or even improve dominant color to pick a darker lighter shade based on whether the icon was mostly white or mostly black...
OS: Mac OS X → Android
Hardware: x86 → All
Component: Theme and Visual Design → Favicon Handling
We have completed our launch of our new Firefox on Android. The development of the new versions use GitHub for issue tracking. If the bug report still reproduces in a current version of [Firefox on Android nightly](https://play.google.com/store/apps/details?id=org.mozilla.fenix) an issue can be reported at the [Fenix GitHub project](https://github.com/mozilla-mobile/fenix/). If you want to discuss your report please use [Mozilla's chat](https://wiki.mozilla.org/Matrix#Connect_to_Matrix) server https://chat.mozilla.org and join the [#fenix](https://chat.mozilla.org/#/room/#fenix:mozilla.org) channel.
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → INCOMPLETE
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: