Closed Bug 669851 Opened 12 years ago Closed 12 years ago
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 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)."
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?
I would like to land this, but we still need some data.
tracking-fennec: --- → ?
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
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
(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.)
Attachment #544428 - Flags: review?(mark.finkle) → review?(jmuizelaar)
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 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.
Attachment #544428 - Flags: review?(jmuizelaar) → review+
i think mark did that. i concur. when we land, please notify qa about possible regressions.
http://hg.mozilla.org/integration/mozilla-inbound/rev/a5932b908282 I will send an email to the qa-mobile team.
Status: NEW → ASSIGNED
Whiteboard: [Input] → [Input][inbound]
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.
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
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 8
For those following along at home, this change is in the latest nightly build from https://wiki.mozilla.org/Mobile/Platforms/Android
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
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
Status: RESOLVED → VERIFIED
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.
Attachment #544428 - Flags: approval-mozilla-aurora?
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.
Attachment #544428 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Pushed to Aurora for Firefox 7 (folded patch with both code and test changes): http://hg.mozilla.org/releases/mozilla-aurora/rev/bbcb26987398
Kevin, can you verify this against aurora? thanks.
Compared aurora 8-3 build versus aurora 816 build. Verified fixed: Device: Toshiba Thrive OS Android 3.1
12 years ago
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.
I agree, many sites were unusably ugly before this landed
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.
You need to log in before you can comment on or make changes to this bug.