Closed Bug 884848 Opened 11 years ago Closed 11 years ago

Don't automatically scale icons in LoadFaviconTask

Categories

(Firefox for Android Graveyard :: General, defect)

Product:
Firefox for Android Graveyard
Component:
General
Platform:
ARM
Android
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(fennec24+)

Status:
RESOLVED FIXED
Milestone:
Firefox 25
Tracking Flags:
Tracking Status
fennec 24+ ---

People

(Reporter: sole, Assigned: wesj)

Details

Attachments

(2 files, 1 obsolete file)

result, from left to right: release, beta, aurora, nightly
385.80 KB, image/png
Details
Patch v2
8.26 KB, patch
Margaret
: review+
Details | Diff | Splinter Review 
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.
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.
Yes.

Nexus 4.
Attached patch Patch (obsolete) — — Splinter Review 
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+
(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.
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 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?
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)
(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?
Attachment #764947 - Flags: review?(margaret.leibovic)
(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.
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.
Please feel free to change summary as required :-)
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
Attached patch Patch v2 — — Splinter Review 
Split out the homescreen favicon bits
Attachment #764947 - Attachment is obsolete: true
Attachment #767839 - Flags: review?(margaret.leibovic)
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+
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
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 25
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: