1378297 - Add DXGITextureHostD3D11::GetAsSurface() support for webrender wrench

Status: RESOLVED FIXED

(Core :: Graphics: WebRender, enhancement)

Description

[Sotaro Ikeda [:sotaro]]

wrench requests TextureHost::GetAsSurface() support. It is necessary to implement DXGITextureHostD3D11::GetAsSurface() for webrender wrench.

Comment 1

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Add DXGITextureHostD3D11::GetAsSurface() support for webrender wrench

Comment 2

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Add DXGITextureHostD3D11::GetAsSurface() support for webrender wrench

Comment 3

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Add DXGITextureHostD3D11::GetAsSurface() support for webrender wrench

Comment 4

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8883486 [details] [diff] [review]
patch - Add DXGITextureHostD3D11::GetAsSurface() support for webrender wrench

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

::: gfx/layers/d3d11/TextureD3D11.cpp
@@ +893,5 @@
> +  RefPtr<IDXGIKeyedMutex> mutex;
> +  hr = mTexture->QueryInterface((IDXGIKeyedMutex**)getter_AddRefs(mutex));
> +
> +  if (SUCCEEDED(hr) && mutex) {
> +    hr = mutex->AcquireSync(0, 2000);

Looks like you never release this again, please use AutoLockD3D11Texture instead.

@@ +900,5 @@
> +    }
> +  }
> +
> +  context->CopyResource(cpuTexture, mTexture);
> +  if (!context) {

We've already dereferenced context at this point, why are we checking it now?

@@ +912,5 @@
> +  }
> +
> +  RefPtr<DataSourceSurface> surf =
> +    gfx::CreateDataSourceSurfaceFromData(IntSize(textureDesc.Width, textureDesc.Height),
> +                                         GetFormat(),

I assume this only really works for RGB(A) data. Can we check the format to make sure it's not YUV or anything unexpected?

Comment 5

[Sotaro Ikeda [:sotaro]]

(In reply to Matt Woodrow (:mattwoodrow) from comment #4)
> Comment on attachment 8883486 [details] [diff] [review]
> patch - Add DXGITextureHostD3D11::GetAsSurface() support for webrender wrench
> 
> Review of attachment 8883486 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/layers/d3d11/TextureD3D11.cpp
> @@ +893,5 @@
> > +  RefPtr<IDXGIKeyedMutex> mutex;
> > +  hr = mTexture->QueryInterface((IDXGIKeyedMutex**)getter_AddRefs(mutex));
> > +
> > +  if (SUCCEEDED(hr) && mutex) {
> > +    hr = mutex->AcquireSync(0, 2000);
> 
> Looks like you never release this again, please use AutoLockD3D11Texture
> instead.

Sorry, this part was not necessary KeyedMutex is already handled by LockD3DTexture() and UnlockD3DTexture() under AutoLockTextureHostWithoutCompositor. I am going to remove this part.

> 
> @@ +900,5 @@
> > +    }
> > +  }
> > +
> > +  context->CopyResource(cpuTexture, mTexture);
> > +  if (!context) {
> 
> We've already dereferenced context at this point, why are we checking it now?

It seemed added by mistake:( I will remove it in a next patch.

> 
> @@ +912,5 @@
> > +  }
> > +
> > +  RefPtr<DataSourceSurface> surf =
> > +    gfx::CreateDataSourceSurfaceFromData(IntSize(textureDesc.Width, textureDesc.Height),
> > +                                         GetFormat(),
> 
> I assume this only really works for RGB(A) data. Can we check the format to
> make sure it's not YUV or anything unexpected?

I'll add it in a next patch.

Comment 6

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Add DXGITextureHostD3D11::GetAsSurface() support for webrender wrench

Comment 7

[Sotaro Ikeda [:sotaro]]

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

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d8da7d04200e
Add DXGITextureHostD3D11::GetAsSurface() support for webrender wrench r=mattwoodrow

Comment 9

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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