Closed Bug 1271491 Opened 4 years ago Closed 4 years ago

PDMFactory shouldn't require initialisation in the main thread

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla49
Tracking Status
firefox49 --- fixed

People

(Reporter: jya, Assigned: jya)

References

(Depends on 1 open bug)

Details

Attachments

(7 files, 2 obsolete files)

58 bytes, text/x-review-board-request
cpearce
: review+
Details
58 bytes, text/x-review-board-request
mattwoodrow
: review+
Details
58 bytes, text/x-review-board-request
cpearce
: review+
Details
58 bytes, text/x-review-board-request
cpearce
: review+
Details
58 bytes, text/x-review-board-request
cpearce
: review+
Details
58 bytes, text/x-review-board-request
cpearce
: review+
Details
58 bytes, text/x-review-board-request
ayang
: review+
Details
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.
Assignee: nobody → jyavenard
Depends on: 1271538
Instead rely on MediaPrefs.

Review commit: https://reviewboard.mozilla.org/r/52171/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/52171/
Attachment #8751699 - Flags: review?(cpearce)
Attachment #8751700 - Flags: review?(cpearce)
Attachment #8751701 - Flags: review?(matt.woodrow)
Attachment #8751702 - Flags: review?(cpearce)
Attachment #8751703 - Flags: review?(cpearce)
Attachment #8751704 - Flags: review?(cpearce)
Attachment #8751705 - Flags: review?(cpearce)
Attachment #8751706 - Flags: review?(cpearce)
Attachment #8751707 - Flags: review?(ayang)
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/
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/
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/
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/
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 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
Attachment #8751701 - Flags: review?(matt.woodrow) → review+
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
Attachment #8751699 - Flags: review?(cpearce) → review+
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
Attachment #8751700 - Flags: review?(cpearce) → review+
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 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.
Attachment #8751702 - Flags: review?(cpearce) → review+
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
Attachment #8751703 - Flags: review?(cpearce) → review+
(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.
(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 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.
Attachment #8751704 - Flags: review?(cpearce) → review+
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
Attachment #8751705 - Flags: review?(cpearce) → review+
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.
Attachment #8751706 - Flags: review?(cpearce) → review+
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/
Attachment #8751701 - Attachment description: MozReview Request: Bug 1271491: P3. Allow initialization of WMFPlatformDecoderModule from any threads. r?mattwoodrow → MozReview Request: Bug 1271491: P2. Allow initialization of WMFPlatformDecoderModule from any threads. r?mattwoodrow
Attachment #8751702 - Attachment description: MozReview Request: Bug 1271491: P4. Remove refcounting the number of time apple's linkers are called. r?cpearce → MozReview Request: Bug 1271491: P3. Remove refcounting the number of time apple's linkers are called. r?cpearce
Attachment #8751703 - Attachment description: MozReview Request: Bug 1271491: [ffmpeg] P5. Remove requirements to call Init on the main thread. r?cpearce → MozReview Request: Bug 1271491: [ffmpeg] P4. Remove requirements to call Init on the main thread. r?cpearce
Attachment #8751704 - Attachment description: MozReview Request: Bug 1271491: [GMP] P6. Allow GMPDecoderModule::Init() to be called off the main thread. r?cpearce → MozReview Request: Bug 1271491: [GMP] P5. Allow GMPDecoderModule::Init() to be called off the main thread. r?cpearce
Attachment #8751706 - Attachment description: MozReview Request: Bug 1271491: P8. Remove the need to call PDMFactory::Init(). r?cpearce → MozReview Request: Bug 1271491: P6. Remove the need to call PDMFactory::Init(). r?cpearce
Attachment #8751707 - Attachment description: MozReview Request: Bug 1271491: P9. Remove unused members. r?alfredo → MozReview Request: Bug 1271491: P7. Remove unused members. r?alfredo
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 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 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 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 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 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/
Attachment #8751700 - Attachment is obsolete: true
Attachment #8751705 - Attachment is obsolete: true
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 on attachment 8751707 [details]
MozReview Request: Bug 1271491: P7. Remove unused members. r?alfredo

https://reviewboard.mozilla.org/r/52187/#review49355
Attachment #8751707 - Flags: review?(ayang) → review+
You need to log in before you can comment on or make changes to this bug.