926646 - Can't compute dominant color for non-cached favicon

Status: RESOLVED FIXED

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

Description

[:Margaret Leibovic]

Regression from bug 914296? It looks like somehow the background is failing to be drawn.

Comment 1

[Chris Kitching [:ckitching]]

Urgh.

In the preferences screen, right?
Drat. We never did do Bug 913746. The cache stores colours keyed by favicon url - since the favicons in the preferences screen aren't in the cache it fails to find the resource to use once the colour cache misses.
A possible workaround is to prepopulate the colour cache with the appropriate values when the preferences screen is loaded - that'd make it work without needing to do Bug 913746, which, if I remember correctly, we approximately don't want to do.

Comment 2

[:Margaret Leibovic]

(In reply to Chris Kitching [:ckitching] from comment #1)

> In the preferences screen, right?

No, in the awesomescreen as well. But you're right that this does only seem to affect search engine icons, not regular favicons (it also affects my bugzilla search icon).

Comment 3

[:Margaret Leibovic]

I'll own sorting this out. It seems like this evolved into bug 939172.

Comment 4

[Richard Newman [:rnewman]]

I don't know if we've fully addressed the "can't compute dominant color for non-cached favicon" bug. I've seen that in the awesomescreen on fx-team.

Comment 5

[:Margaret Leibovic]

The white background issue was resolved by bug 933459, but now instead of showing a white background, we show nothing. I'm going to try to uplift that bug to aurora in order to resolve this white background issue.

This is an improvement, but let's morph this bug into figuring out how to get the dominant favicon color when we don't have a cached favicon.

Given that bug 946802 gives us a bigger Bing icon, none of the search engines we ship will suffer from this problem, so I don't think this is worth tracking for 27 anymore. However, this does still affect third-party search engines, so it would be good to fix at some point.

Comment 6

[Richard Newman [:rnewman]]

(In reply to :Margaret Leibovic from comment #5)

> This is an improvement, but let's morph this bug into figuring out how to
> get the dominant favicon color when we don't have a cached favicon.

An idea: do Bug 941868 but for our search engine favicons. They should come out of the cache, where they're handy-dandy Bitmaps of the correct size, rather than whatever we do now (base64 data URIs?).

Comment 7

[Chris Kitching [:ckitching]]

(In reply to Richard Newman [:rnewman] from comment #6)
> An idea: do Bug 941868 but for our search engine favicons. They should come
> out of the cache, where they're handy-dandy Bitmaps of the correct size,
> rather than whatever we do now (base64 data URIs?).

Please do that. Bonus points for enabling expiry of those icons with everything else (When that gets implemented) allowing them to be refetched from the websites when people change their logos (Like Amazon did recentlyish - the bundled icon doesn't match the one you get from the website).

Also, there's a bug (Approximately) for (A dirty hack related to) that, from long ago: Bug 913746.
I forget the details of the discussion which led to us deciding not to do that back then. Maybe things have changed?

Perhaps a neater solution would just be to have the bundled icons be present in the initial SQLite database. That way, when a request hits the favicon system it'll find them in the database and problem solved. (Presumably that's what you had in mind)

Comment 8

[Richard Newman [:rnewman]]

(In reply to Chris Kitching [:ckitching] from comment #7)

> Please do that. Bonus points for enabling expiry of those icons with
> everything else (When that gets implemented) allowing them to be refetched
> from the websites when people change their logos (Like Amazon did
> recentlyish - the bundled icon doesn't match the one you get from the
> website).

We don't want your first experience to have no favicons, so we have to tread carefully here.


> Perhaps a neater solution would just be to have the bundled icons be present
> in the initial SQLite database. That way, when a request hits the favicon
> system it'll find them in the database and problem solved. (Presumably
> that's what you had in mind)

We currently have a shitty multi-source way of doing these things:

* Some icons (e.g., about:home's) come from the JAR, in hard-coded sizes, one image per size.
* Some icons come from the distribution file.
* Some are provided by search engines.

I would like to see these (a) normalized, and (b) implemented in a way that can be efficiently deserialized from the disk. I don't think jamming them into the database is the right answer -- indeed, I don't think storing favicons in the DB is necessarily the right answer for normal websites, either.

Loading the favicons out of the JAR is quicker than hitting the DB by a huge amount (~1ms total, IIRC). That's why I didn't bother persisting them back into the DB -- why use a solution that's 50x slower?

Comment 9

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to Richard Newman [:rnewman] from comment #8)

> We currently have a shitty multi-source way of doing these things:

Not shitty, just the way it is. Data comes from different places.

> * Some icons (e.g., about:home's) come from the JAR, in hard-coded sizes,
> one image per size.
> * Some icons come from the distribution file.
> * Some are provided by search engines.

And these are all legit places for data

> I would like to see these (a) normalized, and (b) implemented in a way that
> can be efficiently deserialized from the disk. I don't think jamming them
> into the database is the right answer -- indeed, I don't think storing
> favicons in the DB is necessarily the right answer for normal websites,
> either.

OK

> Loading the favicons out of the JAR is quicker than hitting the DB by a huge
> amount (~1ms total, IIRC). That's why I didn't bother persisting them back
> into the DB -- why use a solution that's 50x slower?

And now you just convinced me that we don't need to do anything! The three non-shitty sources of data above are all in the JAR.

Comment 10

[Chris Kitching [:ckitching]]

(In reply to Mark Finkle (:mfinkle) from comment #9)
> (In reply to Richard Newman [:rnewman] from comment #8)
> 
> > We currently have a shitty multi-source way of doing these things:
> 
> Not shitty, just the way it is. Data comes from different places.

But why does the same data need to come from different places when it doesn't have to?

> > * Some icons (e.g., about:home's) come from the JAR, in hard-coded sizes,
> > one image per size.
> > * Some icons come from the distribution file.
> > * Some are provided by search engines.
> 
> And these are all legit places for data

They are. And you need three different systems for loading data to from them. In general there's no reason why things shouldn't live in all these places, but putting the *same* thing (favicons) in all three seems needlessly irritating.

> > Loading the favicons out of the JAR is quicker than hitting the DB by a huge
> > amount (~1ms total, IIRC). That's why I didn't bother persisting them back
> > into the DB -- why use a solution that's 50x slower?
> 
> And now you just convinced me that we don't need to do anything! The three
> non-shitty sources of data above are all in the JAR.

Unfortunately, it is only bundled favicons (and not all of them) which are loaded from these distinctly minty locations. Favicons for normal websites get stuck in the legitimately excrement-flavoured database. Richard is (correctly) complaining that my proposed solution would cause those icons that currently live outside the database to have to suffer its slowness.

Holy cow, I had no idea the database was *that* slow, though. Sounds like work needs to be done to find a better place to put the persistent favicon cache (jdbm2 or similar key-value store? Filesystem?). Once we have a sufficiently noninsane place to persistently store favicons we can do whatever we want, although it'd still likely be worth giving some extra-special treatment to bundled favicons so they can be loaded *uber* fast on startup. Stick them in one nice little readily-accessible uncompressed file we can just hoover up from disk or something.

In the meantime, *something* needs to happen to make this bug go away. Since this bug appears to have been retitled as an error I seem to remember writing (although perhaps not, since "color") I should perhaps provide some background as to why it exists.
BitmapUtils.getDominantColor is the handy function used for computing these colours, but it's quite expensive. The new favicon cache computes the value at most once for any favicon (Irrespective of how many sizes it might have or whatnot). The thinking was that, since every favicon in use will be in the favicon cache, it makes sense to store associated information such as the dominant colour in the cache as well. This both prevents redundantly recalculating this information and makes it go away exactly when we stop caring (When the associated favicon record is dropped from the cache). As a result, FaviconView decides which colour it needs for the background by querying the favicon cache directly using the cache key it was given last time it was updated (which is, in particular, the favicon URL of the icon it is displaying).
Unfortunately, since the icons for bundled search engines are loaded via a completely different mechanism, and come from a completely different place, the cache can do nothing but shrug and return nothing useful when queried for the dominant colour of a bitmap it does not have a means of accessing. This partitioning of the favicon storage is irritating.

One could imagine a nasty hack that would make this problem go away that consists of special-casing such favicons to call BitmapUtils.getDominantColor directly, perhaps implementing some caching of the result in FaviconView or something. That'd be decidedly ugly, but I guess it depends how rapidly you want something that works vs. something that both works and makes you not want to jump off the roof.

Since I seem once again to be demonstrating my tendancy to write small essays instead of bug comments, I'll go back to what I should be doing now. Hopefully this clarifies things somewhat. I never thought I'd legitimately find a way to spend >1 month of my time pondering how best to handle 32x32px icons.

Comment 11

[Richard Newman [:rnewman]]

(In reply to Mark Finkle (:mfinkle) from comment #9)

> Not shitty, just the way it is. Data comes from different places.

It's definitely shitty in that it makes our favicon handling more complex and less performant, and we're able to change that.


> And these are all legit places for data

Disagree.

The app icon should be a built resource; we shouldn't need to parse and extract data from a JAR in order to paint the homescreen.

And we should never have to base64-decode an icon at runtime when we know it in advance, which is what we have to do for searchplugins (and at first run for distribution bookmarks, followed by dumping them into the DB).

What we have right now is *convenient*. It's burning a user's CPU cycles to avoid effort at build time.

Imagine a build phase that bundled all of our branding icons and searchplugin icons into a single JAR entry, or a single .ico on disk. Tada! ~free cache population.

(And don't we wait for Gecko to tell us the icons for search engines?)


> > Loading the favicons out of the JAR is quicker than hitting the DB by a huge
> > amount (~1ms total, IIRC). That's why I didn't bother persisting them back
> > into the DB -- why use a solution that's 50x slower?
> 
> And now you just convinced me that we don't need to do anything! The three
> non-shitty sources of data above are all in the JAR.

No they're not. Distributions and searchplugin icons are base64-encoded inside XML files.

Even if they were, do we want to be dipping into the JAR more than once, sometimes in the critical path for displaying an icon?

My point is: we should precache favicons that we're likely to show in the UI, and we should represent those icons on disk in a way that makes loading and precaching cheap.

Comment 12

[Richard Newman [:rnewman]]

... but all of that's a different bug, which I'll file when I get a chance.

Comment 13

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to Chris Kitching [:ckitching] from comment #10)
> (In reply to Mark Finkle (:mfinkle) from comment #9)
> > (In reply to Richard Newman [:rnewman] from comment #8)
> > 
> > > We currently have a shitty multi-source way of doing these things:
> > 
> > Not shitty, just the way it is. Data comes from different places.
> 
> But why does the same data need to come from different places when it
> doesn't have to?

It's only the same data in the sense that it has an image associated with it. Search engines are stored per locale, built-in about: pages have favicons stored in the JAR (or omni.ja) and distribution bookmarks and search engines are stored in /system/ locations created by OEMs. It does need to originate in different places and we need to deal with it.

> > > * Some icons (e.g., about:home's) come from the JAR, in hard-coded sizes,
> > > one image per size.
> > > * Some icons come from the distribution file.
> > > * Some are provided by search engines.
> > 
> > And these are all legit places for data
> 
> They are. And you need three different systems for loading data to from
> them. In general there's no reason why things shouldn't live in all these
> places, but putting the *same* thing (favicons) in all three seems
> needlessly irritating.

And we need to deal with it.

Comment 14

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to Richard Newman [:rnewman] from comment #11)
> (In reply to Mark Finkle (:mfinkle) from comment #9)
> 
> > Not shitty, just the way it is. Data comes from different places.
> 
> It's definitely shitty in that it makes our favicon handling more complex
> and less performant, and we're able to change that.

To some degree. But again, it's the real world, and we need to deal with it. Shitty or not.

> > And these are all legit places for data
> 
> Disagree.
> 
> The app icon should be a built resource; we shouldn't need to parse and
> extract data from a JAR in order to paint the homescreen.

To me JAR == APK and even the resource: would live there. Of all the image data, this is the one that is easiest to "fix". Let's just do it.

> And we should never have to base64-decode an icon at runtime when we know it
> in advance, which is what we have to do for searchplugins (and at first run
> for distribution bookmarks, followed by dumping them into the DB).

Distributions will usually live in /system/org.mozilla.firefox/distribution and is placed there by the OEM, not Mozilla.

> Imagine a build phase that bundled all of our branding icons and
> searchplugin icons into a single JAR entry, or a single .ico on disk. Tada!
> ~free cache population.
> 
> (And don't we wait for Gecko to tell us the icons for search engines?)

You could do that for the ones we ship, with some effort, but we'd still need to deal tih base64 encoded search engine images from user installed search engines: open search or add-ons.

> My point is: we should precache favicons that we're likely to show in the
> UI, and we should represent those icons on disk in a way that makes loading
> and precaching cheap.

My point is: I agree, but we still need to deal with existing formats, like the open search and distribution formats, at non-build time. If we can extract images at build time and move them to a "nicer" location and format, then go ahead, but that is not 100% of the solution. We can't wish away the formats, we need to deal with them.

Comment 15

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to Richard Newman [:rnewman] from comment #11)

> Imagine a build phase that bundled all of our branding icons and
> searchplugin icons into a single JAR entry, or a single .ico on disk. Tada!
> ~free cache population.

Sounds like a good bug to file. Multi-locale builds will make this difficult, but they never make anything easy.

Comment 16

[Richard Newman [:rnewman]]

(In reply to Mark Finkle (:mfinkle) from comment #14)

> To some degree. But again, it's the real world, and we need to deal with it.
> Shitty or not.

Yeah, I'm proposing dealing with it (in a number of different possible ways), if we're finding that there's an opportunity for speed improvements over the current system.


> > And we should never have to base64-decode an icon at runtime when we know it
> > in advance, which is what we have to do for searchplugins (and at first run
> > for distribution bookmarks, followed by dumping them into the DB).
> 
> Distributions will usually live in /system/org.mozilla.firefox/distribution
> and is placed there by the OEM, not Mozilla.

Sure. And distribution users should be the only ones affected, if we can get away with preprocessing for everyone else.

Extra points if:

* We allow distros to provide binary icons, not base64 data URIs, or
* we cache them, because we only process distributions once.

A lot of this is malleable. We shouldn't be acting as if everything is set in stone.


> You could do that for the ones we ship, with some effort, but we'd still
> need to deal tih base64 encoded search engine images from user installed
> search engines: open search or add-ons.

... once. After all, it's not like some third party sneaks search engines in behind our backs -- we process them, so we can do whatever we want at that point.

Lots of bugs to file!

Comment 18

[Brad Lassey [:blassey] (use needinfo?)]

filter on [mass-p5]

Comment 19

[Richard Newman [:rnewman]]

This should be fixed when Bug 961600 lands.