888445 - Only whitelist NVIDIA for SkiaGL canvas

Status: RESOLVED FIXED

(Core :: Graphics: Canvas2D, defect)

Description

[James Willcox (:snorp) ([e-mail]) (he/him)]

We have a few test failures on PVR and Adreno with SkiaGL. For the first landing lets only whitelist NVIDIA and then fix the others later.

Comment 1

[James Willcox (:snorp) ([e-mail]) (he/him)]

Attachment: Only use SkiaGL canvas on NVIDIA

Comment 2

[James Willcox (:snorp) ([e-mail]) (he/him)]

Sorry the patch is so hard to read. The indentation there was messed up and it was driving me insane.

Comment 3

[Benoit Jacob [:bjacob] (mostly away)]

Comment on attachment 769103 [details] [diff] [review]
Only use SkiaGL canvas on NVIDIA

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

It is sad that we have to restrict Skia/GL only to NVIDIA. But another part of me rejoices that this underlines the usefulness of our separate-java-thread-querying-GL-info at startup on android...

::: content/canvas/src/CanvasRenderingContext2D.cpp
@@ +874,5 @@
> +        nsRefPtr<GLContext> glContext;
> +        nsCOMPtr<nsIGfxInfo> gfxInfo = do_GetService("@mozilla.org/gfx/info;1");
> +        nsString vendor;
> +
> +        if (!mForceSoftware && gfxInfo &&

Please move gfxInfo&& to a new line. Also, I don't suppose that gfxInfo is available at all on B2G (I am not aware of it being implemented there). Please confirm that it is intentional that this disables Skia/GL on B2G, and will continue to even if you check for Qualcomm below. We have little or no interest in blacklisting on B2G due to the lower variety of hw and drivers there. So I would rather prefer if you wrote this code so as to skip any blacklist check (i.e. wholesale whitelist) on B2G.

@@ +876,5 @@
> +        nsString vendor;
> +
> +        if (!mForceSoftware && gfxInfo &&
> +            NS_SUCCEEDED(gfxInfo->GetAdapterVendorID(vendor)) &&
> +            StringBeginsWith(vendor, NS_LITERAL_STRING("NVIDIA"))) {

After multi-line if() condition, please move { to a new line

Comment 4

[James Willcox (:snorp) ([e-mail]) (he/him)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8e87e5a44607

Comment 5

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/8e87e5a44607