1212795 - Add methods to load the various frameworks required by the PDM

Status: RESOLVED FIXED

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

Description

[Jean-Yves Avenard [:jya]]

The PlatformDecoder module relies on some frameworks to be available which are typically loaded by gfx.

In the past, we had crashes due to an apple video decoder being created prior the gfx layer.

We shouldn't rely on some frameworks to be loaded by other parts of the code.

It will also help when making gtests.

Comment 1

[Jean-Yves Avenard [:jya]]

Attachment: P1. Ensure the check if HW acceleration is allowed is performed on the main thread.

The downside is that a restart is now required if the user change the preference to force HW decoding ; however this is the same behaviour as on Windows.

Comment 2

[Jean-Yves Avenard [:jya]]

Attachment: P2. Ensure all frameworks required for video decoding on mac are loaded.

We used to rely on the compositor to have been started, which is a dangerous assumption..

Comment 3

[Jean-Yves Avenard [:jya]]

Attachment: P3. Make AppleDecoderModule detects if the required modules are loaded.

Comment 4

[JW Wang [:jwwang] [:jw_wang]]

Comment on attachment 8671753 [details] [diff] [review]
P3. Make AppleDecoderModule detects if the required modules are loaded.

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

::: dom/media/platforms/apple/AppleDecoderModule.cpp
@@ +120,5 @@
>  {
> +  return (sIsCoreMediaAvailable &&
> +          (aMimeType.EqualsLiteral("audio/mpeg") ||
> +           aMimeType.EqualsLiteral("audio/mp4a-latm"))) ||
> +    (MacIOSurfaceLib::isInit() && (sIsVTAvailable || sIsVDAAvailable) &&

The implementation of MacIOSurfaceLib::isInit() looks racy to me which calls LoadLibrary() if |isLoaded| is false. I suppose MacIOSurfaceLib::isInit() should be called on the main thread only otherwise it depends on a tricky call flow to ensure LoadLibrary() never happens off the main thread.

It would make code clearer if AppleDecoderModule::Init() stores the result of MacIOSurfaceLib::isInit() after calling MacIOSurfaceLib::LoadLibrary().

Comment 5

[Jean-Yves Avenard [:jya]]

(In reply to JW Wang [:jwwang] from comment #4)
> Comment on attachment 8671753 [details] [diff] [review]
> P3. Make AppleDecoderModule detects if the required modules are loaded.
> 
> Review of attachment 8671753 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/media/platforms/apple/AppleDecoderModule.cpp
> @@ +120,5 @@
> >  {
> > +  return (sIsCoreMediaAvailable &&
> > +          (aMimeType.EqualsLiteral("audio/mpeg") ||
> > +           aMimeType.EqualsLiteral("audio/mp4a-latm"))) ||
> > +    (MacIOSurfaceLib::isInit() && (sIsVTAvailable || sIsVDAAvailable) &&
> 
> The implementation of MacIOSurfaceLib::isInit() looks racy to me which calls
> LoadLibrary() if |isLoaded| is false. I suppose MacIOSurfaceLib::isInit()
> should be called on the main thread only otherwise it depends on a tricky
> call flow to ensure LoadLibrary() never happens off the main thread.

lots of code in the gfx base is racy.

However here, the initialization always occur in the main thread and no thread will call isInit until the intialization is completed on the main thread.

So this is not racy.

(But I still put the MOZ_ASSERT exactly to remove the ambiguity)

But I'll cache the result (though one could argue that accessing the cache value itself would be racy :) ) as isInit also reads its own cache value

Comment 6

[Jean-Yves Avenard [:jya]]

Probably better to modify MacIOSurfaceLib::isInit() to make it safe-thread (simply making MacIOSurfaceLib::isLoaded atomic.

Comment 7

[JW Wang [:jwwang] [:jw_wang]]

Is MacIOSurfaceLib::LoadLibrary() safe to be called on all threads?

Comment 8

[Jean-Yves Avenard [:jya]]

(In reply to JW Wang [:jwwang] from comment #7)
> Is MacIOSurfaceLib::LoadLibrary() safe to be called on all threads?

All the functions I've ever seen in our code loading a framework or an external library do so on the main thread.
This one is no exception.

Comment 9

[Jean-Yves Avenard [:jya]]

Attachment: P4: Make MacIOSurfaceLib::isLoaded atomic.

This static boolean is accessed across multiple threads

Comment 10

[JW Wang [:jwwang] [:jw_wang]]

Comment on attachment 8671788 [details] [diff] [review]
P4: Make MacIOSurfaceLib::isLoaded atomic.

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

It doesn't fix the race for a thread (non main thread) can see MacIOSurfaceLib::isInit() returns true and start using MacIOSurfaceLib before MacIOSurfaceLib::LoadLibrary() is returned on the main thread.

|MOZ_ASSERT(sIOSurfaceFramework)| in MacIOSurfaceLib::isInit() is also racy if we allow MacIOSurfaceLib::isInit() to be called off the main thread for the main thread at the same time could be updating |sIOSurfaceFramework| in MacIOSurfaceLib::LoadLibrary().

To avoid race, the client code should establish an order where that MacIOSurfaceLib::isInit() returns true is observed on the main thread before any other calls to MacIOSurfaceLib.

Comment 11

[Jean-Yves Avenard [:jya]]

(In reply to JW Wang [:jwwang] from comment #10)
> Comment on attachment 8671788 [details] [diff] [review]
> P4: Make MacIOSurfaceLib::isLoaded atomic.
> 
> Review of attachment 8671788 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> It doesn't fix the race for a thread (non main thread) can see
> MacIOSurfaceLib::isInit() returns true and start using MacIOSurfaceLib
> before MacIOSurfaceLib::LoadLibrary() is returned on the main thread.
> 

There's no race. 

I don't see the point in fixing a case that do not happen. 

No thread will ever be kicked off until the main thread as completed

The MediaDecoderReader won't be started until this main thread task has been completed. 


> |MOZ_ASSERT(sIOSurfaceFramework)| in MacIOSurfaceLib::isInit() is also racy
> if we allow MacIOSurfaceLib::isInit() to be called off the main thread for
> the main thread at the same time could be updating |sIOSurfaceFramework| in
> MacIOSurfaceLib::LoadLibrary().
> 
> To avoid race, the client code should establish an order where that
> MacIOSurfaceLib::isInit() returns true is observed on the main thread before
> any other calls to MacIOSurfaceLib.

Comment 12

[JW Wang [:jwwang] [:jw_wang]]

(In reply to Jean-Yves Avenard [:jya] from comment #11)
> There's no race. 
> 
> I don't see the point in fixing a case that do not happen. 
> 
> No thread will ever be kicked off until the main thread as completed
> 
> The MediaDecoderReader won't be started until this main thread task has been
> completed. 

I am talking about a generate case other than our media code. The p4 change implies it is safe to call MacIOSurfaceLib::isInit() off the main thread but in fact it is not. Our media code is using MacIOSurfaceLib::isInit() in a right way and we don't need p4 to complete this bug.

Comment 13

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0c37c92b4532
https://hg.mozilla.org/integration/mozilla-inbound/rev/1b1595492af0
https://hg.mozilla.org/integration/mozilla-inbound/rev/abdbd41a5c04

Comment 14

[Guilherme Lima]

This landed this morning
https://hg.mozilla.org/mozilla-central/rev/0c37c92b4532
https://hg.mozilla.org/mozilla-central/rev/1b1595492af0
https://hg.mozilla.org/mozilla-central/rev/abdbd41a5c04