Closed Bug 1389980 Opened 7 years ago Closed 7 years ago

Under Quantum DOM, WMF may be interacted with on different "main" threads which may not have MSCOM initialized

Categories

(Core :: Audio/Video: Playback, enhancement, P1)

Product:
Core
Component:
Audio/Video: Playback
Version:
55 Branch
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla57
Tracking Flags:
Tracking Status
firefox57 --- fixed

People

(Reporter: cpearce, Assigned: cpearce)

Details

Attachments

(2 files)

Bug 1389980 - Remove MP4Decoder::IsVideoAccelerated() as it is unused.
59 bytes, text/x-review-board-request
mattwoodrow
: review+
Details
Bug 1389980 - Ensure we only interact with WMF on MTA threads.
59 bytes, text/x-review-board-request
bugzilla
: review+
mattwoodrow
: review+
Details 
In the upcoming Quantum DOM work we'll end up with a pool of "main" threads. So once Quantum DOM is turned on, every time the media code is called from the "main" thread, we could actually be on a different OS thread.

Note that all tabs in a tab group should run on the same physical thread, and only one of the threads in the main thread pool run at once.

There are a few places where we interact with OS decoding libraries on the main thread.

I think this probably won't be a problem on platforms other than Windows, as we use the platform decoders from our SharedThreadPool threads without obvious problems.

On Windows this may be a problem as on the main thread we are supposed to have initialized MSCOM in single threaded apartment mode, and the WMF documentation says that the IMFTransform should be used in multi-threaded apartment mode:
https://msdn.microsoft.com/en-us/library/windows/desktop/ee892371(v=vs.85).aspx

We initialize MSCOM on the main thread in single threaded mode here:
http://searchfox.org/mozilla-central/rev/c329d562fb6c6218bdb79290faaf015467ef89e2/dom/ipc/ContentProcess.h#46
http://searchfox.org/mozilla-central/rev/c329d562fb6c6218bdb79290faaf015467ef89e2/toolkit/xre/nsAppRunner.cpp#4767

It's not clear to me that all threads in the pool of "main" threads are initialized with MSCOM.

Additionally, I had often wondered whether initializing WMF the first time in canPlayType with MSCOM in STA mode would cause issues when we used it later off-main-thread in MTA mode.

I've found the following places where we interact with OS decoding libraries on the main thread, there may be others:
* PDMFactory::PDMFactory() calls CreatePDMs() which initializes WMF in the WMFDecocderModule. The PDMFactory constructor is called in MP4Decoder::IsSupportedType() on the main thread.
* MP4Decoder::IsVideoAccelerated() creates an H.264 decoder (do we still call this?).

I think we should somehow make our interaction with WMF not occur on the main thread, and only occur on threads where we know MSCOM has been initialized with MTA.
Are the "main" thread like different processes, or they are actual threads on the same process?

The PDMFactory constructor calls PDMFactory::EnsureInit which checks if the various PDMs are initialised.

In either case, I don't think there's anything to do. The WMF will be initialised on the current process as needed
Anything to do on the PDM side of things that is
(In reply to Jean-Yves Avenard [:jya] from comment #1)
> Are the "main" thread like different processes, or they are actual threads
> on the same process?

The main threads are actual threads in the same process, we just only run one at a time, and switch between them based on our own scheduling.

It's not clear to me that we initialize MSCOM on all of them, and the first main thread definitely gets initialized to STA. I think we should be only interacting with WMF from threads of the same COM compartment model.
 
> The PDMFactory constructor calls PDMFactory::EnsureInit which checks if the
> various PDMs are initialised.
> 
> In either case, I don't think there's anything to do. The WMF will be
> initialised on the current process as needed

We create an IMFTransform in D3D11DXVA2Manager::InitInternal() on the main thread, which is likely an STA thread, and we'll end up using it on decode pool threads which are MTA. So it may get confused.

I think we can just delegate interaction with IMFTransform instances to a thread initialized to MTA to make this safe.
Comment on attachment 8897212 [details]
Bug 1389980 - Ensure we only interact with WMF on MTA threads.

https://reviewboard.mozilla.org/r/168484/#review173916

::: dom/media/platforms/wmf/DXVA2Manager.cpp:789
(Diff revision 1)
> +  // https://msdn.microsoft.com/en-us/library/windows/desktop/ee892371(v=vs.85).aspx#components
> +  // The main thread (where this function is called) is STA, not MTA.
> +  RefPtr<MFTDecoder> mft;
> +  mozilla::mscom::EnsureMTA([&]() -> void {
> +    mft = new MFTDecoder();
> +    hr = mft->Create(CLSID_VideoProcessorMFT);

hr is used over two different threads here, does ensureMTA guarantees synchronisations once it returns?
Comment on attachment 8897211 [details]
Bug 1389980 - Remove MP4Decoder::IsVideoAccelerated() as it is unused.

https://reviewboard.mozilla.org/r/168482/#review173918

::: commit-message-824d4:1
(Diff revision 1)
> +Bug 1389980 - Remove MP4Decoder::IsVideoAccelerated() as it is unused. r?mattwoodrow

IIUC, it's only unused at this stage due to the about:support not being able to use the GPU process

it's an information that needs to be put back in abotu:support IMHO.
(In reply to Jean-Yves Avenard [:jya] from comment #6)
> Comment on attachment 8897212 [details]
> Bug 1389980 - Ensure we only interact with WMF on MTA threads.
> 
> https://reviewboard.mozilla.org/r/168484/#review173916
> 
> ::: dom/media/platforms/wmf/DXVA2Manager.cpp:789
> (Diff revision 1)
> > +  // https://msdn.microsoft.com/en-us/library/windows/desktop/ee892371(v=vs.85).aspx#components
> > +  // The main thread (where this function is called) is STA, not MTA.
> > +  RefPtr<MFTDecoder> mft;
> > +  mozilla::mscom::EnsureMTA([&]() -> void {
> > +    mft = new MFTDecoder();
> > +    hr = mft->Create(CLSID_VideoProcessorMFT);
> 
> hr is used over two different threads here, does ensureMTA guarantees
> synchronisations once it returns?

As you can see in the code [1], EnsureMTA relies on SetEvent() to signal to the calling thread when the closure has finished. MSDN[2] doesn't say whether SetEvent() contains a memory barrier explicitly, but I think it's reasonable to assume given that SetEvent() is a key piece of synchronization infrastructure used to send signals between threads, it must contain a memory barrier.

[1] https://searchfox.org/mozilla-central/rev/13148faaa91a1c823a7d68563d9995480e714979/ipc/mscom/EnsureMTA.h#87
[2] https://msdn.microsoft.com/en-us/library/windows/desktop/ms686211(v=vs.85).aspx
(In reply to Jean-Yves Avenard [:jya] from comment #7)
> Comment on attachment 8897211 [details]
> Bug 1389980 - Remove MP4Decoder::IsVideoAccelerated() as it is unused.
> 
> https://reviewboard.mozilla.org/r/168482/#review173918
> 
> ::: commit-message-824d4:1
> (Diff revision 1)
> > +Bug 1389980 - Remove MP4Decoder::IsVideoAccelerated() as it is unused. r?mattwoodrow
> 
> IIUC, it's only unused at this stage due to the about:support not being able
> to use the GPU process
> 
> it's an information that needs to be put back in abotu:support IMHO.

I thought the issue was that MP4Decoder::IsVideoAccelerated is unreliable because the decoder we created to test whether hardware acceleration is available may be hardware accelerated because we've hit the maximum number of active hardware decoders?

I agree it's handy having something like this in about:support, but how can we make it reliable? I'd happily review a patch to re-add something that would work reliably.
How can QA verify this, Chris? Do we have automated tests for it?
Flags: needinfo?(cpearce)
In particular, are there any plugins or anything that need to be installed to trigger this?
Comment on attachment 8897212 [details]
Bug 1389980 - Ensure we only interact with WMF on MTA threads.

https://reviewboard.mozilla.org/r/168484/#review173916

> hr is used over two different threads here, does ensureMTA guarantees synchronisations once it returns?

Yes, by default EnsureMTA is synchronous.
Comment on attachment 8897212 [details]
Bug 1389980 - Ensure we only interact with WMF on MTA threads.

https://reviewboard.mozilla.org/r/168484/#review174706

This looks good to me. I'm glad that you found and are using mscom::EnsureMTA!
Attachment #8897212 - Flags: review?(aklotz) → review+
(In reply to Andrew Overholt [:overholt] from comment #10)
> How can QA verify this, Chris? Do we have automated tests for it?

I don't have specific STR to reproduce a problem here. I'm making this change based on my understanding of how MSCOM and Quantum DOM threads may interact.

To test this, QA could disable WebM by setting media.webm.enabled=false (so they don't use our bundled VP9 software decoder) and playing videos in multiple tabs from different web sites, where the web sites are *not* the same and aren't opened by clicking links on each other (so the tabs are different tab groups).

I'd imagine the kinds of problems we'd see without this patch would be subtle and hard to pin down, i.e. QA may not find an issue, but it's probably worth having QA test media playback with multiple main threads anyway.


(In reply to Bill McCloskey (:billm) from comment #11)
> In particular, are there any plugins or anything that need to be installed
> to trigger this?

Not exactly. The potential problems here would be with the graphics drivers that are installed on the user's system. We don't execute arbitrary plugins for media playback. So it's definitely possible that some drivers would have issues with this and not others. We'd probably need to test on the main groups of drivers to get some sense as to whether that driver family has problems. That is, we should run the above test on machines with NVIDIA, Intel, and AMD GPUs. But I don't think we'd get value out of exhaustively testing all combinations of drivers and graphics cards.
Flags: needinfo?(cpearce)
Gah! If we have to do a read back, for example if the video is being drawn to a canvas, we'll hit the IsCurrentThreadMTA() assertions I added:

>	xul.dll!mozilla::MFTDecoder::Input(IMFSample * aSample) Line 301	C++
 	xul.dll!mozilla::D3D11DXVA2Manager::CopyToBGRATexture(ID3D11Texture2D * aInTexture, ID3D11Texture2D * * aOutTexture) Line 1046	C++
 	xul.dll!mozilla::layers::D3D11ShareHandleImage::GetAsSourceSurface() Line 101	C++
 	xul.dll!nsLayoutUtils::SurfaceFromElement(mozilla::dom::HTMLVideoElement * aElement, unsigned int aSurfaceFlags, RefPtr<mozilla::gfx::DrawTarget> & aTarget) Line 7533	C++
 	xul.dll!nsLayoutUtils::SurfaceFromElement(mozilla::dom::Element * aElement, unsigned int aSurfaceFlags, RefPtr<mozilla::gfx::DrawTarget> & aTarget) Line 7566	C++
 	xul.dll!mozilla::dom::CanvasRenderingContext2D::DrawImage(const mozilla::dom::HTMLImageElementOrSVGImageElementOrHTMLCanvasElementOrHTMLVideoElementOrImageBitmap & aImage, double aSx, double aSy, double aSw, double aSh, double aDx, double aDy, double aDw, double aDh, unsigned char aOptional_argc, mozilla::ErrorResult & aError) Line 5298	C++
 	xul.dll!mozilla::dom::CanvasRenderingContext2D::DrawImage(const mozilla::dom::HTMLImageElementOrSVGImageElementOrHTMLCanvasElementOrHTMLVideoElementOrImageBitmap & aImage, double aDx, double aDy, double aDw, double aDh, mozilla::ErrorResult & aError) Line 234	C++
 	xul.dll!mozilla::dom::CanvasRenderingContext2DBinding::drawImage(JSContext * cx, JS::Handle<JSObject *> obj, mozilla::dom::CanvasRenderingContext2D * self, const JSJitMethodCallArgs & args) Line 2692	C++
 	xul.dll!mozilla::dom::GenericBindingMethod(JSContext * cx, unsigned int argc, JS::Value * vp) Line 3053	C++
 	xul.dll!js::CallJSNative(JSContext * cx, bool(*)(JSContext *, unsigned int, JS::Value *) native, const JS::CallArgs & args) Line 293	C++

Test URL:
https://stanko.github.io/html-canvas-video-player/
Comment on attachment 8897211 [details]
Bug 1389980 - Remove MP4Decoder::IsVideoAccelerated() as it is unused.

https://reviewboard.mozilla.org/r/168482/#review175660
Attachment #8897211 - Flags: review?(matt.woodrow) → review+
Comment on attachment 8897212 [details]
Bug 1389980 - Ensure we only interact with WMF on MTA threads.

https://reviewboard.mozilla.org/r/168484/#review175662
Attachment #8897212 - Flags: review?(matt.woodrow) → review+
Pushed by cpearce@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/c502e40ee20d
Remove MP4Decoder::IsVideoAccelerated() as it is unused. r=mattwoodrow
https://hg.mozilla.org/integration/autoland/rev/9949baa51ef4
Ensure we only interact with WMF on MTA threads. r=aklotz,mattwoodrow
Assignee: nobody → cpearce
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: