Don't automatically scale icons in LoadFaviconTask

RESOLVED FIXED in Firefox 25

Status

()

Firefox for Android
General
RESOLVED FIXED
5 years ago
2 years ago

People

(Reporter: sole, Assigned: wesj)

Tracking

Trunk
Firefox 25
ARM
Android
Points:
---

Firefox Tracking Flags

(fennec24+)

Details

Attachments

(2 attachments, 1 obsolete attachment)

I am getting pixelated icons instead of the smooth icons I'd expect after providing so many icons in the <head>

Demonstration page here:
http://people.mozilla.org/~spenades/refapps/rtcamera/20130619-favicons/

This seems to be a regression in Nightly (17th june at least), as Aurora, Release and Beta display a properly smooth favicon and home screen icon.
(Reporter)

Comment 1

5 years ago
Created attachment 764781 [details]
result, from left to right: release, beta, aurora, nightly

Updated

5 years ago
tracking-fennec: --- → ?
Keywords: regression, regressionwindow-wanted
Able to reproduce this if you remove the home-screen short-cut and re-add it? Which device?

I gave this a whirl on my LG Nexus 4 (Android 4.2.2), and Galaxy S4 (Android 4.2.2) and on both devices the added short-cut resembled what you're seeing in release, Aurora and Beta. Not seeing the 8-bit looking version.
(Reporter)

Comment 3

5 years ago
Yes.

Nexus 4.
(Assignee)

Comment 4

5 years ago
Created attachment 764947 [details] [diff] [review]
Patch

This is two fixes.

1.) I think this is due to recent changes for favicons which made us scale the results of all requests to Favicon.loadFavicon(). When we draw the icon for the shortcut we actually scale that again to make it fit. Since the decision about what to scale it to is based on a dp size (R.dimen.awesomebar_row_favicon_size_small = 16dp), on some phones this looks worse than on others. I'm not positive that explains the screenshot you posted, but it seems plausible...

This adds a flag so that we can request unscaled icons. In order to avoid overloading this method with arguments, I just converted the current persist argument into a flag. I think maybe we should also switch R.dimen.awesomebar_row_favicon_size_small to be in pixel units. Anything bigger may scale up to larger sizes ok... but I need to think about that more.

2.) This also makes us not draw the box behind icons if we have a large one. It will instead just be resized to getPreferredIconSize (which on ICS and up is given by the system) http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/GeckoAppShell.java#791
Attachment #764947 - Flags: review?(margaret.leibovic)
tracking-fennec: ? → 24+

Comment 5

5 years ago
(In reply to Wesley Johnston (:wesj) from comment #4)

> 1.) I think this is due to recent changes for favicons which made us scale
> the results of all requests to Favicon.loadFavicon(). When we draw the icon
> for the shortcut we actually scale that again to make it fit. Since the
> decision about what to scale it to is based on a dp size
> (R.dimen.awesomebar_row_favicon_size_small = 16dp), on some phones this
> looks worse than on others. I'm not positive that explains the screenshot
> you posted, but it seems plausible...

Are you sure the scaling is what caused this bug? Looking at blame, that was introduced in bug 819973, which landed in 21.

I would be curious to find the exact changeset that caused this regression.

Comment 6

5 years ago
I also couldn't reproduce this on my Nexus 4 on the latest Nightly.

Sole, can you provide any more details about how you ran into this? Does it happen consistently? How does the favicon look in the browser (e.g. in the awesomescreen)?
Flags: needinfo?(sole)

Comment 7

5 years ago
Comment on attachment 764947 [details] [diff] [review]
Patch

Review of attachment 764947 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/base/DoorHangerPopup.java
@@ +39,5 @@
>      private int mYOffset;
>  
>      // Stores a set of all active DoorHanger notifications. A DoorHanger is
>      // uniquely identified by its tabId and value.
> +    private HashSet<DoorHanger> mDoorHangers;

Looks like some weird changes got in here.

::: mobile/android/base/GeckoAppShell.java
@@ +824,5 @@
> +        } else if (aType.equalsIgnoreCase(SHORTCUT_TYPE_WEBAPP) || aSource.getWidth() >= insetSize || aSource.getHeight() >= insetSize) {
> +            // otherwise, if this is a webapp or if the icons is lare enough, just draw it
> +            Rect iconBounds = new Rect(0, 0, size, size);
> +            canvas.drawBitmap(aSource, null, iconBounds, null);
> +            return bitmap;

I like this change, but I'm worried that the scaling changes may not be correct. Perhaps it would be best to split this into a separate patch.

::: mobile/android/base/resources/values/dimens.xml
@@ +22,5 @@
>      <dimen name="awesomebar_list_padding">0dp</dimen>
>  
>      <dimen name="awesomebar_header_row_height">20dp</dimen>
>      <dimen name="awesomebar_row_height">48dp</dimen>
> +    <dimen name="awesomebar_row_favicon_size_small">16px</dimen>

Why the change to px?
(Reporter)

Comment 8

5 years ago
I can't reproduce this with the latest Nightly (20th June) either--adding or removing the bookmark and the home screen icon looks fine now! O_O

Manually querying for updates says there are no available updates, just to be safe!

Now the icons don't look "8 bit" anymore.
Flags: needinfo?(sole)

Comment 9

5 years ago
(In reply to Soledad Penades [:sole] [:spenades] from comment #8)
> I can't reproduce this with the latest Nightly (20th June) either--adding or
> removing the bookmark and the home screen icon looks fine now! O_O
> 
> Manually querying for updates says there are no available updates, just to
> be safe!
> 
> Now the icons don't look "8 bit" anymore.

If no one can reproduce this anymore, I think we should close it as WORKSFORME. However, we should definitely keep an eye out in case this happens again.

(In reply to :Margaret Leibovic from comment #7)
> ::: mobile/android/base/GeckoAppShell.java
> @@ +824,5 @@
> > +        } else if (aType.equalsIgnoreCase(SHORTCUT_TYPE_WEBAPP) || aSource.getWidth() >= insetSize || aSource.getHeight() >= insetSize) {
> > +            // otherwise, if this is a webapp or if the icons is lare enough, just draw it
> > +            Rect iconBounds = new Rect(0, 0, size, size);
> > +            canvas.drawBitmap(aSource, null, iconBounds, null);
> > +            return bitmap;
> 
> I like this change, but I'm worried that the scaling changes may not be
> correct. Perhaps it would be best to split this into a separate patch.

Actually, bug 884865 was filed about that. Do you want to move this into a patch there?

Updated

5 years ago
Attachment #764947 - Flags: review?(margaret.leibovic)
(Assignee)

Comment 10

5 years ago
(In reply to :Margaret Leibovic from comment #9)
> Actually, bug 884865 was filed about that. Do you want to move this into a
> patch there?

I still kinda want this patch in if we do that. A page could give us a 256px by 256px icon. We could want a 256x256 icon. But instead the Favicon service would give us something scaled down to 64x64 which we then scale up to 256x256. I'd prefer if we had a way to say "Give me everything you've got." I can move this patch there if you'd prefer....
Separating out the patches seems like a good idea. Clear and specific patches are better than patches that do too much.

Comment 12

5 years ago
Yeah, I think we should split the patch, keeping the scaling part in here. However, we should change the bug summary, since the issue described here isn't reproducible, so this bug would now be about an optimization more than fixing a specific problem.
(Reporter)

Comment 13

5 years ago
Please feel free to change summary as required :-)

Updated

5 years ago
Assignee: nobody → wjohnston
Component: Awesomescreen → General
Keywords: regression, regressionwindow-wanted
OS: Mac OS X → Android
Hardware: x86 → ARM
Summary: Icons in the home screen and favicon in the url bar look pixellated even after providing alternative high res icons → Don't automatically scale icons in LoadFaviconTask
(Assignee)

Comment 14

5 years ago
Created attachment 767839 [details] [diff] [review]
Patch v2

Split out the homescreen favicon bits
Attachment #764947 - Attachment is obsolete: true
Attachment #767839 - Flags: review?(margaret.leibovic)

Comment 15

5 years ago
Comment on attachment 767839 [details] [diff] [review]
Patch v2

Review of attachment 767839 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/base/BrowserApp.java
@@ +641,4 @@
>                      }
> +
> +                    Favicons favicons = Favicons.getInstance();
> +                    favicons.loadFavicon(url, tab.getFaviconURL(), 0,

So, looking into the implementation of loadFavicon, this will try to find the favicon in the cache, and in AllPagesTab we actually store scaled favicons in there:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/awesomebar/AllPagesTab.java#884

The favicon loading/caching code from the AwesomeBarTabs code is a mess, and I think we should try to improve it with the new about:home. I think I saw Sriram working on some bugs to clean this up, but let's file a follow-up to make sure we just cache full size images, and then scale them right where they're used.
Attachment #767839 - Flags: review?(margaret.leibovic) → review+
(Assignee)

Comment 16

5 years ago
Landed with wrong bug number. Will push a backout-in fix:
https://hg.mozilla.org/integration/mozilla-inbound/rev/df9231393766
https://hg.mozilla.org/mozilla-central/rev/341910d20d7b
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 25
You need to log in before you can comment on or make changes to this bug.