Closed Bug 1167651 Opened 5 years ago Closed 5 years ago

Support WARP devices with ANGLE in WebGL

Categories

(Core :: Canvas: WebGL, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla42
Tracking Status
firefox42 --- fixed

People

(Reporter: kyle_fung, Assigned: kyle_fung, Mentored)

References

Details

Attachments

(3 files, 18 obsolete files)

13.19 KB, patch
Details | Diff | Splinter Review
1.23 KB, patch
Details | Diff | Splinter Review
11.01 KB, patch
Details | Diff | Splinter Review
No description provided.
Assignee: nobody → kfung
Attached patch webgl-support-warp.patch (obsolete) — Splinter Review
Attachment #8609564 - Flags: feedback?(jmuizelaar)
The patch actually has quite a few mistakes in it. I'll fix those and upload a new one soon. I wouldn't look at this until then.
It seems that once a display has been created, it can't be destroyed unless libEGL.cpp has a new function added to it, so the WARP display cannot be EGL_DEFAULT_DISPLAY as a display has already been made with it. Worked around this by not using EGL_DEFAULT_DISPLAY for WARP and a preprocessor variable to skip the WindowFromDC check in eglGetPlatformDisplayEXT() instead of checking if the display is EGL_DEFAULT_DISPLAY (as in the first version of the patch).
Attachment #8609564 - Attachment is obsolete: true
Attachment #8609564 - Flags: feedback?(jmuizelaar)
Attachment #8609900 - Flags: feedback?(jmuizelaar)
Comment on attachment 8609900 [details] [diff] [review]
Fixed defined names, and turned off WindowFromDC() check with a new define

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

This looks reasonable to me.
Attachment #8609900 - Flags: feedback?(jmuizelaar) → feedback+
Attached patch webgl-support-warp-v3.patch (obsolete) — Splinter Review
Moved GetAndInitWARPDisplay() out of a Windows-only set of code as it is referenced outside of a Windows-only piece of code.

Try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=7c0260901635
Attachment #8609900 - Attachment is obsolete: true
Attached patch webgl-support-warp-v4.patch (obsolete) — Splinter Review
Attachment #8611334 - Attachment is obsolete: true
Attachment #8612893 - Flags: review?(jmuizelaar)
Comment on attachment 8612893 [details] [diff] [review]
webgl-support-warp-v4.patch

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

::: gfx/gl/GLLibraryEGL.cpp
@@ +300,5 @@
> +    //Try to initialize a WARP display first
> +    if (gfxPrefs::WebGLANGLEUseWARP() && gfxPlatform::CanUseDirect3D11ANGLE() &&
> +        IsExtensionSupported(ANGLE_platform_angle_d3d)) {
> +        mEGLDisplay = GetAndInitWARPDisplay(*this,
> +                                            EGL_DEFAULT_DISPLAY);

It look this will create a WARP device first. We actually want to fallback to WARP if the first options fail. Sorry if I caused confusion about the order that should happen.

::: gfx/thebes/gfxPrefs.h
@@ +367,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.angle.use-warp",                  WebGLANGLEUseWARP, bool, false);

This pref should default to true, because we'll only use warp as a fallback.
Attachment #8612893 - Flags: review?(jmuizelaar) → review-
Attached patch webgl-support-warp-v5.patch (obsolete) — Splinter Review
The order of the display creations cannot be reversed as creating EGL_DEFAULT_DISPLAY will not allow the WARP display to be made using EGL_DEFAULT_DISPLAY later.
I've changed the option from use-warp to force-warp, as WARP displays are used as a fallback to the other ANGLE displays, and set its default to false.
Also made changes so that surface sharing is only used when ANGLE and the compositor are both WARP are both not WARP.
Attachment #8612893 - Attachment is obsolete: true
Attachment #8613165 - Flags: review?(jmuizelaar)
Attached patch webgl-support-warp-v6.patch (obsolete) — Splinter Review
Enable surface sharing if WebGL is blacklisted for GPU and moved check for WebGL blacklisting after a WARP device has been attempted to be created.
Attachment #8613165 - Attachment is obsolete: true
Attachment #8613165 - Flags: review?(jmuizelaar)
Attachment #8617587 - Flags: review?(jmuizelaar)
Comment on attachment 8617587 [details] [diff] [review]
webgl-support-warp-v6.patch

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

Will we fail with WARP IfMajorPerformanceCaveat?

Also, it may be worth splitting this patch up into the refactoring pieces and the pieces that actually do the checking and enabling.

::: gfx/gl/GLLibraryEGL.cpp
@@ +311,5 @@
> +#endif
> +    //Fallback to a WARP display if WebGL ANGLE is blacklisted
> +    nsCOMPtr<nsIGfxInfo> gfxInfo = do_GetService("@mozilla.org/gfx/info;1");
> +    if ((!IsAngleSupported(gfxInfo) ||
> +        gfxPrefs::WebGLANGLEForceWARP() && gfxPlatform::CanUseDirect3D11ANGLE()) &&

Shouldn't we require gfxPlatform::CanUseDirect3D11ANGLE to be true for both WebGLANGLEForceWARP and !IsAngleSupported(gfxInfo)?

::: gfx/layers/client/ClientCanvasLayer.cpp
@@ +108,3 @@
>          if (mGLContext->IsANGLE() &&
> +            (angleSupport != nsIGfxInfo::FEATURE_STATUS_OK || gfxPrefs::WebGLANGLEForceWARP())
> +            == gfxPrefs::LayersD3D11ForceWARP() &&

I think this condition is wrong. Shouldn't we be checking if we're actually using WARP instead of if ForceWARP?
Attachment #8617587 - Flags: review?(jmuizelaar) → review-
Depends on: 1182547
Moved some of the query extension code to Bug 1182547
Attached patch enable-warp.patch (obsolete) — Splinter Review
-Changed ANGLE so that eglGetPlatformDisplay will work on EGL_DEFAULT_DISPLAY.
-Added IsWARP() to some classes so that contexts can be checked if they use WARP
-Create a WARP device for EGL only if WEBGL_ANGLE is blacklisted for the system or if WebGL WARP is forced through a pref
-Allow surface sharing only if the compositing device and the WebGL context are both WARP or both not WARP
Attachment #8617587 - Attachment is obsolete: true
Attachment #8632976 - Flags: review?(jgilbert)
Attached patch surface-sharing-check.patch (obsolete) — Splinter Review
Make sure that surface sharing will be checked, as it currently will not be checked in some configurations.
Attachment #8632981 - Flags: review?(bas)
Comment on attachment 8632976 [details] [diff] [review]
enable-warp.patch

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

Cool, put let's not remove prefs by accident!

::: dom/canvas/WebGLContext.cpp
@@ -559,5 @@
>      nsRefPtr<GLContext> gl;
>  
>  #ifdef XP_WIN
> -    if (!forceEnabled &&
> -        IsFeatureInBlacklist(gfxInfo, nsIGfxInfo::FEATURE_WEBGL_ANGLE))

If we're removing this, we *must* include a way to override the blacklist for accelerated (non-WARP) ANGLE.

::: gfx/gl/GLLibraryEGL.cpp
@@ +104,5 @@
> +
> +static EGLDisplay
> +GetAndInitWARPDisplay(GLLibraryEGL& egl, void *displayType)
> +{
> +    EGLint attrib_list[3] = { LOCAL_EGL_PLATFORM_ANGLE_TYPE_ANGLE,

Leave this as [].

@@ +125,5 @@
> +IsAngleSupported(const nsCOMPtr<nsIGfxInfo>& gfxInfo)
> +{
> +    int32_t angleSupport;
> +    gfxInfo->GetFeatureStatus(nsIGfxInfo::FEATURE_WEBGL_ANGLE, &angleSupport);
> +    if (angleSupport == nsIGfxInfo::FEATURE_STATUS_OK) {

You could just return this conditional.

@@ +315,2 @@
>  
> +    //Fallback to a WARP display if WebGL ANGLE is blacklisted or forced

WebGL w/WARP is really just WebGL-ANGLE-D3D-WARP, as opposed to WebGL-ANGLE-D3D-Hardware.
"// Fallback to a WARP display if non-WARP is blacklisted, or if WARP is forced."

@@ +315,5 @@
>  
> +    //Fallback to a WARP display if WebGL ANGLE is blacklisted or forced
> +    if (IsExtensionSupported(ANGLE_platform_angle_d3d) &&
> +        (!angleSupported || gfxPrefs::WebGLANGLEForceWARP()) &&
> +        gfxPlatform::CanUseDirect3D11ANGLE())

I find this is clearer if you have intermediate vars:
bool angleSupported = IsExtensionSupported(ANGLE_platform_angle_d3d);
bool accelAngleSupported = angleSupported && IsAccelAngleSupported(gfxInfo);
bool warpAngleSupported = angleSupported && gfxPlatform::CanUseDirect3D11ANGLE();

bool shouldTryWARP = warpAngleSupported  && !accelAngleSupported;
if (gfxPrefs::WebGLANGLEForceWARP())
    shouldTryWARP = true;

if (shouldTryWARP) {
    ...
}

This makes the override nature of the pref clearer. (yes, logically it's the same)

@@ +337,5 @@
> +        mEGLDisplay = GetAndInitDisplay(*this, EGL_DEFAULT_DISPLAY);
> +
> +        const char* vendor = (char*)fQueryString(mEGLDisplay, LOCAL_EGL_VENDOR);
> +        if (vendor && (strstr(vendor, "TransGaming") != 0 ||
> +                       strstr(vendor, "Google Inc.") != 0))

I don't think this is right, since I think the Android GL emulation is "Google Inc." as well.
Probably just check for IsExtensionSupported(ANGLE_platform_angle_d3d).

@@ +348,4 @@
>  
> +            // D3D11 ANGLE only works with OMTC; there's a bug in the non-OMTC layer
> +            // manager, and it's pointless to try to fix it.  We also don't try
> +            // D3D11 ANGLE if the layer manager is prefering D3D9 (hrm, do we care?)

We only care if we can't share properly with the layer manager. IIRC, we can still share (somewhat poorly) with D3D9Ex if we're using D3D11.
Attachment #8632976 - Flags: review?(jgilbert) → review-
Comment on attachment 8632981 [details] [diff] [review]
surface-sharing-check.patch

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

::: gfx/thebes/gfxWindowsPlatform.cpp
@@ +1943,5 @@
>        gfxCriticalError() << "Exception occurred initializing WARP D3D11 device!";
>        return;
>      }
> +
> +    //Only test for texture sharing on Windows 8 since it puts the device into

nit: Style is space after //
Attachment #8632981 - Flags: review?(bas) → review+
Attached patch enable-warp-v2.patch (obsolete) — Splinter Review
Addressed most review comments except the overriding of forceEnabled. That code will come in another patch.
Attachment #8632976 - Attachment is obsolete: true
Attachment #8633770 - Flags: review?(jgilbert)
Attached patch force-enable.patch (obsolete) — Splinter Review
Addressed issue from review pertaining to forceEnabled.
I have moved the checking and use of the webgl.force-enabled pref to where it's actually used rather than passing it from function to function.
I don't particularly think this is a good solution, but I cannot think of anything else... do you have any suggestions?
Attachment #8633772 - Flags: review?(jgilbert)
Attached patch surface-sharing-check-v2.patch (obsolete) — Splinter Review
Fixed styling issue.
Attachment #8632981 - Attachment is obsolete: true
Comment on attachment 8633770 [details] [diff] [review]
enable-warp-v2.patch

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

Good, though it seems like this shouldn't work on Android. Does it?

::: gfx/gl/GLDefs.h
@@ +51,5 @@
> +// ANGLE_platform_angle_d3d
> +#define LOCAL_EGL_PLATFORM_ANGLE_ANGLE                  0x3201
> +#define LOCAL_EGL_PLATFORM_ANGLE_TYPE_ANGLE             0x3202
> +#define LOCAL_EGL_PLATFORM_ANGLE_TYPE_D3D11_WARP_ANGLE  0x3206
> +#define LOCAL_EGL_NONE                                  0x3038

We already have LOCAL_EGL_NONE in GLConsts.h.

::: gfx/gl/GLLibraryEGL.cpp
@@ +102,5 @@
>      return lib;
>  }
> +
> +static EGLDisplay
> +GetAndInitWARPDisplay(GLLibraryEGL& egl, void *displayType)

Star against type. Also, doesn't this type have an EGL typedef anyway?

@@ +121,5 @@
> +    return display;
> +}
> +
> +static bool
> +IsAngleSupported(const nsCOMPtr<nsIGfxInfo>& gfxInfo)

Fine, though you really only need a pointer to nsIGfxInfo.

@@ +315,4 @@
>      }
>  
> +    bool accelAngleSupport = mIsANGLE && IsAngleSupported(gfxInfo);
> +    bool warpAngleSupport = mIsANGLE && gfxPlatform::CanUseDirect3D11ANGLE();

Thanks, this looks clearer.

IsAngleSupported might be IsAccelAngleSupported, though. We should really update that feature to be explicit. I fear we might be mixing "should we skip all of ANGLE" vs. "should we skip running ANGLE on this particular D3D driver".

Also, this whole area should really be `if (mIsANGLE)`, since we need to support EGL on Android and B2G, and later on Windows but non-ANGLE.

@@ +337,5 @@
> +    // If falling back to WARP did not work, then fail
> +    if (mEGLDisplay == EGL_NO_DISPLAY && !accelAngleSupport) {
> +        NS_ERROR("Fallback WARP ANGLE context failed to initialize.");
> +        return false;
> +    }

`if (mIsANGLE)` should end about here, I think.

::: gfx/gl/GLLibraryEGL.h
@@ +108,5 @@
>      GLLibraryEGL()
>          : mInitialized(false),
>            mEGLLibrary(nullptr),
> +          mIsANGLE(false),
> +          mIsWARP(false)

These should really be `, mFoo(foo)\n, mBar(bar)`, but fine to not fix.

::: gfx/layers/client/ClientCanvasLayer.cpp
@@ +103,2 @@
>          if (mGLContext->IsANGLE() &&
> +            (mGLContext->IsWARP() == gfxWindowsPlatform::GetPlatform()->IsWARP()) &&

Does it really not work across backends? Maybe a better question for :bas.
Attachment #8633770 - Flags: review?(jgilbert) → review-
Comment on attachment 8633772 [details] [diff] [review]
force-enable.patch

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

::: gfx/gl/GLLibraryEGL.cpp
@@ +315,5 @@
>      }
>  
>      bool accelAngleSupport = mIsANGLE && IsAngleSupported(gfxInfo);
>      bool warpAngleSupport = mIsANGLE && gfxPlatform::CanUseDirect3D11ANGLE();
> +    bool forceAccelAngle = Preferences::GetBool("webgl.force-enabled", false);

Yeah, this really comes out of nowhere. Threading the bool through is inelegant, but clear. The WebGL code shouldn't rely on code in a different module hopefully doing pref checking properly.
Attachment #8633772 - Flags: review?(jgilbert) → review-
Attached patch enable-warp-v3.patch (obsolete) — Splinter Review
Fixed most things. Some things I left because I copied them from neighboring code and I'd like to leave it looking consistent.
The client canvas layer using shared surfaces will only work if both the compositing device and the WebGL device are WARP or both not WARP. Otherwise, the surface sharing will fail.
Attachment #8633770 - Attachment is obsolete: true
Attachment #8634285 - Flags: review?(jgilbert)
Attached patch force-enable-v2.patch (obsolete) — Splinter Review
Threaded the bool through until EnsureInitialized(). Set the default value of forceEnabled to false.
Attachment #8633772 - Attachment is obsolete: true
Attachment #8634287 - Flags: review?(jgilbert)
Attachment #8634285 - Flags: review?(jgilbert) → review+
Comment on attachment 8634287 [details] [diff] [review]
force-enable-v2.patch

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

::: gfx/gl/GLLibraryEGL.cpp
@@ +318,1 @@
>          bool shouldTryWARP = warpAngleSupport && !accelAngleSupport;

`forceEnabled` is really `forceAccel`, so we shouldn't try WARP first in this case.

It's not exactly clear whether we should fall back to WARP if forceAccel is on. I think we should fail instead of falling back in this case.

@@ +350,5 @@
> +                    mEGLDisplay = GetAndInitDisplay(*this,
> +                                                    LOCAL_EGL_D3D11_ONLY_DISPLAY_ANGLE);
> +                } else if (gfxPrefs::WebGLANGLETryD3D11() && gfxPlatform::CanUseDirect3D11ANGLE()) {
> +                    mEGLDisplay = GetAndInitDisplay(*this,
> +                                                    LOCAL_EGL_D3D11_ELSE_D3D9_DISPLAY_ANGLE);

This logic should really all be in WebGL code. I'll likely take a run at this later. Let's land this first though.
Attachment #8634287 - Flags: review?(jgilbert) → review-
Attached patch force-enable-v3.patch (obsolete) — Splinter Review
Thanks for the feedback!
In this patch, WARP will not be tried if forceAccel is on.
But this will be overridden if the user sets force-warp to true.

I think this makes sense because the user pref for forcing WARP is webgl.angle.force-warp whereas the user pref for forceAccel is webgl.force-enabled, which doesn't explicitly state what kind of driver to use.

If the user sets webgl.angle.force-warp and also webgl.force-enable I think it will be confusing if their WebGL context fails to initialize.
Attachment #8634287 - Attachment is obsolete: true
Attachment #8635320 - Flags: review?(jgilbert)
Attachment #8635320 - Flags: review?(jgilbert) → review+
Attached patch enable-warp-v4.patch (obsolete) — Splinter Review
Shifted an ifdef down so that Linux and OSX builds can compile, as GetAndInitWARPDisplay and IsAccelAngleSupported were not defined on Linux and OSX systems.
Attachment #8634285 - Attachment is obsolete: true
Attached patch force-enable-v4.patch (obsolete) — Splinter Review
Changed declaration of GLContextProviderCGL::CreateHeadless so that it can compile on OSX since the base CreateHeadless declaration was changed in the previous version of the patch.
Attachment #8635320 - Attachment is obsolete: true
Attached patch surface-sharing-check-v3.patch (obsolete) — Splinter Review
Rebased the patch to work in nightly.
Attachment #8633773 - Attachment is obsolete: true
Attached patch enable-warp-v4-actual-file.patch (obsolete) — Splinter Review
Uploaded the wrong file.
Attachment #8637954 - Attachment is obsolete: true
Try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=256b88df75c8

Patches go in this order:
enable-warp-v4-actual-file.patch
surface-sharing-check-v3.patch
force-enable-v4.patch
Keywords: checkin-needed
These patches need rebasing on top of inbound tip.
Keywords: checkin-needed
Rebased
Attachment #8637962 - Attachment is obsolete: true
Attachment #8637959 - Attachment is obsolete: true
Attachment #8637958 - Attachment is obsolete: true
Keywords: checkin-needed
Depends on: 1188167
You need to log in before you can comment on or make changes to this bug.