This might be the only part of angle that we need to patch.

I was able to get something working yesterday however performance is greatly reduced due to what looks like contention spread all over d3d11. I'll look into this on Monday.

It looks like the performance problem was not in fact caused by this synchronization strategy but is actually caused by the ANGLE D3D11 backend. I'll look into why next. The same content runs fine in IE11 so the problem doesn't seem foundational to D3D11.

The ANGLE guys say that the D3D11 perf issues are largely related to texSubImage/bufferSubData calls being slow. Maybe other things, but those are the main points they mentioned.

To be clear, they have solutions/are solving these, but we certainly don't have the fixes in our ANGLE yet. Also, specifically, Intel D3D11 drivers are supposedly much slower for these cases. I was hearing that D3D11 perf was as bad as 25% of the D3D9 perf. (75% worse)

I've filed bug 1068169 about the problem I was seeing with BenWa's test case.

A working version

Comment on attachment 8510700 [details] [diff] [review] Current version Review of attachment 8510700 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/gl/SharedSurface.h @@ +117,4 @@ > virtual void Fence() = 0; > virtual bool WaitSync() = 0; > virtual bool PollSync() = 0; > + virtual void AcquireProducer() {} What's this for? ::: gfx/gl/SharedSurfaceANGLE.cpp @@ +67,4 @@ > egl->fDestroySurface(egl->Display(), pbuffer); > return nullptr; > } > + void *keyedMutex = nullptr; Star to against type, not identifier. @@ +157,5 @@ > void > +SharedSurface_ANGLEShareHandle::ProducerAcquireImpl() > +{ > + if (mKeyedMutex) > + mKeyedMutex->AcquireSync(0, INFINITE); 4-space @@ +172,5 @@ > + mKeyedMutex->ReleaseSync(0); > + return; > + } else if (mFence) { > + MOZ_ASSERT(mGL->IsExtensionSupported(GLContext::NV_fence)); > + mGL->fSetFence(mFence, LOCAL_GL_ALL_COMPLETED_NV); Why do we set the fence if we don't wait on it? ::: gfx/gl/SharedSurfaceANGLE.h @@ +52,4 @@ > EGLContext context, > EGLSurface pbuffer, > HANDLE shareHandle, > + IDXGIKeyedMutex* keyedMutex, const RefPtr<IDXGIKeyedMutex>& keyedMutex, @@ +79,5 @@ > HANDLE GetShareHandle() { > return mShareHandle; > } > + > + RefPtr<ID3D11Texture2D> GetConsumerTexture() { const

The required ANGLE patch is here: https://chromium-review.googlesource.com/#/c/225642/

Pretty similar to the previous patch with feedback addressed.

Comment on attachment 8512190 [details] [diff] [review] keyed-mutex-clean.patch Review of attachment 8512190 [details] [diff] [review]: ----------------------------------------------------------------- You have a bunch of unnecessary copies going on here, for some reason. ::: gfx/gl/GLConsts.h @@ +5188,4 @@ > #define LOCAL_EGL_COVERAGE_SAMPLE_RESOLVE_NV 0x3131 > #define LOCAL_EGL_D3D_TEXTURE_2D_SHARE_HANDLE_ANGLE 0x3200 > #define LOCAL_EGL_D3D_TEXTURE_2D_KEYED_MUTEX_ANGLE 0x3209 > +#define LOCAL_EGL_KEYED_MUTEX_ANGLE 0x3202 It looks like we don't need this. ::: gfx/gl/SharedSurfaceANGLE.cpp @@ +68,4 @@ > egl->fDestroySurface(egl->Display(), pbuffer); > return nullptr; > } > + void* keyedMutex = nullptr; If we're going to hold a ref to this, we should hold it as early as possible. @@ +99,4 @@ > EGLContext context, > EGLSurface pbuffer, > HANDLE shareHandle, > + const RefPtr<IDXGIKeyedMutex> keyedMutex, const Foo& @@ +180,5 @@ > +SharedSurface_ANGLEShareHandle::ConsumerAcquireImpl() > +{ > + if (!mConsumerTexture) { > + RefPtr<ID3D11Texture2D> tex; > + HRESULT hr = gfxWindowsPlatform::GetPlatform()->GetD3D11Device()->OpenSharedResource(mShareHandle, Consider using an `auto` to reduce how long this line is. Also we should really not be pulling the d3d device like this, but we can deal with this later maybe. ::: gfx/gl/SharedSurfaceANGLE.h @@ +52,4 @@ > EGLContext context, > EGLSurface pbuffer, > HANDLE shareHandle, > + const RefPtr<IDXGIKeyedMutex> keyedMutex, const Foo& @@ +79,5 @@ > HANDLE GetShareHandle() { > return mShareHandle; > } > + > + const RefPtr<ID3D11Texture2D> GetConsumerTexture() const { const Foo&

Technically we should also make an extension for this, but since this is all ANGLE-specific, I'm not sure how worth-it it is.

(In reply to Jeff Gilbert [:jgilbert] from comment #14) > > ::: gfx/gl/SharedSurfaceANGLE.h > @@ +52,4 @@ > > EGLContext context, > > EGLSurface pbuffer, > > HANDLE shareHandle, > > + const RefPtr<IDXGIKeyedMutex> keyedMutex, > > const Foo& This can't be const reference because we're taking ownership of it.

(In reply to Jeff Muizelaar [:jrmuizel] from comment #16) > (In reply to Jeff Gilbert [:jgilbert] from comment #14) > > > > ::: gfx/gl/SharedSurfaceANGLE.h > > @@ +52,4 @@ > > > EGLContext context, > > > EGLSurface pbuffer, > > > HANDLE shareHandle, > > > + const RefPtr<IDXGIKeyedMutex> keyedMutex, > > > > const Foo& > > This can't be const reference because we're taking ownership of it. We should be able to. We don't modify the RefPtr object, we modify the underlying object. The only thing that can't be a reference is the member var.

(In reply to Jeff Gilbert [:jgilbert] from comment #17) > (In reply to Jeff Muizelaar [:jrmuizel] from comment #16) > > (In reply to Jeff Gilbert [:jgilbert] from comment #14) > > This can't be const reference because we're taking ownership of it. > > We should be able to. We don't modify the RefPtr object, we modify the > underlying object. The only thing that can't be a reference is the member > var. True.