Generalize the downloaded GPU blacklist to non-Windows platforms

RESOLVED FIXED in mozilla10

Status

()

Core
Graphics
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: bjacob, Assigned: drs)

Tracking

unspecified
mozilla10
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 8 obsolete attachments)

48.57 KB, patch
bjacob
: review+
Details | Diff | Splinter Review
1.27 KB, patch
drs
: review+
Details | Diff | Splinter Review
2.27 KB, patch
drs
: review-
Details | Diff | Splinter Review
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.
(Reporter)

Updated

6 years ago
Blocks: 668008
Blocking mobile OGL layers.
Blocks: 607684
(Assignee)

Comment 2

6 years ago
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.
Assignee: nobody → dsherk
Attachment #567663 - Flags: review?(joe)
(Assignee)

Updated

6 years ago
Attachment #567663 - Flags: review?(joe) → review?(bjacob)
(Assignee)

Comment 3

6 years ago
Created attachment 567666 [details] [diff] [review]
Patch v1.0, generalized blocklisting.

Whoops, left something in the other one.
Attachment #567663 - Attachment is obsolete: true
Attachment #567666 - Flags: review?(bjacob)
Attachment #567663 - Flags: review?(bjacob)
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?
(Assignee)

Comment 5

6 years ago
Sounds good to me.
Created attachment 567856 [details] [diff] [review]
Hash model/HW for vendor/deviceid

Feel free to fold this patch into yours
(Assignee)

Comment 7

6 years ago
Have you tested to see if these two patches together work on Android?
No I have not. How do I test this feature?
(Assignee)

Comment 9

6 years ago
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.
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.
Created attachment 568551 [details] [diff] [review]
Android fix, qfold into Patch v1.0

Simple fix to compile on android
(Assignee)

Updated

6 years ago
Blocks: 695912
(Assignee)

Comment 12

6 years ago
Created attachment 570181 [details] [diff] [review]
Patch v1.2, generalized blocklisting.

Folded BenWa's patches in and cleared up some bitrot.
Attachment #567666 - Attachment is obsolete: true
Attachment #567856 - Attachment is obsolete: true
Attachment #568551 - Attachment is obsolete: true
Attachment #570181 - Flags: review?(bjacob)
Attachment #567666 - Flags: review?(bjacob)
(Assignee)

Comment 13

6 years ago
Running on Try: https://tbpl.mozilla.org/?tree=Try&rev=20a50709f71c
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.
(Assignee)

Comment 15

6 years ago
Ok, I'll get myself setup with an Android dev env tomorrow so I can try and fix it.
(Assignee)

Comment 16

6 years ago
Created attachment 570354 [details] [diff] [review]
Patch v1.3, generalized blocklisting.

Fixed driver suggestion to only happen on Windows.
Attachment #570181 - Attachment is obsolete: true
Attachment #570354 - Flags: review?(bjacob)
Attachment #570181 - Flags: review?(bjacob)

Updated

6 years ago
Blocks: 689598
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
Attachment #570354 - Flags: review?(bjacob) → review-
(Assignee)

Comment 18

6 years ago
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.
Attachment #570354 - Attachment is obsolete: true
Attachment #570919 - Flags: review?(bjacob)
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
Attachment #570919 - Flags: review?(bjacob) → review-
(Assignee)

Comment 20

6 years ago
Created attachment 571124 [details] [diff] [review]
Patch v1.5, generalized blocklisting.

Fixed missed code review comment.
Attachment #570919 - Attachment is obsolete: true
Attachment #571124 - Flags: review?(bjacob)
(Assignee)

Comment 21

6 years ago
Created attachment 571126 [details] [diff] [review]
Patch v1.5, generalized blocklisting.
Attachment #571124 - Attachment is obsolete: true
Attachment #571126 - Flags: review?(bjacob)
Attachment #571124 - Flags: review?(bjacob)
(Reporter)

Updated

6 years ago
Attachment #571126 - Flags: review?(bjacob) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/1f1d6394148c
The stuff that landed still has Android "pretend to be PCI" code. Was that intentional?
(Assignee)

Comment 24

6 years ago
(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.
https://hg.mozilla.org/mozilla-central/rev/1f1d6394148c
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla10
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.
Attachment #572221 - Flags: review?(dsherk)
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.
Attachment #572222 - Flags: review?(dsherk)
(Assignee)

Updated

6 years ago
Attachment #572221 - Flags: review?(dsherk) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/cf8b201e8980
(Assignee)

Comment 29

6 years ago
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.
Attachment #572222 - Flags: review?(dsherk) → review-
(In reply to Benoit Jacob [:bjacob] from comment #28)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/cf8b201e8980

https://hg.mozilla.org/mozilla-central/rev/cf8b201e8980

Updated

6 years ago
Depends on: 700124

Updated

6 years ago
Depends on: 700931
(Assignee)

Updated

6 years ago
Blocks: 705959
(Assignee)

Updated

6 years ago
No longer blocks: 668008
(Assignee)

Updated

6 years ago
Blocks: 706702

Updated

5 years ago
Depends on: 801699
You need to log in before you can comment on or make changes to this bug.