Closed
Bug 747274
Opened 12 years ago
Closed 12 years ago
Tegra hits slow path 'bits_image_fetch_bilinear_afine_pad_r5g6b5'
Categories
(Core :: Graphics, defect)
Tracking
()
RESOLVED
FIXED
mozilla14
People
(Reporter: BenWa, Unassigned)
Details
(Whiteboard: [ARM-opt])
Attachments
(2 files, 1 obsolete file)
4.20 KB,
patch
|
blassey
:
approval-mozilla-central+
|
Details | Diff | Splinter Review |
3.57 KB,
patch
|
jmaher
:
review+
|
Details | Diff | Splinter Review |
The performance on tegra is bad and is TERRIBLE with a large display port because we hit bits_image_fetch_biilnear_afine_pad_r5g6b5 in pixmans which is just a terrible slow path. The performance makes the browser unusable. We should use nearest filtering everyone (at the very least for backgrounds) and file a bug to add a fast path for bilinear filtering in pixman without neon. Nearest make the browser run at decent performance. Requesting beta blocker.
Reporter | ||
Comment 1•12 years ago
|
||
Also that function accounts for 60-80% of our time spent during panning.
Comment 2•12 years ago
|
||
I'd lean towards soft blocking, given that there are few Tegra 2 handsets out there; it's only super-common on tablets, which we aren't targeting for 1.0.
Reporter | ||
Comment 3•12 years ago
|
||
Considering how heavy the performance degradation is and how simple it is to fix I'd vote for this being a hard blocker for fennec release.
Reporter | ||
Comment 4•12 years ago
|
||
I just realized that tegra is mostly tablet and that we're going to be targeting phones only for the first release. JP, can we get numbers on neon support for our target release market?
Summary: Tegra hits slow path 'bits_image_fetch_biilnear_afine_pad_r5g6b5' → Tegra hits slow path 'bits_image_fetch_bilinear_afine_pad_r5g6b5'
Comment 5•12 years ago
|
||
Irina, how many phones are Tegra based.
Updated•12 years ago
|
tracking-fennec: --- → 15+
Comment 6•12 years ago
|
||
Out of our addressable market, around 10%.
Comment 7•12 years ago
|
||
(In reply to Irina Sandu from comment #6) > Out of our addressable market, around 10%. That seems unexpectedly high.
Comment 8•12 years ago
|
||
(In reply to Jeff Muizelaar [:jrmuizel] from comment #7) > That seems unexpectedly high. Agreed. Especially if by chance it happens to be a *loud* minority.
Comment 9•12 years ago
|
||
Attachment #617484 -
Flags: review?(jmuizelaar)
Attachment #617484 -
Flags: review?(ajuma)
Comment 10•12 years ago
|
||
Comment on attachment 617484 [details] [diff] [review] Bug 747274 - Add a pref (default to true on Android) to forcible use nearest pixel filtering for background drawing ># HG changeset patch ># User George Wright <george@mozilla.com> > >Bug 747274 - Add a pref (default to true on Android) to forcible use nearest pixel filtering for background drawing > >diff --git a/layout/base/nsLayoutUtils.cpp b/layout/base/nsLayoutUtils.cpp >index 766d91c..eee12ad 100644 >--- a/layout/base/nsLayoutUtils.cpp >+++ b/layout/base/nsLayoutUtils.cpp >@@ -163,16 +163,30 @@ nsLayoutUtils::Are3DTransformsEnabled() > s3DTransformPrefCached = true; > mozilla::Preferences::AddBoolVarCache(&s3DTransformsEnabled, > "layout.3d-transforms.enabled"); > } > > return s3DTransformsEnabled; > } > >+bool >+nsLayoutUtils::UseNearestFiltering() >+{ >+ static bool sUseNearestFilteringEnabled; >+ static bool sUseNearestFilteringPrefCached = false; Use sUseNearestFilterinPrefInitialized instead of cached because we're not boolcache. >+ >+ if (!sUseNearestFilteringPrefCached) { >+ sUseNearestFilteringPrefCached = true; >+ sUseNearestFilteringEnabled = mozilla::Preferences::GetBool("gfx.filter.nearest.force-enabled", false); >+ } >+ >+ return sUseNearestFilteringEnabled; >+}
Attachment #617484 -
Flags: review?(jmuizelaar) → review+
Comment 11•12 years ago
|
||
Comment on attachment 617484 [details] [diff] [review] Bug 747274 - Add a pref (default to true on Android) to forcible use nearest pixel filtering for background drawing Review of attachment 617484 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/base/nsLayoutUtils.cpp @@ +168,5 @@ > return s3DTransformsEnabled; > } > > +bool > +nsLayoutUtils::UseNearestFiltering() I think the word "background" should be part of this name (e.g. UseBackgroundNearestFiltering?).
Attachment #617484 -
Flags: review?(ajuma) → review+
Comment 12•12 years ago
|
||
(In reply to Ali Juma [:ajuma] from comment #11) > > I think the word "background" should be part of this name (e.g. > UseBackgroundNearestFiltering?). I strongly agree.
Comment 13•12 years ago
|
||
Attachment #617484 -
Attachment is obsolete: true
Comment 14•12 years ago
|
||
Comment on attachment 617486 [details] [diff] [review] Bug 747274 - Add a pref (default to true on Android) to forcible use nearest pixel filtering for background drawing. r=jrmuizel,ajuma [Approval Request Comment] Please see https://wiki.mozilla.org/Tree_Rules for information on mozilla-central landings that do not require approval. Possible risk to Fennec Native 14 (if any):
Attachment #617486 -
Flags: approval-mozilla-central?
Updated•12 years ago
|
Attachment #617486 -
Flags: approval-mozilla-central? → approval-mozilla-central+
Comment 16•12 years ago
|
||
(In reply to Siarhei Siamashka from comment #8) > (In reply to Jeff Muizelaar [:jrmuizel] from comment #7) > > That seems unexpectedly high. > > Agreed. Especially if by chance it happens to be a *loud* minority. It is based on data of the worldwide Android install base, adjusted for the range of devices that we support. Out of ~100 million addressable devices in January 2012, ~9 million run on Tegra. The most representative devices in market that run on Tegra 2: Motorola Atrix, Motorola Atrix 4G, Motorola Droid Bionic, Samsung Galaxy R, Droid X2, LG Optimus 2X Samsung Galaxy R and a part of the Samsung S II install base.
Updated•12 years ago
|
blocking-fennec1.0: ? → +
Comment 17•12 years ago
|
||
Attachment #617612 -
Flags: review?(jmaher)
Updated•12 years ago
|
Whiteboard: [autoland:617486,617612:-b do -p linux,android,android-xul -u reftest,reftest-1 -t none]
Updated•12 years ago
|
Whiteboard: [autoland:617486,617612:-b do -p linux,android,android-xul -u reftest,reftest-1 -t none] → [autoland-try:617486,617612:-b do -p linux,android,android-xul -u reftest,reftest-1 -t none]
Comment 18•12 years ago
|
||
Comment on attachment 617612 [details] [diff] [review] Bug 747274 - Mark reftests that should now pass on Android because of the different filtering algorithm Review of attachment 617612 [details] [diff] [review]: ----------------------------------------------------------------- thanks for the comments in here!
Attachment #617612 -
Flags: review?(jmaher) → review+
Updated•12 years ago
|
Whiteboard: [autoland-try:617486,617612:-b do -p linux,android,android-xul -u reftest,reftest-1 -t none] → [autoland-in-queue]
Comment 19•12 years ago
|
||
Autoland Patchset: Patches: 617486, 617612 Branch: mozilla-central => try Destination: http://hg.mozilla.org/try/pushloghtml?changeset=496cef96cb8d Try run started, revision 496cef96cb8d. To cancel or monitor the job, see: https://tbpl.mozilla.org/?tree=Try&rev=496cef96cb8d
Comment 20•12 years ago
|
||
I backed this out because of the Android R1 bustage from both central and inbound): https://hg.mozilla.org/mozilla-central/rev/72fcb7c13613 Please reland once you have a patch which has been tested on the try server. Thanks!
Comment 21•12 years ago
|
||
Regression :( Robocop Checkerboarding Benchmark increase 21.8% on Android 2.2 (Native) Mozilla-Inbound ------------------------------------------------------------------------------------------------------ Previous: avg 0.813 stddev 0.006 of 30 runs up to revision d2b34e4338bb New : avg 0.990 stddev 0.000 of 5 runs since revision 344a48cc9998 Change : +0.177 (21.8% / z=27.800) Graph : http://mzl.la/I4Nh5G Changeset range: http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=d2b34e4338bb&tochange=344a48cc9998 Changesets: * http://hg.mozilla.org/integration/mozilla-inbound/rev/190fc7cd65c6 : George Wright <gwright@mozilla.com> - Bug 747274 - Add a pref (default to true on Android) to forcibly use nearest pixel filtering for background drawing. r=jrmuizel,ajuma a=blassey : http://bugzilla.mozilla.org/show_bug.cgi?id=747274 * http://hg.mozilla.org/integration/mozilla-inbound/rev/9b2440c92909 : Brad Lassey <blassey@mozilla.com> - bug 746139 - local JNI frame won't be popped if init fails r=dougt a=lsblakk : http://bugzilla.mozilla.org/show_bug.cgi?id=746139 * http://hg.mozilla.org/integration/mozilla-inbound/rev/344a48cc9998 : Brad Lassey <blassey@mozilla.com> - bug 746132 - Screenshot buffers won't be freed if tab not found r=kats a=lsblakk : http://bugzilla.mozilla.org/show_bug.cgi?id=746132
Comment 22•12 years ago
|
||
Try run for 496cef96cb8d is complete. Detailed breakdown of the results available here: https://tbpl.mozilla.org/?tree=Try&rev=496cef96cb8d Results (out of 15 total builds): success: 13 warnings: 2 Builds (or logs if builds failed) available at: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/autolanduser@mozilla.com-496cef96cb8d
Updated•12 years ago
|
Whiteboard: [autoland-in-queue]
Comment 23•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/b88fba85586b Landed directly on m-c to make the uplift, as I've been informed there may not be another merge from m-i before then.
Reporter | ||
Comment 24•12 years ago
|
||
This patch is insufficient to remove all significant source of 'bits_image_fetch_biilnear_afine_pad_r5g6b5', we need to confirm if this is sufficient.
Updated•12 years ago
|
Target Milestone: --- → mozilla14
Comment 25•12 years ago
|
||
(In reply to Benoit Girard (:BenWa) from comment #24) > This patch is insufficient to remove all significant source of > 'bits_image_fetch_biilnear_afine_pad_r5g6b5', we need to confirm if this is > sufficient. Not sure I follow - do we need a separate bug for follow up?
Reporter | ||
Comment 26•12 years ago
|
||
I'll file a new bug when we get our hands on a tegra phone.
Updated•12 years ago
|
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Updated•10 years ago
|
Whiteboard: [ARM-opt]
You need to log in
before you can comment on or make changes to this bug.
Description
•