1457427 - Use ControlMessage to open a new driver from SourceStream

Status: RESOLVED FIXED

(Core :: Audio/Video: MediaStreamGraph, enhancement, P2)

Description

[Alex Chronopoulos [:achronop]]

SourceMediaStream::OpenNewAudioCallbackDriver opens a new AudioCallbackDriver every time the channel count constraint is updated, on a track coming from a gUM request. Currently the MSG mutex is locked for that update. The correct way to do it is to send a ControlMessage to the MSG that will cause the update.

Besides the above this bug includes the followings:

OnThread assert of AudioCallbackDriver has been improved to track if we are in audio thread or not. A flag had been used till now. The flag is able to tell if the callback is on at a specific time but it's not possible to tell if we are on that thread.

`MediaStreamGraphImpl::UpdateStreamOrder()` and  `MediaStreamGraphImpl::CreateOrDestroyAudioStreams()` methods, assert for `OnGraphThreadOrNotRunning`, which is not necessary since they are called once, from the methods `UpdateGraph()` and `Process` methods respectively which conatin a more limited assert check: `OnGraphThread`.

Small restructuring GraphDriver, switchin code has been wrapped around a helper method. Not a real change is the functionality.

Small restructuring in MediaEngineWebRTC.cpp to improve readability. Not real change in functionality. A bit unrelated to MSG/GraphDriver. I include it here to avoid creating a separate bug for a low priority change.

Comment 1

[Alex Chronopoulos [:achronop]]

Attachment: Bug 1457427 - Restructure EnumerateDevices method is webrtc engine to improve readability.

Restructuring of the existing code no functional change.

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

Comment 2

[Alex Chronopoulos [:achronop]]

Attachment: Bug 1457427 - Create a new GraphDriver method which performs the switching to next driver.

Restructuring of the existing code no functional change.

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

Comment 3

[Alex Chronopoulos [:achronop]]

Attachment: Bug 1457427 - In SourceMediaStream request driver change through ControlMessage to avoid locking on non priority thread.

That method is used on update of channelCount constraint. By raising a ControlMessage to MediaStreamGraph we avoid the lock the mutex on a non priority thread. Unfortunately we have to send the message in main thread first, thankfully this will change soon.

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

Comment 4

[Alex Chronopoulos [:achronop]]

Attachment: Bug 1457427 - Improve OnThread method of AudioCallbackDriver to track the thread id of the audio thread so be more accurate.

The class has been enhanced to store the thread id at the beginning of the callback and use it in order to compare with the current thread id when OnThread method is called. The old mechanism which includes the mInCallback flag has been removed.

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

Comment 5

[Alex Chronopoulos [:achronop]]

Attachment: Bug 1457427 - Restrict the asserts of two MediaStreamGraph methods to align it with their callers.

These two methods assert for OnGraphThreadOrNotRunning when their callers assert for OnGraphThread. Since the second assert is more limited we do not have to assert for a wider eventuality.

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

Comment 6

[Alex Chronopoulos [:achronop]]

Attachment: Bug 1457427 - Change the assert in SetInputListener of GraphDriver to verify that the driver is not started.

With the updated OnThread method SetInputListener method cannot continue asserting for being in audio callback. This check is not realistic since it is used on a newly created driver before being started.

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

Comment 7

[Alex Chronopoulos [:achronop]]

Try run:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=09514730e6bb610c9dc0a8c5ea4ab249a3a620e4
and some corrections:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=3efce4ba3f47dc2800e19f36e50902a5364d97ed

Comment 8

[Paul Adenot (:padenot)]

Comment on attachment 8971570 [details]
Bug 1457427 - Create a new GraphDriver method which performs the switching to next driver.

https://reviewboard.mozilla.org/r/240334/#review246444

Comment 9

[Paul Adenot (:padenot)]

Comment on attachment 8971571 [details]
Bug 1457427 - In SourceMediaStream request driver change through ControlMessage to avoid locking on non priority thread.

https://reviewboard.mozilla.org/r/240336/#review246448

This is unnecessary with bug 1404977.

Comment 10

[Paul Adenot (:padenot)]

Comment on attachment 8971572 [details]
Bug 1457427 - Improve OnThread method of AudioCallbackDriver to track the thread id of the audio thread so be more accurate.

https://reviewboard.mozilla.org/r/240338/#review246450

Comment 11

[Paul Adenot (:padenot)]

Comment on attachment 8971571 [details]
Bug 1457427 - In SourceMediaStream request driver change through ControlMessage to avoid locking on non priority thread.

https://reviewboard.mozilla.org/r/240336/#review246452

That said you'll probably land before me.

Comment 12

[Paul Adenot (:padenot)]

Comment on attachment 8971573 [details]
Bug 1457427 - Restrict the asserts of two MediaStreamGraph methods to align it with their callers.

https://reviewboard.mozilla.org/r/240340/#review246454

Comment 13

[Paul Adenot (:padenot)]

Comment on attachment 8971574 [details]
Bug 1457427 - Change the assert in SetInputListener of GraphDriver to verify that the driver is not started.

https://reviewboard.mozilla.org/r/240342/#review246456

Again not necessary with my changes, but fine to land now, I'll rebase on top.

Comment 14

[Alex Chronopoulos [:achronop]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8a52ed4fdc284c84a4c418913ec05df1b8f5033a

Comment 15

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8971569 [details]
Bug 1457427 - Restructure EnumerateDevices method is webrtc engine to improve readability.

https://reviewboard.mozilla.org/r/240332/#review246898

::: dom/media/webrtc/MediaEngineWebRTC.cpp
(Diff revision 1)
> -  if (MediaEngineSource::IsVideo(aMediaSource)) {
> -    // We spawn threads to handle gUM runnables, so we must protect the member vars
> -    MutexAutoLock lock(mMutex);

Skipping the lock looks like a functional change. Since this is now done by the caller, assert here that the mutex is locked by us.

::: dom/media/webrtc/MediaEngineWebRTC.cpp:261
(Diff revision 1)
> -      return;
> -    }
> +}
>  
> +void
> +MediaEngineWebRTC::EnumerateMicrophoneDevices(uint64_t aWindowId,
> +                                              dom::MediaSourceEnum aMediaSource,

`aMediaSource` is not used as this is always for microphones. Remove.

::: dom/media/webrtc/MediaEngineWebRTC.cpp:263
(Diff revision 1)
>  
> +void
> +MediaEngineWebRTC::EnumerateMicrophoneDevices(uint64_t aWindowId,
> +                                              dom::MediaSourceEnum aMediaSource,
> +                                              nsTArray<RefPtr<MediaEngineSource> >* aSources)
> +{

Assert that the mutex is held by us.
Preferably add a thread assert to these methods you are touching as well, if possible.

::: dom/media/webrtc/MediaEngineWebRTC.cpp:334
(Diff revision 1)
> +  } else {
> +    EnumerateMicrophoneDevices(aWindowId, aMediaSource, aSources);

Assert that aMediaSource is now microphone, or do ```
else if (aMediaSource == dom::MediaSourceEnum::Microphone) {
  ...
} else {
  MOZ_CRASH(...);
}
```

Comment 16

[Alex Chronopoulos [:achronop]]

Comment on attachment 8971569 [details]
Bug 1457427 - Restructure EnumerateDevices method is webrtc engine to improve readability.

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

Comment 17

[Alex Chronopoulos [:achronop]]

Comment on attachment 8971570 [details]
Bug 1457427 - Create a new GraphDriver method which performs the switching to next driver.

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

Comment 18

[Alex Chronopoulos [:achronop]]

Comment on attachment 8971571 [details]
Bug 1457427 - In SourceMediaStream request driver change through ControlMessage to avoid locking on non priority thread.

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

Comment 19

[Alex Chronopoulos [:achronop]]

Comment on attachment 8971572 [details]
Bug 1457427 - Improve OnThread method of AudioCallbackDriver to track the thread id of the audio thread so be more accurate.

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

Comment 20

[Alex Chronopoulos [:achronop]]

Comment on attachment 8971573 [details]
Bug 1457427 - Restrict the asserts of two MediaStreamGraph methods to align it with their callers.

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

Comment 21

[Alex Chronopoulos [:achronop]]

Comment on attachment 8971574 [details]
Bug 1457427 - Change the assert in SetInputListener of GraphDriver to verify that the driver is not started.

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

Comment 22

[Alex Chronopoulos [:achronop]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=685990d3d473028b5d507ed683f9b04b26b3b756

Comment 23

[Alex Chronopoulos [:achronop]]

Comment on attachment 8971569 [details]
Bug 1457427 - Restructure EnumerateDevices method is webrtc engine to improve readability.

https://reviewboard.mozilla.org/r/240332/#review246898

> Skipping the lock looks like a functional change. Since this is now done by the caller, assert here that the mutex is locked by us.

Updated

> `aMediaSource` is not used as this is always for microphones. Remove.

Updated

> Assert that the mutex is held by us.
> Preferably add a thread assert to these methods you are touching as well, if possible.

Updated

> Assert that aMediaSource is now microphone, or do ```
> else if (aMediaSource == dom::MediaSourceEnum::Microphone) {
>   ...
> } else {
>   MOZ_CRASH(...);
> }
> ```

Updated

Comment 24

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8971569 [details]
Bug 1457427 - Restructure EnumerateDevices method is webrtc engine to improve readability.

https://reviewboard.mozilla.org/r/240332/#review247466

Comment 25

[Alex Chronopoulos [:achronop]]

Comment on attachment 8971569 [details]
Bug 1457427 - Restructure EnumerateDevices method is webrtc engine to improve readability.

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

Comment 26

[Alex Chronopoulos [:achronop]]

Comment on attachment 8971570 [details]
Bug 1457427 - Create a new GraphDriver method which performs the switching to next driver.

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

Comment 27

[Alex Chronopoulos [:achronop]]

Comment on attachment 8971571 [details]
Bug 1457427 - In SourceMediaStream request driver change through ControlMessage to avoid locking on non priority thread.

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

Comment 28

[Alex Chronopoulos [:achronop]]

Comment on attachment 8971572 [details]
Bug 1457427 - Improve OnThread method of AudioCallbackDriver to track the thread id of the audio thread so be more accurate.

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

Comment 29

[Alex Chronopoulos [:achronop]]

Comment on attachment 8971573 [details]
Bug 1457427 - Restrict the asserts of two MediaStreamGraph methods to align it with their callers.

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

Comment 30

[Alex Chronopoulos [:achronop]]

Comment on attachment 8971574 [details]
Bug 1457427 - Change the assert in SetInputListener of GraphDriver to verify that the driver is not started.

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

Comment 31

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b8edee5bd7d2
Restructure EnumerateDevices method is webrtc engine to improve readability. r=pehrsons
https://hg.mozilla.org/integration/autoland/rev/1b0e22638045
Create a new GraphDriver method which performs the switching to next driver. r=padenot
https://hg.mozilla.org/integration/autoland/rev/dc1bf6cad8ce
In SourceMediaStream request driver change through ControlMessage to avoid locking on non priority thread. r=padenot
https://hg.mozilla.org/integration/autoland/rev/dd269b789ebf
Improve OnThread method of AudioCallbackDriver to track the thread id of the audio thread so be more accurate. r=padenot
https://hg.mozilla.org/integration/autoland/rev/b3a339994f64
Restrict the asserts of two MediaStreamGraph methods to align it with their callers. r=padenot
https://hg.mozilla.org/integration/autoland/rev/c5e2684bc485
Change the assert in SetInputListener of GraphDriver to verify that the driver is not started. r=padenot

Comment 32

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/b8edee5bd7d2
https://hg.mozilla.org/mozilla-central/rev/1b0e22638045
https://hg.mozilla.org/mozilla-central/rev/dc1bf6cad8ce
https://hg.mozilla.org/mozilla-central/rev/dd269b789ebf
https://hg.mozilla.org/mozilla-central/rev/b3a339994f64
https://hg.mozilla.org/mozilla-central/rev/c5e2684bc485

Comment 33

[Alex Chronopoulos [:achronop]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a8e3334c78c2fa921fe5bb0688833a12a3b93088