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)

ARM
Android
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla14
Tracking Status
blocking-fennec1.0 --- +
fennec 15+ ---

People

(Reporter: BenWa, Unassigned)

Details

(Whiteboard: [ARM-opt])

Attachments

(2 files, 1 obsolete file)

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.
Also that function accounts for 60-80% of our time spent during panning.
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.
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.
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'
Irina, how many phones are Tegra based.
tracking-fennec: --- → 15+
Out of our addressable market, around 10%.
(In reply to Irina Sandu from comment #6)
> Out of our addressable market, around 10%.

That seems unexpectedly high.
(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 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 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+
(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 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?
Attachment #617486 - Flags: approval-mozilla-central? → approval-mozilla-central+
(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.
blocking-fennec1.0: ? → +
Whiteboard: [autoland:617486,617612:-b do -p linux,android,android-xul -u reftest,reftest-1 -t none]
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 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+
Whiteboard: [autoland-try:617486,617612:-b do -p linux,android,android-xul -u reftest,reftest-1 -t none] → [autoland-in-queue]
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
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!
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
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
Whiteboard: [autoland-in-queue]
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.
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.
Target Milestone: --- → mozilla14
(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?
I'll file a new bug when we get our hands on a tegra phone.
Should this bug be resolved?
OS: Mac OS X → Android
Hardware: x86 → ARM
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Whiteboard: [ARM-opt]
You need to log in before you can comment on or make changes to this bug.