Closed Bug 1457427 Opened 7 years ago Closed 7 years ago

Use ControlMessage to open a new driver from SourceStream

Categories

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

Product:
Core
Component:
Audio/Video: MediaStreamGraph
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla62
Tracking Flags:
Tracking Status
firefox62 --- fixed

People

(Reporter: achronop, Assigned: achronop)

Details

Attachments

(6 files)

Bug 1457427 - Restructure EnumerateDevices method is webrtc engine to improve readability.
59 bytes, text/x-review-board-request
pehrsons
: review+
Details
Bug 1457427 - Create a new GraphDriver method which performs the switching to next driver.
59 bytes, text/x-review-board-request
padenot
: review+
Details
Bug 1457427 - In SourceMediaStream request driver change through ControlMessage to avoid locking on non priority thread.
59 bytes, text/x-review-board-request
padenot
: review+
Details
Bug 1457427 - Improve OnThread method of AudioCallbackDriver to track the thread id of the audio thread so be more accurate.
59 bytes, text/x-review-board-request
padenot
: review+
Details
Bug 1457427 - Restrict the asserts of two MediaStreamGraph methods to align it with their callers.
59 bytes, text/x-review-board-request
padenot
: review+
Details
Bug 1457427 - Change the assert in SetInputListener of GraphDriver to verify that the driver is not started.
59 bytes, text/x-review-board-request
padenot
: review+
Details 
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.
Assignee: nobody → achronop
Rank: 19
Priority: -- → P2
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
Attachment #8971570 - Flags: review?(padenot) → review+
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.
Attachment #8971571 - Flags: review?(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
Attachment #8971572 - Flags: review?(padenot) → review+
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.
Attachment #8971571 - Flags: review+
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
Attachment #8971573 - Flags: review?(padenot) → review+
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.
Attachment #8971574 - Flags: review?(padenot) → review+
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(...);
}
```
Attachment #8971569 - Flags: review?(apehrson)
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 on attachment 8971569 [details]
Bug 1457427 - Restructure EnumerateDevices method is webrtc engine to improve readability.

https://reviewboard.mozilla.org/r/240332/#review247466
Attachment #8971569 - Flags: review?(apehrson) → review+
Pushed by achronop@gmail.com:
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
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: