669851 - Don't use nearest-neighbour filtering when neon isn't available

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Chris Lord [:cwiiis]]

Attachment: Don't use nearest-neighbour filtering when NEON isn't available

Although the speed boost is nice, the rendering quality when using nearest neighbour filtering is unacceptable, especially on tablet (where images are often enlarged).

I think we should either just disable this and take the speed hit, or at least special-case things so images are never scaled up with nearest-neighbour filtering (scaling downwards is less of an issue).

Attached is a patch that does the former.

Comment 1

[Matt Brubeck (:mbrubeck)]

Comment on attachment 544428 [details] [diff] [review]
Don't use nearest-neighbour filtering when NEON isn't available

The code change is fine and I am in favor of this change, but we need some consensus on acceptable performance.  Last time we did this, it was backed out over performance worries.  (Personally I feel that rendering perf on Tegra is "good enough" with this change, and I agree that nearest neighbor is unacceptable - especially with all our competitors using good-quality rendering, and users noticing that all pages look much worse in Firefox.)

I would even consider backporting this low-risk mobile-only change to beta and aurora, since we are already changing this for non-Tegra devices in Firefox 6, and "better image rendering" sounds better in release notes than "better image rendering (on some devices)."

Comment 2

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

The last time we did this and it got backed out, I was a bit irate over the process. In the end, we decided we needed some way to observe the performance changes. Something objective, not subjective. We still don't have a good way to do it. FennecMark/FennecBench has Tpan and Tzoom and those numbers were affected last time we landed, iirc.

If you push this to Try Server, do we see a difference in Tpan and Tzoom?

Comment 3

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

I would like to land this, but we still need some data.

Comment 4

[Aakash Desai [:aakashd]]

FYI: here's a spread of issues reported from beta users about image scaling - https://input.mozilla.com/en-US/?product=mobile&version=6.0&date_start=2011-07-12&q=image

Comment 5

[Wesley Johnston (:wesj)]

I pushed this to try for talos tests:

with patch: http://tbpl.mozilla.org/?tree=Try&rev=c06d50b0c66e
without: http://tbpl.mozilla.org/?tree=Try&rev=0133de301483

Comment 6

[Matt Brubeck (:mbrubeck)]

(In reply to comment #5)
> with patch: http://tbpl.mozilla.org/?tree=Try&rev=c06d50b0c66e
> without: http://tbpl.mozilla.org/?tree=Try&rev=0133de301483

Page load speed is essentially unchanged by this patch - the difference is within the normal noise level of the test:

tp4m: 679.1 (without patch)
tp4m: 682.6 (with patch - 0.5% slower)

Unfortunately, tpan and tzoom both failed to complete on one or both TryServer runs.  I've requested some rebuilds to try to get more data.

However, I disagree that we should block this change on getting more measurements.  Measurements will help us when we start trying working on making this code faster, but for now we *know* that it will make certain things slower, and we know that the slowness is noticeable in some circumstances (zooming or quick scrolling on pages with lots of large images).  Quantifying this will be useful for a baseline for future work, but it won't tell us whether to land this patch or not.  The numbers can't tell us what trade-off to make; only the user experience can tell us that.

We've already seen the user experience trade-offs with and without the patch.  I argue that the experience with the patch, on the modern Tegra hardware that it affects, is a strong net gain.  Tegra chips are starting to take over the high-end market, and I think we need to make this change now if we want to be competitive in this market in the coming months.  Image scaling is driving away users much more than a usually-slight increase in checkerboarding would.

I still think we should land this patch now for Firefox 8, and on Aurora for Firefox 7.  (As I said a month ago, I'd love to get it into Firefox 6 beta, but it may or may not be too late for that.)

Comment 7

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

I am fine with landing this to get user feedback in Nightly. Jeff has seen this patch before, so hopefully it shouldn't have any surprises for him.

Comment 8

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 544428 [details] [diff] [review]
Don't use nearest-neighbour filtering when NEON isn't available

The code change is obviously fine here. Someone needs to ensure that it's the right thing to do.

Comment 9

[Doug Turner (:dougt)]

i think mark did that.  i concur.

when we land, please notify qa about possible regressions.

Comment 10

[Matt Brubeck (:mbrubeck)]

http://hg.mozilla.org/integration/mozilla-inbound/rev/a5932b908282

I will send an email to the qa-mobile team.

Comment 11

[Marco Bonardo [:mak]]

Just my 2 cents, I recently got a galaxy tab 10.1, I tried to look at some photos on Picasa through a mail link from a friend, they were looking so bad that I had to use the default browser.  Having to wait some ms more would not have been that bad.

Comment 12

[Matt Brubeck (:mbrubeck)]

A couple of reftests that had been inconsistent on Android are now consistent with other platforms.  Adjusted the reftest annotations:
http://hg.mozilla.org/integration/mozilla-inbound/rev/e966ad69a364

Comment 13

[Marco Bonardo [:mak]]

http://hg.mozilla.org/mozilla-central/rev/a5932b908282
http://hg.mozilla.org/mozilla-central/rev/e966ad69a364

Comment 14

[Matt Brubeck (:mbrubeck)]

For those following along at home, this change is in the latest nightly build from https://wiki.mozilla.org/Mobile/Platforms/Android

Comment 15

[Antoine Turmel [:GeekShadow]]

Just tested on Nightly 8.0a1 build 20110804 and work great with Asus EEE Pad Transformer (TF101/Tegra2/Android 3.2)

and +1 for this change in beta and aurora

Comment 16

[Kevin Brosnan [Ex-Mozilla]]

Verified - image quality is much improved - Mozilla/5.0 (Android; Linux armv7l; rv:8.0a1) Gecko/20110804 Firefox/8.0a1 Fennec/8.0a1 ID:20110804092929

Comment 17

[Matt Brubeck (:mbrubeck)]

Comment on attachment 544428 [details] [diff] [review]
Don't use nearest-neighbour filtering when NEON isn't available

Requesting approval to land this in Aurora for Firefox 7.  This is a low-risk mobile-only change (the only code touched is #ifdef MOZ_GFX_OPTIMIZE_MOBILE) to address top feedback from users, and make us more usable and competitive on newer Android devices.

Comment 18

[LegNeato]

Comment on attachment 544428 [details] [diff] [review]
Don't use nearest-neighbour filtering when NEON isn't available

Approved for releases/mozilla-aurora. Please land ASAP.

Comment 19

[Matt Brubeck (:mbrubeck)]

Pushed to Aurora for Firefox 7 (folded patch with both code and test changes):
http://hg.mozilla.org/releases/mozilla-aurora/rev/bbcb26987398

Comment 20

[Tony Chung [:tchung]]

Kevin, can you verify this against aurora?  thanks.

Comment 21

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

Compared aurora 8-3 build versus aurora 816 build.  Verified fixed:
Device: Toshiba Thrive
OS Android 3.1

Comment 22

[Martijn Wargers (dead)]

Perhaps this should be backed out, considering bug 689707? Notice bug 689707, comment 4.

Comment 23

[Matt Brubeck (:mbrubeck)]

We got even more negative feedback before this was checked in, and many people described it as a show-stopper, for example https://plus.google.com/u/0/111789281962401312919/posts/4fimmnxCz1M

I know we need to improve the speed, but given the current speed/quality trade-off, I think we chose the better of two bad options.

Comment 24

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

I agree, many sites were unusably ugly before this landed

Comment 25

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

Piling on here. I too want to keep the better looking scaling. I can live with a slight delay when loading the image. Plus, we can make that faster.