Closed Bug 1378297 Opened 5 years ago Closed 5 years ago

Add DXGITextureHostD3D11::GetAsSurface() support for webrender wrench

Categories

(Core :: Graphics: WebRender, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla56
Tracking Status
firefox56 --- fixed

People

(Reporter: sotaro, Assigned: sotaro)

Details

Attachments

(1 file, 3 obsolete files)

wrench requests TextureHost::GetAsSurface() support. It is necessary to implement DXGITextureHostD3D11::GetAsSurface() for webrender wrench.
Assignee: nobody → sotaro.ikeda.g
Attachment #8883486 - Flags: review?(matt.woodrow)
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?
(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.
Attachment #8883486 - Attachment is obsolete: true
Attachment #8883486 - Flags: review?(matt.woodrow)
Attachment #8883785 - Flags: review?(matt.woodrow)
Attachment #8883785 - Flags: review?(matt.woodrow) → review+
Pushed by sikeda@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d8da7d04200e
Add DXGITextureHostD3D11::GetAsSurface() support for webrender wrench r=mattwoodrow
https://hg.mozilla.org/mozilla-central/rev/d8da7d04200e
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in before you can comment on or make changes to this bug.