Closed Bug 1164970 Opened 9 years ago Closed 9 years ago

Add failIfMajorPerformanceCaveat attribute to WebGL context

Categories

(Core :: Graphics: CanvasWebGL, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla41
Tracking Status
firefox41 --- fixed

People

(Reporter: kyle_fung, Assigned: kyle_fung, Mentored)

References

Details

(Keywords: dev-doc-complete)

Attachments

(1 file, 6 obsolete files)

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
Assignee: nobody → kfung
Attachment #8605944 - Flags: feedback?(jmuizelaar)
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.
Attachment #8605944 - Flags: feedback?(jmuizelaar) → feedback+
Attachment #8605978 - Flags: review?(jmuizelaar)
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 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
Attachment #8605978 - Flags: review?(jmuizelaar) → review?(jgilbert)
Attachment #8605978 - Flags: review?(jgilbert) → review?(dglastonbury)
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.
Attachment #8605978 - Flags: review?(dglastonbury) → review?(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.
Attachment #8605978 - Flags: review?(jgilbert) → review-
Sorry for the delay, I was working on something else for a bit.
Attachment #8605944 - Attachment is obsolete: true
Attachment #8605978 - Attachment is obsolete: true
Attachment #8608220 - Flags: review?(jgilbert)
Attachment #8608220 - Flags: review?(jgilbert)
Changed access to preferences back to using gfxPrefs as advised by jrmuizel
Attachment #8608220 - Attachment is obsolete: true
Attachment #8608303 - Flags: review?(jgilbert)
Corrected the default value
...Really sorry about the number of emails you're going to get
Attachment #8608303 - Attachment is obsolete: true
Attachment #8608303 - Flags: review?(jgilbert)
Attachment #8608306 - Flags: review?(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.
Attachment #8608306 - Flags: review?(jgilbert) → review+
Shifted variable declaration of gfxInfo to be closer to its use.
Attachment #8608306 - Attachment is obsolete: true
Keywords: checkin-needed
webidl changes require DOM peer review
Keywords: checkin-needed
Flags: needinfo?(ehsan)
Comment on attachment 8608346 [details] [diff] [review]
webgl-failifmajorperformancecaveat-v6.patch

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

r=me on the WebIDL change.
Attachment #8608346 - Flags: review+
Flags: needinfo?(ehsan)
Attachment #8608346 - Attachment is obsolete: true
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/3cbbdeb91bc4
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: