1271491 - PDMFactory shouldn't require initialisation in the main thread

Status: RESOLVED FIXED

(Core :: Audio/Video: Playback, defect)

Description

[Jean-Yves Avenard [:jya]]

Right now, everything using the PDMFactory must ensure that the PDMFactory::Init() function has been called at least once and on the main thread.

One of the core reason it was required was the reliance on the reading of Preferences ; which can only be done on the main thread.

With the move to using MediaPrefs, this is no longer a restriction.

Apparently, there is no problem to load the external frameworks on secondary thread.

so the PDMFactory should move to a new access type, using a singleton that would be initialized upon the first use with no need to rely that PDMFactory::Init has been called.

Comment 1

[Jean-Yves Avenard [:jya]]

Attachment: MozReview Request: Bug 1271491: [WMF] P1. Don't use main thread only preferences methods. r?cpearce

Instead rely on MediaPrefs.

Review commit: https://reviewboard.mozilla.org/r/52171/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/52171/

Comment 2

[Jean-Yves Avenard [:jya]]

Attachment: MozReview Request: Bug 1271491: [WMF] P2. Allow creation of DXVA manager on any thread. r?cpearce

Review commit: https://reviewboard.mozilla.org/r/52173/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/52173/

Comment 3

[Jean-Yves Avenard [:jya]]

Attachment: MozReview Request: Bug 1271491: P2. Allow initialization of WMFPlatformDecoderModule from any threads. r?mattwoodrow

This is a very rough and lame attempt at making some parts of gfx thread-safe, however some parts rely on Preferences::AddBoolCache which isn't thread-safe.
Must still be called once gfx has been initialized.

Review commit: https://reviewboard.mozilla.org/r/52175/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/52175/

Comment 4

[Jean-Yves Avenard [:jya]]

Attachment: MozReview Request: Bug 1271491: P3. Remove refcounting the number of time apple's linkers are called. r?cpearce

This code was unused as Link() / Unlink() are only ever called once in the life time of the gecko.

Review commit: https://reviewboard.mozilla.org/r/52177/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/52177/

Comment 5

[Jean-Yves Avenard [:jya]]

Attachment: MozReview Request: Bug 1271491: [ffmpeg] P4. Remove requirements to call Init on the main thread. r?cpearce

FFMpegRuntimeLinker/FFVPXRuntimeLinker::Init() aren't thread safe, but they can be called on any threads.

Review commit: https://reviewboard.mozilla.org/r/52179/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/52179/

Comment 6

[Jean-Yves Avenard [:jya]]

Attachment: MozReview Request: Bug 1271491: [GMP] P5. Allow GMPDecoderModule::Init() to be called off the main thread. r?cpearce

Review commit: https://reviewboard.mozilla.org/r/52181/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/52181/

Comment 7

[Jean-Yves Avenard [:jya]]

Attachment: MozReview Request: Bug 1271491: [WMF] P7. Check that D3D didn't crash earlier. r?cpearce

Currently, the test launch a synchronous dispatch to the main thread. It may be acceptable to simply use an asynchronous check instead, using the cached result instead.

Review commit: https://reviewboard.mozilla.org/r/52183/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/52183/

Comment 8

[Jean-Yves Avenard [:jya]]

Attachment: MozReview Request: Bug 1271491: P6. Remove the need to call PDMFactory::Init(). r?cpearce

PDMFactory will automatically load and initialize the required frameworks upon first use.
Also fix constness of some methods.

Review commit: https://reviewboard.mozilla.org/r/52185/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/52185/

Comment 9

[Jean-Yves Avenard [:jya]]

Attachment: MozReview Request: Bug 1271491: P7. Remove unused members. r?alfredo

Review commit: https://reviewboard.mozilla.org/r/52187/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/52187/

Comment 10

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8751701 [details]
MozReview Request: Bug 1271491: P2. Allow initialization of WMFPlatformDecoderModule from any threads. r?mattwoodrow

https://reviewboard.mozilla.org/r/52175/#review49079

Comment 11

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

Comment on attachment 8751699 [details]
MozReview Request: Bug 1271491: [WMF] P1. Don't use main thread only preferences methods. r?cpearce

https://reviewboard.mozilla.org/r/52171/#review49211

Comment 12

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

Comment on attachment 8751700 [details]
MozReview Request: Bug 1271491: [WMF] P2. Allow creation of DXVA manager on any thread. r?cpearce

https://reviewboard.mozilla.org/r/52173/#review49213

Comment 13

[Matt Woodrow (:mattwoodrow)]

Is this blocking on the crash guard being made threadsafe?

That working is important to protect our users, I don't think it's ok to just break it.

Comment 14

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

Comment on attachment 8751702 [details]
MozReview Request: Bug 1271491: P3. Remove refcounting the number of time apple's linkers are called. r?cpearce

https://reviewboard.mozilla.org/r/52177/#review49229

I assume we're just letting the OS handle unlinking at shutdown.

Comment 15

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

Comment on attachment 8751703 [details]
MozReview Request: Bug 1271491: [ffmpeg] P4. Remove requirements to call Init on the main thread. r?cpearce

https://reviewboard.mozilla.org/r/52179/#review49231

Comment 16

[Jean-Yves Avenard [:jya]]

(In reply to Matt Woodrow (:mattwoodrow) from comment #13)
> Is this blocking on the crash guard being made threadsafe?
> 
> That working is important to protect our users, I don't think it's ok to
> just break it.

No, that's what P7 does; it fixes what you describe. It checks that the CrashGuard is set. I've made it use a sync dispatch for now (because other places in the DXVA always allocate the dxva on the main thread already).
I've made it easy to change that to a normal dispatch, in which case the value of the crash guard may be slightly out of data: but I don't think it matters.

Comment 17

[Jean-Yves Avenard [:jya]]

(In reply to Chris Pearce (:cpearce) from comment #14)
> Comment on attachment 8751702 [details]
> MozReview Request: Bug 1271491: P4. Remove refcounting the number of time
> apple's linkers are called. r?cpearce
> 
> https://reviewboard.mozilla.org/r/52177/#review49229
> 
> I assume we're just letting the OS handle unlinking at shutdown.

We are... In fact, there are many places in our code linking libraries that never unlink them... When main() exits, it's all unloaded.

Comment 18

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

Comment on attachment 8751704 [details]
MozReview Request: Bug 1271491: [GMP] P5. Allow GMPDecoderModule::Init() to be called off the main thread. r?cpearce

https://reviewboard.mozilla.org/r/52181/#review49265

::: dom/media/platforms/agnostic/gmp/GMPDecoderModule.cpp:188
(Diff revision 1)
>  {
> -  MOZ_ASSERT(NS_IsMainThread());
> -
> +  if (!NS_IsMainThread()) {
> +    nsCOMPtr<nsIThread> mainThread = do_GetMainThread();
> +    nsCOMPtr<nsIRunnable> runnable =
> +      NS_NewRunnableFunction([]() { Init(); });
> +    SyncRunnable::DispatchToThread(mainThread, runnable);

Sync dispatch. boo sux! But I understand why you need it. We'll need a better way to get the GMP codecs list updated.

Comment 19

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

Comment on attachment 8751705 [details]
MozReview Request: Bug 1271491: [WMF] P7. Check that D3D didn't crash earlier. r?cpearce

https://reviewboard.mozilla.org/r/52183/#review49277

Comment 20

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

Comment on attachment 8751706 [details]
MozReview Request: Bug 1271491: P6. Remove the need to call PDMFactory::Init(). r?cpearce

https://reviewboard.mozilla.org/r/52185/#review49279

::: dom/media/Benchmark.cpp:137
(Diff revision 1)
>  void
>  Benchmark::Init()
>  {
>    MOZ_ASSERT(NS_IsMainThread());
>  
> -  PDMFactory::Init();
> +  MediaPrefs::GetSingleton();

It seems that the only place that calls Benchmark::Init() is in gtest/TestMediaDataDecoder.cpp#26, so you may as well delete Benchmark::Init() and just call MediaPrefs::GetSingleton() at gtest/TestMediaDataDecoder.cpp#26.

Comment 21

[Jean-Yves Avenard [:jya]]

Comment on attachment 8751699 [details]
MozReview Request: Bug 1271491: [WMF] P1. Don't use main thread only preferences methods. r?cpearce

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52171/diff/1-2/

Comment 22

[Jean-Yves Avenard [:jya]]

Comment on attachment 8751701 [details]
MozReview Request: Bug 1271491: P2. Allow initialization of WMFPlatformDecoderModule from any threads. r?mattwoodrow

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52175/diff/1-2/

Comment 23

[Jean-Yves Avenard [:jya]]

Comment on attachment 8751702 [details]
MozReview Request: Bug 1271491: P3. Remove refcounting the number of time apple's linkers are called. r?cpearce

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52177/diff/1-2/

Comment 24

[Jean-Yves Avenard [:jya]]

Comment on attachment 8751703 [details]
MozReview Request: Bug 1271491: [ffmpeg] P4. Remove requirements to call Init on the main thread. r?cpearce

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52179/diff/1-2/

Comment 25

[Jean-Yves Avenard [:jya]]

Comment on attachment 8751704 [details]
MozReview Request: Bug 1271491: [GMP] P5. Allow GMPDecoderModule::Init() to be called off the main thread. r?cpearce

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52181/diff/1-2/

Comment 26

[Jean-Yves Avenard [:jya]]

Comment on attachment 8751706 [details]
MozReview Request: Bug 1271491: P6. Remove the need to call PDMFactory::Init(). r?cpearce

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52185/diff/1-2/

Comment 27

[Jean-Yves Avenard [:jya]]

Comment on attachment 8751707 [details]
MozReview Request: Bug 1271491: P7. Remove unused members. r?alfredo

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52187/diff/1-2/

Comment 28

[Jean-Yves Avenard [:jya]]

https://reviewboard.mozilla.org/r/52185/#review49279

> It seems that the only place that calls Benchmark::Init() is in gtest/TestMediaDataDecoder.cpp#26, so you may as well delete Benchmark::Init() and just call MediaPrefs::GetSingleton() at gtest/TestMediaDataDecoder.cpp#26.

having an Init function makes it simple to not have to modify the gtest or any users of that code (like the fuzz team which are using the benchmark as a way to test the decoders)

Comment 29

[Alfredo Yang (:alfredo)]

Comment on attachment 8751707 [details]
MozReview Request: Bug 1271491: P7. Remove unused members. r?alfredo

https://reviewboard.mozilla.org/r/52187/#review49355

Comment 30

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/57272b83ae53
https://hg.mozilla.org/integration/mozilla-inbound/rev/eda1e9a01c4e
https://hg.mozilla.org/integration/mozilla-inbound/rev/d645ebd74e86
https://hg.mozilla.org/integration/mozilla-inbound/rev/076253655f65
https://hg.mozilla.org/integration/mozilla-inbound/rev/30e33b58645e
https://hg.mozilla.org/integration/mozilla-inbound/rev/91217b1ad736
https://hg.mozilla.org/integration/mozilla-inbound/rev/65a790075868

Comment 31

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

https://hg.mozilla.org/mozilla-central/rev/57272b83ae53
https://hg.mozilla.org/mozilla-central/rev/eda1e9a01c4e
https://hg.mozilla.org/mozilla-central/rev/d645ebd74e86
https://hg.mozilla.org/mozilla-central/rev/076253655f65
https://hg.mozilla.org/mozilla-central/rev/30e33b58645e
https://hg.mozilla.org/mozilla-central/rev/91217b1ad736
https://hg.mozilla.org/mozilla-central/rev/65a790075868