Last Comment Bug 747274 - Tegra hits slow path 'bits_image_fetch_bilinear_afine_pad_r5g6b5'
: Tegra hits slow path 'bits_image_fetch_bilinear_afine_pad_r5g6b5'
Status: RESOLVED FIXED
[ARM-opt]
:
Product: Core
Classification: Components
Component: Graphics (show other bugs)
: unspecified
: ARM Android
: -- normal (vote)
: mozilla14
Assigned To: Nobody; OK to take it and work on it
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-04-19 18:13 PDT by Benoit Girard (:BenWa)
Modified: 2014-02-27 08:54 PST (History)
12 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
15+


Attachments
Bug 747274 - Add a pref (default to true on Android) to forcible use nearest pixel filtering for background drawing (4.06 KB, patch)
2012-04-23 08:10 PDT, George Wright (:gw280) (:gwright)
ajuma.bugzilla: review+
jmuizelaar: review+
Details | Diff | Splinter Review
Bug 747274 - Add a pref (default to true on Android) to forcible use nearest pixel filtering for background drawing. r=jrmuizel,ajuma (4.20 KB, patch)
2012-04-23 08:32 PDT, George Wright (:gw280) (:gwright)
blassey.bugs: approval‑mozilla‑central+
Details | Diff | Splinter Review
Bug 747274 - Mark reftests that should now pass on Android because of the different filtering algorithm (3.57 KB, patch)
2012-04-23 13:16 PDT, George Wright (:gw280) (:gwright)
jmaher: review+
Details | Diff | Splinter Review

Description Benoit Girard (:BenWa) 2012-04-19 18:13:37 PDT
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.
Comment 1 Benoit Girard (:BenWa) 2012-04-19 18:14:22 PDT
Also that function accounts for 60-80% of our time spent during panning.
Comment 2 Joe Drew (not getting mail) 2012-04-19 18:52:38 PDT
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.
Comment 3 Benoit Girard (:BenWa) 2012-04-19 19:14:02 PDT
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.
Comment 4 Benoit Girard (:BenWa) 2012-04-20 07:55:56 PDT
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?
Comment 5 JP Rosevear [:jpr] 2012-04-20 12:18:41 PDT
Irina, how many phones are Tegra based.
Comment 6 Irina Sandu 2012-04-22 20:13:43 PDT
Out of our addressable market, around 10%.
Comment 7 Jeff Muizelaar [:jrmuizel] 2012-04-23 07:01:41 PDT
(In reply to Irina Sandu from comment #6)
> Out of our addressable market, around 10%.

That seems unexpectedly high.
Comment 8 Siarhei Siamashka 2012-04-23 07:05:18 PDT
(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 George Wright (:gw280) (:gwright) 2012-04-23 08:10:58 PDT
Created attachment 617484 [details] [diff] [review]
Bug 747274 - Add a pref (default to true on Android) to forcible use nearest pixel filtering for background drawing
Comment 10 Jeff Muizelaar [:jrmuizel] 2012-04-23 08:14:51 PDT
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;
>+}
Comment 11 Ali Juma [:ajuma] 2012-04-23 08:17:07 PDT
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?).
Comment 12 Jeff Muizelaar [:jrmuizel] 2012-04-23 08:20:12 PDT
(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 George Wright (:gw280) (:gwright) 2012-04-23 08:32:42 PDT
Created 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
Comment 14 George Wright (:gw280) (:gwright) 2012-04-23 08:33:16 PDT
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):
Comment 15 George Wright (:gw280) (:gwright) 2012-04-23 10:30:49 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/190fc7cd65c6
Comment 16 Irina Sandu 2012-04-23 12:00:11 PDT
(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.
Comment 17 George Wright (:gw280) (:gwright) 2012-04-23 13:16:55 PDT
Created attachment 617612 [details] [diff] [review]
Bug 747274 - Mark reftests that should now pass on Android because of the different filtering algorithm
Comment 18 Joel Maher ( :jmaher) 2012-04-23 13:44:00 PDT
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!
Comment 19 Mozilla RelEng Bot 2012-04-23 13:48:49 PDT
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 :Ehsan Akhgari 2012-04-23 16:08:35 PDT
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 :Ehsan Akhgari 2012-04-23 16:34:52 PDT
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 Mozilla RelEng Bot 2012-04-24 01:00:41 PDT
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
Comment 23 George Wright (:gw280) (:gwright) 2012-04-24 07:32:54 PDT
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.
Comment 24 Benoit Girard (:BenWa) 2012-04-24 07:39:06 PDT
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.
Comment 25 JP Rosevear [:jpr] 2012-04-24 10:13:20 PDT
(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?
Comment 26 Benoit Girard (:BenWa) 2012-04-24 10:26:50 PDT
I'll file a new bug when we get our hands on a tegra phone.
Comment 27 Matt Brubeck (:mbrubeck) 2012-04-26 16:33:45 PDT
Should this bug be resolved?

Note You need to log in before you can comment on or make changes to this bug.