Closed Bug 668004 Opened 10 years ago Closed 10 years ago

Generalize the downloaded GPU blacklist to non-Windows platforms

Categories

(Core :: Graphics, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla10

People

(Reporter: bjacob, Assigned: drs)

References

Details

Attachments

(3 files, 8 obsolete files)

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.
Blocks: 668008
Blocking mobile OGL layers.
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)
Attachment #567663 - Flags: review?(joe) → review?(bjacob)
Whoops, left something in the other one.
Attachment #567663 - Attachment is obsolete: true
Attachment #567666 - Flags: review?(bjacob)
Attachment #567663 - Flags: review?(bjacob)
Sounds good to me.
Feel free to fold this patch into yours
Have you tested to see if these two patches together work on Android?
No I have not. How do I test this feature?
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.
Simple fix to compile on android
Blocks: 695912
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)
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.
Ok, I'll get myself setup with an Android dev env tomorrow so I can try and fix it.
Fixed driver suggestion to only happen on Windows.
Attachment #570181 - Attachment is obsolete: true
Attachment #570354 - Flags: review?(bjacob)
Attachment #570181 - Flags: review?(bjacob)
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-
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-
Fixed missed code review comment.
Attachment #570919 - Attachment is obsolete: true
Attachment #571124 - Flags: review?(bjacob)
Attachment #571124 - Attachment is obsolete: true
Attachment #571126 - Flags: review?(bjacob)
Attachment #571124 - Flags: review?(bjacob)
Attachment #571126 - Flags: review?(bjacob) → review+
The stuff that landed still has Android "pretend to be PCI" code. Was that intentional?
(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
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla10
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)
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)
Attachment #572221 - Flags: review?(dsherk) → review+
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-
Depends on: 700124
Depends on: 700931
Blocks: 705959
No longer blocks: 668008
Blocks: 706702
Depends on: 801699
You need to log in before you can comment on or make changes to this bug.