618898 - (dxgl) use NVX d3d-gl interop to get faster WebGL rendering

Status: RESOLVED FIXED

(Core :: Graphics: CanvasWebGL, defect)

Description

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Attachment: use nvx dx-gl interop for webgl

Recent nvidia drivers support OpenGL-D3D object interop.  This can significantly speed up WebGL, especially at large canvas sizes, by removing the ReadPixels & subsequent texture upload.

This could work fine on both d3d9 and d3d10, but the initial patch just makes it work with d3d10 layers.  Additional work for d3d9 layers is needed, but is relatively simple (just needs a texture created from our global device).

The way this patch works is to create a D3D9 device solely so that we can create a D3D9 Texture to interop with, because the NVX interop currently supports d3d9 only.  We create that texture with a share handle, then we open it from d3d10 so that we can use it from there.  It's a bit roundabout, but seems to work quite well (and I've confirmed that the dobule-interop of d3d10<->d3d9<->gl should work fine with NV).

The patch is built on top of bug 618892, but doesn't really depend on it.

Asking bas for some feedback on the d3d9 device manager changes.

Comment 1

[Bas Schouten (:bas.schouten)]

Comment on attachment 497306 [details] [diff] [review]
use nvx dx-gl interop for webgl

I think in theory this looks fine.

I wonder how this performs performance-wise compared to ANGLE on D3D9 though?

It seems D3D9 gives us some added security benefits in the sense that the HLSL compiler seems a lot more robust than some of the GLSL compiler that lives in the graphics UMD. Additionally the HLSL compiler can in the future be run inside the content process away from the graphics UMD which presumably will always require privileges.

Additionally the bytecode coming from the HLSL compiler goes through another extensive validation pass inside D3D9 (I played with this last week and it seems practically impossible to do anything evil to it even if you're injecting evil bytecode sequences directly).

It seems like until NVidia has done security testing on their GLSL compiler (which they told us they hadn't I believe last time they spoke to us) this provides us with some nice added security barriers after the ANGLE validator.

Having said that if using the GL NVidia driver directly is faster we should definitely do that.

I also wonder how this all works with the Optimus architecture.

Comment 2

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

(In reply to comment #1)
> Comment on attachment 497306 [details] [diff] [review]
> use nvx dx-gl interop for webgl
> 
> I think in theory this looks fine.
> 
> I wonder how this performs performance-wise compared to ANGLE on D3D9 though?

Tons faster.  There are some issues in ANGLE in certain situations -- especially since it often has to convert GL vertex arrays to D3D streams/structs, which often involves lots of copying.  Once that's fixed (they're working on it), we can see where things are at -- as you say, there are advantages to always going through ANGLE.  But the perf difference is staggering in some cases (70+fps -> 17fps on 1920x1200 aquarium benchmark from nvx interop -> angle/pbuffer interup).

> It seems D3D9 gives us some added security benefits in the sense that the HLSL
> compiler seems a lot more robust than some of the GLSL compiler that lives in
> the graphics UMD. Additionally the HLSL compiler can in the future be run
> inside the content process away from the graphics UMD which presumably will
> always require privileges.

We can (will?) be able to do that with GL actually, since there is the possibility of doing separate compile-to-bytecode and load-bytecode steps with GL.

Comment 3

[Harsh86]

Is this intended for Firefox 4.0?

Comment 4

[tenzoKritz]

Since the patch already has r+ and the fact that Fx5 is turning out to be performance tuneup release, and light on new features. Wouldn't this patch be good for Fx5? The window for Fx5 will close soon so, better earlier than later.

Comment 5

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

Thanks for the heads up, it seems that this was at risk of getting forgotten. CC'ing some people to limit the risk that that happens.

Comment 6

[Harsh86]

The patch doesn't apply cleanly anymore. Only one line fails though, easy to fix.

Also the patch doesn't work, because there's a typo in the code.
This line:
{ (PRFuncPtr*) &fDXUnlockObjectsNVX, { "wglDXUnlockObjectNVX", NULL } },

should be:
{ (PRFuncPtr*) &fDXUnlockObjectsNVX, { "wglDXUnlockObjectsNVX", NULL } },

Otherwise the interop doesn't work.

Also the extension isn't experimental any more, so you can probably drop the NVX for NV

Comment 7

[NVD]

In the latest Nvidia 275.xx beta drivers(WHQL and final release soon), there's a newer Nvidia GL-DX interop extension, WGL_NV_DX_interop2. Perhaps Mozilla could use the newer extension in a future Firefox release.

http://www.nvidia.com/object/winxp-275.27-beta-driver.html

http://www.nvidia.com/object/win7-winvista-64bit-275.27-beta-driver.html

http://www.nvidia.com/object/notebook-win7-winvista-64bit-275.27-beta-driver.html

Comment 9

[Kelsey Gilbert [:jgilbert]]

WGL_NS_DX_interop2 just expands the initial extension to handle D3D10/11.
It is available here: http://www.opengl.org/registry/specs/NV/DX_interop2.txt

Comment 10

[Kelsey Gilbert [:jgilbert]]

Attachment: Add support for d3d10 path for comparison testing.

This is built on bug 716859.

Performance on WebGL pasta is ~31-32 fps with ANGLE, ~30-32 fps with native GL and the interop.

Comment 11

[Kelsey Gilbert [:jgilbert]]

Asking for feedback from Bas about SharedSurface_D3D10Interop::Create, and making sure it's doing reasonable things in d3d land.

Comment 12

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Hm, if it's only hitting 30fps, you may want another benchmark... at least something that was hitting 60fps on either case to make sure it still hits 60fps.  I just realized actually, I want to add a target fps to my rAF update patch (though maybe the pref is enough?) so that we can do a target fps of 200fps or 1000fps so we can get the max rate and compare things that way.

Comment 13

[Kelsey Gilbert [:jgilbert]]

I don't think we should ever benchmark something that's flirting with the 60fps cap when the two are so close together. If we remove the cap (or shift it to something sufficiently high), then we can test whatever we want.

It's true though, that to hit only 30fps on my machine means it'll be so GPU bound as to hide anything that's not really quite bad. (like readback)

If we can get a pref for max-fps, that'd be ideal. It's a prevalent configuration dimension in gaming.

Comment 14

[Bas Schouten (:bas.schouten)]

(In reply to Jeff Gilbert [:jgilbert] from comment #10)
> Created attachment 685397 [details] [diff] [review]
> Add support for d3d10 path for comparison testing.
> 
> This is built on bug 716859.
> 
> Performance on WebGL pasta is ~31-32 fps with ANGLE, ~30-32 fps with native
> GL and the interop.

Are you saying it's -not- faster with Native GL?

Comment 15

[Kelsey Gilbert [:jgilbert]]

(In reply to Bas Schouten (:bas.schouten) from comment #14)
> (In reply to Jeff Gilbert [:jgilbert] from comment #10)
> > Created attachment 685397 [details] [diff] [review]
> > Add support for d3d10 path for comparison testing.
> > 
> > This is built on bug 716859.
> > 
> > Performance on WebGL pasta is ~31-32 fps with ANGLE, ~30-32 fps with native
> > GL and the interop.
> 
> Are you saying it's -not- faster with Native GL?

Correct, though I'm running a old 275.33 nvidia driver. For this demo, at least.

Comment 16

[Kelsey Gilbert [:jgilbert]]

It's probably also notable that this is a Quadro, not a GeForce.

Comment 17

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

This will really only be a big win on XP, too -- I mean with ANGLE on your d3d10 machine, you're not doing readback, right?  Or did you force readback to happen?

Also, if you set layout.frame_rate to something like 200 and restart, rAF will try to hit 200 fps.  But some of the frameworks do internal rate limiting as well, so you still might not see faster than 60fps.  three.js I think limits to 60fps, though you can probably hack that out in the source.. not sure about others.  Just depends on finding some content with a fps display that's frame rate limited.

Comment 18

[Kelsey Gilbert [:jgilbert]]

On WebGL Aquarium, layout.frame_rate=200 yields about 103 fps with ANGLE and about 110fps with GL. My lock code here seems messed up, though, and results in weird rendering issues when the normal stream of webgl rendering is interrupted. Changing tabs seems to lock at least one of the buffers such that it's never updated again, for example.

Comment 19

[Kelsey Gilbert [:jgilbert]]

(In reply to Vladimir Vukicevic [:vlad] [:vladv] from comment #17)
> This will really only be a big win on XP, too -- I mean with ANGLE on your
> d3d10 machine, you're not doing readback, right?  Or did you force readback
> to happen?
Yes, doing the d3d9 version of this will win us full acceleration on XP.
With ANGLE, we're using ANGLE's d3d share handle extension, not readback.
I can force readback to happen, but that tanks framerate, as we know.

Comment 20

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

(In reply to Jeff Gilbert [:jgilbert] from comment #19)
> Yes, doing the d3d9 version of this will win us full acceleration on XP.
> With ANGLE, we're using ANGLE's d3d share handle extension, not readback.
> I can force readback to happen, but that tanks framerate, as we know.

Right; the main driver for this is really XP, though being able to optionally skip ANGLE is nice (though it might lead to stability issues given the experimental extension, we'll see!).  So I suppose seeing the same perf between ANGLE and OpenGL on d3d10 systems is a good sign -- means that GL/NVX is as efficient (or slightly more) than going through ANGLE.

Comment 21

[NVD]

Nvidia has released GeForce 310.70 WHQL drivers. It has significant amount of new GL_OES extensions.

http://www.nvidia.com/object/winxp-310.70-whql-driver.html

http://www.nvidia.com/object/win8-win7-winvista-64bit-310.70-whql-driver.html

http://www.nvidia.com/object/notebook-win8-win7-winvista-64bit-310.70-whql-driver.html

Comment 22

[Kelsey Gilbert [:jgilbert]]

(In reply to NVD from comment #21)
> Nvidia has released GeForce 310.70 WHQL drivers. It has significant amount
> of new GL_OES extensions.
> 
> http://www.nvidia.com/object/winxp-310.70-whql-driver.html
> 
> http://www.nvidia.com/object/win8-win7-winvista-64bit-310.70-whql-driver.html
> 
> http://www.nvidia.com/object/notebook-win8-win7-winvista-64bit-310.70-whql-
> driver.html

I don't see the list of new extensions anywhere, but in general, OES exts on desktop will not be useful to us until we are capable of using native GLES2 on desktop. I filed bug 822518 for this.

Comment 23

[NVD]

Jeff, here's the list.

http://www.geeks3d.com/20121023/nvidia-r310-33-beta-graphics-drivers-with-opengl-4-3-support-for-windows/

Except for the extensions below, they were removed from 310.70 WHQL.

GL_OES_compressed_ETC2_RGB8_texture
GL_OES_compressed_ETC2_sRGB8_texture
GL_OES_compressed_ETC2_punchthroughA_RGBA8_texture
GL_OES_compressed_ETC2_punchthroughA_sRGB8_alpha_texture
GL_OES_compressed_ETC2_RGBA8_texture
GL_OES_compressed_ETC2_sRGB8_alpha8_texture
GL_OES_compressed_EAC_R11_unsigned_texture
GL_OES_compressed_EAC_R11_signed_texture
GL_OES_compressed_EAC_RG11_unsigned_texture
GL_OES_compressed_EAC_RG11_signed_texture

Comment 24

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Those are just adding the GLES compressed texture formats (including ETC2, which is good) to the desktop.  This is all good for WebGL.  The other OES extensions don't make sense to expose via desktop GL; I'm not surprised that they were removed, as having them visible seems like it was a bug.

Comment 25

[u480271]

Attachment: d3d11interop.patch

Here's my attempt to port the D3D10 version to OMTC D3D11 on Windows to avoid the slow read back when rendering to OGL via WebGL and compositor is D3D11.

Comment 26

[u480271]

Attachment: Use DX_NV_Interop2 for faster sharing.

Comment 27

[u480271]

Attachment: Use DX_NV_Interop2 for faster sharing.

Comment 28

[u480271]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a48db5b55d63

Comment 29

[Kelsey Gilbert [:jgilbert]]

Sorry for letting this sit. I'm looking to get fast E10S compositing running, and this would be more code to immediately port over. I recommend we wait on this a little longer to see if we can't just rebase it directly on the new stuff.

Does this block anything for you?

Comment 30

[u480271]

Nope. I'm just going through my branches and recording everything that I've changed in patches so we can track them.

Do you want to remove the review request?

Comment 31

[Kelsey Gilbert [:jgilbert]]

Yeah, let's mothball this temporarily.

Comment 32

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8581472 [details] [diff] [review]
Use DX_NV_Interop2 for faster sharing.

Mothballing for now.

Comment 33

[Benoit Girard (:BenWa)]

Attachment: Part 1: Add WGL_NV_DX_interop

Let's land the support for WGL_NV_DX_interop first, while we work on the rest.

Comment 34

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8671557 [details] [diff] [review]
Part 1: Add WGL_NV_DX_interop

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

::: gfx/gl/GLConsts.h
@@ +5951,5 @@
>  
> +// WGL_NV_DX_interop
> +#define LOCAL_WGL_ACCESS_READ_ONLY                           0x0000
> +#define LOCAL_WGL_ACCESS_READ_WRITE                          0x0001
> +#define LOCAL_WGL_ACCESS_WRITE_DISCARD                       0x0002

Did you get these by regenerating this file? This is a generated file. Add non-generated entries to GLDefs.h.

::: gfx/gl/GLContextProviderWGL.cpp
@@ +144,5 @@
>          NS_WARNING("wglMakeCurrent failed");
>          return false;
>      }
>  
> +    GLLibraryLoader::PlatformLookupFunction lookupFunc =

A `const` would be nice.

Comment 35

[Benoit Girard (:BenWa)]

Attachment: Part 1: Add WGL_NV_DX_interop

Comment 36

[Benoit Girard (:BenWa)]

Attachment: patch

Comment 37

[Benoit Girard (:BenWa)]

There's some work that happened on this for GDC that on this branch:
https://github.com/jdashg/gecko-dev/commits/demo-hacks

Pulling in that work into patches we can land behind a preference for now so that it stop rotting.

Comment 38

[Benoit Girard (:BenWa)]

I've made some progress on this but I'm getting FRAMEBUFFER_UNSUPPORTED in ReadBuffer::Create under GLScreenBuffer::CreateRead. It's calling AttachBuffersToFB() with a colorRB and not septh/stencil to target 0.

Comment 39

[Benoit Girard (:BenWa)]

Attachment: Part 2: Add D3D11SharedSurfaceInterop

This will hit the error I have above but I'd like to get this in the tree where it will finally stop rotting. Next week I'll check if we get errors when creating mProdRB that might be the cause.

Comment 40

[Benoit Girard (:BenWa)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f066d7c6b5566699ae3624b59b20ab51d9eda71e
Bug 618898 - Part 1: Add WGL_NV_DX_interop. r=jgilbert

Comment 41

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/f066d7c6b556

Comment 42

[Benoit Girard (:BenWa)]

Dan might help me take a look. I can't figure out why this wont work since I'm passing in and calling the APIs the same way as a working build on the same hardware. There must be a difference but I can't put my finger on it.

Part 2 posted above should do. The relevant failure is at gl->IsFramebufferComplete(fb) in Readbuffer::Create()

Comment 43

[u480271]

Attachment: Fixed up BenWa's port of patch to work on my local machine. f=bgirard

Benoit,

This works for me. I copied most of SharedSurfaceANGLE and made a small change to GLScreenBuffer. PTAL.

Comment 44

[u480271]

Back to you, Benoit.

Comment 45

[Benoit Girard (:BenWa)]

Comment on attachment 8683935 [details] [diff] [review]
Fixed up BenWa's port of patch to work on my local machine. f=bgirard

Cool, I confirmed that it works. I guess all that's left for now it to land it behind a preference.

Comment 46

[Benoit Girard (:BenWa)]

Attachment: patch

Comment 47

[Benoit Girard (:BenWa)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ac7ac3ce2bc9

Comment 48

[Benoit Girard (:BenWa)]

I'm done very limited testing but the performance seems slightly better than D3D11 angle. I need more data to confirm.

Comment 49

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8685011 [details] [diff] [review]
patch

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

We're close!

::: gfx/gl/GLConsts.h
@@ +5670,5 @@
>  
> +// WGL_NV_DX_interop
> +#define LOCAL_WGL_ACCESS_READ_ONLY                           0x0000
> +#define LOCAL_WGL_ACCESS_READ_WRITE                          0x0001
> +#define LOCAL_WGL_ACCESS_WRITE_DISCARD                       0x0002

This is a generated file. Unless you got this from updating and regenerating the file, these should go in GLDefs.h. (r-)

::: gfx/gl/GLScreenBuffer.cpp
@@ +105,5 @@
>                      factory = SurfaceFactory_ANGLEShareHandle::Create(gl, caps, forwarder, flags);
>                  }
> +
> +                if (!factory && gfxPrefs::WebGLDXGLEnabled()) {
> +                  factory = SurfaceFactory_D3D11Interop::Create(gl, caps, forwarder, flags);

Bad indent.

::: gfx/gl/SharedSurfaceD3D11Interop.cpp
@@ +149,5 @@
> +    ~DXGLDevice() {
> +        if (!mWGL->fDXCloseDevice(mDXGLDeviceHandle)) {
> +            uint32_t error = GetLastError();
> +            printf_stderr("wglDXCloseDevice(0x%x) failed: GetLastError(): 0x%x\n",
> +                          mDXGLDeviceHandle, error);

MOZ_ASSERT here?

@@ +160,5 @@
> +        if (!ret) {
> +            uint32_t error = GetLastError();
> +            printf_stderr("wglDXRegisterObject(0x%x, 0x%x, %u, 0x%x, 0x%x) failed:"
> +                          " GetLastError(): 0x%x\n", mDXGLDeviceHandle, dxObject, name,
> +                          type, access, error);

MOZ_ASSERT here and elsewhere below, unless we know (and have left a comment about) any good reasons this might fail.

@@ +205,5 @@
> +                                   GLContext* gl,
> +                                   const gfx::IntSize& size,
> +                                   bool hasAlpha)
> +{
> +    ID3D11Device* d3d = dxgl->mD3D;

const auto& d3d is nearly preferable.

@@ +221,5 @@
> +        return nullptr;
> +    }
> +
> +    IDXGIResource* textureDXGI;
> +    hr = textureD3D->QueryInterface(__uuidof(IDXGIResource), (void**)&textureDXGI);

Shouldn't this be:
RefPtr<IDXGIResource> textureDXGI;
hr = textureD3D->QueryInterface(getter_AddRefs(textureDXGI));

@@ +228,5 @@
> +        return nullptr;
> +    }
> +
> +    RefPtr<IDXGIKeyedMutex> keyedMutex;
> +    hr = textureD3D->QueryInterface((IDXGIKeyedMutex**) getter_AddRefs(keyedMutex));

Doesn't getter_Addrefs() return a T**?

@@ +235,5 @@
> +        return nullptr;
> +    }
> +
> +    HANDLE sharedHandle;
> +    textureDXGI->GetSharedHandle(&sharedHandle);

This is infallible?

@@ +243,5 @@
> +    gl->fGenRenderbuffers(1, &renderbufferGL);
> +    HANDLE objectWGL = dxgl->RegisterObject(textureD3D, renderbufferGL,
> +                                            LOCAL_GL_RENDERBUFFER,
> +                                            LOCAL_WGL_ACCESS_READ_WRITE);
> +                                            //LOCAL_WGL_ACCESS_WRITE_DISCARD_NV);

I don't actually know. Ideally we'd be WRITE_DISCARD, unless it marks it truly read-only. If it just makes an undefined-data scratch surface for us to write into, but we can still read from it, we want WRITE_DISCARD.

Does WRITE_DISCARD not work?

@@ +294,5 @@
> +
> +    mGL->fDeleteRenderbuffers(1, &mProdRB);
> +
> +    if (!mDXGL->UnregisterObject(mObjectWGL))
> +    NS_WARNING("Failed to release a DXGL object, possibly leaking it.");

Bad indent.

@@ +315,5 @@
> +
> +void
> +SharedSurface_D3D11Interop::Fence()
> +{
> +    mGL->fFinish();

Surely we don't need Finish() here?

We don't really have a Fence() option with this API. I do think that these Fence() funcs are about to be removed, so we can leave this for now.

@@ +336,5 @@
> +{
> +    MOZ_ASSERT(!mLockedForGL);
> +
> +    if (mKeyedMutex) {
> +        HRESULT hr = mKeyedMutex->AcquireSync(0, 10000);

Pull these magic numbers out into named constants.

@@ +339,5 @@
> +    if (mKeyedMutex) {
> +        HRESULT hr = mKeyedMutex->AcquireSync(0, 10000);
> +        if (hr == WAIT_TIMEOUT) {
> +            // Doubt we should do this? Maybe Wait for ever?
> +            MOZ_CRASH();

Uh, this is fine for now, though please put some text in MOZ_CRASH.

I think it's a bunch of work to make ProducerAcquire properly fallible.

@@ +363,5 @@
> +    // Now we have unlocked for GL, we can release to consumer.
> +    if (mKeyedMutex) {
> +        mKeyedMutex->ReleaseSync(0);
> +    }
> +    Fence();

Fence() calls glFinish. Surely we only need to do this if we don't have a keyed mutex!

@@ +387,5 @@
> +        }
> +    }
> +
> +    if (mConsumerKeyedMutex) {
> +        HRESULT hr = mConsumerKeyedMutex->AcquireSync(0, 10000);

Pull the magic numbers out into constants.

@@ +407,5 @@
> +SharedSurface_D3D11Interop::Fence_ContentThread_Impl()
> +{
> +    if (mFence) {
> +        MOZ_ASSERT(mGL->IsExtensionSupported(GLContext::NV_fence));
> +        mGL->fSetFence(mFence, LOCAL_GL_ALL_COMPLETED_NV);

I don't think we should (or need to) be mixing extensions like this.

::: gfx/gl/SharedSurfaceD3D11Interop.h
@@ +28,5 @@
> +    const HANDLE mSharedHandle;
> +    const RefPtr<ID3D11Texture2D> mTextureD3D;
> +    RefPtr<IDXGIKeyedMutex> mKeyedMutex;
> +    RefPtr<IDXGIKeyedMutex> mConsumerKeyedMutex;
> +    RefPtr<ID3D11Texture2D> mConsumerTexture;

I think we can safely get rid of the Consumer stuff, but I can do this in a follow-up.

Comment 50

[Benoit Girard (:BenWa)]

As noted in IRC I'm going to keep the glFinish in this landing and fix it in a follow-up. Finally getting a patch for this 5 years later :D.

Comment 51

[Benoit Girard (:BenWa)]

Attachment: patch

Comment 52

[Benoit Girard (:BenWa)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=743ecf4230e0

Comment 53

[Benoit Girard (:BenWa)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/bd2c8c5472226a34003fd6d0c272481772531883
Bug 618898 - Add D3D11SharedSurfaceInterop. r=jgilbert

Comment 54

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/bd2c8c547222