Add methods to load the various frameworks required by the PDM

RESOLVED FIXED in Firefox 44

Status

()

Core
Audio/Video: Playback
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: jya, Assigned: jya)

Tracking

Trunk
mozilla44
Points:
---

Firefox Tracking Flags

(firefox44 fixed)

Details

Attachments

(4 attachments)

(Assignee)

Description

2 years ago
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.
(Assignee)

Comment 1

2 years ago
Created attachment 8671741 [details] [diff] [review]
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.
Attachment #8671741 - Flags: review?(jwwang)
(Assignee)

Updated

2 years ago
Assignee: nobody → jyavenard
(Assignee)

Comment 2

2 years ago
Created attachment 8671752 [details] [diff] [review]
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..
Attachment #8671752 - Flags: review?(jwwang)
(Assignee)

Comment 3

2 years ago
Created attachment 8671753 [details] [diff] [review]
P3. Make AppleDecoderModule detects if the required modules are loaded.
Attachment #8671753 - Flags: review?(jwwang)
Attachment #8671741 - Flags: review?(jwwang) → review+
Attachment #8671752 - Flags: review?(jwwang) → review+
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().
Attachment #8671753 - Flags: review?(jwwang) → review+
(Assignee)

Comment 5

2 years ago
(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
(Assignee)

Comment 6

2 years ago
Probably better to modify MacIOSurfaceLib::isInit() to make it safe-thread (simply making MacIOSurfaceLib::isLoaded atomic.
Is MacIOSurfaceLib::LoadLibrary() safe to be called on all threads?
(Assignee)

Comment 8

2 years ago
(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.
(Assignee)

Comment 9

2 years ago
Created attachment 8671788 [details] [diff] [review]
P4: Make MacIOSurfaceLib::isLoaded atomic.

This static boolean is accessed across multiple threads
Attachment #8671788 - Flags: review?(jwwang)
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.
Attachment #8671788 - Flags: review?(jwwang) → review-
(Assignee)

Comment 11

2 years ago
(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.
(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 14

2 years ago
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
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox44: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
You need to log in before you can comment on or make changes to this bug.