1254663 - Tiny thumbnail favicons on Top Sites/Bookmarks

Status: RESOLVED DUPLICATE of bug 1388396

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

Description

[Brian Nicholson (:bnicholson)]

Attachment: Top Sites screenshot

The thumbnails for Top Sites look pretty silly on my phone. Favicons are just tiny specks in big colored tiles.

In the screenshot, the purple favicon is for healthline.com, the orange is people.mozilla.org, and the red is google.com. Google is 32x32 (as opposed to 16x16, like the other two), so it looks slightly better, but still way too small IMO.

It looks like the favicons aren't being upscaled at all. Regression?

Comment 1

[:Margaret Leibovic]

I noticed that in bug 1254634, but maybe you're right that this could be a recent regression... 

ahunt, could this be related to your work?

Comment 2

[Andrzej Hunt :ahunt]

(In reply to :Margaret Leibovic from comment #1)
> I noticed that in bug 1254634, but maybe you're right that this could be a
> recent regression... 
> 
> ahunt, could this be related to your work?

This shouldn't be related to the topsites cursor work (that didn't touch anything favicon related), I did however touch our favicon code while working on the apple-touch-icon, and some of those patches didn't actually land until last week - it could be related to that? I'll try to look into this either on Friday, or next week.

Any idea when this started happening?

Comment 3

[Andrzej Hunt :ahunt]

Actually, I can reproduce this on release (which is before any of the touch-icon changes landed I think), so it seems to be unrelated to my patches. I'm only able to reproduce on an N7, or N6P, but not on an N4, i.e. devices with higher pixel densities.

Comment 4

[Andrzej Hunt :ahunt]

It turns out we don't upscale images in the TwoLinePageRow (and TabHistoryItemRow) - this worked well in the past, but isn't great for high density devices (e.g. Nexus 6P). 

My proposed solution is to enable upscaling, but limit it to a factor based on the screen density. I.e. we can scale assuming we want to preserve physical icon size for an icon produced for a 96dpi device - this means an N4 doesn't change in icon size, an N6P now gets an icon that's physically similar to the N4.

Comment 5

[Andrzej Hunt :ahunt]

(In reply to Andrzej Hunt :ahunt from comment #4)
> It turns out we don't upscale images in the TwoLinePageRow (and
> TabHistoryItemRow) - this worked well in the past, but isn't great for high
> density devices (e.g. Nexus 6P). 
> 
> My proposed solution is to enable upscaling, but limit it to a factor based
> on the screen density. I.e. we can scale assuming we want to preserve
> physical icon size for an icon produced for a 96dpi device - this means an
> N4 doesn't change in icon size, an N6P now gets an icon that's physically
> similar to the N4.

This comment is only relevant to bookmarks/topsites, top sites seems to be completely independent.

Comment 6

[Andrzej Hunt :ahunt]

Attachment: Bug 1254663 - Fall back to generated favicons if downloaded icon is too small

Review commit: https://reviewboard.mozilla.org/r/53998/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/53998/

Comment 7

[Andrzej Hunt :ahunt]

Attachment: MozReview Request: Bug 1254663 - Approach 1 Part 2: upscale TwoLinePageRow favicons to consistent physical size r?sebastian

Review commit: https://reviewboard.mozilla.org/r/54000/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/54000/

Comment 8

[Andrzej Hunt :ahunt]

Attachment: n4_doubleicons.png

(In reply to Andrzej Hunt :ahunt from comment #5)
> (In reply to Andrzej Hunt :ahunt from comment #4)
> > It turns out we don't upscale images in the TwoLinePageRow (and
> > TabHistoryItemRow) - this worked well in the past, but isn't great for high
> > density devices (e.g. Nexus 6P). 
> > 
> > My proposed solution is to enable upscaling, but limit it to a factor based
> > on the screen density. I.e. we can scale assuming we want to preserve
> > physical icon size for an icon produced for a 96dpi device - this means an
> > N4 doesn't change in icon size, an N6P now gets an icon that's physically
> > similar to the N4.
> 
> This comment is only relevant to bookmarks/topsites, top sites seems to be
> completely independent.

For topsites we just don't scale favicons at all. My proposed patch similarly preserves physical size. I'm wondering if we'd want to do any additional enlargement here, since even physical icons look small for some sites?

Here's a screenshot when we double the physical size (remove the /2 factor from my proposed patches in TopSitesGridItemView) - it looks a bit blurry, but the icons are (to me) a lot more recogniseable than at their default size. Note this only affects sites where we're using favicons, the tiles behave as before.

Comment 9

[Andrzej Hunt :ahunt]

Comment on attachment 8754491 [details]
Bug 1254663 - Fall back to generated favicons if downloaded icon is too small

Sebastian: you seem to be most familiar with our favicons, do you have any opinions on this approach?

Comment 10

[Sebastian Kaspari (:sebastian; :pocmo)]

Only quick drive-by: Doesn't the favicon loader do scaling? (Up to 2x I think).

Comment 11

[Andrzej Hunt :ahunt]

(In reply to Sebastian Kaspari (:sebastian) from comment #10)
> Only quick drive-by: Doesn't the favicon loader do scaling? (Up to 2x I
> think).

I can't seem to find this, we do however do downscaling when placing an item in the cache: we store favicons at max 32x32dp (which, IIUC, is scaled for higher density screens), and therefore downscale if necessary.

The options we have are probably:
- Upscale when storing icons in the cache (where we currently downscale). This would ensure that all icons are the same size in the cache, and therefore shown at the same size in topsites / etc. (This would result in slightly more memory usage, but most sites have larger favicons anyway).
OR
- upscale icons to e.g. a maximum of 2x their physical size (max 64dp since we cache at 32dp). This would result in inconsistent icon sizing for sites that supply icons smaller than 32x32dp (which happens mainly when you're on high density devices, e.g. the 6P).

I'm pretty sure we need to do the latter either way (we currently scale based on pixel and not physical sizes). The former might also be necessary. We'll have slightly blurry favicons either way, but I don't think there's anything we can really do about that issue.

(I can try and draw a diagram if that helps, I'm not sure my explanations are particularly clear here.)

Comment 12

[Andrzej Hunt :ahunt]

(In reply to Andrzej Hunt :ahunt from comment #11)
> I'm pretty sure we need to do the latter either way (we currently scale
> based on pixel and not physical sizes). The former might also be necessary.
> We'll have slightly blurry favicons either way, but I don't think there's
> anything we can really do about that issue.

Oops, thinko here. In option 1 the icons will already be upscaled to have a density-appropriate pixel size, we wouldn't need to do any more density-dependent manipulations after that.

Comment 13

[Andrzej Hunt :ahunt]

(In reply to Andrzej Hunt :ahunt from comment #11)
> - Upscale when storing icons in the cache (where we currently downscale).
> This would ensure that all icons are the same size in the cache, and
> therefore shown at the same size in topsites / etc. (This would result in
> slightly more memory usage, but most sites have larger favicons anyway).

A further advantage here is that favicons in topsites would now be the same size as the fallback generated letter icons. I think this is technically the simplest solution.

Comment 14

[Andrzej Hunt :ahunt]

Attachment: MozReview Request: Bug 1254663 - Approach 2: scale all favicons to the same size before caching r?sebastian

This is equivalent to the previous 2 commits: instead of scaling favicons later,
we can enlarge smaller icons here, resulting in all favicons being displayed at
the same size in the rest of the UI. When retrieving favicons we always push them
into the cache, and then fetch them from the cache to display them, hence this
works for now (but this is also liable to break if we ever change to only fetch
from the cache on later loads, and use the fetched icon directly on first load).

Review commit: https://reviewboard.mozilla.org/r/56970/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/56970/

Comment 15

[Andrzej Hunt :ahunt]

Comment on attachment 8754491 [details]
Bug 1254663 - Fall back to generated favicons if downloaded icon is too small

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53998/diff/1-2/

Comment 16

[Andrzej Hunt :ahunt]

Comment on attachment 8754492 [details]
MozReview Request: Bug 1254663 - Approach 1 Part 2: upscale TwoLinePageRow favicons to consistent physical size r?sebastian

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/54000/diff/1-2/

Comment 17

[Andrzej Hunt :ahunt]

I've pushed 2 separate approaches: the first one is more involved, but I think this is superior since it allows tuning every location where we might need to scale favicons. The second approach is very simple, but it results in the scaling being implicit on our FaviconCache, which seems quite brittle to me.

Comment 18

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8754494 [details]
n4_doubleicons.png

I am not sure if this is actually an improvement. Now we have big icons but they are very pixelated. Isn't that what the previous code tried to avoid but not scaling more than 2x? I'll flag antlam to get his opinion too.

While working on bug 1264901 I saw another issue with favicons being loaded too small - even though the original favicon is larger (I was downloading 144px favicons but they are loaded as 72px favicons). I'd like to debug this a bit before coming back to this bug here.

I also have a band-aid patch for bug 1265710 which will give us icons in higher quality. This should help here too hopefully.

Comment 19

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

(In reply to Sebastian Kaspari (:sebastian) from comment #18)
> Comment on attachment 8754494 [details]
> n4_doubleicons.png
> 
> I am not sure if this is actually an improvement. Now we have big icons but
> they are very pixelated. Isn't that what the previous code tried to avoid
> but not scaling more than 2x? I'll flag antlam to get his opinion too.

Agree, scaling up will always result in pixelated images and I'd like to avoid that.

> While working on bug 1264901 I saw another issue with favicons being loaded
> too small - even though the original favicon is larger (I was downloading
> 144px favicons but they are loaded as 72px favicons). I'd like to debug this
> a bit before coming back to this bug here.
> 
> I also have a band-aid patch for bug 1265710 which will give us icons in
> higher quality. This should help here too hopefully.

+1

Comment 20

[Andrzej Hunt :ahunt]

After some chat on IRC: I'm going to repurpose this bug into using the fallback icons if the site-provided favicon is too small. I'm not sure what exact criteria to use, I'm guessing something like if the favicon is < 50% the width of the container (maybe after 2x scaling?) would be appropriate. (Or maybe even if 2x scaling results in a favicon smaller than the cached size?)

(Note: this doesn't affect the use of apple-touch-icons, i.e. bug 1265710. I.e. we'll only use fallback icons if both icons are too small.)

Comment 21

[Sebastian Kaspari (:sebastian; :pocmo)]

I remember that I talked with antlam about that when working on the top sites rewrite. @antlam: Back then did we decide which icon size is too small to be used/displayed?

Comment 22

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

(In reply to Sebastian Kaspari (:sebastian) from comment #21)
> I remember that I talked with antlam about that when working on the top
> sites rewrite. @antlam: Back then did we decide which icon size is too small
> to be used/displayed?

Since our default favicons are 32dp squared, we can just use that as our criteria. 

Ahunt, can I see two screenshots of the following?

1) if the icon is smaller than the default favicons (32dp squared)

2) if the icon is smaller than 50% of the default favicon dimensions (16dp squared)

This would give some icons a chance to still be used if they aren't THAT much smaller but I'm not sure how it will work in practice. A screenshot should help :)

Thanks!

Comment 23

[Andrzej Hunt :ahunt]

(In reply to Anthony Lam (:antlam) from comment #22)
> (In reply to Sebastian Kaspari (:sebastian) from comment #21)
> > I remember that I talked with antlam about that when working on the top
> > sites rewrite. @antlam: Back then did we decide which icon size is too small
> > to be used/displayed?
> 
> Since our default favicons are 32dp squared, we can just use that as our
> criteria. 
> 
> Ahunt, can I see two screenshots of the following?
> 
> 1) if the icon is smaller than the default favicons (32dp squared)
> 
> 2) if the icon is smaller than 50% of the default favicon dimensions (16dp
> squared)
> 
> This would give some icons a chance to still be used if they aren't THAT
> much smaller but I'm not sure how it will work in practice. A screenshot
> should help :)
> 
> Thanks!

Here are a bunch of screenshots:
https://docs.google.com/document/d/1JRSiyOKh3p0fkwExbAbG3CJImmdS0uWvyYyuf2Z_XfE/edit?usp=sharing

Let me know if there are any other combinations worth testing!

Comment 24

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

I think removing the smaller icons altogether (and using our new defaults) works the best.

Blowing up a smaller icon isn't ideal, and doesn't look recognizable anyways.

Comment 25

[Andrzej Hunt :ahunt]

Comment on attachment 8754491 [details]
Bug 1254663 - Fall back to generated favicons if downloaded icon is too small

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53998/diff/2-3/

Comment 26

[Andrzej Hunt :ahunt]

The new favicon code makes this an easy thing to fix!

I'm wondering whether we'd want to at least extract the dominant colour from the too-small favicons, which would allow us to make better generated icons: that would be dependent on adding an override mechanism (which I investigated in Bug 1299619). Maybe we should have some telemetry here to check just how often generated favicons are used in real life?

Comment 27

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8754491 [details]
Bug 1254663 - Fall back to generated favicons if downloaded icon is too small

https://reviewboard.mozilla.org/r/53998/#review75934

I wonder if we should add a flag to the request for this (something like discardSmallIcons()). This way the caller can control this behavior. There could be some consumers (like the tab strip) that could live with smaller icons. What do you think? Just reflag me for review.

Comment 28

[Andrzej Hunt :ahunt]

Attachment: Bug 1254663 - Process too-small downloaded icons to avoid multiple downloads

Review commit: https://reviewboard.mozilla.org/r/79070/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/79070/

Comment 29

[Andrzej Hunt :ahunt]

Comment on attachment 8754491 [details]
Bug 1254663 - Fall back to generated favicons if downloaded icon is too small

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53998/diff/3-4/

Comment 30

[Andrzej Hunt :ahunt]

Here's an updated approach. I only noticed two places where we might want smaller icons, so I made ignoring smaller icons the default setting.

I'm not sure how big an icon the media-notification uses: the media notification seems to grab it's icon from Tab: we also use the same icon (that Tab has obtained) for the various site ID and login doorhangers / popups, where small icons are still useful. However the media-notification seems to do it's own handling of smaller icons (as opposed to using a generated icon), so maybe we don't need to handle that.

Comment 31

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8754491 [details]
Bug 1254663 - Fall back to generated favicons if downloaded icon is too small

https://reviewboard.mozilla.org/r/53998/#review79078

Can you add a unit tests for this too?

::: mobile/android/base/java/org/mozilla/gecko/icons/IconRequestBuilder.java:83
(Diff revision 4)
> +    public IconRequestBuilder permitSmallIcons(boolean permitSmallIcons) {
> +        request.ignoreSmallIcons = !permitSmallIcons;
> +        return this;
> +    }

Okay, this is a bit weird. Can we just use ignore*  or permit* everywhere? :)

My gut feeling is that by default the icon code should just load whatever, so I guess a method like ignoreSmallIcons() or skipSmallIcons() (To match the naming of the other methods) would be the most straight-forward approach.

::: mobile/android/base/java/org/mozilla/gecko/icons/IconTask.java:134
(Diff revision 4)
> +                    // Ignore icons that are less than 2/3 the requested size: we fallback
> +                    // to generated icons in this case. We want to do this here since the smaller
> +                    // icon may already be in memory, in which case we can skip directly to generated
> +                    // icons.
> +
> +                    // TODO: we should still store this smaller icon in memory / process it for those
> +                    // requests where it is useful?

I wonder if we could move this to the IconDownloader instead of doing this for all icons (This would allow us to add the URL to the failure cache too)?

Comment 32

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8791789 [details]
Bug 1254663 - Process too-small downloaded icons to avoid multiple downloads

https://reviewboard.mozilla.org/r/79070/#review79080

::: mobile/android/base/java/org/mozilla/gecko/icons/IconTask.java:140
(Diff revision 1)
> +
> +                        // Process the loaded icon to ensure that the icon is cached: the first load
> +                        // will download the icon: if we don't store the icon on disk / in-memory,
> +                        // then we will end up downloading the same (too-small) icon every time
> +                        // an icon is requested, unless we put it in local storage first.
> +                        // Our response will be discarded (and the generated response returned instead),
> +                        // hence we don't need to care about any changes any of the processors might make.
> +                        processIcon(request, response);

Mh, this kind of works but storing an icon we never use seems like a weird workaround.

Comment 33

[Andrzej Hunt :ahunt]

(In reply to Sebastian Kaspari (:sebastian) from comment #32)
> 
> Mh, this kind of works but storing an icon we never use seems like a weird
> workaround.

This lets us keep using the actual favicon for e.g. the tab-icon on tablets (while using , although I think that's probably the only place that can actually use these small icons.

(Looking at reviewboard, I think I might have uploaded some outdated patches, I'll need to recheck this...)

Comment 34

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

We fixed this in bug 1388396.