Closed Bug 1324028 Opened 8 years ago Closed 7 years ago

Consider using edge colour as Favicon background if entire edge is one colour

Categories

(Firefox for Android Graveyard :: Theme and Visual Design, defect, P1)

Product:
Firefox for Android Graveyard
Component:
Theme and Visual Design
Type:
defect

Tracking

(firefox53 affected, firefox57 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 57
Tracking Flags:
Tracking Status
firefox53 --- affected
firefox57 --- fixed

People

(Reporter: ahunt, Assigned: sebastian)

Details

(Whiteboard: [MobileAS])

Attachments

(4 files, 3 obsolete files)

Bug 1324028 - Use edge colour as dominant favicon color if consistent.
59 bytes, text/x-review-board-request
mcomella
: review+
Details
Bug 1324028 - IconGenerator: Do not add alpha channel to colors of generated icons.
59 bytes, text/x-review-board-request
mcomella
: review+
Details
updated_topsites.png
741.50 KB, image/png
Details
updated_highlights.png
358.20 KB, image/png
Details 
Some favicons have a solid background, in these cases using the background colour as the "dominant" colour could be nice.

I.e. if the icon has a red background, we should consider painting a red surface behind the favicon when displaying it in a FaviconView.

I've attached screenshots of before/after when using an experimental patch that simply extracts the colour of the pixels along the edge, and sets that as the background colour if all pixels are that colour.

I don't know if we'd actually want to do this in practice - there are likely to be performance impacts caused by iterating over favicon contents, and I haven't tested a wide variety of favicons yet.

(Maybe we could add some metrics here, and do some A/B testing to evaluate how colour extraction performance would be affected by using this code?)
I remember trying something like that before and it didn't look well. But I didn't look at all the pixels on the edges; with that the result might be better.

(In reply to Andrzej Hunt :ahunt from comment #0)
> I don't know if we'd actually want to do this in practice - there are likely
> to be performance impacts caused by iterating over favicon contents, and I
> haven't tested a wide variety of favicons yet.

BitmapUtils.getDominantColor() has been iterating over *all* pixels of the icon, so the performance should be okay. The palette class from the support library is even faster and it would be interesting to see what they do. Renderscript would be an option too, but with our build system this is could be painful to use.
This isn't directly related - but I'm wondering if we can use this approach to help with:
https://trello.com/c/RU5EDMJP/78-i-want-to-be-able-to-differentiate-popular-google-properties-in-top-sites

If _all_ the edge pixels are transparent, then we could start adjusting the background colour based on the first letter of the domain (i.e. what I think we do for generated favicons). I'm not sure this would work for all icons though, so testing would be required.

(The alternative is just to hardcode specific background colours for some subdomains - that requires a bit more maintenance, but might be worth it for certain dominant sites.)
Attachment #8819333 - Attachment is obsolete: true
This looks pretty good in combination with the last activity stream UI update. I would like to revive this patch.
Assignee: nobody → s.kaspari
Status: NEW → ASSIGNED
Priority: -- → P2
Whiteboard: [MobileAS]
Attachment #8819334 - Attachment is obsolete: true
Attachment #8819329 - Attachment is obsolete: true
Sebastian, re comment 11, this doesn't look like this works for duckduckgo – any ideas?
Flags: needinfo?(s.kaspari)
I downloaded the Duckduckgo logo and opened it in GIMP. It's indeed not using the same color on the edges (e.g. #da5823 vs.#d95621 when looking at the corners [top/left, bottom/right]). I'm not sure if this is intentional (soft gradient?) or maybe a compression artifact. We could maybe try to define a threshold and look for "almost" the same color. But that's maybe something for a follow-up bug to investigate.
Flags: needinfo?(s.kaspari)
In top sites it's not a problem because they are perfect squares and the Duckduckgo icon is large enough to fill it completely. In highlights we might start to load the og:image soon and then the icon is only a fallback (Not sure whether the og:image for DuckDuckGo is better here though).
Comment on attachment 8894949 [details]
Bug 1324028 - Use edge colour as dominant favicon color if consistent.

https://reviewboard.mozilla.org/r/166068/#review171494

::: mobile/android/base/java/org/mozilla/gecko/icons/processing/ColorProcessor.java:95
(Diff revision 1)
> +            return null;
> +        }
> +
> +        // Bottom:
> +        bitmap.getPixels(edge, 0, width, 0, height - 1, width, 1);
> +        if (!edgecolor.equals(getEdgeColor(edge, width))) {

Should we use a delta, rather than directly equal, for images that are compressed and may be slightly off?

::: mobile/android/base/java/org/mozilla/gecko/icons/processing/ColorProcessor.java:115
(Diff revision 1)
> +
> +        return edgecolor;
> +    }
> +
> +    /**
> +     * Obtain the colour for a given edge.

nit: if the colors are all the same.

::: mobile/android/base/java/org/mozilla/gecko/icons/processing/ColorProcessor.java:120
(Diff revision 1)
> +     * Obtain the colour for a given edge.
> +     *
> +     * @param edge An array containing the color values of the pixels constituting the edge of a bitmap.
> +     * @param length The length of the array to be traversed. Must be smaller than, or equal to
> +     * the total length of the array.
> +     * @return The colour contained within the array, or -1 if colours vary.

nit: -1 -> null

::: mobile/android/base/java/org/mozilla/gecko/icons/processing/ColorProcessor.java:122
(Diff revision 1)
> +     * @param edge An array containing the color values of the pixels constituting the edge of a bitmap.
> +     * @param length The length of the array to be traversed. Must be smaller than, or equal to
> +     * the total length of the array.
> +     * @return The colour contained within the array, or -1 if colours vary.
> +     */
> +    private @ColorInt Integer getEdgeColor(@ColorInt int[] edge, int length) {

micro-optimization: maybe we should return an `int` (as opposed to `Integer`) to prevent auto-boxing (since this will run 4n where n is the number of favicons, which is usually ~14ish so 48 unnecessary allocations (if the return value is non-null and we ignore that the system can share instances)). Seems mostly trivial but considering all the other work we're doing in top sites, seems better to avoid it.

-1 won't work because `0xffffffff == -1` but I assume `Integer.MAX_VALUE` will work because ColorInts won't use the full range of the data type (you should verify this).

::: mobile/android/base/java/org/mozilla/gecko/icons/processing/ColorProcessor.java:122
(Diff revision 1)
> +     * @param edge An array containing the color values of the pixels constituting the edge of a bitmap.
> +     * @param length The length of the array to be traversed. Must be smaller than, or equal to
> +     * the total length of the array.
> +     * @return The colour contained within the array, or -1 if colours vary.
> +     */
> +    private @ColorInt Integer getEdgeColor(@ColorInt int[] edge, int length) {

nit: getEdgeColorFromSingleDimension?

Just to differentiate from the other `getEdgeColor`

::: mobile/android/base/java/org/mozilla/gecko/icons/processing/ColorProcessor.java:125
(Diff revision 1)
> +     * @return The colour contained within the array, or -1 if colours vary.
> +     */
> +    private @ColorInt Integer getEdgeColor(@ColorInt int[] edge, int length) {
> +        @ColorInt int color = edge[0];
> +
> +        for (int i = 1; i < length; i++) {

super-nit: ++i is marginally faster because it doesn't have to store the initial value of i. It's useful for graphics stuff where there are many iterations. :)
Attachment #8894949 - Flags: review?(michael.l.comella) → review+
Comment on attachment 8894954 [details]
Bug 1324028 - IconGenerator: Do not add alpha channel to colors of generated icons.

https://reviewboard.mozilla.org/r/166072/#review171950

This logically makes sense to me (why would you want to add transparency in the edge of the image) but I'm not sure why it makes sense in the context of these patches (i.e. is it necessary?).

Can you elaborate on ^ in the commit summary?

r+ because it makes logical sense and I don't want to block you.
Attachment #8894954 - Flags: review?(michael.l.comella) → review+
Iteration: --- → 1.28
Priority: P2 → P1
Comment on attachment 8894949 [details]
Bug 1324028 - Use edge colour as dominant favicon color if consistent.

https://reviewboard.mozilla.org/r/166068/#review178620

::: mobile/android/base/java/org/mozilla/gecko/icons/processing/ColorProcessor.java:95
(Diff revision 1)
> +            return null;
> +        }
> +
> +        // Bottom:
> +        bitmap.getPixels(edge, 0, width, 0, height - 1, width, 1);
> +        if (!edgecolor.equals(getEdgeColor(edge, width))) {

I would like to do this in a follow-up. I'm not sure what delta would be good and after seeing things like ColorUtil.grayscaleFromRGB() I'm not even sure a static value works for all colors.

::: mobile/android/base/java/org/mozilla/gecko/icons/processing/ColorProcessor.java:122
(Diff revision 1)
> +     * @param edge An array containing the color values of the pixels constituting the edge of a bitmap.
> +     * @param length The length of the array to be traversed. Must be smaller than, or equal to
> +     * the total length of the array.
> +     * @return The colour contained within the array, or -1 if colours vary.
> +     */
> +    private @ColorInt Integer getEdgeColor(@ColorInt int[] edge, int length) {

The ARGB values should use the full 32 bit of int (0x00000000 - 0xFFFFFFFF).
Pushed by s.kaspari@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/99e579176288
Use edge colour as dominant favicon color if consistent. r=mcomella
https://hg.mozilla.org/integration/autoland/rev/73d4ed6f999e
IconGenerator: Do not add alpha channel to colors of generated icons. r=mcomella
Backout by archaeopteryx@coole-files.de:
https://hg.mozilla.org/integration/autoland/rev/0f53d1e3f8b1
Backed out changeset 73d4ed6f999e 
https://hg.mozilla.org/integration/autoland/rev/11d7fb143eaf
Backed out changeset 99e579176288 for failing 'test' suite and on request from sebastian. r=backout
Pushed by s.kaspari@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/3ed13281b548
Use edge colour as dominant favicon color if consistent. r=mcomella
https://hg.mozilla.org/integration/autoland/rev/8be5e1248645
IconGenerator: Do not add alpha channel to colors of generated icons. r=mcomella
Iteration: 1.28 → 1.29
Thank you! Re-landed with test case fixed. :)
Flags: needinfo?(s.kaspari)
FYI: spoke with bbell for https://bugzilla.mozilla.org/show_bug.cgi?id=1388396#c12 and he actually wants to remove the functionality where we show a favicon centered in a colored background, if it doesn't look to janky.
https://hg.mozilla.org/mozilla-central/rev/3ed13281b548
https://hg.mozilla.org/mozilla-central/rev/8be5e1248645
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox57: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
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

Creator:
Created:
Updated:
Size: