1164970 - Add failIfMajorPerformanceCaveat attribute to WebGL context

Status: RESOLVED FIXED

(Core :: Graphics: CanvasWebGL, defect)

Description

[Kyle Fung]

Add the attribute failIfMajorPerformanceCaveat to the WebGL context, and have context creation fail when layer acceleration is not supported.

Attribute specification:
https://www.khronos.org/registry/webgl/specs/1.0/#5.2

Comment 1

[Kyle Fung]

Attachment: webgl-failifmajorperformancecaveat.patch

Comment 2

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 8605944 [details] [diff] [review]
webgl-failifmajorperformancecaveat.patch

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

It would be good to also add a pref to disable this behaviour. See gfxPrefs.h for how to do that.

::: dom/canvas/WebGLContext.cpp
@@ +502,5 @@
>      return status != nsIGfxInfo::FEATURE_STATUS_OK;
>  }
>  
> +static bool
> +LayerAccelerationSupport(const nsCOMPtr<nsIGfxInfo>& gfxInfo)

I'd call this HasAcceleratedLayers()

@@ +609,5 @@
>      bool requireCompatProfile = webgl->IsWebGL2() ? false : true;
>  
>      nsRefPtr<GLContext> gl;
>  
> +    if (!layerSupport) {

I'd just call the function directly here.

Comment 3

[Kyle Fung]

Attachment: webgl-failifmajorperformancecaveat-v2.patch

Comment 4

[Kyle Fung]

Comment on attachment 8605978 [details] [diff] [review]
webgl-failifmajorperformancecaveat-v2.patch

I set 'true' as the default value. Not entirely sure if this is appropriate but I think it makes sense.

Comment 5

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 8605978 [details] [diff] [review]
webgl-failifmajorperformancecaveat-v2.patch

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

Jeff Gilbert is a better candidate for review than I am.

::: gfx/thebes/gfxPrefs.h
@@ +368,5 @@
>    DECL_GFX_PREF(Live, "ui.click_hold_context_menus.delay",     UiClickHoldContextMenusDelay, int32_t, 500);
>    DECL_GFX_PREF(Once, "webgl.angle.force-d3d11",               WebGLANGLEForceD3D11, bool, false);
>    DECL_GFX_PREF(Once, "webgl.angle.try-d3d11",                 WebGLANGLETryD3D11, bool, false);
> +  DECL_GFX_PREF(Once, "webgl.disable-failIfMajorPerformanceCaveat",
> +                WebGLDisableFailIfMajorPerformanceCaveat, bool, true);

This should default to false. It might be better to not use camelcase as I don't think there are other prefs that do.

You should also add a entry to modules/libpref/init/all.js

Comment 6

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8605978 [details] [diff] [review]
webgl-failifmajorperformancecaveat-v2.patch

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

::: dom/canvas/WebGLContext.cpp
@@ +613,5 @@
> +        !HasAcceleratedLayers(gfxInfo)) {
> +      Nullable<dom::WebGLContextAttributes> contextAttributes;
> +      webgl->GetContextAttributes(contextAttributes);
> +      if (contextAttributes.Value().mFailIfMajorPerformanceCaveat)
> +          return nullptr;

This code should not be buried in this function. It should be near the webgl.disabled code. Otherwise it may appear that creating a GL context fails, when it's really that we've chosen not to allow context creation when we lack accel layers.

Comment 7

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8605978 [details] [diff] [review]
webgl-failifmajorperformancecaveat-v2.patch

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

Please address the two review comments.

Comment 8

[Kyle Fung]

Attachment: webgl-failifmajorperformancecaveat-v3.patch

Sorry for the delay, I was working on something else for a bit.

Comment 9

[Kyle Fung]

Attachment: webgl-failifmajorperformancecaveat-v4.patch

Changed access to preferences back to using gfxPrefs as advised by jrmuizel

Comment 10

[Kyle Fung]

Attachment: webgl-failifmajorperformancecaveat-v5.patch

Corrected the default value
...Really sorry about the number of emails you're going to get

Comment 11

[Kyle Fung]

Try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=104ed9b61c84

Comment 12

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8608306 [details] [diff] [review]
webgl-failifmajorperformancecaveat-v5.patch

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

Nit.

::: dom/canvas/WebGLContext.cpp
@@ +890,5 @@
>      }
>  
>      // Get some prefs for some preferred/overriden things
>      NS_ENSURE_TRUE(Preferences::GetRootBranch(), NS_ERROR_FAILURE);
> +    nsCOMPtr<nsIGfxInfo> gfxInfo = do_GetService("@mozilla.org/gfx/info;1");

Why is this above the disabled block? Keep var decls close to their use.

Comment 13

[Kyle Fung]

Attachment: webgl-failifmajorperformancecaveat-v6.patch

Shifted variable declaration of gfxInfo to be closer to its use.

Comment 14

[Ryan VanderMeulen (PTO, back July 27)]

webidl changes require DOM peer review

Comment 15

[(no longer active)]

Comment on attachment 8608346 [details] [diff] [review]
webgl-failifmajorperformancecaveat-v6.patch

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

r=me on the WebIDL change.

Comment 16

[Kyle Fung]

Attachment: webgl-failifmajorperformancecaveat-v7.patch

Comment 17

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3cbbdeb91bc4

Comment 18

[Ekanan Ketunuti]

https://hg.mozilla.org/mozilla-central/rev/3cbbdeb91bc4

Comment 20

[Florian Scholz (Open Web Docs)]

https://developer.mozilla.org/en-US/Firefox/Releases/41#WebGL
https://developer.mozilla.org/en-US/docs/Web/API/HTMLCanvasElement/getContext
https://developer.mozilla.org/en-US/docs/Web/API/WebGLRenderingContext/getContextAttributes