Software decoding outputs layers::Image types that aren't supported directly by d3d9/11's CreateTextureSourceForImage functions, so fall back to the default compositor implementation. This uses CreateDataTextureSourceAround which also isn't implemented on these compositors, so we can't render the frames. We need to either implement the formats directly into the compositor, or implement the fallback path.

GetTextureClient isn't guaranteed to succeed, so we need to handle the other cases. This just shares the existing code that ImageClient uses.

When this code runs in the GPU process the content device isn't available. The compositor device is, and if we use that we need to not use KEYEDMUTEX since it has issues if you only lock/unlock on the same device. We still sort of need shared (even though it's one device) since the IPDL layers only knows how to share a HANDLE and not an ID3D11Texture2D*.

This lets callers know about compositor capabilities without dragging in all the IPDL/IPC headers (which causes compilation failures if we include it into the wrong spot).

The TextureHost ctor uses this to check if we're cross-process and errors if we try share raw memory pointers between processes.

Apologies for the size of this patch, it sort of does a few separate things, but they've very interconnected and getting it to compile separately turned out to be a real pain. Firstly, it changes the way we initialize decoders from using a layers backend to using a KnowsCompositor. KnowsCompositor includes the layers backend, but also has other information (like max texture size) and also has a pointer to an appropriate texture allocator. It lets us stop accessing the VideoBridge singleton in DXVAManager and just use the allocator provided. Ideally we'd do something similar for ImageBridge and we could unconditionally use it and never lookup global singletons. Secondly, it removes the KnowsCompositor interface from VideoBridge (which is a singleton, and thus can't properly 'know' a single compositor) and instead puts it on VideoDecoderParent (which exists per-decoder, and is uniquely associated with a real compositor). Lastly it serializes the KnowsCompositor that we use to create the RemoteVideoDecoder across IPDL and initializes the VideoDecoderParent with this. Now whenever VideoDecoderParent tries to allocate textures, it will use the correct layers backend and max texture size for the actual Compositor that it is pushing to.

This is what we use for the decoder TaskQueue when running decoders in the content process. I initially had the TaskQueue running on a single thread since I think that makes more sense if we can guarantee that only hardware decoders will be running. Switching back to a ThreadPool should work significantly better if we're supporting software decoding. Note that I'm still not 100% sure if we should run software decoding in the GPU process, but I'm aiming to have it work at least. We have had issues uploading to the GPU from software decoding (IMFYCbCrImage::GetTextureClient in particular), so having this in the GPU process is nice.

Comment on attachment 8797879 [details] [diff] [review] Part 5: Use KnowsCompositor to initialize decoders and make VideoDecoderParent one Review of attachment 8797879 [details] [diff] [review]: ----------------------------------------------------------------- r=me with comments addressed. ::: dom/media/ipc/RemoteVideoDecoder.cpp @@ +146,5 @@ > > already_AddRefed<MediaDataDecoder> > RemoteDecoderModule::CreateVideoDecoder(const CreateDecoderParams& aParams) > { > + if (!aParams.mKnowsCompositor) { I still think it would be nice to do all decoding out of process, not just hw ones ::: dom/media/ipc/VideoDecoderParent.h @@ +17,5 @@ > namespace dom { > > class VideoDecoderParent final : public PVideoDecoderParent, > + public MediaDataDecoderCallback, > + public layers::KnowsCompositor couldn't we use a forwarding wrapper instead? I'm no fan of multiple-inheritance like so, especially for types that appears to have no direct link with the class it inherits from. Especially as you only have two functions to worry about. ::: dom/media/platforms/wmf/DXVA2Manager.cpp @@ +529,5 @@ > public: > D3D11DXVA2Manager(); > virtual ~D3D11DXVA2Manager(); > > + HRESULT Init(layers::KnowsCompositor* aKnowsCompositor, I think it would be more elegant to pass this to the constructor, especially as it doesn't change over time ::: gfx/ipc/GfxMessageUtils.h @@ -33,5 @@ > #endif > > -namespace mozilla { > - > -typedef gfxImageFormat PixelFormat; that's a tad out of scope.... please move this to another patch

(In reply to Jean-Yves Avenard [:jya] from comment #7) > > I still think it would be nice to do all decoding out of process, not just > hw ones We are! That's what this patch set does. > couldn't we use a forwarding wrapper instead? > I'm no fan of multiple-inheritance like so, especially for types that > appears to have no direct link with the class it inherits from. > Especially as you only have two functions to worry about. Yeah ok, that's fair enough, I'll add that in. > > ::: dom/media/platforms/wmf/DXVA2Manager.cpp > @@ +529,5 @@ > > public: > > D3D11DXVA2Manager(); > > virtual ~D3D11DXVA2Manager(); > > > > + HRESULT Init(layers::KnowsCompositor* aKnowsCompositor, > > I think it would be more elegant to pass this to the constructor, especially > as it doesn't change over time The Init() function is the only thing that uses it, it isn't stored anywhere. > > ::: gfx/ipc/GfxMessageUtils.h > @@ -33,5 @@ > > #endif > > > > -namespace mozilla { > > - > > -typedef gfxImageFormat PixelFormat; > > that's a tad out of scope.... please move this to another patch Not sure how that got into this patch, will remove.

Pushed by mwoodrow@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/66adb66eeed5 Part 1: Make sure we create a TextureClient for VideoBridge even if the layers::Image doesn't support GetTextureClient. r=nical https://hg.mozilla.org/integration/mozilla-inbound/rev/9aec9ca6a659 Part 2: Allow using the compositor device for IMFYCbCrImage. r=dvander https://hg.mozilla.org/integration/mozilla-inbound/rev/0d5d18412866 Part 3: Split KnowsCompositor into a standalone header. r=nical https://hg.mozilla.org/integration/mozilla-inbound/rev/19e24514eb8f Part 4: Set OtherPid() for VideoBridge. r=dvander https://hg.mozilla.org/integration/mozilla-inbound/rev/7bafa704f350 Part 5: Use KnowsCompositor to initialize decoders and create one for VideoDecoderParent to use. r=nical,jya https://hg.mozilla.org/integration/mozilla-inbound/rev/4b28c86a0536 Part 6: Use SharedThreadPool for GPU process decoders. r=dvander

https://hg.mozilla.org/mozilla-central/rev/66adb66eeed5 https://hg.mozilla.org/mozilla-central/rev/9aec9ca6a659 https://hg.mozilla.org/mozilla-central/rev/0d5d18412866 https://hg.mozilla.org/mozilla-central/rev/19e24514eb8f https://hg.mozilla.org/mozilla-central/rev/7bafa704f350 https://hg.mozilla.org/mozilla-central/rev/4b28c86a0536

Mark 51 as won't fix. If it's worth uplifting to 51, feel free to nominate it.