Jeff Muizelaar [:jrmuizel] Assignee
	Description • 10 years ago

      No description provided.

	Jeff Muizelaar [:jrmuizel] Assignee
	Comment 1 • 10 years ago

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

	Jeff Muizelaar [:jrmuizel] Assignee
	Comment 3 • 10 years ago

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.

	Jeff Muizelaar [:jrmuizel] Assignee
	Comment 4 • 10 years ago

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.

	Kelsey Gilbert [:jgilbert]
	Comment 5 • 10 years ago

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.

	Kelsey Gilbert [:jgilbert]
	Comment 6 • 10 years ago

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)

	Jeff Muizelaar [:jrmuizel] Assignee
	Comment 7 • 10 years ago

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

	Jeff Muizelaar [:jrmuizel] Assignee
	Comment 8 • 10 years ago

A working version

	Kelsey Gilbert [:jgilbert]
	Comment 11 • 10 years ago

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

	Jeff Muizelaar [:jrmuizel] Assignee
	Comment 12 • 10 years ago

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

	Jeff Muizelaar [:jrmuizel] Assignee
	Comment 13 • 10 years ago

Pretty similar to the previous patch with feedback addressed.

	Kelsey Gilbert [:jgilbert]
	Comment 14 • 10 years ago

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&

	Kelsey Gilbert [:jgilbert]
	Comment 15 • 10 years ago

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.

	Jeff Muizelaar [:jrmuizel] Assignee
	Comment 16 • 10 years ago

(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.

	Kelsey Gilbert [:jgilbert]
	Comment 17 • 10 years ago

(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.

	Jeff Muizelaar [:jrmuizel] Assignee
	Comment 18 • 10 years ago

(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.

	Ryan VanderMeulen [:RyanVM]
	Comment 19 • 10 years ago

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