904890 - Fast video via a video memory d3d9 texture client

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

[Nick Cameron [:nrc]]

For DXVA video on OMTC we need a d3d9 texture client and host which uses video memory rather than systemmem. Interestingly, this is going to have to work with the d3d11 compositor.

Comment 1

[Nick Cameron [:nrc]]

See Bug 875247 for an existing but apparently slightly buggy d3d11 version. We might not need thatif we can just use the d3d9 version everywhere.

Comment 2

[Nick Cameron [:nrc]]

Just a note that we are probably better off doing this with a new texture client, rather than the deprecated ones since we are already half way to using the new flavour on Windows.

Comment 3

[Matt Woodrow (:mattwoodrow)]

Adding dependency on new-texture for d3d11.

Comment 4

[Matt Woodrow (:mattwoodrow)]

Attachment: Part 1: Add a TextureClient for shared d3d9 surfaces

Comment 5

[Matt Woodrow (:mattwoodrow)]

Attachment: Part 2: Implement ISharedImage for D3D9SurfaceImage

Comment 6

[Matt Woodrow (:mattwoodrow)]

Attachment: Part 3: Don't crash if our d3d11 texture doesn't have a mutex

The textures being produced from the hardware video decoder don't have locking enabled. Looks like we manually synchronize on the client side before sending them to ensure that nothing races.

Comment 7

[Matt Woodrow (:mattwoodrow)]

Attachment: Part 4: Enable hardware accelerated video decoding for OMTC+D3D11

Comment 8

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8346363 [details] [diff] [review]
Part 4: Enable hardware accelerated video decoding for OMTC+D3D11

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

::: content/media/wmf/WMFReader.cpp
@@ +124,5 @@
>  
>    if (layerManager->GetBackendType() != LayersBackend::LAYERS_D3D9 &&
> +      layerManager->GetBackendType() != LayersBackend::LAYERS_D3D10 &&
> +      !(layerManager->GetBackendType() == LayersBackend::LAYERS_CLIENT &&
> +        layerManager->AsShadowForwarder()->GetCompositorBackendType() == LayersBackend::LAYERS_D3D11)) {

We have other code using DXVA: WMFVideoDecoder in content/media/fmp4/wmf.

For this, we extract the LayersBackend type in MP4Reader::InitLayersBackendType() and pass it through PlatformDecoderModule::CreateH264Decoder() (called by MP4Reader::ReadMetadata()) so that WMFVideoDecoder::InitializeDXVA(mozilla::layers::LayersBackend aLayersBackend) receives it and can decide whether to use DXVA.

We should also extract and pass the compositor backend type through this call chain too. Please let me know if that's a dumb idea. I'll write a patch to do that.

Comment 9

[Chris Pearce [:cpearce (Not reading bugmail)]]

Needinfo myself so bugzilla nags me to follow up on comment 8.

Comment 10

[Matt Woodrow (:mattwoodrow)]

(In reply to Chris Pearce (:cpearce) from comment #8)
 
> We have other code using DXVA: WMFVideoDecoder in content/media/fmp4/wmf.
> 
> For this, we extract the LayersBackend type in
> MP4Reader::InitLayersBackendType() and pass it through
> PlatformDecoderModule::CreateH264Decoder() (called by
> MP4Reader::ReadMetadata()) so that
> WMFVideoDecoder::InitializeDXVA(mozilla::layers::LayersBackend
> aLayersBackend) receives it and can decide whether to use DXVA.
> 
> We should also extract and pass the compositor backend type through this
> call chain too. Please let me know if that's a dumb idea. I'll write a patch
> to do that.

Not sure if this is what you mean, but I think we can just pass the compositor backend in place of the layer backend, if the real layers backend is LAYERS_CLIENT. No point passing around two values, when all we really care about is the 'effective' backend (which may we should have LayerManager API for).

Comment 11

[Chris Pearce [:cpearce (Not reading bugmail)]]

If you think that there's zero chance of the two cases layer manager's LayersBackend==LAYERS_D3D11 and GetCompositorBackendType()==LAYERS_D3D11 being confused and D3D9 surfaces not being handled correctly, then I'm cool with just passing the 'effective' backend type.

Can we handle D3D9 surfaces in the D3D11 layer manager backend? Or does (and will) the layer manager never use a D3D11 backend, only the compositor?

Comment 12

[Nick Cameron [:nrc]]

(In reply to Chris Pearce (:cpearce) from comment #11)
> If you think that there's zero chance of the two cases layer manager's
> LayersBackend==LAYERS_D3D11 and GetCompositorBackendType()==LAYERS_D3D11
> being confused and D3D9 surfaces not being handled correctly, then I'm cool
> with just passing the 'effective' backend type.
> 
> Can we handle D3D9 surfaces in the D3D11 layer manager backend? Or does (and
> will) the layer manager never use a D3D11 backend, only the compositor?

There is no d3d11 layer manager - only d3d10 (and there is no d3d10 compositor).

Comment 13

[Nicolas Silva [:nical]]

Comment on attachment 8346360 [details] [diff] [review]
Part 1: Add a TextureClient for shared d3d9 surfaces

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

r=me with comments fixed

::: gfx/layers/d3d9/TextureD3D9.h
@@ +138,5 @@
>    SurfaceInitMode mInitMode;
>    bool mInitialized;
>  };
>  
> +class TextureClientD3D9Shared : public TextureClient

I'd prefer that we stick to the following convention: <type>TextureClient<backend> like SharedTextureClientD3D9 (deprecated textures don't always do that but the new ones do)

@@ +166,5 @@
> +    mHandle = aSharedHandle;
> +    mDesc = aDesc;
> +  }
> +
> +  virtual gfx::IntSize GetSize() const

MOZ_OVERRIDE

Comment 14

[Nicolas Silva [:nical]]

Comment on attachment 8346361 [details] [diff] [review]
Part 2: Implement ISharedImage for D3D9SurfaceImage

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

::: gfx/layers/D3D9SurfaceImage.h
@@ +31,5 @@
>  
> +  D3D9SurfaceImage();
> +  virtual ~D3D9SurfaceImage();
> +
> +  virtual ISharedImage* AsSharedImage() { return this; }

nit: MOZ_OVERRIDE

@@ +60,5 @@
>    // is complete, whereupon the texture is safe to use.
>    void EnsureSynchronized();
>  
>    gfxIntSize mSize;
>    RefPtr<IDirect3DTexture9> mTexture;

It would be even better if accessing this texture had to go through the TextureClient. Otherwise the TextureClient can't ensure that the the memory is managed properly (which may not be critical in this particular case, but it's a good practice).

Comment 15

[Nick Cameron [:nrc]]

Comment on attachment 8346363 [details] [diff] [review]
Part 4: Enable hardware accelerated video decoding for OMTC+D3D11

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

::: content/media/wmf/WMFReader.cpp
@@ +124,5 @@
>  
>    if (layerManager->GetBackendType() != LayersBackend::LAYERS_D3D9 &&
> +      layerManager->GetBackendType() != LayersBackend::LAYERS_D3D10 &&
> +      !(layerManager->GetBackendType() == LayersBackend::LAYERS_CLIENT &&
> +        layerManager->AsShadowForwarder()->GetCompositorBackendType() == LayersBackend::LAYERS_D3D11)) {

No d3d9 implementation? :-(

Comment 16

[Matt Woodrow (:mattwoodrow)]

(In reply to Nick Cameron [:nrc] from comment #15)
> Comment on attachment 8346363 [details] [diff] [review]
> Part 4: Enable hardware accelerated video decoding for OMTC+D3D11
> 
> Review of attachment 8346363 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: content/media/wmf/WMFReader.cpp
> @@ +124,5 @@
> >  
> >    if (layerManager->GetBackendType() != LayersBackend::LAYERS_D3D9 &&
> > +      layerManager->GetBackendType() != LayersBackend::LAYERS_D3D10 &&
> > +      !(layerManager->GetBackendType() == LayersBackend::LAYERS_CLIENT &&
> > +        layerManager->AsShadowForwarder()->GetCompositorBackendType() == LayersBackend::LAYERS_D3D11)) {
> 
> No d3d9 implementation? :-(

Not until someone writes new-textures for d3d9!

Comment 17

[Matt Woodrow (:mattwoodrow)]

Attachment: Part 1: Implement ISharedImage for D3D9SurfaceImage

Carrying forward r=nical

Comment 18

[Matt Woodrow (:mattwoodrow)]

Attachment: Part 2: Don't crash if our d3d11 texture doesn't have a mutex

Carrying forward r=Bas

Comment 19

[Matt Woodrow (:mattwoodrow)]

Attachment: Part 3: Add a layers API to expose the 'effective' backend

Comment 20

[Matt Woodrow (:mattwoodrow)]

Attachment: Part 4: Enable hardware accelerated video decoding for OMTC+D3D11

Comment 21

[Matt Woodrow (:mattwoodrow)]

Attachment: Part 5: Add a d3d9 texture host

Comment 22

[Matt Woodrow (:mattwoodrow)]

With the above patch queue and new-textures enabled, HW accelerated video decoding works on both d3d11 and d3d9 compositors.

Comment 23

[Matt Woodrow (:mattwoodrow)]

The patches don't break things without new-textures enabled, but we hit the readback path D3D9SurfaceImage::DeprecatedGetAsSurface.

According to the comments in content/media this will perform worse than just decoding in software to begin with.

So if we're planning to ship d3d9/11 without new-textures enabled, then landing of this should probably wait until we flip the pref. Otherwise this can land ASAP.

Comment 24

[Nicolas Silva [:nical]]

Comment on attachment 8362844 [details] [diff] [review]
Part 5: Add a d3d9 texture host

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

r=me with the surface format change below.

::: gfx/layers/d3d9/TextureD3D9.cpp
@@ +79,5 @@
> +      break;
> +    }
> +    case SurfaceDescriptor::TSurfaceDescriptorD3D10: {
> +      result = new DXGITextureHostD3D9(aFlags, aDesc);
> +      break;

Wooops! That was my bad.

@@ +1678,5 @@
> +    HRESULT hr = deviceManager->device()->CreateTexture(mSize.width,
> +                                                        mSize.height,
> +                                                        1,
> +                                                        D3DUSAGE_RENDERTARGET,
> +                                                        D3DFMT_X8R8G8B8,

Please use SurfaceFormatToD3D9Format(mFormat) here, instead of hardcoding XRGB.

Comment 25

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8362842 [details] [diff] [review]
Part 3: Add a layers API to expose the 'effective' backend

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

r+ with that

::: gfx/layers/Layers.h
@@ +422,5 @@
> +   * Type of layers backend that will be used to composite this layer tree.
> +   * When compositing is done remotely, then this returns the layers type
> +   * of the compositor.
> +   */
> +  virtual LayersBackend GetEffectiveBackendType() { return GetBackendType(); }

I think "GetCompositorBackendType" would be a better name.

Comment 26

[Matt Woodrow (:mattwoodrow)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/74a5aba9bd53
https://hg.mozilla.org/integration/mozilla-inbound/rev/500ff989a1d1
https://hg.mozilla.org/integration/mozilla-inbound/rev/2f41d32a3249
https://hg.mozilla.org/integration/mozilla-inbound/rev/e31ba8d051be
https://hg.mozilla.org/integration/mozilla-inbound/rev/d3861e435171

Comment 27

[Matt Woodrow (:mattwoodrow)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/32f9d85a4ad8

Comment 28

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/74a5aba9bd53
https://hg.mozilla.org/mozilla-central/rev/500ff989a1d1
https://hg.mozilla.org/mozilla-central/rev/2f41d32a3249
https://hg.mozilla.org/mozilla-central/rev/e31ba8d051be
https://hg.mozilla.org/mozilla-central/rev/d3861e435171
https://hg.mozilla.org/mozilla-central/rev/32f9d85a4ad8