Last Comment Bug 668004 - Generalize the downloaded GPU blacklist to non-Windows platforms
: Generalize the downloaded GPU blacklist to non-Windows platforms
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Graphics (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla10
Assigned To: Doug Sherk (:drs) (inactive)
:
Mentors:
Depends on: 700124 700931 801699
Blocks: opengl-mobile 689598 695912 705959 706702
  Show dependency treegraph
 
Reported: 2011-06-28 13:10 PDT by Benoit Jacob [:bjacob] (mostly away)
Modified: 2012-10-16 06:40 PDT (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch v1.0, generalized blocklisting. (46.75 KB, patch)
2011-10-17 20:36 PDT, Doug Sherk (:drs) (inactive)
no flags Details | Diff | Review
Patch v1.0, generalized blocklisting. (46.75 KB, patch)
2011-10-17 20:43 PDT, Doug Sherk (:drs) (inactive)
no flags Details | Diff | Review
Hash model/HW for vendor/deviceid (1.78 KB, patch)
2011-10-18 13:29 PDT, Benoit Girard (:BenWa)
no flags Details | Diff | Review
Android fix, qfold into Patch v1.0 (1000 bytes, patch)
2011-10-20 15:52 PDT, Benoit Girard (:BenWa)
no flags Details | Diff | Review
Patch v1.2, generalized blocklisting. (48.42 KB, patch)
2011-10-27 21:37 PDT, Doug Sherk (:drs) (inactive)
no flags Details | Diff | Review
Patch v1.3, generalized blocklisting. (48.58 KB, patch)
2011-10-28 13:58 PDT, Doug Sherk (:drs) (inactive)
jacob.benoit.1: review-
Details | Diff | Review
Patch v1.4, generalized blocklisting. (48.66 KB, patch)
2011-10-31 20:27 PDT, Doug Sherk (:drs) (inactive)
jacob.benoit.1: review-
Details | Diff | Review
Patch v1.5, generalized blocklisting. (48.66 KB, patch)
2011-11-01 13:19 PDT, Doug Sherk (:drs) (inactive)
no flags Details | Diff | Review
Patch v1.5, generalized blocklisting. (48.57 KB, patch)
2011-11-01 13:21 PDT, Doug Sherk (:drs) (inactive)
jacob.benoit.1: review+
Details | Diff | Review
dont return ERROR_FAILURE in GetFeatureStatusImpl (1.27 KB, patch)
2011-11-05 12:16 PDT, Benoit Jacob [:bjacob] (mostly away)
bugzilla: review+
Details | Diff | Review
change false to NO_INFO and add FIXME comment (2.27 KB, patch)
2011-11-05 12:23 PDT, Benoit Jacob [:bjacob] (mostly away)
bugzilla: review-
Details | Diff | Review

Description Benoit Jacob [:bjacob] (mostly away) 2011-06-28 13:10:25 PDT
Since bug 625160, we use a downloaded GPU blacklist (downloaded as part of the addons blacklist, blocklist.xml) in addition to the built-in GPU blacklist.

But we only do that on Windows.

We need the same on Mac and on X11.
Comment 1 Benoit Girard (:BenWa) 2011-10-07 22:27:24 PDT
Blocking mobile OGL layers.
Comment 2 Doug Sherk (:drs) (inactive) 2011-10-17 20:36:03 PDT
Created attachment 567663 [details] [diff] [review]
Patch v1.0, generalized blocklisting.

Initial patch to generalize blocklisting. Confirmed to work on OSX and Linux. Might not build on Android. If this compiles correctly on Android, it should almost certainly work first shot.
Comment 3 Doug Sherk (:drs) (inactive) 2011-10-17 20:43:50 PDT
Created attachment 567666 [details] [diff] [review]
Patch v1.0, generalized blocklisting.

Whoops, left something in the other one.
Comment 4 Benoit Girard (:BenWa) 2011-10-18 13:12:25 PDT
We're going to need to implement the following for Android:
http://mxr.mozilla.org/mozilla-central/source/widget/src/android/GfxInfo.cpp#154
http://mxr.mozilla.org/mozilla-central/source/widget/src/android/GfxInfo.cpp#184
http://mxr.mozilla.org/mozilla-central/source/widget/src/android/GfxInfo.cpp#199

Since VendorID/DeviceID are strings on android we discussed hashing them. Sound good to everyone?
Comment 5 Doug Sherk (:drs) (inactive) 2011-10-18 13:17:35 PDT
Sounds good to me.
Comment 6 Benoit Girard (:BenWa) 2011-10-18 13:29:51 PDT
Created attachment 567856 [details] [diff] [review]
Hash model/HW for vendor/deviceid

Feel free to fold this patch into yours
Comment 7 Doug Sherk (:drs) (inactive) 2011-10-18 13:31:40 PDT
Have you tested to see if these two patches together work on Android?
Comment 8 Benoit Girard (:BenWa) 2011-10-18 13:35:30 PDT
No I have not. How do I test this feature?
Comment 9 Doug Sherk (:drs) (inactive) 2011-10-18 13:40:24 PDT
This wiki article gives instructions that work for Windows, and some of what you need for Linux/Mac:
https://wiki.mozilla.org/Blocklisting/Testing

It seems like on Android the instructions should be pretty similar. You can change the blocklist URL to a local file to make it easier. For this, you can use browser/app/blocklist.xml. Add an entry that matches up with your device and check if the feature gets disabled.

The only thing I don't know about is how to get to the error console on Android, and whether or not downloading the blocklist is implemented on it at all.
Comment 10 Martijn Wargers [:mwargers] (gone per 2016-05-31 :-( ) 2011-10-18 13:43:25 PDT
In about:config, you can set the devtools.errorconsole.enabled pref to true to let the error console show up in the preferences.

Whether or not downloading the blocklist is implemented on it at all, perhaps Mark or Matt can answer that question.
Comment 11 Benoit Girard (:BenWa) 2011-10-20 15:52:20 PDT
Created attachment 568551 [details] [diff] [review]
Android fix, qfold into Patch v1.0

Simple fix to compile on android
Comment 12 Doug Sherk (:drs) (inactive) 2011-10-27 21:37:00 PDT
Created attachment 570181 [details] [diff] [review]
Patch v1.2, generalized blocklisting.

Folded BenWa's patches in and cleared up some bitrot.
Comment 13 Doug Sherk (:drs) (inactive) 2011-10-27 21:41:05 PDT
Running on Try: https://tbpl.mozilla.org/?tree=Try&rev=20a50709f71c
Comment 14 Benoit Girard (:BenWa) 2011-10-27 21:42:45 PDT
I worked on testing this out on Android. I wrote a patch to give proper Device/Driver version in GfxInfo.cpp but I couldn't get the blocklist to update by changing the blocklist URI to the sdcard or changing the default blocklist.xml and clearing the profile.
Comment 15 Doug Sherk (:drs) (inactive) 2011-10-27 21:44:14 PDT
Ok, I'll get myself setup with an Android dev env tomorrow so I can try and fix it.
Comment 16 Doug Sherk (:drs) (inactive) 2011-10-28 13:58:54 PDT
Created attachment 570354 [details] [diff] [review]
Patch v1.3, generalized blocklisting.

Fixed driver suggestion to only happen on Windows.
Comment 17 Benoit Jacob [:bjacob] (mostly away) 2011-10-28 15:08:38 PDT
Comment on attachment 570354 [details] [diff] [review]
Patch v1.3, generalized blocklisting.

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

The design of all this seems to be made more complex by the fact that NO_INFO has acquired a new second meaning, in addition to its original meaning.

The original meaning of NO_INFO is "no bad news", "no reason to blacklist, so go ahead, use the feature".

The new meaning of NO_INFO in this patch seems to be "we don't know yet".

I won't r- the patch for that but we need a follow-up bug for solving this problem. I think we should split NO_INFO into two separate enum values, one of each of these two meanings: "don't know yet" and "do know that the feature is OK to use".

::: widget/src/android/GfxInfo.cpp
@@ +188,5 @@
> +  if (mozilla::AndroidBridge::Bridge()->GetStaticStringField("android/os/Build", "HARDWARE", str)) {
> +    *aAdapterVendorID = HashString(str);
> +    return NS_OK;
> +  }
> +

We don't want to coerce Android to return PCI-vendor-id-style values. Different OSes need to have different fields.

Leave GetAdapterVendorID unchanged and add/edit a different function that returns a string.

@@ +193,2 @@
>    *aAdapterVendorID = 0;
> +  return NS_ERROR_FAILURE;

Return NS_OK here like before. NS_ERROR_FAILURE would throw an exception in the about:support page, breaking it.

@@ +209,5 @@
> +  if (mozilla::AndroidBridge::Bridge()->GetStaticStringField("android/os/Build", "MODEL", str)) {
> +    *aAdapterDeviceID = HashString(str);
> +    return NS_OK;
> +  }
> +

same.

@@ +214,2 @@
>    *aAdapterDeviceID = 0;
> +  return NS_ERROR_FAILURE;

same.

::: widget/src/cocoa/GfxInfo.mm
@@ +55,5 @@
>  #include "nsICrashReporter.h"
>  #define NS_CRASHREPORTER_CONTRACTID "@mozilla.org/toolkit/crash-reporter;1"
>  #endif
>  
> +#define MAC_OS_X_VERSION_MASK       0x0000FFFF // Not supported

what does not supported mean here?

::: widget/src/xpwidgets/GfxDriverInfo.cpp
@@ +41,5 @@
>  using namespace mozilla::widget;
>  
>  PRUint32 GfxDriverInfo::allAdapterVendors = 0;
>  PRInt32 GfxDriverInfo::allFeatures = 0;
> +PRUint64 GfxDriverInfo::allDriverVersions = 0xffffffffffffffffULL;

~(PRUint64(0)) ?
~(PRUint64)0 ?

(long bikeshed discussion ensued)

::: widget/src/xpwidgets/GfxInfoBase.cpp
@@ +585,5 @@
> +  OperatingSystem os = DRIVER_OS_UNKNOWN;
> +  if (aOS)
> +    os = *aOS;
> +
> +  PRUint32 adapterVendor = 0;

ID
Comment 18 Doug Sherk (:drs) (inactive) 2011-10-31 20:27:42 PDT
Created attachment 570919 [details] [diff] [review]
Patch v1.4, generalized blocklisting.

Dealt with code review comments. We decided to stall dealing with Android because it'll be an expensive refactor and we want to get Mac blocklisting out ASAP. I have instead disabled downloaded blocklisting on Android, although BenWa reports that it doesn't work at all to begin with. 

I don't like avoiding suggestions like the NO_INFO comment, which I agree with, but I'll include changes for that in a follow-up patch so that we can land this patch alone without worrying about big refactors.
Comment 19 Benoit Jacob [:bjacob] (mostly away) 2011-11-01 13:06:01 PDT
Comment on attachment 570919 [details] [diff] [review]
Patch v1.4, generalized blocklisting.

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

One of my earlier review comments is not addressed:

::: widget/src/android/GfxInfo.cpp
@@ +193,2 @@
>    *aAdapterVendorID = 0;
> +  return NS_ERROR_FAILURE;

In my previous review I asked for this to remain NS_OK
Comment 20 Doug Sherk (:drs) (inactive) 2011-11-01 13:19:27 PDT
Created attachment 571124 [details] [diff] [review]
Patch v1.5, generalized blocklisting.

Fixed missed code review comment.
Comment 21 Doug Sherk (:drs) (inactive) 2011-11-01 13:21:00 PDT
Created attachment 571126 [details] [diff] [review]
Patch v1.5, generalized blocklisting.
Comment 22 Benoit Jacob [:bjacob] (mostly away) 2011-11-03 07:57:33 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/1f1d6394148c
Comment 23 Jeff Muizelaar [:jrmuizel] 2011-11-03 07:58:31 PDT
The stuff that landed still has Android "pretend to be PCI" code. Was that intentional?
Comment 24 Doug Sherk (:drs) (inactive) 2011-11-03 12:09:41 PDT
(In reply to Jeff Muizelaar [:jrmuizel] from comment #23)
> The stuff that landed still has Android "pretend to be PCI" code. Was that
> intentional?

It's disabled right now. I suppose it would have been better to just remove it, but it should have no impact.
Comment 25 Ed Morley [:emorley] 2011-11-03 13:09:04 PDT
https://hg.mozilla.org/mozilla-central/rev/1f1d6394148c
Comment 26 Benoit Jacob [:bjacob] (mostly away) 2011-11-05 12:16:03 PDT
Created attachment 572221 [details] [diff] [review]
dont return ERROR_FAILURE in GetFeatureStatusImpl

WebGL anti-aliasing is broken on linux, and can't even be force-enabled, because GetFeatureStatusImpl returns NS_ERROR_FAILURE. Looking at the code, it's trying to parse a driver version, which I believe only works on Windows, then returns NS_ERROR_FAILURE except on Mac, which I believe is the only reason why it's not broken on Mac.

GetFeatureStatusImpl should never return NS_ERROR_FAILURE, I believe. Regardless of how we structure C++ code in WebGLContext.cpp, GetFeatureStatus returning NS_ERROR_FAILURE would mean an exception thrown in about:support, breaking it. We already discussed that we don't want it to throw: bug 651981, bug 642502 comment 13.
Comment 27 Benoit Jacob [:bjacob] (mostly away) 2011-11-05 12:23:57 PDT
Created attachment 572222 [details] [diff] [review]
change false to NO_INFO and add FIXME comment

I really don't know what that aValue = false; was doing here, especially as aValue is a PRInt32 and its possible values are the FEATURE symbolic constants.

GetFeatureStatusImpl relies on aValue getting set to FEATURE_NO_INFO if we don't know anything. That's a new second meaning of FEATURE_NO_INFO so this patch adds a big FIXME comment about that.

Moreover, if GetPrefNameForFeature fails, or if Preferences::GetInt fails, aValue didn't get correctly set to FEATURE_NO_INFO, which would have broken GetFeatureStatusImpl. This patch fixes that.
Comment 28 Benoit Jacob [:bjacob] (mostly away) 2011-11-05 13:49:30 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/cf8b201e8980
Comment 29 Doug Sherk (:drs) (inactive) 2011-11-05 13:58:01 PDT
Comment on attachment 572222 [details] [diff] [review]
change false to NO_INFO and add FIXME comment

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

I'm not really comfortable with landing this, especially if it doesn't fix anything that is known to be broken. I'm thinking that the functionality of this patch would be useful in the case where you want to assume something is blocked, then run GetPrefValueForFeature to confirm it. 

If we want to land this, we should at least fix the comment above the function.

::: widget/src/xpwidgets/GfxInfoBase.cpp
@@ +178,5 @@
> +  // GetFeatureStatusImpl.
> +  // TODO: dissociate these two meanings into two different symbolic values; clean up / rationaliz
> +  // this logic
> +  aValue = nsIGfxInfo::FEATURE_NO_INFO;
> +

So the line directly above this function reads "// If the pref doesn't exist, aValue is not touched, and returns false." This new functionality breaks this.

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