Last Comment Bug 719233 - Whitelist only known working devices for direct texture support
: Whitelist only known working devices for direct texture support
Status: RESOLVED FIXED
:
Product: Firefox for Android
Classification: Client Software
Component: General (show other bugs)
: unspecified
: ARM Android
: P1 normal (vote)
: Firefox 12
Assigned To: James Willcox (:snorp) (jwillcox@mozilla.com)
:
Mentors:
Depends on: 720361 721383
Blocks: 713569 714289
  Show dependency treegraph
 
Reported: 2012-01-18 14:20 PST by James Willcox (:snorp) (jwillcox@mozilla.com)
Modified: 2012-02-07 06:34 PST (History)
9 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed


Attachments
Only use direct texturing on whitelisted devices (23.94 KB, patch)
2012-01-18 14:22 PST, James Willcox (:snorp) (jwillcox@mozilla.com)
blassey.bugs: review+
Details | Diff | Splinter Review
Only use direct texturing on whitelisted devices (25.49 KB, patch)
2012-01-18 17:32 PST, James Willcox (:snorp) (jwillcox@mozilla.com)
blassey.bugs: review+
blassey.bugs: approval‑mozilla‑aurora+
blassey.bugs: approval‑mozilla‑beta+
Details | Diff | Splinter Review

Description James Willcox (:snorp) (jwillcox@mozilla.com) 2012-01-18 14:20:52 PST
The direct texture (gralloc) support is somewhat fragile, and has problems on a few devices. Initially, nsWindow::HasDirectTexture() was designed to filter out the problem devices, but it doesn't appear to be enough. Whitelisting known-good devices will allow us to use the feature without sacrificing stability.
Comment 1 James Willcox (:snorp) (jwillcox@mozilla.com) 2012-01-18 14:22:48 PST
Created attachment 589646 [details] [diff] [review]
Only use direct texturing on whitelisted devices
Comment 2 James Willcox (:snorp) (jwillcox@mozilla.com) 2012-01-18 14:25:13 PST
The attached patch whitelists the Droid Pro and Galaxy Nexus, which are the devices I have in my possession that work without issue.

There are also prefs to force the device to be either whitelisted or blacklisted. Those would be direct-texture.force.enabled and direct-texture.force.disabled, respectively.
Comment 3 Brad Lassey [:blassey] (use needinfo?) 2012-01-18 16:27:41 PST
Comment on attachment 589646 [details] [diff] [review]
Only use direct texturing on whitelisted devices

Review of attachment 589646 [details] [diff] [review]:
-----------------------------------------------------------------

::: widget/android/AndroidGraphicBuffer.cpp
@@ +116,5 @@
>      int32_t left;
>      int32_t top;
>      int32_t right;
>      int32_t bottom;
> +} ARect;

any reason to change this other than "just because"?
Comment 4 James Willcox (:snorp) (jwillcox@mozilla.com) 2012-01-18 17:13:34 PST
(In reply to Brad Lassey [:blassey] from comment #3)
> Comment on attachment 589646 [details] [diff] [review]
> Only use direct texturing on whitelisted devices
> 
> Review of attachment 589646 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: widget/android/AndroidGraphicBuffer.cpp
> @@ +116,5 @@
> >      int32_t left;
> >      int32_t top;
> >      int32_t right;
> >      int32_t bottom;
> > +} ARect;
> 
> any reason to change this other than "just because"?

There is a conflicting definition in AndroidJavaWrappers.h

This bug has one problem that I'm trying to work out. Rendering is messed up (wrong tile size?) until you rotate or otherwise resize the screen.
Comment 5 James Willcox (:snorp) (jwillcox@mozilla.com) 2012-01-18 17:32:50 PST
Created attachment 589721 [details] [diff] [review]
Only use direct texturing on whitelisted devices

Fixed the rendering issue, needed to resend SIZE_CHANGED after changing tile system
Comment 6 James Willcox (:snorp) (jwillcox@mozilla.com) 2012-01-18 18:03:58 PST
http://hg.mozilla.org/integration/mozilla-inbound/rev/aba1658ce66c
Comment 7 Brad Lassey [:blassey] (use needinfo?) 2012-01-18 19:38:35 PST
Comment on attachment 589721 [details] [diff] [review]
Only use direct texturing on whitelisted devices

[Triage Comment]
Comment 8 Matt Brubeck (:mbrubeck) 2012-01-19 10:47:00 PST
https://hg.mozilla.org/mozilla-central/rev/aba1658ce66c
Comment 9 Scoobidiver (away) 2012-01-20 00:21:37 PST
Wouldn't it be easier to maintain with a downloaded black(white)list such as on desktop (see https://wiki.mozilla.org/Firefox/Projects/VideoDriverBlacklisting)?
Comment 10 Kevin Brosnan [:kbrosnan] 2012-01-20 00:33:51 PST
It has come up often in discussion of this bug. As I understand from the outcome of one of the meetings, Snorp is checking with the GFX team to see how feasible implementing that blacklist and when make a good time frame for inclusion.
Comment 11 Brad Lassey [:blassey] (use needinfo?) 2012-01-29 22:01:48 PST
Snorp, please add this and whatever it depends on in the aurora landing queue
Comment 12 Naoki Hirata :nhirata (please use needinfo instead of cc) 2012-02-06 01:52:49 PST
Can we push this to 11 as well?
Comment 13 Brad Lassey [:blassey] (use needinfo?) 2012-02-06 16:18:05 PST
Comment on attachment 589721 [details] [diff] [review]
Only use direct texturing on whitelisted devices

[Triage Comment]
also needed on beta now
Comment 14 Brad Lassey [:blassey] (use needinfo?) 2012-02-06 18:41:09 PST
https://hg.mozilla.org/releases/mozilla-beta/rev/5f2ccd447bba
Comment 15 Martijn Wargers [:mwargers] (not working for Mozilla) 2012-02-07 06:00:47 PST
I just confirmed that this tinderbox build:
http://ftp.mozilla.org/pub/mozilla.org/mobile/tinderbox-builds/mozilla-beta-android/1328588649/fennec-11.0.en-US.android-arm.apk
now suffers from bug 721383.

Can this be backed out of Firefox 11, please?
Comment 16 Mark Finkle (:mfinkle) (use needinfo?) 2012-02-07 06:03:39 PST
(In reply to Martijn Wargers [:mw22] (QA - IRC nick: mw22) from comment #15)
> I just confirmed that this tinderbox build:
> http://ftp.mozilla.org/pub/mozilla.org/mobile/tinderbox-builds/mozilla-beta-
> android/1328588649/fennec-11.0.en-US.android-arm.apk
> now suffers from bug 721383.
> 
> Can this be backed out of Firefox 11, please?

what devices are you seeing the hang? Do we also hang on those devices on Aurora and Nightly?
Comment 17 Martijn Wargers [:mwargers] (not working for Mozilla) 2012-02-07 06:06:18 PST
See also comments in bug 721383, I did bisecting there. I can see this hang/crash on the LG Optimus Black, I haven't see it on other devices. Yes, I'm also seeing it on Aurora and Nightly.
Comment 18 James Willcox (:snorp) (jwillcox@mozilla.com) 2012-02-07 06:34:08 PST
The actual whitelist change is easy enough to back out, but the initialization changes are going to be relatively painful. I would prefer to just figure out what is going on there, instead.

Is the Black the only phone that has this issue?

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