Tegra hits slow path 'bits_image_fetch_bilinear_afine_pad_r5g6b5'

RESOLVED FIXED in mozilla14

Status

()

Core
Graphics
RESOLVED FIXED
5 years ago
3 years ago

People

(Reporter: BenWa, Unassigned)

Tracking

unspecified
mozilla14
ARM
Android
Points:
---

Firefox Tracking Flags

(blocking-fennec1.0 +, fennec15+)

Details

(Whiteboard: [ARM-opt])

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

5 years ago
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

5 years ago
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.
(Reporter)

Comment 3

5 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

5 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

5 years ago
Irina, how many phones are Tegra based.
tracking-fennec: --- → 15+

Comment 6

5 years ago
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.

Comment 8

5 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.
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
Attachment #617484 - Flags: review?(jmuizelaar)
Attachment #617484 - Flags: review?(ajuma)
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

5 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+
(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.
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
Attachment #617484 - Attachment is obsolete: true
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+
https://hg.mozilla.org/integration/mozilla-inbound/rev/190fc7cd65c6

Comment 16

5 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

5 years ago
blocking-fennec1.0: ? → +
Created attachment 617612 [details] [diff] [review]
Bug 747274 - Mark reftests that should now pass on Android because of the different filtering algorithm
Attachment #617612 - Flags: review?(jmaher)
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+

Updated

5 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

5 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
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

Comment 22

5 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

5 years ago
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.
(Reporter)

Comment 24

5 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

5 years ago
Target Milestone: --- → mozilla14

Comment 25

5 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

5 years ago
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
Last Resolved: 5 years ago
Resolution: --- → FIXED
Whiteboard: [ARM-opt]
You need to log in before you can comment on or make changes to this bug.