Closed Bug 1404977 Opened 7 years ago Closed 6 years ago

Rework device enumeration

Categories

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

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

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla63
Tracking Flags:
Tracking Status
firefox63 --- fixed

People

(Reporter: padenot, Assigned: padenot)

References

(Depends on 6 open bugs, Blocks 2 open bugs, Regressed 1 open bug)

Details

(Keywords: feature)

Attachments

(19 files, 40 obsolete files)

Bug 1404977 - Part 11 - Make sure the default device is the first element in the list.
59 bytes, text/x-review-board-request
pehrsons
: review+
Details
Bug 1404977 - Tests P1: Add test to ensure multiple, concurrent gUM calls in separate iframes succeed.
59 bytes, text/x-review-board-request
pehrsons
: review+
Details
Bug 1404977 - Tests P2: Add test to ensure multiple, concurrent gUM calls in a single window succeed.
59 bytes, text/x-review-board-request
pehrsons
: review+
Details
Bug 1404977 - Part 1 - Add missing lock to the PREF_CUBEB_BACKEND branch of the pref callback in CubebUtils.cpp.
59 bytes, text/x-review-board-request
pehrsons
: review+
Details
Bug 1404977 - Part 2 - Augment AudioDeviceInfo with a cubeb device id.
59 bytes, text/x-review-board-request
pehrsons
: review+
Details
Bug 1404977 - Part 3 - Remove global statics, introduce an audio device enumerator
59 bytes, text/x-review-board-request
pehrsons
: review+
Details
Bug 1404977 - Part 5 - Allow querying the number of input channels from a WebRTCAudioDataListener.
59 bytes, text/x-review-board-request
pehrsons
: review+
Details
Bug 1404977 - Part 6 - Remove unused include for lock-free FIFO.
59 bytes, text/x-review-board-request
pehrsons
: review+
Details
Bug 1404977 - Part 7 - Make each MediaEngineWebRTCMicrophoneSource independent.
59 bytes, text/x-review-board-request
pehrsons
: review+
Details
Bug 1404977 - Part 8 - Tell the MSG the MediaEngineAudioSource are now independent and that we can have multiple of them, cleanup the MSG-side API for managing them.
59 bytes, text/x-review-board-request
pehrsons
: review+
Details
Bug 1404977 - Part 9 - Propagate the changes to the GraphDrivers, simplifying them, and brokering all access through the MSG.
59 bytes, text/x-review-board-request
pehrsons
: review+
Details
Bug 1404977 - Part 10 - Make logging uniform, and consistently use GraphImpl() to access the graph impl.
59 bytes, text/x-review-board-request
pehrsons
: review+
Details
Bug 1404977 - Part 12 - Make DeviceChanged() notification thread safe by using the MSG message queue.
59 bytes, text/x-review-board-request
pehrsons
: review+
Details
Bug 1404977 - Part 13 - Remove useless mutex, and assert why they were useless, in WebRTCAudioDataListeners.
59 bytes, text/x-review-board-request
pehrsons
: review+
Details
Bug 1404977 - Part 14 - Add a way to set the global cubeb* singleton at runtime, from a test.
59 bytes, text/x-review-board-request
pehrsons
: review+
Details
Bug 1404977 - Part 15 - Invalidate the device cache before re-enumerating devices when the cubeb backend does not support dynamic device collection invalidation.
59 bytes, text/x-review-board-request
pehrsons
: review+
Details
Bug 1404977 - Part 16 - Unit test CubebDeviceEnumerator.
59 bytes, text/x-review-board-request
pehrsons
: review+
Details
Bug 1404977 - Part 17 - Re-implement the workaround for the lack of input device enumeration on Android.
59 bytes, text/x-review-board-request
pehrsons
: review+
Details
Bug 1404977 - Exit NotifyInputData early if there's no input listener.
59 bytes, text/x-review-board-request
achronop
: review+
Details 
Refactor the device enumeration code to use something that is closer to cubeb's way of doing things: a single enumerate call, and then iterate on the collection. We need to dissociate the old code from the new code, so that we can remove the old code without much risk later. There is very little value is shoe-horning the new cubeb API in terms of the old API (GetNumOfRecordingDevices, GetRecordingDeviceName with an index, etc.), these days, because all of our tier-1 platforms use duplex, and it's unclear if we want to keep an non-duplex path.
Blocks: webrtc-call-quality
Depends on: 1404976
Depends on: 1404975
Assignee: nobody → padenot
Rank: 9
No longer depends on: 1404975, 1404976
Priority: P3 → P1
Depends on: 1404978
Depends on: 1404976, 1404975, 1404980
Depends on: 1404979
@padenot: similar here: because of the extra attention for the 57 release, if this is really a P1 bug we need to mark which versions are affect. As this sounds to me like it's refactoring of working code to improve things I would think this is a P2.
Flags: needinfo?(padenot)
Tech debt does not qualify as P1 under the new priority schema. Adjusting to top-rank P2.
Rank: 9 → 10
Priority: P1 → P2
Flags: needinfo?(padenot)
Depends on: 1299515
Bryce, those are the patches I was talking about via email, rebase to current central. They make your thing much easier to implement, I think.
Flags: needinfo?(bvandyk)
Cheers, will do.
Flags: needinfo?(bvandyk)
This looks close to the work I am doing for Bug 1152401. How far have you get with it? Can I steal it?
This is cleaned-up a bit and split for easier review. More clean-ups can be done (this is only touching stuff below MediaEngineWebRTC), but at least now we can have a mic per document.

What is needed is tests, of course.

https://paul.cx/public/testing-multi-gum.html allows manual testing. I know Bryce has a better version but I lost the link.
Pen with my modified version: https://codepen.io/SingingTree/pen/yjaRzv

I've been working on some tests locally that gUM streams with different constraints and check to see that you can get multiple streams and that the constraints all hold (with and without iframes). I'll grab these patches now, rebase, and see how the tests are looking.
Comment on attachment 8968567 [details]
Bug 1404977 - Part 1 - Add missing lock to the PREF_CUBEB_BACKEND branch of the pref callback in CubebUtils.cpp.

https://reviewboard.mozilla.org/r/237254/#review246906

::: dom/media/CubebUtils.cpp:249
(Diff revision 2)
> -    if (value.IsEmpty()) {
> -      sCubebBackendName = nullptr;
> -    } else {
> +  } else if (strcmp(aPref, PREF_CUBEB_OUTPUT_DEVICE) == 0) {
> +    StaticMutexAutoLock lock(sMutex);
> +    GetPrefAndSetString(aPref, sCubebOutputDeviceName);

How does this swap existing cubeb streams to the new output device?

If it doesn't, file a bug and add a comment.

::: dom/media/GraphDriver.cpp:601
(Diff revision 2)
>      MonitorAutoLock lock(GraphImpl()->GetMonitor());
>      FallbackToSystemClockDriver();
>      return true;
>    }
>  
> +  cubeb_devid forcedOutputDeviceId  = nullptr;

s/  =/ =/

::: dom/media/GraphDriver.cpp:606
(Diff revision 2)
> +  cubeb_devid forcedOutputDeviceId  = nullptr;
> +  char* forcedOutputDeviceName = CubebUtils::GetForcedOutputDevice();
> +  if (forcedOutputDeviceName) {
> +    nsTArray<RefPtr<AudioDeviceInfo>> deviceInfos;
> +    GetDeviceCollection(deviceInfos, CubebUtils::Output);
> +    for (auto device : deviceInfos) {

s/auto/const auto&/

::: dom/media/tests/mochitest/test_getUserMedia_basicAudio_loopback.html:24
(Diff revision 2)
> -
> +    await SpecialPowers.pushPrefEnv({"set": [
> +      ["media.cubeb.output_device", audioDevice]
> +    ]});

`audioDevice` is the input device, not the output one, no?

This shouldn't be needed. Set the pref in the test runner instead: https://searchfox.org/mozilla-central/rev/795575287259a370d00518098472eaa5b362bfa7/testing/mochitest/runtests.py#1912
Attachment #8968567 - Flags: review?(apehrson)
Comment on attachment 8968568 [details]
Bug 1404977 - Part 2 - Augment AudioDeviceInfo with a cubeb device id.

https://reviewboard.mozilla.org/r/237256/#review246912

::: commit-message-cc6af:1
(Diff revision 2)
> +Bug 1404977 - Part 2 - Augment AudioDeviceID with a cubeb device id. r?pehrsons

It seems to me like we need
s/AudioDeviceID/AudioDeviceInfo/

::: dom/media/AudioDeviceInfo.h:24
(Diff revision 2)
> +  AudioDeviceInfo(const nsAString& aName,
> +                  const nsAString& aGroupId,
> +                  const nsAString& aVendor,
> +                  uint16_t aType,
> +                  uint16_t aState,
> +                  uint16_t aPreferred,
> +                  uint16_t aSupportedFormat,
> +                  uint16_t aDefaultFormat,
> +                  uint32_t aMaxChannels,
> +                  uint32_t aDefaultRate,
> +                  uint32_t aMaxRate,
> +                  uint32_t aMinRate,
> +                  uint32_t aMaxLatency,
> +                  uint32_t aMinLatency);

This ctor doesn't seem to be used per [1], is it needed?

Supporting both having an ID and not does add a bit of complexity that would be great to get rid of.


[1] https://searchfox.org/mozilla-central/search?q=symbol:_ZN15AudioDeviceInfoC1ERK12nsTSubstringIDsES3_S3_tttttjjjjjj&redirect=false

::: dom/media/AudioDeviceInfo.h:66
(Diff revision 2)
> +  AudioDeviceID mDeviceId;
>    nsString mName;
>    nsString mGroupId;
>    nsString mVendor;
>    uint16_t mType;
>    uint16_t mState;
>    uint16_t mPreferred;
>    uint16_t mSupportedFormat;
>    uint16_t mDefaultFormat;
>    uint32_t mMaxChannels;
>    uint32_t mDefaultRate;
>    uint32_t mMaxRate;
>    uint32_t mMinRate;
>    uint32_t mMaxLatency;
>    uint32_t mMinLatency;

Hmm, any reason these are not const?

::: dom/media/AudioDeviceInfo.cpp:14
(Diff revision 2)
>  
> -AudioDeviceInfo::AudioDeviceInfo(const nsAString& aName,
> +using namespace mozilla;
> +using namespace mozilla::CubebUtils;
> +
> +AudioDeviceInfo::AudioDeviceInfo(cubeb_device_info* aInfo)
> +:AudioDeviceInfo(aInfo->devid,

indentation
Attachment #8968568 - Flags: review?(apehrson)
Comment on attachment 8972010 [details]
Bug 1404977 - Part 3 - Remove global statics, introduce an audio device enumerator

https://reviewboard.mozilla.org/r/240750/#review246916

This is not a complete review. There are enough open issues to warrant a deeper look later.

::: commit-message-77496:1
(Diff revision 1)
> +Bug 1404977 - Part 3 - Remove global statics, introduce and audio device enumerator r?pehrsons

s/and/an/

::: dom/media/webrtc/MediaEngineWebRTC.h:125
(Diff revision 1)
>  
>  protected:
>    virtual ~MediaEngineWebRTCAudioCaptureSource() = default;
>  };
>  
> -// Small subset of VoEHardware
> +struct CubebDeviceCollectionDestroyFunctor

Why/where is this needed?

::: dom/media/webrtc/MediaEngineWebRTC.h:133
(Diff revision 1)
> -  int SetRecordingDevice(int aIndex)
> +// This class implement a cache for accessing the audio device list. It can be
> +// access on any thread.

s/implement/implements/
s/access/accessed/

::: dom/media/webrtc/MediaEngineWebRTC.h:133
(Diff revision 1)
> -  int SetRecordingDevice(int aIndex)
> +// This class implement a cache for accessing the audio device list. It can be
> +// access on any thread.

IMO, which threads can call what methods should generally go in the method documentations.

::: dom/media/webrtc/MediaEngineWebRTC.h:135
(Diff revision 1)
>    }
> +};
>  
> -  int SetRecordingDevice(int aIndex)
> +// This class implement a cache for accessing the audio device list. It can be
> +// access on any thread.
> +class CubebDeviceEnumerator final

I'd love to see unit tests of this class on top of a mocked cubeb impl.

::: dom/media/webrtc/MediaEngineWebRTC.h:140
(Diff revision 1)
> +class CubebDeviceEnumerator final
> -  {
> +{
> -    mSelectedDevice = aIndex;
> -    return 0;
> -  }
> +NS_INLINE_DECL_THREADSAFE_REFCOUNTING(CubebDeviceEnumerator)
> +public:
> +  CubebDeviceEnumerator();
> +  void EnumerateAudioInputDevice(nsTArray<RefPtr<AudioDeviceInfo>>& aDevices);

s/EnumerateAudioInputDevice/EnumerateAudioInputDevices/

::: dom/media/webrtc/MediaEngineWebRTC.h:158
(Diff revision 1)
> -  // list.  This is updated on mDevices updates.  Many devices in mDevices
> -  // won't be included in the array (wrong type, etc), or if a device is
> +  Mutex mMutex;
> +  nsTArray<RefPtr<AudioDeviceInfo>> mDevices;

The mutex protects mDevices I assume. Please document.

::: dom/media/webrtc/MediaEngineWebRTC.h:458
(Diff revision 1)
>    // gUM runnables can e.g. Enumerate from multiple threads
>    Mutex mMutex;
> -  RefPtr<mozilla::AudioInput> mAudioInput;
> -  bool mFullDuplex;
> +  RefPtr<mozilla::CubebDeviceEnumerator> mEnumerator;
> +  bool mFullDuplex;  
>    bool mDelayAgnostic;
>    bool mExtendedFilter;
>    bool mHasTabVideoSource;

This needs info on what is protected by mMutex.

::: dom/media/webrtc/MediaEngineWebRTC.h:461
(Diff revision 1)
>    nsCOMPtr<nsIThread> mThread;
>  
>    // gUM runnables can e.g. Enumerate from multiple threads
>    Mutex mMutex;
> -  RefPtr<mozilla::AudioInput> mAudioInput;
> -  bool mFullDuplex;
> +  RefPtr<mozilla::CubebDeviceEnumerator> mEnumerator;
> +  bool mFullDuplex;  

trailing space

::: dom/media/webrtc/MediaEngineWebRTC.cpp:37
(Diff revision 1)
>  
> -// statics from AudioInputCubeb
> -nsTArray<int>* AudioInputCubeb::mDeviceIndexes;
> -int AudioInputCubeb::mDefaultDevice = -1;
> -nsTArray<nsCString>* AudioInputCubeb::mDeviceNames;
> -cubeb_device_collection AudioInputCubeb::mDevices = { nullptr, 0 };
> +using namespace CubebUtils;
> +
> +MediaEngineWebRTC::MediaEngineWebRTC(MediaEnginePrefs &aPrefs)
> +  : mMutex("mozilla::MediaEngineWebRTC")
> +  , mEnumerator()

Unnecessary

::: dom/media/webrtc/MediaEngineWebRTC.cpp:51
(Diff revision 1)
> -  cubeb_device_collection devices = { nullptr, 0 };
> +CubebDeviceEnumerator::CubebDeviceEnumerator()
> +  : mMutex("CubebDeviceListMutex")
> +{
> +  cubeb_register_device_collection_changed(GetCubebContext(),
> +                                           CUBEB_DEVICE_TYPE_INPUT,
> +                                           &mozilla::CubebDeviceEnumerator::AudioDeviceListChanged_s,
> +                                           this);
> +}
>  
> -  if (CUBEB_OK != cubeb_enumerate_devices(cubebContext,
> +CubebDeviceEnumerator::~CubebDeviceEnumerator()
> +{
> +  cubeb_register_device_collection_changed(GetCubebContext(),
> -                                          CUBEB_DEVICE_TYPE_INPUT,
> +                                           CUBEB_DEVICE_TYPE_INPUT,
> -                                          &devices)) {
> +                                           nullptr,
> +                                           this);
> +}
> +
> +void
> +CubebDeviceEnumerator::EnumerateAudioInputDevice(nsTArray<RefPtr<AudioDeviceInfo>>& aDevices)
> +{

I don't think these methods should be defined in between MediaEngineWebRTC methods.

Put them either before or after MediaEngineWebRTC definitions, or in its own file.

::: dom/media/webrtc/MediaEngineWebRTC.cpp:265
(Diff revision 1)
> -    if (!mAudioInput) {
> -      if (!SupportsDuplex()) {
> +    if (!mEnumerator) {
> +      mEnumerator = new CubebDeviceEnumerator();
> -        return;
> -      }
> -      mAudioInput = new mozilla::AudioInputCubeb();
>      }

More bitrot to come from bug 1457427 here.

::: dom/media/webrtc/MediaEngineWebRTC.cpp:277
(Diff revision 1)
> -
> -      RefPtr<MediaEngineSource> aSource;
> +    // For some reason the "fake" device for automation is marked as DISABLED,
> +    // so white-list it.

If this is referring to the sine-source, we're no longer using it.

In any case, shouldn't this be fixed in the pulse impl of cubeb?

::: dom/media/webrtc/MediaEngineWebRTC.cpp:292
(Diff revision 1)
> -        aSources->AppendElement(aSource.get());
> -      } else {
> -        aSource = new MediaEngineWebRTCMicrophoneSource(
> -            new mozilla::AudioInputCubeb(i),
> -            i, deviceName, uniqueId,
> -            mDelayAgnostic, mExtendedFilter);
> +
> +      if ((devices[i]->State() == CUBEB_DEVICE_STATE_ENABLED ||
> +           (devices[i]->State() == CUBEB_DEVICE_STATE_DISABLED &&
> +            devices[i]->FriendlyName().Equals(NS_LITERAL_STRING("Sine source at 440 Hz"))))) {
> +        MOZ_ASSERT(devices[i]->Type() == CUBEB_DEVICE_TYPE_INPUT);
> +        // XXX do something for the default device.

Drop, resolve, or explain why it's not needed right now and file a followup bug.

::: dom/media/webrtc/MediaEngineWebRTC.cpp:295
(Diff revision 1)
> -            new mozilla::AudioInputCubeb(i),
> -            i, deviceName, uniqueId,
> -            mDelayAgnostic, mExtendedFilter);
> -        devicesForThisWindow->Put(uuid, aSource);
> -        aSources->AppendElement(aSource);
> +            devices[i]->FriendlyName().Equals(NS_LITERAL_STRING("Sine source at 440 Hz"))))) {
> +        MOZ_ASSERT(devices[i]->Type() == CUBEB_DEVICE_TYPE_INPUT);
> +        // XXX do something for the default device.
> +        RefPtr<MediaEngineSource> source =
> +          new MediaEngineWebRTCMicrophoneSource(
> +            mEnumerator,

Why does each microphone source need the enumerator?

::: dom/media/webrtc/MediaEngineWebRTC.cpp:298
(Diff revision 1)
> -        devicesForThisWindow->Put(uuid, aSource);
> -        aSources->AppendElement(aSource);
> +        RefPtr<MediaEngineSource> source =
> +          new MediaEngineWebRTCMicrophoneSource(
> +            mEnumerator,
> +            devices[i]->GetDeviceID().ref(),
> +            devices[i]->FriendlyName(),
> +            // Lie and provide the name as UUID

A hack like this needs justification for why it's needed, not just a statement to its existence.
Attachment #8972010 - Flags: review?(apehrson)
Comment on attachment 8972011 [details]
Bug 1404977 - Part 4 - Format a comment and remove an unused and now useless method.

https://reviewboard.mozilla.org/r/240752/#review246922
Attachment #8972011 - Flags: review?(apehrson) → review+
Comment on attachment 8972013 [details]
Bug 1404977 - Part 6 - Remove unused include for lock-free FIFO.

https://reviewboard.mozilla.org/r/240756/#review246928
Attachment #8972013 - Flags: review?(apehrson) → review+
Comment on attachment 8968569 [details]
Bug 1404977 - Part 10 - Make logging uniform, and consistently use GraphImpl() to access the graph impl.

https://reviewboard.mozilla.org/r/237258/#review246944

::: dom/media/GraphDriver.cpp:34
(Diff revision 2)
>  namespace mozilla {
>  
>  GraphDriver::GraphDriver(MediaStreamGraphImpl* aGraphImpl)
>    : mIterationStart(0),
>      mIterationEnd(0),
>      mGraphImpl(aGraphImpl),

If we want all users of mGraphImpl to go through GraphImpl(), can we make mGraphImpl private?

Now you had a couple of stray uses that I've marked, but the compiler could have caught them.

::: dom/media/GraphDriver.cpp:90
(Diff revision 2)
>  }
>  
>  GraphTime
>  GraphDriver::StateComputedTime() const
>  {
>    return mGraphImpl->mStateComputedTime;

This one is inconsistent

::: dom/media/GraphDriver.cpp:403
(Diff revision 2)
> -      mGraphImpl->mGraphDriverAsleep = false; // atomic
> +      GraphImpl()->mGraphDriverAsleep = false; // atomic
>      }
>      mWaitState = WAITSTATE_WAITING_FOR_NEXT_ITERATION;
>    }
>    if (!timeout.IsZero()) {
>      mGraphImpl->GetMonitor().Wait(timeout);

This one is inconsistent

::: dom/media/GraphDriver.cpp:631
(Diff revision 2)
>    bool firstStream = CubebUtils::GetFirstStream();
>  
>    MOZ_ASSERT(!NS_IsMainThread(),
>        "This is blocking and should never run on the main thread.");
>  
>    mSampleRate = output.rate = mGraphImpl->GraphRate();

This one is inconsistent

::: dom/media/GraphDriver.cpp:766
(Diff revision 2)
>      NS_WARNING("Could not start cubeb stream for MSG.");
>      return false;
>    }
>  
>    {
>      MonitorAutoLock mon(mGraphImpl->GetMonitor());

This one is inconsistent

::: dom/media/MediaStreamGraph.cpp:107
(Diff revision 2)
>      LOG(LogLevel::Debug,
> -        ("Adding media stream %p to graph %p, count %zu",
> +        ("%p:  Adding media stream %p, count %zu",
> -         aStream,
>           this,
> +         aStream,
>           mStreams.Length()));

No need for both these!

It's an old rebase gone wrong IIRC. Would be great to get rid of.

::: dom/media/MediaStreamGraph.cpp:147
(Diff revision 2)
>    LOG(LogLevel::Debug,
> -      ("Removed media stream %p from graph %p, count %zu",
> +      ("%p: Removed media stream %p, count %zu",
> -       aStream,
>         this,
> +       aStream,
>         mStreams.Length()));

Another dupe-log.

::: dom/media/MediaStreamGraph.cpp
(Diff revision 2)
>  }
>  
>  void
>  MediaStreamGraphImpl::RunMessagesInQueue()
>  {
> -  TRACE_AUDIO_CALLBACK();

Looks like you were bitten by bitrot.
Attachment #8968569 - Flags: review?(apehrson) → review+
Comment on attachment 8972015 [details]
Bug 1404977 - Part 8 - Tell the MSG the MediaEngineAudioSource are now independent and that we can have multiple of them, cleanup the MSG-side API for managing them.

https://reviewboard.mozilla.org/r/240760/#review246934

This is not really a complete review, but enough outstanding issues to warrant publishing this now.

::: commit-message-4b781:1
(Diff revision 1)
> +Bug 1404977 - Part 8 - Tell the MSG the MediaEngineAudioSource are now independant and that we can have multiple of them, cleanup the MSG-side API for managing them. r?pehrsons

s/independant/independent/

::: commit-message-4b781:3
(Diff revision 1)
> +Bug 1404977 - Part 8 - Tell the MSG the MediaEngineAudioSource are now independant and that we can have multiple of them, cleanup the MSG-side API for managing them. r?pehrsons
> +
> +The MSG now can feed microphone data to all its input listener. This paves the

s/listener/listeners/

::: dom/media/MediaStreamGraph.h:120
(Diff revision 1)
> +   * Number of audio input channels.
> +   */
> +  virtual uint32_t InputChannelCount() = 0;
> +
> +  /**

This should be in part 5.

::: dom/media/MediaStreamGraph.cpp:806
(Diff revision 1)
> -    NS_ASSERTION(false, "Input from multiple mics not yet supported; bug 1238038");
> -    // Need to support separate input-only AudioCallback drivers; they'll
> -    // call us back on "other" threads.  We will need to echo-cancel them, though.
> +  // We don't support opening multiple input device in a graph for now.
> +  if (listeners.IsEmpty() && mInputDeviceUsers.Count() > 1) {
> +    listeners.RemoveElement(aID);
>      return;

Can you move the comment inside the if block?

::: dom/media/MediaStreamGraph.cpp:832
(Diff revision 1)
>          ("OpenAudioInput: starting new AudioCallbackDriver(input) %p", driver));
>        driver->SetInputListener(aListener);
>        CurrentDriver()->SwitchAtNextIteration(driver);
>     } else {
>       LOG(LogLevel::Error, ("OpenAudioInput in shutdown!"));
> -     LOG(LogLevel::Debug, ("OpenAudioInput in shutdown!"));
> +     MOZ_ASSERT(false, "Can't open cubeb inputs in shutdown");

Use `MOZ_ASSERT_UNREACHABLE`

::: dom/media/MediaStreamGraph.cpp:873
(Diff revision 1)
>  
>  void
> -MediaStreamGraphImpl::CloseAudioInputImpl(AudioDataListener *aListener)
> +MediaStreamGraphImpl::CloseAudioInputImpl(CubebUtils::AudioDeviceID aID, AudioDataListener* aListener)
>  {
>    MOZ_ASSERT(OnGraphThreadOrNotRunning());
> -  uint32_t count;
> +  // It is possible to not know the ID here, find it first.

How is this possible?

::: dom/media/MediaStreamGraph.cpp:888
(Diff revision 1)
> +  nsTArray<RefPtr<AudioDataListener>>* listeners = mInputDeviceUsers.GetValue(aID);
> +
> +  MOZ_ASSERT(listeners);
> +  DebugOnly<bool> wasPresent = listeners->RemoveElement(aListener);
> +  MOZ_ASSERT(wasPresent);
> +  // Check whether there is still a consumer for this audio input device

Again I like to see the comment in the if block. It tends to get clearer.

And the comment gets shorter too, "There is still a consumer for this audio input device"

::: dom/media/MediaStreamGraph.cpp:954
(Diff revision 1)
> -                                   TrackRate aRate, uint32_t aChannels)
> +                                       TrackRate aRate, uint32_t aChannels)
>  {
> -  for (auto& listener : mAudioInputs) {
> +  if (!mInputDeviceID) {
> +    return;
> +  }
> +  // When/if we decide to support multiple input device per graph, this needs

s/device/devices/

::: dom/media/MediaStreamGraph.cpp:968
(Diff revision 1)
> +  MonitorAutoLock lock(mMonitor);
> +  // Either we have an audio input device, or we just removed the audio input
> +  // this iteration, and we're switching back to an output-only driver next
> +  // iteration.
> +  MOZ_ASSERT(mInputDeviceID || CurrentDriver()->Switching());

The monitor lock is not going out of scope at the `#endif`. Is that by intention?

::: dom/media/MediaStreamGraph.cpp:981
(Diff revision 1)
> +void MediaStreamGraphImpl::DeviceChanged()
> +{

Can we add a threading assert here?

::: dom/media/MediaStreamGraph.cpp:995
(Diff revision 1)
> +  AudioCallbackDriver* audioCallbackDriver = CurrentDriver()->AsAudioCallbackDriver();
> +  MOZ_ASSERT(audioCallbackDriver);

Is there no risk that `CurrentDriver()` already has a `NextDriver()` here?

::: dom/media/MediaStreamGraph.cpp:2750
(Diff revision 1)
>  
>  nsresult
> -SourceMediaStream::OpenAudioInput(int aID,
> +SourceMediaStream::OpenAudioInput(CubebUtils::AudioDeviceID aID,
>                                    AudioDataListener *aListener)
>  {
>    if (GraphImpl()) {

It's weird that we allow calls to OpenAudioInput() from any thread, but we reset GraphImpl() on the graph thread without any locking between these.

To me it seems that GraphImpl() is safe to call here because we'll only reset it once all references are dropped.

But then we don't need to be graceful here, and instead we should crash hard. Perhaps a MOZ_RELEASE_ASSERT? Thoughts?

::: dom/media/MediaStreamGraph.cpp:2751
(Diff revision 1)
>  nsresult
> -SourceMediaStream::OpenAudioInput(int aID,
> +SourceMediaStream::OpenAudioInput(CubebUtils::AudioDeviceID aID,
>                                    AudioDataListener *aListener)
>  {
>    if (GraphImpl()) {
>      mInputListener = aListener;

This can be called on any thread but we'll hit threading issues if we get calls to this instance from mixed threads.

Furthermore OpenAudioInput is not idempotent.

Can we reflect this with a comment at the declaration of the method, and an assert that mInputListener is unset here?

::: dom/media/MediaStreamGraph.cpp:2763
(Diff revision 1)
> -SourceMediaStream::CloseAudioInput()
> +SourceMediaStream::CloseAudioInput(CubebUtils::AudioDeviceID aID,
> +                                   AudioDataListener* aListener)
>  {
> +  MOZ_ASSERT(mInputListener == aListener);
>    // Destroy() may have run already and cleared this
>    if (GraphImpl() && mInputListener) {

Same here re: crashing hard.

::: dom/media/MediaStreamGraph.cpp:2772
(Diff revision 1)
>  }
>  
>  void
>  SourceMediaStream::DestroyImpl()
>  {
> -  CloseAudioInput();
> +  CloseAudioInput(nullptr, mInputListener);

You're casting a `nullptr` to an `AudioDeviceID`.

There's a lot of indirection between `AudioDeviceID` and the underlying `void*` type. Could you add an enum or some consts that explicitly captures the invalid states of `AudioDeviceID` so we can use that for tests and assignments like here?

::: dom/media/MediaStreamGraphImpl.h:389
(Diff revision 1)
> -   * explicitly set to finished by the caller.
> -   */
> -  void OpenAudioInputImpl(int aID,
> +  void OpenAudioInputImpl(CubebUtils::AudioDeviceID aID,
> +                          AudioDataListener *aListener);
> +  /* Called on the main thread when something requests audio from an input
> +   * audio device aID. */
> +  virtual nsresult OpenAudioInput(CubebUtils::AudioDeviceID aID,
> +                                  AudioDataListener *aListener) override;

s/`AudioDataListener *`/`AudioDataListener*`/

::: dom/media/MediaStreamGraphImpl.h:394
(Diff revision 1)
> +                                  AudioDataListener *aListener) override;
> +  /* Runs off a message on the graph when a input audio from aID is not needed
> +   * anymore, for a particular stream. It can be that other streams still need
> +   * audio from this audio input device. */
> +  void CloseAudioInputImpl(CubebUtils::AudioDeviceID aID,
> -                          AudioDataListener *aListener);
> +                           AudioDataListener *aListener);

s/`AudioDataListener *`/`AudioDataListener*`/

::: dom/media/MediaStreamGraphImpl.h:399
(Diff revision 1)
> -                          AudioDataListener *aListener);
> +                           AudioDataListener *aListener);
> -  virtual nsresult OpenAudioInput(int aID,
> +  /* Called on the main thread when input audio from aID is not needed
> +   * anymore, for a particular stream. It can be that other streams still need
> +   * audio from this audio input device. */
> +  virtual void CloseAudioInput(CubebUtils::AudioDeviceID aID,
> -                                  AudioDataListener *aListener) override;
> +                               AudioDataListener *aListener) override;

s/`AudioDataListener *`/`AudioDataListener*`/

::: dom/media/MediaStreamGraphImpl.h:412
(Diff revision 1)
> +                        TrackRate aRate, uint32_t aChannels);
> +  /* Called on the graph thread when there is new input data for listeners. This
> +   * is the raw audio input for this MediaStreamGraph. */
> +  void NotifyInputData(const AudioDataValue* aBuffer, size_t aFrames,
> +                       TrackRate aRate, uint32_t aChannels);
> +  void DeviceChanged();

Needs docs.

::: dom/media/MediaStreamGraphImpl.h:456
(Diff revision 1)
> +  /** The audio input channel count for a MediaStreamGraph is the max of all the
> +   * channel count requested by the listeners. The max channel count is
> +   * delievered to the listener themselve, and they take care of downmixing. */

s/all the channel count/all the channel counts/
s/delievered to the listener/delivered to the listeners/
s/themselve/themselves/

::: dom/media/MediaStreamGraphImpl.h:473
(Diff revision 1)
> +    // When/if we decide to support multiple input device per graph, this needs
> +    // loop over them.
> +    nsTArray<RefPtr<AudioDataListener>>* listeners =
> +      mInputDeviceUsers.GetValue(mInputDeviceID);
> +    MOZ_ASSERT(listeners);
> +    for (auto& listener : *listeners) {

`const auto&` would make sense
Attachment #8972015 - Flags: review?(apehrson)
Comment on attachment 8972012 [details]
Bug 1404977 - Part 5 - Allow querying the number of input channels from a WebRTCAudioDataListener.

https://reviewboard.mozilla.org/r/240754/#review246924

::: commit-message-a6649:1
(Diff revision 1)
> +Bug 1404977 - Part 5 - Allow querying the number of input channel from a WebRTCAudioDataListener. r?pehrsons

s/channel/channels/

::: dom/media/webrtc/MediaEngineWebRTC.h:187
(Diff revision 1)
>                         const AudioDataValue* aBuffer,
>                         size_t aFrames,
>                         TrackRate aRate,
>                         uint32_t aChannels) override;
>  
> +  uint32_t InputChannelCount() override;

This seems to only be called from `MSGImpl::AudioInputChannelCount()`.

Am I correct in that this can never change?

Then it would make more sense to be passed on the side of the listener to `OpenAudioInput()`. Or for a compromise, pass it to the ctor and make it const for immutability, keep the method and drop the lock.

::: dom/media/webrtc/MediaEngineWebRTC.h:187
(Diff revision 1)
>                         const AudioDataValue* aBuffer,
>                         size_t aFrames,
>                         TrackRate aRate,
>                         uint32_t aChannels) override;
>  
> +  uint32_t InputChannelCount() override;

What is this overriding?
Attachment #8972012 - Flags: review?(apehrson)
Comment on attachment 8972015 [details]
Bug 1404977 - Part 8 - Tell the MSG the MediaEngineAudioSource are now independent and that we can have multiple of them, cleanup the MSG-side API for managing them.

https://reviewboard.mozilla.org/r/240760/#review247100

::: dom/media/MediaStreamGraph.cpp:997
(Diff revision 1)
> +  if (audioCallbackDriver->InputChannelCount() != AudioInputChannelCount()) {
> +    AudioCallbackDriver* newDriver = new AudioCallbackDriver(this, AudioInputChannelCount());

This calls AudioInputChannelCount() twice but doesn't consider a case where the two calls return different values.

Probably not so important.

But each call potentially churns through quite a lot of mutexes, so we should keep that down.

If the reevaluation was instead handled by closing an audio input and then re-opening it with a (same or mutated) listener with the updated channel count, we'd get a simpler solution with less locks and fewer code paths (this one wouldn't be necessary for one). Feasible?
Comment on attachment 8972014 [details]
Bug 1404977 - Part 7 - Make each MediaEngineWebRTCMicrophoneSource independent.

https://reviewboard.mozilla.org/r/240758/#review246930

r- for flawed handling of "channelCount" constraint.

::: commit-message-f2964:1
(Diff revision 1)
> +Bug 1404977 - Part 7 - Make each MediaEngineWebRTCMicrophoneSource independant. r?pehrsons

s/independant/independent/

::: dom/media/webrtc/MediaEngineWebRTC.h:387
(Diff revision 1)
> +  void SetUserInputChannelCount(uint32_t aUserInputChannelCount);
>  
>    // Owning thread only.
>    RefPtr<WebRTCAudioDataListener> mListener;
> +  RefPtr<mozilla::CubebDeviceEnumerator> mEnumerator;
> +  // Number of times this devices has been opened for this MSG.

s/devices/device/

::: dom/media/webrtc/MediaEngineWebRTC.h:423
(Diff revision 1)
>    const nsMainThreadPtrHandle<media::Refcountable<dom::MediaTrackSettings>> mSettings;
>  
> +  // The number of channels asked for by content, after clamping to the range of
> +  // legal channel count for this particular device. This is the number of
> +  // channels of the input buffer received.
> +  uint32_t mUserInputChannelCount;

All these different notions of channel count are confusing me. Can we call this `mRequestedChannelCount` instead to make things clearer?

::: dom/media/webrtc/MediaEngineWebRTCAudio.cpp:434
(Diff revision 1)
> +  if (!info) {
>      return NS_ERROR_FAILURE;
>    }

What case is this covering?

An `ApplyConstraints()` on a device that had been unplugged? Shouldn't that be handled elsewhere by the controlling code getting notified about the unplug, ending the track and stopping and deallocating this source?

Can we instead of `mDeviceID` have an immutable `mDeviceInfo` member so this lookup is no longer needed?

::: dom/media/webrtc/MediaEngineWebRTCAudio.cpp:442
(Diff revision 1)
> +  // First, check channelCount violation wrt constraints. This throws in case of
> +  // error.

Throws? It seems to return NS_ERROR_FAILURE. Higher that leads to a promise rejection, not a throw, IIRC.

::: dom/media/webrtc/MediaEngineWebRTCAudio.cpp:455
(Diff revision 1)
> +  // Get the number of channels asked for by content, and clamp it between the
> +  // pref and the maximum number of channels that the device supports.
>    prefs.mChannels = c.mChannelCount.Get(std::min(prefs.mChannels,
> -                                        static_cast<int32_t>(maxChannels)));
> +                                        static_cast<int32_t>(info->MaxChannels())));

This doesn't do what the comment suggests.

It only considers the pref or the device's max channels if there was no ideal constraint passed in for channel count: https://searchfox.org/mozilla-central/rev/795575287259a370d00518098472eaa5b362bfa7/dom/media/webrtc/MediaTrackConstraints.h#84

This could use a test.

::: dom/media/webrtc/MediaEngineWebRTCAudio.cpp:479
(Diff revision 1)
> -      if (prefs.mChannels != mNetPrefs.mChannels) {
> -        // If the channel count changed, tell the MSG to open a new driver with
> +      if (prefs == mNetPrefs) {
> +        return NS_OK;
> -        // the correct channel count.
> -        MOZ_ASSERT(!mAllocations.IsEmpty());
> -        RefPtr<SourceMediaStream> stream;
> -        for (const Allocation& allocation : mAllocations) {
> -          if (allocation.mStream && allocation.mStream->GraphImpl()) {
> -            stream = allocation.mStream;
> -            break;
> -          }
> -        }
> +      }

If this exits early, we won't get the log just below, which might be confusing when debugging.

::: dom/media/webrtc/MediaEngineWebRTCAudio.cpp:547
(Diff revision 1)
> +  MOZ_ASSERT(!mAllocations.IsEmpty() &&
> +             mAllocations[0].mStream &&
> +             mAllocations[0].mStream->GraphImpl()->CurrentDriver()->OnThread());
> +  mUserInputChannelCount = aUserInputChannelCount;
> +  mAllocations[0].mStream->GraphImpl()->ReevaluateInputDevice();

There's no general guarantee that mAllocations[0].mStream is non-null, are you sure this is guaranteed for your cases?

::: dom/media/webrtc/MediaEngineWebRTCAudio.cpp:551
(Diff revision 1)
> +  }
> +  MOZ_ASSERT(!mAllocations.IsEmpty() &&
> +             mAllocations[0].mStream &&
> +             mAllocations[0].mStream->GraphImpl()->CurrentDriver()->OnThread());
> +  mUserInputChannelCount = aUserInputChannelCount;
> +  mAllocations[0].mStream->GraphImpl()->ReevaluateInputDevice();

So this calls
1) `MSGImpl::ReevaluateInputDevice()` which calls
2) `MSGImpl::AudioInputChannelCount()` which calls
3) `AudioDataListener::InputChannelCount()` which calls 
4) `MediaEngineWebRTCMicrophoneSource::InputChannelCount()`.

That last call is a call back into ourselves and the one before includes locking the listener's mutex.

Hairy.

Like I commented in part 8, I'd rather see step 1 being `CloseAudioInput(); OpenAudioInput();`.

If that's not possible it would be cleaner to update the graph's record of the requested channel count for the OpenAudioInput request we originally made, so that we can avoid the cyclic probing back into the microphone source (making steps 3 and 4 unnecessary).

::: dom/media/webrtc/MediaEngineWebRTCAudio.cpp:715
(Diff revision 1)
> -  if (sChannelsOpen == 0) {
> +  if (mChannelsOpen == 0) {
>      return NS_ERROR_FAILURE;
>    }

Allocate() increments `mChannelsOpen`, so if it is 0 here we have grave problems. This shouldn't be a graceful return. Make it an assert or a crash.

::: dom/media/webrtc/MediaEngineWebRTCAudio.cpp:724
(Diff revision 1)
> +  // For now, we only allow opening a single audio input device per page,
> +  // because we can only have one MSG per page.

The correct term would be "document", no?

::: dom/media/webrtc/MediaEngineWebRTCAudio.cpp:751
(Diff revision 1)
>      }
>  
>      // Make sure logger starts before capture
>      AsyncLatencyLogger::Get(true);
>  
>      // Must be *before* StartSend() so it will notice we selected external input (full_duplex)

I don't think we have StartSend() anymore? Wasn't it part of the old processing API in voice engine?
Attachment #8972014 - Flags: review?(apehrson) → review-
Comment on attachment 8972016 [details]
Bug 1404977 - Part 9 - Propagate the changes to the GraphDrivers, simplifying them, and brokering all access through the MSG.

https://reviewboard.mozilla.org/r/240762/#review246942

Looks mostly good, but I'd like a second pass at this when issues have been fixed.

::: dom/media/GraphDriver.h:380
(Diff revision 1)
>  #if defined(XP_WIN)
>                              , public audio::DeviceChangeListener
>  #endif
>  {
>  public:
> -  explicit AudioCallbackDriver(MediaStreamGraphImpl* aGraphImpl);
> +  /** If aInputChannelCount is zero, then this driver is output-only. */

Oh this comment needs to be for `mInputChannelCount` !

::: dom/media/GraphDriver.h:381
(Diff revision 1)
>                              , public audio::DeviceChangeListener
>  #endif
>  {
>  public:
> -  explicit AudioCallbackDriver(MediaStreamGraphImpl* aGraphImpl);
> +  /** If aInputChannelCount is zero, then this driver is output-only. */
> +  explicit AudioCallbackDriver(MediaStreamGraphImpl* aGraphImpl, uint32_t aInputChannelCount);

No need for `explicit` with two args.

::: dom/media/GraphDriver.h:503
(Diff revision 1)
>    /* The sample rate for the aforementionned cubeb stream. This is set on
>     * initialization and can be read safely afterwards. */
>    uint32_t mSampleRate;
>    /* The number of input channels from cubeb.  Should be set before opening cubeb
>     * and then be static. */
> -  uint32_t mInputChannels;
> +  uint32_t mInputChannelCount;

Immutable you said, then this should be `const`.

Seems to be true for more members. Whether you want to constify them now is up to you. In any case any added compiler checks of our assumptions are very beneficial. Immutability is especially great with all the threading we have going on.

::: dom/media/GraphDriver.cpp:658
(Diff revision 1)
>    input = output;
> -  input.channels = mInputChannels;
> +  input.channels = mInputChannelCount;
>    input.layout = CUBEB_LAYOUT_UNDEFINED;
>  
> -#ifdef MOZ_WEBRTC
> -  if (mGraphImpl->mInputWanted) {
> +  cubeb_stream* stream = nullptr;
> +  bool inputWanted = !!mInputChannelCount;

s/!!mInputChannelCount/mInputChannelCount > 0/

::: dom/media/GraphDriver.cpp:660
(Diff revision 1)
>    input.layout = CUBEB_LAYOUT_UNDEFINED;
>  
> -#ifdef MOZ_WEBRTC
> -  if (mGraphImpl->mInputWanted) {
> -    StaticMutexAutoLock lock(AudioInputCubeb::Mutex());
> -    uint32_t userChannels = 0;
> +  cubeb_stream* stream = nullptr;
> +  bool inputWanted = !!mInputChannelCount;
> +  CubebUtils::AudioDeviceID output_id;
> +  if (forcedOutputDeviceId) {

A device id doesn't seem like something naturally supporting `operator!`. Can we make this more explicit?

::: dom/media/GraphDriver.cpp:660
(Diff revision 1)
> -    uint32_t userChannels = 0;
> -    AudioInputCubeb::GetUserChannelCount(mGraphImpl->mInputDeviceID, userChannels);
> -    input.channels = mInputChannels = std::min<uint32_t>(8, userChannels);
> +  if (forcedOutputDeviceId) {
> +    output_id = forcedOutputDeviceId;
> +  } else {
> +    output_id = GraphImpl()->mOutputDeviceID;
>    }

This seems like a good fit for
```
output_id = IsInvalid(forceOutputDeviceId)
          ? GraphImpl()->mOutputDeviceID
          : forcedOutputDeviceId;
```

::: dom/media/GraphDriver.cpp:699
(Diff revision 1)
> -    }
> +   }
> -  }
> -  SetMicrophoneActive(mGraphImpl->mInputWanted);
>  
> -  cubeb_stream_register_device_changed_callback(mAudioStream,
> -                                                AudioCallbackDriver::DeviceChangedCallback_s);
>  
> +#ifdef XP_MACOSX
> +   PanOutputIfNeeded(!!mInputChannelCount);
> +#endif
> +
> +cubeb_stream_register_device_changed_callback(
> + mAudioStream, AudioCallbackDriver::DeviceChangedCallback_s);
> +
> -  if (!StartStream()) {
> +   if (!StartStream()) {
> -    LOG(LogLevel::Warning, ("AudioCallbackDriver couldn't start stream."));
> +     LOG(LogLevel::Warning, ("%p: AudioCallbackDriver couldn't start a cubeb stream.", GraphImpl()));
> -    return false;
> +     return false;
> -  }
> +   }
>  
> -  LOG(LogLevel::Debug, ("AudioCallbackDriver started."));
> +   LOG(LogLevel::Debug, ("%p: AudioCallbackDriver started.", GraphImpl()));
> -  return true;
> +   return true;

Indentation is off. 3 spaces should be 2.

::: dom/media/GraphDriver.cpp:703
(Diff revision 1)
>  
> -  cubeb_stream_register_device_changed_callback(mAudioStream,
> -                                                AudioCallbackDriver::DeviceChangedCallback_s);
>  
> +#ifdef XP_MACOSX
> +   PanOutputIfNeeded(!!mInputChannelCount);

s/!!mInputChannelCount/inputWanted/

::: dom/media/GraphDriver.cpp:706
(Diff revision 1)
> +cubeb_stream_register_device_changed_callback(
> + mAudioStream, AudioCallbackDriver::DeviceChangedCallback_s);

Real funky indentation here.

::: dom/media/GraphDriver.cpp:965
(Diff revision 1)
>      MOZ_ASSERT_UNREACHABLE("We should not underrun in full duplex");
>      mIterationEnd = stateComputedTime;
>    }
>  
>    // Process mic data if any/needed
> -  if (aInputBuffer) {
> +  if (aInputBuffer && mInputChannelCount) {

s/mInputChannelCount/mInputChannelCount > 0/

Though if mInputChannelCount is 0, isn't it an error to have an aInputBuffer? An assert of this seems like an appropriate check to me.

::: dom/media/GraphDriver.cpp:1110
(Diff revision 1)
> -AudioCallbackDriver::SetMicrophoneActive(bool aActive)
> -{
> -  mMicrophoneActive = aActive;
> -
>  #ifdef XP_MACOSX
> -  PanOutputIfNeeded(mMicrophoneActive);
> +  PanOutputIfNeeded(!!mInputChannelCount);

s/!!mInputChannelCount/mInputChannelCount > 0/
Attachment #8972016 - Flags: review?(apehrson)
Iframe pen counterpart to the pen in comment 20: https://codepen.io/SingingTree/pen/BxWePB?editors=1010 Bit hacky, but hopefully gets the job done. Both cases seem happy on my Windows box.

I'm hitting MediaEngineWebRTCAudio.cpp:863 ending up with zombies when testing on Linux. After closing Firefox I get left with the following spew until I kill the remaining process:

> Child 37801, MediaStreamGrph] WARNING: No audio stream!: 'mDriver->mAudioStream || aOperation == INIT', file /home/b/projects/mozilla/mozilla-central/dom/media/GraphDriver.cpp, line 469
> [Child 37801, CubebOperation #1] WARNING: Could not stop cubeb stream for MSG.: file /home/b/projects/mozilla/mozilla-central/dom/media/GraphDriver.cpp, line 776

and this one in a separate case:

> Child 40758, Chrome_ChildThread] WARNING: pipe error: Bad file descriptor: file /home/b/projects/mozilla/mozilla-central/ipc/chromium/src/chrome/common/ipc_channel_posix.cc, line 709
These are the patches rebased on (almost) latest Nightly. I wont change the status of Paul's patches in order to let him handle them. I will get the review comments from the initial patches and apply them here. Sorry for the mess, I am not aware of a cleaner way to do that.

I tested a little the rebased patches and I get a crash when gUM is executed from different iframes in the same process. It is hitting the assert `MOZ_ASSERT(audioCallbackDriver)` in the new method `void MediaStreamGraphImpl::ReevaluateInputDevice()`. I am looking at it.

I tried also 2 gUM requests, each in different document but on the same process. This works, the second request took some time to be completed. I will check again on an opt build.
Comment on attachment 8974941 [details]
Bug 1404977 - Part 1 - Add a pref to force the output device by name, for testing.

https://reviewboard.mozilla.org/r/243318/#review249254

It doesn't seem like you went through and fixed any of the original patch's review comments. I've copied over those comments here, but I'm not going to do that for all the patches. r- on this one for that and I'm clearing the others.

Also please mark Paul's patches obsolete in bugzilla if you are taking over the work.

::: dom/media/CubebUtils.cpp:254
(Diff revision 1)
>    } else if (strcmp(aPref, PREF_CUBEB_BACKEND) == 0) {
> -    nsAutoCString value;
> -    Preferences::GetCString(aPref, value);
> -    if (value.IsEmpty()) {
> -      sCubebBackendName = nullptr;
> -    } else {
> +    StaticMutexAutoLock lock(sMutex);
> +    GetPrefAndSetString(aPref, sCubebBackendName);
> +  } else if (strcmp(aPref, PREF_CUBEB_OUTPUT_DEVICE) == 0) {
> +    StaticMutexAutoLock lock(sMutex);
> +    GetPrefAndSetString(aPref, sCubebOutputDeviceName);

How does this swap existing cubeb streams to the new output device?

If it doesn't, file a bug and add a comment.

::: dom/media/GraphDriver.cpp:607
(Diff revision 1)
>      MonitorAutoLock lock(GraphImpl()->GetMonitor());
>      FallbackToSystemClockDriver();
>      return true;
>    }
>  
> +  cubeb_devid forcedOutputDeviceId  = nullptr;

s/  =/ =/

::: dom/media/GraphDriver.cpp:612
(Diff revision 1)
> +  cubeb_devid forcedOutputDeviceId  = nullptr;
> +  char* forcedOutputDeviceName = CubebUtils::GetForcedOutputDevice();
> +  if (forcedOutputDeviceName) {
> +    nsTArray<RefPtr<AudioDeviceInfo>> deviceInfos;
> +    GetDeviceCollection(deviceInfos, CubebUtils::Output);
> +    for (auto device : deviceInfos) {

s/auto/const auto&/

::: dom/media/tests/mochitest/test_getUserMedia_basicAudio_loopback.html:24
(Diff revision 1)
> -
> +    await SpecialPowers.pushPrefEnv({"set": [
> +      ["media.cubeb.output_device", audioDevice]
> +    ]});

audioDevice is the input device, not the output one, no?

This shouldn't be needed. Set the pref in the test runner instead: https://searchfox.org/mozilla-central/rev/795575287259a370d00518098472eaa5b362bfa7/testing/mochitest/runtests.py#1912
Attachment #8974941 - Flags: review?(apehrson) → review-
Comment on attachment 8974942 [details]
Bug 1404977 - Part 2 - Augment AudioDeviceID with a cubeb device id.

https://reviewboard.mozilla.org/r/243320/#review249256
Attachment #8974942 - Flags: review?(apehrson)
Comment on attachment 8974943 [details]
Bug 1404977 - Part 3 - Remove global statics, introduce and audio device enumerator

https://reviewboard.mozilla.org/r/243322/#review249258
Attachment #8974943 - Flags: review?(apehrson)
Comment on attachment 8974944 [details]
Bug 1404977 - Part 4 - Format a comment and remove an unused and now useless method.

https://reviewboard.mozilla.org/r/243324/#review249260
Attachment #8974944 - Flags: review?(apehrson)
Comment on attachment 8974945 [details]
Bug 1404977 - Part 5 - Allow querying the number of input channel from a WebRTCAudioDataListener.

https://reviewboard.mozilla.org/r/243326/#review249262
Attachment #8974945 - Flags: review?(apehrson)
Comment on attachment 8974946 [details]
Bug 1404977 - Part 6 - Remove unused include for lock-free FIFO.

https://reviewboard.mozilla.org/r/243328/#review249264
Attachment #8974946 - Flags: review?(apehrson)
Comment on attachment 8974947 [details]
Bug 1404977 - Part 7 - Make each MediaEngineWebRTCMicrophoneSource independant.

https://reviewboard.mozilla.org/r/243330/#review249266
Attachment #8974947 - Flags: review?(apehrson)
Comment on attachment 8974948 [details]
Bug 1404977 - Part 8 - Tell the MSG the MediaEngineAudioSource are now independant and that we can have multiple of them, cleanup the MSG-side API for managing them.

https://reviewboard.mozilla.org/r/243332/#review249268
Attachment #8974948 - Flags: review?(apehrson)
Comment on attachment 8974949 [details]
Bug 1404977 - Part 9 - Propagate the changes to the GraphDrivers, simplifying them, and brokering all access through the MSG.

https://reviewboard.mozilla.org/r/243334/#review249270
Attachment #8974949 - Flags: review?(apehrson)
Comment on attachment 8974950 [details]
Bug 1404977 - Part 10 - Make logging uniform, and consistently use GraphImpl() to access the graph impl.

https://reviewboard.mozilla.org/r/243336/#review249272
Attachment #8974950 - Flags: review?(apehrson)
Comment on attachment 8968567 [details]
Bug 1404977 - Part 1 - Add missing lock to the PREF_CUBEB_BACKEND branch of the pref callback in CubebUtils.cpp.

https://reviewboard.mozilla.org/r/237254/#review246906

> How does this swap existing cubeb streams to the new output device?
> 
> If it doesn't, file a bug and add a comment.

It does not swap existing streams and I do not think this is something we would like to support. When the pref is updated the user has to create a new stream. If there is an existing one, it must be closed and a new one must be created, in order to see the change.

> s/  =/ =/

Fixed

> s/auto/const auto&/

Fixed

> `audioDevice` is the input device, not the output one, no?
> 
> This shouldn't be needed. Set the pref in the test runner instead: https://searchfox.org/mozilla-central/rev/795575287259a370d00518098472eaa5b362bfa7/testing/mochitest/runtests.py#1912

First, you are right. We need to sent the output to the virtual test device "Null" and not to the `audioDevice`. Second if we change it in the runner it will change for every test.

Hmm I am not sure about the intention of this change since the audio is directed to the Null output anyway. I could see some value on stopping configuring the "Null" test device as the default device and to use that pref instead, to direct the audio there. A change like that  will not affect the local configuration of the user something that we are doing now and it's a little annoing. However, this is something we should change globally for all of the tests not to a specific one.

In any case I would suggest to take those changes in a separate bug. Changing the test device might cause errors and this bug is already big enough. For now I will just undo that change. Feel free to let me know if I am getting something wrongly.
Comment on attachment 8968568 [details]
Bug 1404977 - Part 2 - Augment AudioDeviceInfo with a cubeb device id.

https://reviewboard.mozilla.org/r/237256/#review246912

> It seems to me like we need
> s/AudioDeviceID/AudioDeviceInfo/

Fixed

> This ctor doesn't seem to be used per [1], is it needed?
> 
> Supporting both having an ID and not does add a bit of complexity that would be great to get rid of.
> 
> 
> [1] https://searchfox.org/mozilla-central/search?q=symbol:_ZN15AudioDeviceInfoC1ERK12nsTSubstringIDsES3_S3_tttttjjjjjj&redirect=false

Fixed

> Hmm, any reason these are not const?

Fixed

> indentation

Fixed
Comment on attachment 8972010 [details]
Bug 1404977 - Part 3 - Remove global statics, introduce an audio device enumerator

https://reviewboard.mozilla.org/r/240750/#review246916

> Why/where is this needed?

Removed

> s/implement/implements/
> s/access/accessed/

Fixed

> IMO, which threads can call what methods should generally go in the method documentations.

Fixed

> I'd love to see unit tests of this class on top of a mocked cubeb impl.

I hope you mean in a different bug.

> s/EnumerateAudioInputDevice/EnumerateAudioInputDevices/

Fixed

> The mutex protects mDevices I assume. Please document.

Fixed

> This needs info on what is protected by mMutex.

I see a comment there, not very detailed, I think the short answer is everything ...

> trailing space

Fixed

> Unnecessary

Why not, it's good to specify the members even when the default ctor is being called.

> I don't think these methods should be defined in between MediaEngineWebRTC methods.
> 
> Put them either before or after MediaEngineWebRTC definitions, or in its own file.

Fixed

> More bitrot to come from bug 1457427 here.

Done

> If this is referring to the sine-source, we're no longer using it.
> 
> In any case, shouldn't this be fixed in the pulse impl of cubeb?

Fixed. Cubeb get those values directly from pulseaudio without touching them. In any case we do not need it any more.

> Drop, resolve, or explain why it's not needed right now and file a followup bug.

The truth is we should do something about default devices, we do not offer a default with that patches on.

> Why does each microphone source need the enumerator?

It does not look necessary indeed. This change is correlated with changes in part 7. I will try to update it there.

> A hack like this needs justification for why it's needed, not just a statement to its existence.

It's not a hack it what we have been doing since the beginning.
https://searchfox.org/mozilla-central/source/dom/media/webrtc/MediaEngineWebRTC.cpp#297
Comment on attachment 8972012 [details]
Bug 1404977 - Part 5 - Allow querying the number of input channels from a WebRTCAudioDataListener.

https://reviewboard.mozilla.org/r/240754/#review246924

> s/channel/channels/

Fixed

> What is this overriding?

The relevant change was on part8. I brought it in this patch.

> This seems to only be called from `MSGImpl::AudioInputChannelCount()`.
> 
> Am I correct in that this can never change?
> 
> Then it would make more sense to be passed on the side of the listener to `OpenAudioInput()`. Or for a compromise, pass it to the ctor and make it const for immutability, keep the method and drop the lock.

I am not sure I get it. Do you mean to remove the channel count from the listener and pass it directly to the `OpenAudioInput()`? On the other hand, the compromise is to make it at least const member of the listener (by setting it into the constructor)?
Comment on attachment 8972014 [details]
Bug 1404977 - Part 7 - Make each MediaEngineWebRTCMicrophoneSource independent.

https://reviewboard.mozilla.org/r/240758/#review246930

> s/independant/independent/

Fixed

> All these different notions of channel count are confusing me. Can we call this `mRequestedChannelCount` instead to make things clearer?

Fixed

> What case is this covering?
> 
> An `ApplyConstraints()` on a device that had been unplugged? Shouldn't that be handled elsewhere by the controlling code getting notified about the unplug, ending the track and stopping and deallocating this source?
> 
> Can we instead of `mDeviceID` have an immutable `mDeviceInfo` member so this lookup is no longer needed?

Fixed

> Throws? It seems to return NS_ERROR_FAILURE. Higher that leads to a promise rejection, not a throw, IIRC.

Comment has changed.

> There's no general guarantee that mAllocations[0].mStream is non-null, are you sure this is guaranteed for your cases?

It is called from `MediaStreamGraphImpl::OpenAudioInputImpl()` (this is in part8), which it comes after a successfull enumeration. For the current usage there is a guarantee.

> So this calls
> 1) `MSGImpl::ReevaluateInputDevice()` which calls
> 2) `MSGImpl::AudioInputChannelCount()` which calls
> 3) `AudioDataListener::InputChannelCount()` which calls 
> 4) `MediaEngineWebRTCMicrophoneSource::InputChannelCount()`.
> 
> That last call is a call back into ourselves and the one before includes locking the listener's mutex.
> 
> Hairy.
> 
> Like I commented in part 8, I'd rather see step 1 being `CloseAudioInput(); OpenAudioInput();`.
> 
> If that's not possible it would be cleaner to update the graph's record of the requested channel count for the OpenAudioInput request we originally made, so that we can avoid the cyclic probing back into the microphone source (making steps 3 and 4 unnecessary).

Probably you mean to call `CloseAudioInputImpl()` and `OpenAudioInputImpl()` given that we already are at the audio thread, at that point. However, those methods does more than we need. We want just to switch the driver.

On the top of that I am not sure what is "the graph's record of the requested channel count" mentioned in the final note.

> Allocate() increments `mChannelsOpen`, so if it is 0 here we have grave problems. This shouldn't be a graceful return. Make it an assert or a crash.

Fixed

> The correct term would be "document", no?

Fixed

> I don't think we have StartSend() anymore? Wasn't it part of the old processing API in voice engine?

Comment removed.
I've been maintaining my own copy of these patches and rebasing them along so I can continue work on Windows loopback devices. As a sanity check I've diffed them with the latest updated ones at this point and they're pretty much the same -- I have some formatting changes from clang-format and a couple of the not yet pushed review changes already applied.

I'm interested in the discussion around default device handling. Recapping of my understanding of the situation:
- Prior to these patches we have special handling which labels the default device as "default: " + deviceName, this entry is presented as the first and default device during enumeration. A non default version of the same device will also appear in the list.
- Having the literal string "default: " is a pain for localisation and something we'd like to avoid.
- Post these changes we no longer have special handling for the default device. It will appear in the list with other devices. There is no special handling to ensure it appears first in the list during enumeration.

So my default device may no longer appear first in the list. As a user this can be irksome (only for users with multiple input devices). From a test perspective this could change the behaviour of our gUM tests where the first input device is selected, as this may no longer be the default device. That said, due to our picking of fake devices, or loopbacks via string match, this may not impact any tests at the moment.

For the human use case I would find it more ergonomic if we placed the default device as the first in the offered list. For my testing work I've hacked around this with extra prefs in my branch to make sure I get the device I want.
Comment on attachment 8972015 [details]
Bug 1404977 - Part 8 - Tell the MSG the MediaEngineAudioSource are now independent and that we can have multiple of them, cleanup the MSG-side API for managing them.

https://reviewboard.mozilla.org/r/240760/#review246934

> This should be in part 5.

Fixed

> Can you move the comment inside the if block?

Fixed

> Use `MOZ_ASSERT_UNREACHABLE`

Fixed

> How is this possible?

It's the case when it's called from `SourceMediaStream::DestroyImpl()`.

> Again I like to see the comment in the if block. It tends to get clearer.
> 
> And the comment gets shorter too, "There is still a consumer for this audio input device"

Fixed

> s/device/devices/

Fixed

> The monitor lock is not going out of scope at the `#endif`. Is that by intention?

Fixed

> Can we add a threading assert here?

Device changed callback can arrive with or without an active audio stream. If we register a callback, it is executed regadless a stream is open or not. The only meanigfull threading assert would be to check `!OnAudioThread` since it is not expected to arrive on audio callback.

> Is there no risk that `CurrentDriver()` already has a `NextDriver()` here?

The existing driver will be discarded.

> It's weird that we allow calls to OpenAudioInput() from any thread, but we reset GraphImpl() on the graph thread without any locking between these.
> 
> To me it seems that GraphImpl() is safe to call here because we'll only reset it once all references are dropped.
> 
> But then we don't need to be graceful here, and instead we should crash hard. Perhaps a MOZ_RELEASE_ASSERT? Thoughts?

It makes sense. I wont change it for now. I'll wait for Paul's thoughts.

> This can be called on any thread but we'll hit threading issues if we get calls to this instance from mixed threads.
> 
> Furthermore OpenAudioInput is not idempotent.
> 
> Can we reflect this with a comment at the declaration of the method, and an assert that mInputListener is unset here?

My understanding is that this is called from MediaManager thread at a point that MSG has already been created. Per previous comment, if we assert that MSG is expected to be there this method will do the same thing every time. 

Maybe I do not understand the comment correctly, can you please elaborate a bit more about what you propose here?

> Same here re: crashing hard.

According to the comment above this method can be called more than once for the same stream. Do you think it's a stale comment no more valid?

> You're casting a `nullptr` to an `AudioDeviceID`.
> 
> There's a lot of indirection between `AudioDeviceID` and the underlying `void*` type. Could you add an enum or some consts that explicitly captures the invalid states of `AudioDeviceID` so we can use that for tests and assignments like here?

Device id represents different things for different back ends. For example in Linux is a valid pointer (to a string) but in OSX is a decimal number. Only the NULL case can be represented for sure. I can create a const for that.

> s/`AudioDataListener *`/`AudioDataListener*`/

Fixed

> s/`AudioDataListener *`/`AudioDataListener*`/

Fixed

> s/`AudioDataListener *`/`AudioDataListener*`/

Fixed

> Needs docs.

Done. Please check if it is sufficient.

> s/all the channel count/all the channel counts/
> s/delievered to the listener/delivered to the listeners/
> s/themselve/themselves/

Fixed.

> `const auto&` would make sense

Fixed.
Comment on attachment 8972015 [details]
Bug 1404977 - Part 8 - Tell the MSG the MediaEngineAudioSource are now independent and that we can have multiple of them, cleanup the MSG-side API for managing them.

https://reviewboard.mozilla.org/r/240760/#review247100

> This calls AudioInputChannelCount() twice but doesn't consider a case where the two calls return different values.
> 
> Probably not so important.
> 
> But each call potentially churns through quite a lot of mutexes, so we should keep that down.
> 
> If the reevaluation was instead handled by closing an audio input and then re-opening it with a (same or mutated) listener with the updated channel count, we'd get a simpler solution with less locks and fewer code paths (this one wouldn't be necessary for one). Feasible?

Redusing the number of calls to `AudioInputChannelCount()` it's easy, but I guess this is not the important part of your comment. Close and then Open will not reduce the number of locks inside `AudioInputChannelCount()`. It is also create more work that I am not sure we need here (there is a simiral comment on part7 too).
Comment on attachment 8972016 [details]
Bug 1404977 - Part 9 - Propagate the changes to the GraphDrivers, simplifying them, and brokering all access through the MSG.

https://reviewboard.mozilla.org/r/240762/#review246942

> Oh this comment needs to be for `mInputChannelCount` !

A comment added in the definition of `mInputChannelCount`

> No need for `explicit` with two args.

Fixed

> Immutable you said, then this should be `const`.
> 
> Seems to be true for more members. Whether you want to constify them now is up to you. In any case any added compiler checks of our assumptions are very beneficial. Immutability is especially great with all the threading we have going on.

Fixed

> s/!!mInputChannelCount/mInputChannelCount > 0/

What is the problem with `!!`? I think it's nice that way.

> A device id doesn't seem like something naturally supporting `operator!`. Can we make this more explicit?

Updated, let me know if this is what you mean.

> This seems like a good fit for
> ```
> output_id = IsInvalid(forceOutputDeviceId)
>           ? GraphImpl()->mOutputDeviceID
>           : forcedOutputDeviceId;
> ```

Fixed

> Indentation is off. 3 spaces should be 2.

Fixed.

> s/!!mInputChannelCount/inputWanted/

Fixed

> Real funky indentation here.

Fixed

> s/mInputChannelCount/mInputChannelCount > 0/
> 
> Though if mInputChannelCount is 0, isn't it an error to have an aInputBuffer? An assert of this seems like an appropriate check to me.

You are right but this is not the case apparently. This is an error in cubeb remote. I have raised an issue about it: https://github.com/djg/audioipc-2/issues/42

> s/!!mInputChannelCount/mInputChannelCount > 0/

again
Comment on attachment 8968569 [details]
Bug 1404977 - Part 10 - Make logging uniform, and consistently use GraphImpl() to access the graph impl.

https://reviewboard.mozilla.org/r/237258/#review246944

> If we want all users of mGraphImpl to go through GraphImpl(), can we make mGraphImpl private?
> 
> Now you had a couple of stray uses that I've marked, but the compiler could have caught them.

Fixed

> This one is inconsistent

Fixed

> This one is inconsistent

Fixed

> This one is inconsistent

Fixed

> This one is inconsistent

Fixed

> No need for both these!
> 
> It's an old rebase gone wrong IIRC. Would be great to get rid of.

Fixed.

> Another dupe-log.

Fixed

> Looks like you were bitten by bitrot.

Fixed
Attachment #8974941 - Attachment is obsolete: true
Attachment #8974942 - Attachment is obsolete: true
Attachment #8974943 - Attachment is obsolete: true
Attachment #8974944 - Attachment is obsolete: true
Attachment #8974945 - Attachment is obsolete: true
Attachment #8974946 - Attachment is obsolete: true
Attachment #8974947 - Attachment is obsolete: true
Attachment #8974948 - Attachment is obsolete: true
Attachment #8974949 - Attachment is obsolete: true
Attachment #8974950 - Attachment is obsolete: true
Comment on attachment 8979570 [details]
Bug 1404977 - Part 1 - Add a pref to force the output device by name, for testing.

https://reviewboard.mozilla.org/r/245702/#review252064

If we're moving usage of the pref to another bug, I'd prefer moving this patch to that bug too, so we don't land code that is not used or tested.
Attachment #8979570 - Flags: review?(apehrson) → review-
Comment on attachment 8979571 [details]
Bug 1404977 - Part 2 - Augment AudioDeviceInfo with a cubeb device id.

https://reviewboard.mozilla.org/r/245704/#review252072

I still need to understand when it's ok to not have a device ID. The rest are minor issues and nits.

::: dom/media/AudioDeviceInfo.h:40
(Diff revision 1)
> +  // It is possible to not know the device identifier here. It depends on which
> +  // ctor this instance has been constructed with.

It doesn't depend on the ctor any longer. Also it would be good to know in which cases it is valid to not know the identifier, not just that it's valid *sometimes*.

::: dom/media/AudioDeviceInfo.h:42
(Diff revision 1)
> -                           uint32_t aMinLatency);
> +                  uint32_t aMinLatency);
>  
> +  explicit AudioDeviceInfo(cubeb_device_info* aInfo);
> +  // It is possible to not know the device identifier here. It depends on which
> +  // ctor this instance has been constructed with.
> +  mozilla::Maybe<AudioDeviceID> GetDeviceID();

s/GetDeviceID/DeviceID/ per the other methods

::: dom/media/AudioDeviceInfo.h:42
(Diff revision 1)
> -                           uint32_t aMinLatency);
> +                  uint32_t aMinLatency);
>  
> +  explicit AudioDeviceInfo(cubeb_device_info* aInfo);
> +  // It is possible to not know the device identifier here. It depends on which
> +  // ctor this instance has been constructed with.
> +  mozilla::Maybe<AudioDeviceID> GetDeviceID();

These methods should be const since they're all getters of const members.

::: dom/media/AudioDeviceInfo.h:43
(Diff revision 1)
>  
> +  explicit AudioDeviceInfo(cubeb_device_info* aInfo);
> +  // It is possible to not know the device identifier here. It depends on which
> +  // ctor this instance has been constructed with.
> +  mozilla::Maybe<AudioDeviceID> GetDeviceID();
> +  const nsString& FriendlyName();

Is there a non-friendly name, or why not just "Name()"?

::: dom/media/AudioDeviceInfo.cpp:14
(Diff revision 1)
>  
> -AudioDeviceInfo::AudioDeviceInfo(const nsAString& aName,
> +using namespace mozilla;
> +using namespace mozilla::CubebUtils;
> +
> +AudioDeviceInfo::AudioDeviceInfo(cubeb_device_info* aInfo)
> +:AudioDeviceInfo(aInfo->devid,

still, indentation

::: dom/media/AudioDeviceInfo.cpp:33
(Diff revision 1)
> +AudioDeviceInfo::AudioDeviceInfo(AudioDeviceID aID,
> +                                 const nsAString& aName,
>                                   const nsAString& aGroupId,
>                                   const nsAString& aVendor,
>                                   uint16_t aType,
>                                   uint16_t aState,
>                                   uint16_t aPreferred,
>                                   uint16_t aSupportedFormat,
>                                   uint16_t aDefaultFormat,
>                                   uint32_t aMaxChannels,
>                                   uint32_t aDefaultRate,
>                                   uint32_t aMaxRate,
>                                   uint32_t aMinRate,
>                                   uint32_t aMaxLatency,
>                                   uint32_t aMinLatency)
> -  : mName(aName)
> +  : mDeviceId(aID)

If the ctor without an AudioDeviceID is no longer needed, how can `aID` be a Maybe?

::: dom/media/AudioDeviceInfo.cpp:85
(Diff revision 1)
>  }
>  
> +Maybe<AudioDeviceID>
> +AudioDeviceInfo::GetDeviceID()
> +{
> +  if (mDeviceId) {

This is checking the underlying type for `nullptr`. But that type is completely opaque here. Can we make it up to the caller of the ctor to tell us whether it's a Some() or a Nothing() instead?

In that case, Some(nullptr) would be illegal and we can barf early on.

::: dom/media/CubebUtils.h:11
(Diff revision 1)
>  
>  #if !defined(CubebUtils_h_)
>  #define CubebUtils_h_
>  
>  #include "cubeb/cubeb.h"
> -#include "mozilla/dom/AudioDeviceInfo.h"
> +#include "nsString.h"

Why is this needed? This file doesn't mention any strings.
Attachment #8979571 - Flags: review?(apehrson) → review-
Comment on attachment 8972010 [details]
Bug 1404977 - Part 3 - Remove global statics, introduce an audio device enumerator

https://reviewboard.mozilla.org/r/240750/#review246916

> I hope you mean in a different bug.

No, unit tests need to land when the code they're testing lands or they will never be written.

> I see a comment there, not very detailed, I think the short answer is everything ...

Also `mThread`? Still needs documentation.

> Why not, it's good to specify the members even when the default ctor is being called.

We don't really do this anywhere else.
https://searchfox.org/mozilla-central/search?q=%2C+m%5BA-Za-z%5D%2B%5C(%5C)&case=false&regexp=true&path=dom%2Fmedia%2F**.cpp

> It's not a hack it what we have been doing since the beginning.
> https://searchfox.org/mozilla-central/source/dom/media/webrtc/MediaEngineWebRTC.cpp#297

Please use permalinks, this one appears to have shifted.

What I mean is that the comment needs to reflect *why* it is OK to lie here, not just that it is OK.

And if we always lie, why do we need separate fields for this in the first place?
Comment on attachment 8979572 [details]
Bug 1404977 - Part 3 - Remove global statics, introduce an audio device enumerator

https://reviewboard.mozilla.org/r/245706/#review252086

This is now pretty complete, but with all these nits I'd like to see it for another pass.

::: dom/media/webrtc/MediaEngineWebRTC.h:125
(Diff revision 1)
> -// Small subset of VoEHardware
> -class AudioInput
> +// This class implements a cache for accessing the audio device list. It can be
> +// accessed on any thread.

Thread access should at the very least be in the per-method documentation. That's where I'd look if I want to know how a method works.

::: dom/media/webrtc/MediaEngineWebRTC.h:132
(Diff revision 1)
> +class CubebDeviceEnumerator final
>  {
> +NS_INLINE_DECL_THREADSAFE_REFCOUNTING(CubebDeviceEnumerator)
>  public:
> -  AudioInput() = default;
> -  // Threadsafe because it's referenced from an MicrophoneSource, which can
> +  CubebDeviceEnumerator();
> +  void EnumerateAudioInputDevices(nsTArray<RefPtr<AudioDeviceInfo>>& aDevices);

Needs docs.

::: dom/media/webrtc/MediaEngineWebRTC.h:133
(Diff revision 1)
> -  // had references to it on other threads.
> -  NS_INLINE_DECL_THREADSAFE_REFCOUNTING(AudioInput)
> -
> -  virtual int GetNumOfRecordingDevices(int& aDevices) = 0;
> +  // From a cubeb device id, maybe return the info for this device, if it's
> +  // still a valid id.
> +  already_AddRefed<AudioDeviceInfo>
> +  DeviceInfoFromID(CubebUtils::AudioDeviceID aID);

"Maybe"? So it returns `nullptr` if the ID is invalid? Needs clarification.

::: dom/media/webrtc/MediaEngineWebRTC.h:143
(Diff revision 1)
> -      return;
> -    }
>  
> -    if (aChannels && aChannels < sUserChannelCount) {
> -      sUserChannelCount = aChannels;
> -    }
> +  // Static function called by cubeb when the audio input device list changes
> +  // (i.e. when a new device is made available, or non-available). This
> +  // re-binds to this MediaEngineWebRTC, and simply calls

Unclear what "this MediaEngineWebRTC" refers to.

::: dom/media/webrtc/MediaEngineWebRTC.h:146
(Diff revision 1)
> -  void StartRecording(SourceMediaStream *aStream, AudioDataListener *aListener)
> -  {
> +  // With the mutex taken, invalidates the cached audio input device list.
> +  void AudioDeviceListChanged();

A user of this method shouldn't have to care that it grabs a mutex internally. What I'd like to know is what it observably does and which threads it can be called from.

::: dom/media/webrtc/MediaEngineWebRTC.h:458
(Diff revision 1)
>  
>    nsCOMPtr<nsIThread> mThread;
>  
>    // gUM runnables can e.g. Enumerate from multiple threads
>    Mutex mMutex;
> -  RefPtr<mozilla::AudioInput> mAudioInput;
> +  RefPtr<mozilla::CubebDeviceEnumerator> mEnumerator;

This doesn't seem shared. Could it be a UniquePtr instead? Makes for a clearer ownership situation.

::: dom/media/webrtc/MediaEngineWebRTC.cpp:32
(Diff revision 1)
> -  mDevices = devices;
> -}
>  
>  MediaEngineWebRTC::MediaEngineWebRTC(MediaEnginePrefs &aPrefs)
> -  : mMutex("MediaEngineWebRTC::mMutex"),
> -    mAudioInput(nullptr),
> +  : mMutex("mozilla::MediaEngineWebRTC")
> +  , mEnumerator()

Unnecessary

::: dom/media/webrtc/MediaEngineWebRTC.cpp:186
(Diff revision 1)
> -    if (uniqueId[0] == '\0') {
> -      // Mac and Linux don't set uniqueId!
> -      strcpy(uniqueId, deviceName); // safe given assert and initialization/error-check
> +  // Handle enumeration error
> +  if (devices.IsEmpty()) {
> +    return;
> -    }
> +  }

This seems unnecessary, the for loop below should guard against this.

::: dom/media/webrtc/MediaEngineWebRTC.cpp:191
(Diff revision 1)
> -
> -    RefPtr<MediaEngineSource> aSource;
> +  // For some reason the "fake" device for automation is marked as DISABLED,
> +  // so white-list it.

Looks like you removed the whitelisting but not the comment.

::: dom/media/webrtc/MediaEngineWebRTC.cpp:194
(Diff revision 1)
> -    }
> +  }
>  
> -
> -    RefPtr<MediaEngineSource> aSource;
> -    NS_ConvertUTF8toUTF16 uuid(uniqueId);
> -
> +  // For some reason the "fake" device for automation is marked as DISABLED,
> +  // so white-list it.
> +  for (uint32_t i = 0; i < devices.Length(); i++) {
> +    MOZ_ASSERT(devices[i]->GetDeviceID().isSome());

If we assert that it must be Some here I really don't understand when Nothing is allowed.

::: dom/media/webrtc/MediaEngineWebRTC.cpp:204
(Diff revision 1)
> -        aSource->RequiresSharing()) {
> -      // We've already seen this device, just append.
> -      aSources->AppendElement(aSource.get());
> -    } else {
> -      aSource = new MediaEngineWebRTCMicrophoneSource(
> -          new mozilla::AudioInputCubeb(i),
> +         NS_ConvertUTF16toUTF8(devices[i]->FriendlyName()).get(),
> +         devices[i]->GetDeviceID().ref()));
> +
> +    if (devices[i]->State() == CUBEB_DEVICE_STATE_ENABLED) {
> +      MOZ_ASSERT(devices[i]->Type() == CUBEB_DEVICE_TYPE_INPUT);
> +      // XXX do something for the default device.

This XXX needs to be resolved like I mentioned in the previous review pass.

::: dom/media/webrtc/MediaEngineWebRTC.cpp:207
(Diff revision 1)
> -    } else {
> -      aSource = new MediaEngineWebRTCMicrophoneSource(
> -          new mozilla::AudioInputCubeb(i),
> -          i, deviceName, uniqueId,
> -          mDelayAgnostic, mExtendedFilter);
> -      devicesForThisWindow->Put(uuid, aSource);
> +    if (devices[i]->State() == CUBEB_DEVICE_STATE_ENABLED) {
> +      MOZ_ASSERT(devices[i]->Type() == CUBEB_DEVICE_TYPE_INPUT);
> +      // XXX do something for the default device.
> +      RefPtr<MediaEngineSource> source =
> +        new MediaEngineWebRTCMicrophoneSource(
> +          mEnumerator,

Why does each microphone source need the enumerator?

Looks like this was removed in part 7. `hg absorb` perhaps? Let's remove it here instead to ease the confusion.

::: dom/media/webrtc/MediaEngineWebRTC.cpp:210
(Diff revision 1)
> -          i, deviceName, uniqueId,
> -          mDelayAgnostic, mExtendedFilter);
> -      devicesForThisWindow->Put(uuid, aSource);
> -      aSources->AppendElement(aSource);
> +      RefPtr<MediaEngineSource> source =
> +        new MediaEngineWebRTCMicrophoneSource(
> +          mEnumerator,
> +          devices[i]->GetDeviceID().ref(),
> +          devices[i]->FriendlyName(),
> +          // Lie and provide the name as UUID

*Why* can we lie here? Lying sounds like a bad thing.

::: dom/media/webrtc/MediaEngineWebRTC.cpp:317
(Diff revision 1)
> +  cubeb_register_device_collection_changed(GetCubebContext(),
> +                                           CUBEB_DEVICE_TYPE_INPUT,
> +                                           &mozilla::CubebDeviceEnumerator::AudioDeviceListChanged_s,
> +                                           this);

Shouldn't we handle or assert the error returned here?

::: dom/media/webrtc/MediaEngineWebRTC.cpp:325
(Diff revision 1)
> +  cubeb_register_device_collection_changed(GetCubebContext(),
> +                                           CUBEB_DEVICE_TYPE_INPUT,
> +                                           nullptr,
> +                                           this);

Shouldn't we handle or assert the error returned here?

::: dom/media/webrtc/MediaEngineWebRTC.cpp:332
(Diff revision 1)
> +                                           nullptr,
> +                                           this);
> +}
> +
> +void
> +CubebDeviceEnumerator::EnumerateAudioInputDevices(nsTArray<RefPtr<AudioDeviceInfo>>& aDevices)

Can we reflect that this is an out parameter in its name? `aOutDevices`?

::: dom/media/webrtc/MediaEngineWebRTC.cpp:355
(Diff revision 1)
> +CubebDeviceEnumerator::DeviceInfoFromID(CubebUtils::AudioDeviceID aID)
> +{
> +  MutexAutoLock lock(mMutex);
> +
> +  for (uint32_t i  = 0; i < mDevices.Length(); i++) {
> +    if (mDevices[i]->GetDeviceID().isSome() &&

There's also an implicit `operator bool()` on Maybe: https://searchfox.org/mozilla-central/rev/2aa42f2cab4a110edf21dd7281ac23a1ea8901f9/mfbt/Maybe.h#313

Though I'm OK with explicit.
Attachment #8979572 - Flags: review?(apehrson) → review-
Comment on attachment 8979573 [details]
Bug 1404977 - Part 4 - Format a comment and remove an unused and now useless method.

https://reviewboard.mozilla.org/r/245708/#review252112
Attachment #8979573 - Flags: review?(apehrson) → review+
Comment on attachment 8979574 [details]
Bug 1404977 - Part 5 - Allow querying the number of input channels from a WebRTCAudioDataListener.

https://reviewboard.mozilla.org/r/245710/#review252120

I think this plumbing exercise could be simpler. See the comment thread on Paul's version of this patch.
Attachment #8979574 - Flags: review?(apehrson) → review-
Comment on attachment 8972012 [details]
Bug 1404977 - Part 5 - Allow querying the number of input channels from a WebRTCAudioDataListener.

https://reviewboard.mozilla.org/r/240754/#review246924

> I am not sure I get it. Do you mean to remove the channel count from the listener and pass it directly to the `OpenAudioInput()`? On the other hand, the compromise is to make it at least const member of the listener (by setting it into the constructor)?

This needs a look at the future patches too, to see how it ends up being used.

I mean that if `MediaEngineWebRTCMicrophoneSource::SetUserInputChannelCount()` instead of calling MSGImpl::ReevaluateInputDevice() forwarded the number of channels to the MSGImpl directly, the MSGImpl wouldn't have to query back into the MediaEngineWebRTCMicrophoneSource for its input channel count.

That back-query seems like an unnecessary dependency into the user. The listener interface is used for forwarding information from the graph to the listener impl, but now we're adding a method for sending information in the other direction. It feels like things would be simpler if it was avoided.

We already have data storage keyed off the audio listeners, https://searchfox.org/mozilla-central/rev/eb51d734a8c06ac5973291be9bcc74ed4e999bca/dom/media/MediaStreamGraphImpl.h#637
The value type of that storage could be expanded on to include the requested channel count.
Comment on attachment 8979575 [details]
Bug 1404977 - Part 6 - Remove unused include for lock-free FIFO.

https://reviewboard.mozilla.org/r/245712/#review252122
Attachment #8979575 - Flags: review?(apehrson) → review+
Comment on attachment 8979576 [details]
Bug 1404977 - Part 7 - Make each MediaEngineWebRTCMicrophoneSource independent.

https://reviewboard.mozilla.org/r/245714/#review252124

Incomplete review. r- for now for unsafe code and bad "channelCount" constraint handling. See the threads on Paul's original review request for more details.

::: dom/media/webrtc/MediaEngineWebRTC.h:373
(Diff revision 1)
> +  uint32_t GetUserInputChannelCount();
> +  void SetUserInputChannelCount(uint32_t aUserInputChannelCount);

You changed the member from "User" to "Requested", but not these methods. Same thinking applies here.

::: dom/media/webrtc/MediaEngineWebRTC.h:380
(Diff revision 1)
>  
>    // Owning thread only.
>    RefPtr<WebRTCAudioDataListener> mListener;
> +  RefPtr<mozilla::CubebDeviceEnumerator> mEnumerator;
> +
> +  RefPtr<AudioDeviceInfo> mDeviceInfo;

This needs docs on thread access. Could it be const?

::: dom/media/webrtc/MediaEngineWebRTC.h:382
(Diff revision 1)
> -  // Note: shared across all microphone sources. Owning thread only.
> -  static int sChannelsOpen;
> +  // Number of times this device has been opened for this MSG.
> +  int mChannelsOpen;

This is now only 0 or 1. It's not needed anymore but we could just check whether mState is kReleased or not. Or put in a convenience method for doing this.

::: dom/media/webrtc/MediaEngineWebRTCAudio.cpp:447
(Diff revision 1)
> +  // Get the number of channels asked for by content, and clamp it between the
> +  // pref and the maximum number of channels that the device supports.
>    prefs.mChannels = c.mChannelCount.Get(std::min(prefs.mChannels,
> -                                        static_cast<int32_t>(maxChannels)));
> +                                        static_cast<int32_t>(mDeviceInfo->MaxChannels())));

This doesn't do what the comment suggests.

It only considers the pref or the device's max channels if there was no ideal constraint passed in for channel count: https://searchfox.org/mozilla-central/rev/795575287259a370d00518098472eaa5b362bfa7/dom/media/webrtc/MediaTrackConstraints.h#84

This could use a test.
Attachment #8979576 - Flags: review?(apehrson) → review-
Comment on attachment 8972014 [details]
Bug 1404977 - Part 7 - Make each MediaEngineWebRTCMicrophoneSource independent.

https://reviewboard.mozilla.org/r/240758/#review246930

> Fixed

The lookup is no longer done but the member is now mutable.

> It is called from `MediaStreamGraphImpl::OpenAudioInputImpl()` (this is in part8), which it comes after a successfull enumeration. For the current usage there is a guarantee.

MediaEngineWebRTCAudio shouldn't have to rely on its users to be nice so things are available.

Also, I don't see the same guarantee as you.
Counter example:
- The call to SetUserInputChannelCount() is in part 7.
- It is called on the graph thread after a dispatch from the main thread after a dispatch from the media manager thread in ApplySettings, which is called from Reconfigure or Start.
- Either Reconfigure() or Start() may be followed by Stop() and Deallocate() on the same allocation. If this was the only allocation, mAllocations is going to be empty.

Main thread in particular can take a long long time to run runnables. The graph thread dispatch further relies on main thread stable state so again there's a chance of a long wait.

It would be fairly easy to trigger this by `track.applySettings(blah).then(() => track.stop())` while keeping main thread relatively busy.

Sorry but this is happy path coding not acceptable for landing.

> Probably you mean to call `CloseAudioInputImpl()` and `OpenAudioInputImpl()` given that we already are at the audio thread, at that point. However, those methods does more than we need. We want just to switch the driver.
> 
> On the top of that I am not sure what is "the graph's record of the requested channel count" mentioned in the final note.

In part 5 I discuss this in a bit more detail. "The graph's record" is something we'd have to create in order to not call back into MediaEngineWebRTCMicrophoneSource synchronously. Again part 5 mentions where to do this.

> Fixed

You caught one but not the other.
Comment on attachment 8979577 [details]
Bug 1404977 - Part 8 - Tell the MSG the MediaEngineAudioSource are now independent and that we can have multiple of them, cleanup the MSG-side API for managing them.

https://reviewboard.mozilla.org/r/245716/#review252132

This is so big I cannot consider this review complete. I'll take another look once existing comments and earlier patches have been addressed.

::: dom/media/MediaStreamGraph.h:712
(Diff revision 1)
>    // Users of audio inputs go through the stream so it can track when the
>    // last stream referencing an input goes away, so it can close the cubeb
>    // input.  Also note: callable on any thread (though it bounces through
>    // MainThread to set the command if needed).
> -  nsresult OpenAudioInput(int aID,
> +  nsresult OpenAudioInput(CubebUtils::AudioDeviceID aID,
>                            AudioDataListener *aListener);

s/`AudioDataListener *`/`AudioDataListener* `/

::: dom/media/MediaStreamGraph.h:1326
(Diff revision 1)
> -  virtual nsresult OpenAudioInput(int aID,
> -                                  AudioDataListener *aListener)
> -  {
> -    return NS_ERROR_FAILURE;
> +  virtual nsresult OpenAudioInput(CubebUtils::AudioDeviceID aID,
> +                                  AudioDataListener *aListener) = 0;
> +  virtual void CloseAudioInput(CubebUtils::AudioDeviceID aID,
> +                               AudioDataListener *aListener) = 0;

s/`AudioDataListener *`/`AudioDataListener* `/

::: dom/media/MediaStreamGraph.cpp:61
(Diff revision 1)
>    TRACK_CREATE = TrackEventCommand::TRACK_EVENT_CREATED,
>    TRACK_END = TrackEventCommand::TRACK_EVENT_ENDED,
>    TRACK_UNUSED = TrackEventCommand::TRACK_EVENT_UNUSED,
>  };
>  
> +const void* DefaultDeviceID = nullptr;

This seems like it should be defined in CubebUtils where AudioDeviceID is defined.

Also it's called "Default" but it's used in CloseAudioInput like it was a "InvalidDeviceID" or "AnyDeviceID".

Which device IDs are special needs to be clear and well defined.

::: dom/media/MediaStreamGraph.cpp:802
(Diff revision 1)
>    return ticksWritten;
>  }
>  
>  void
> -MediaStreamGraphImpl::OpenAudioInputImpl(int aID,
> +MediaStreamGraphImpl::OpenAudioInputImpl(CubebUtils::AudioDeviceID aID,
>                                           AudioDataListener *aListener)

s/`AudioDataListener *`/`AudioDataListener* `/

::: dom/media/MediaStreamGraph.cpp:838
(Diff revision 1)
>    }
>  }
>  
>  nsresult
> -MediaStreamGraphImpl::OpenAudioInput(int aID,
> +MediaStreamGraphImpl::OpenAudioInput(CubebUtils::AudioDeviceID aID,
>                                       AudioDataListener *aListener)

s/`AudioDataListener *`/`AudioDataListener* `/

::: dom/media/MediaStreamGraph.cpp:853
(Diff revision 1)
>      return NS_OK;
>    }
>    class Message : public ControlMessage {
>    public:
> -    Message(MediaStreamGraphImpl *aGraph, int aID,
> +    Message(MediaStreamGraphImpl *aGraph, CubebUtils::AudioDeviceID aID,
>              AudioDataListener *aListener) :

s/`AudioDataListener *`/`AudioDataListener* `/

::: dom/media/MediaStreamGraph.cpp:869
(Diff revision 1)
> -MediaStreamGraphImpl::CloseAudioInputImpl(AudioDataListener *aListener)
> +MediaStreamGraphImpl::CloseAudioInputImpl(CubebUtils::AudioDeviceID aID, AudioDataListener* aListener)
>  {
>    MOZ_ASSERT(OnGraphThreadOrNotRunning());
> -  uint32_t count;
> -  DebugOnly<bool> result = mInputDeviceUsers.Get(aListener, &count);
> +  // It is possible to not know the ID here, find it first.
> +  if (aID == nullptr) {

So higher up in the file we had a definition saying that `nullptr` meant the default devie.

Now `nullptr` apparently means two things.

It seems to me like this method needs to take a `Maybe<AudioDeviceID>` where `Nothing()` means "look it up from the listener". This info should then go in a comment where the method is declared.

::: dom/media/MediaStreamGraph.cpp:932
(Diff revision 1)
>      mAbstractMainThread->Dispatch(runnable.forget());
>      return;
>    }
>    class Message : public ControlMessage {
>    public:
> -    Message(MediaStreamGraphImpl *aGraph, AudioDataListener *aListener) :
> +    Message(MediaStreamGraphImpl *aGraph, CubebUtils::AudioDeviceID aID, AudioDataListener *aListener) :

s/`AudioDataListener *`/`AudioDataListener* `/

::: dom/media/MediaStreamGraph.cpp:2751
(Diff revision 1)
>  {
>  }
>  
>  nsresult
> -SourceMediaStream::OpenAudioInput(int aID,
> +SourceMediaStream::OpenAudioInput(CubebUtils::AudioDeviceID aID,
>                                    AudioDataListener *aListener)

s/`AudioDataListener *`/`AudioDataListener* `/

::: dom/media/MediaStreamGraphImpl.h:412
(Diff revision 1)
> +                        TrackRate aRate, uint32_t aChannels);
> +  /* Called on the graph thread when there is new input data for listeners. This
> +   * is the raw audio input for this MediaStreamGraph. */
> +  void NotifyInputData(const AudioDataValue* aBuffer, size_t aFrames,
> +                       TrackRate aRate, uint32_t aChannels);
> +  /* Called every time there are changes to audio device like plug/unplug etc. */

Needs to state the thread it can be called on, like the others.

::: dom/media/MediaStreamGraphImpl.h:457
(Diff revision 1)
> +  /** The audio input channel count for a MediaStreamGraph is the max of all the
> +   * channel counts requested by the listeners. The max channel count is
> +   * delivered to the listener themselves, and they take care of downmixing. */

It's pretty standard to put the `/**` and `*/` on separate lines.
Attachment #8979577 - Flags: review?(apehrson) → review-
Comment on attachment 8972015 [details]
Bug 1404977 - Part 8 - Tell the MSG the MediaEngineAudioSource are now independent and that we can have multiple of them, cleanup the MSG-side API for managing them.

https://reviewboard.mozilla.org/r/240760/#review246934

> Device changed callback can arrive with or without an active audio stream. If we register a callback, it is executed regadless a stream is open or not. The only meanigfull threading assert would be to check `!OnAudioThread` since it is not expected to arrive on audio callback.

First of all that threading information has to go in a comment on the method. Preferably in the method too since there's no meaningful assert we can put.

Buuuut, how is this thread safe then?
I'm looking at the `nsDataHashtable` that is used both here and on the graph thread (NotifyInputData is the closest example).

> My understanding is that this is called from MediaManager thread at a point that MSG has already been created. Per previous comment, if we assert that MSG is expected to be there this method will do the same thing every time. 
> 
> Maybe I do not understand the comment correctly, can you please elaborate a bit more about what you propose here?

The SourceMediaStream doesn't know that it's always the same thread calling this method. That's why we have asserts.

`mInputListener` is up for a race if more than one thread tries to call OpenAudioInput() on the same source stream.

This is minor because you're not touching this code, but normally this needs to be asserted so we can catch misusage. I'd do this by passing an AbstractThread or some such to the ctor and then in each method assert that we're on that thread.

> According to the comment above this method can be called more than once for the same stream. Do you think it's a stale comment no more valid?

Comment seems truthful, but eeeeh:
MediaEngineWebRTCMicrophoneSource calls this from the MediaManager thread, [1].
MSG calls this through DestroyImpl on the graph thread, [2].
That is by definition unsafe!

[1] https://searchfox.org/mozilla-central/rev/eb51d734a8c06ac5973291be9bcc74ed4e999bca/dom/media/webrtc/MediaEngineWebRTCAudio.cpp#757
[2] https://searchfox.org/mozilla-central/rev/eb51d734a8c06ac5973291be9bcc74ed4e999bca/dom/media/MediaStreamGraph.cpp#2076
Comment on attachment 8979578 [details]
Bug 1404977 - Part 9 - Propagate the changes to the GraphDrivers, simplifying them, and brokering all access through the MSG.

https://reviewboard.mozilla.org/r/245718/#review252208

Looks mostly good. Review is pretty complete. But I do want to see the special cases of AudioDeviceID and the input channel back-querying resolved in earlier patches before signing off.

::: dom/media/GraphDriver.h:503
(Diff revision 1)
>     * has been called, and is synchronized internaly. */
>    nsAutoRef<cubeb_stream> mAudioStream;
>    /* The sample rate for the aforementionned cubeb stream. This is set on
>     * initialization and can be read safely afterwards. */
>    uint32_t mSampleRate;
>    /* The number of input channels from cubeb.  Should be set before opening cubeb

"Should be" is no longer true. It is always set before this driver opens cubeb, and it can never be changed.

Because it's const, yay!

::: dom/media/GraphDriver.cpp:590
(Diff revision 1)
>    }
>  #endif
>    return false;
>  }
>  
> +bool IsSome(cubeb_devid id) {return id;}

Your intention is good IMO, but the utils and constaints around AudioDeviceID should stick close to the declaration of AudioDeviceID.

If they work on the cubeb_devid type they should be declared close to its declaration instead.

::: dom/media/GraphDriver.cpp:705
(Diff revision 1)
> -        CubebUtils::ReportCubebStreamInitFailure(firstStream);
> +      CubebUtils::ReportCubebStreamInitFailure(firstStream);
> -      }
> +    }
> -      MonitorAutoLock lock(GraphImpl()->GetMonitor());
> +    MonitorAutoLock lock(GraphImpl()->GetMonitor());
> -      FallbackToSystemClockDriver();
> +    FallbackToSystemClockDriver();
> -      return true;
> +    return true;
> -    }
> +   }

3 spaces again

::: dom/media/GraphDriver.cpp:963
(Diff revision 1)
>      mIterationEnd = stateComputedTime;
>    }
>  
>    // Process mic data if any/needed
>    if (aInputBuffer) {
> -    if (mAudioInput) { // for this specific input-only or full-duplex stream
> +    MOZ_ASSERT(mInputChannelCount);

Please check this explicitly against 0 too

::: dom/media/GraphDriver.cpp:965
(Diff revision 1)
>  
>    // Process mic data if any/needed
>    if (aInputBuffer) {
> -    if (mAudioInput) { // for this specific input-only or full-duplex stream
> -      mAudioInput->NotifyInputData(mGraphImpl, aInputBuffer,
> -                                   static_cast<size_t>(aFrames),
> +    MOZ_ASSERT(mInputChannelCount);
> +    GraphImpl()->NotifyInputData(aInputBuffer, static_cast<size_t>(aFrames),
> +                                mSampleRate, mInputChannelCount);

indentation, this is not left-aligned with the previous args

::: dom/media/GraphDriver.cpp:1106
(Diff revision 1)
> -AudioCallbackDriver::SetMicrophoneActive(bool aActive)
> -{
> -  mMicrophoneActive = aActive;
> -
>  #ifdef XP_MACOSX
> -  PanOutputIfNeeded(mMicrophoneActive);
> +  PanOutputIfNeeded(!!mInputChannelCount);

Please check this explicitly against 0 too

::: dom/media/MediaStreamGraph.cpp:3640
(Diff revision 1)
> +      AudioCallbackDriver* driver = new AudioCallbackDriver(this, 0);
>        mDriver = driver;

Can we skip the rawptr and assign straight to mDriver?
Attachment #8979578 - Flags: review?(apehrson) → review-
Comment on attachment 8972016 [details]
Bug 1404977 - Part 9 - Propagate the changes to the GraphDrivers, simplifying them, and brokering all access through the MSG.

https://reviewboard.mozilla.org/r/240762/#review246942

> What is the problem with `!!`? I think it's nice that way.

Consider it a nit, but semantics. You want input if there's a non-zero count. Using `!!` hides the zero check from the reader, who needs to look up the type and how the implicit cast to bool works, to verify the behavior.
Comment on attachment 8979579 [details]
Bug 1404977 - Part 10 - Make logging uniform, and consistently use GraphImpl() to access the graph impl.

https://reviewboard.mozilla.org/r/245720/#review252116

r=me with nits fixed

::: dom/media/GraphDriver.h:237
(Diff revision 1)
> +  // The MediaStreamGraphImpl that owns this driver. This has a lifetime longer
> +  // than the driver, and will never be null. Hence, it can be accesed without
> +  // monitor.
> +  MediaStreamGraphImpl* mGraphImpl;

Sounds like it should be const too.

::: dom/media/GraphDriver.cpp:973
(Diff revision 1)
> -  if (aInputBuffer) {
> +  if (aInputBuffer && mInputChannelCount) {
> -    MOZ_ASSERT(mInputChannelCount);

This change should be in part 9 no?

Looks like a revert of the changes made there.

::: dom/media/GraphDriver.cpp:973
(Diff revision 1)
>      MOZ_ASSERT_UNREACHABLE("We should not underrun in full duplex");
>      mIterationEnd = stateComputedTime;
>    }
>  
>    // Process mic data if any/needed
> -  if (aInputBuffer) {
> +  if (aInputBuffer && mInputChannelCount) {

s/mInputChannelCount/mInputChannelCount > 0/

::: dom/media/MediaStreamGraph.cpp:77
(Diff revision 1)
>    MOZ_ASSERT(mStreams.IsEmpty() && mSuspendedStreams.IsEmpty(),
>               "All streams should have been destroyed by messages from the main thread");
>    LOG(LogLevel::Debug, ("MediaStreamGraph %p destroyed", this));
>    LOG(LogLevel::Debug, ("MediaStreamGraphImpl::~MediaStreamGraphImpl"));
>  
> +

?
Attachment #8979579 - Flags: review?(apehrson) → review+
Comment on attachment 8979571 [details]
Bug 1404977 - Part 2 - Augment AudioDeviceInfo with a cubeb device id.

https://reviewboard.mozilla.org/r/245704/#review252550

::: dom/media/AudioDeviceInfo.h:40
(Diff revision 1)
> +  // It is possible to not know the device identifier here. It depends on which
> +  // ctor this instance has been constructed with.

No cases anymore, I've made it a normal member.

::: dom/media/AudioDeviceInfo.h:42
(Diff revision 1)
> -                           uint32_t aMinLatency);
> +                  uint32_t aMinLatency);
>  
> +  explicit AudioDeviceInfo(cubeb_device_info* aInfo);
> +  // It is possible to not know the device identifier here. It depends on which
> +  // ctor this instance has been constructed with.
> +  mozilla::Maybe<AudioDeviceID> GetDeviceID();

This is Gecko style: if a method starts with Get, it can fail, if a method does not, it cannot.

That said, it can't fail anymore, so it's not `DeviceID`, and everything is const.

::: dom/media/AudioDeviceInfo.h:43
(Diff revision 1)
>  
> +  explicit AudioDeviceInfo(cubeb_device_info* aInfo);
> +  // It is possible to not know the device identifier here. It depends on which
> +  // ctor this instance has been constructed with.
> +  mozilla::Maybe<AudioDeviceID> GetDeviceID();
> +  const nsString& FriendlyName();

This was the name of the field in cubeb's struct. I'm changing this to simply "Name".

::: dom/media/AudioDeviceInfo.cpp:33
(Diff revision 1)
> +AudioDeviceInfo::AudioDeviceInfo(AudioDeviceID aID,
> +                                 const nsAString& aName,
>                                   const nsAString& aGroupId,
>                                   const nsAString& aVendor,
>                                   uint16_t aType,
>                                   uint16_t aState,
>                                   uint16_t aPreferred,
>                                   uint16_t aSupportedFormat,
>                                   uint16_t aDefaultFormat,
>                                   uint32_t aMaxChannels,
>                                   uint32_t aDefaultRate,
>                                   uint32_t aMaxRate,
>                                   uint32_t aMinRate,
>                                   uint32_t aMaxLatency,
>                                   uint32_t aMinLatency)
> -  : mName(aName)
> +  : mDeviceId(aID)

It's not anymore, you're right.

::: dom/media/AudioDeviceInfo.cpp:85
(Diff revision 1)
>  }
>  
> +Maybe<AudioDeviceID>
> +AudioDeviceInfo::GetDeviceID()
> +{
> +  if (mDeviceId) {

This has now been overtaken by events.
Comment on attachment 8979571 [details]
Bug 1404977 - Part 2 - Augment AudioDeviceInfo with a cubeb device id.

https://reviewboard.mozilla.org/r/245704/#review252072

This has been solved: it always returns a valid ID now. I think it was something that was left from a previous design.
Comment on attachment 8979571 [details]
Bug 1404977 - Part 2 - Augment AudioDeviceInfo with a cubeb device id.

https://reviewboard.mozilla.org/r/245704/#review252566

::: dom/media/CubebUtils.h:11
(Diff revision 1)
>  
>  #if !defined(CubebUtils_h_)
>  #define CubebUtils_h_
>  
>  #include "cubeb/cubeb.h"
> -#include "mozilla/dom/AudioDeviceInfo.h"
> +#include "nsString.h"

Yeah, not needed anymore it seems.
Comment on attachment 8979572 [details]
Bug 1404977 - Part 3 - Remove global statics, introduce an audio device enumerator

https://reviewboard.mozilla.org/r/245706/#review252086

> A user of this method shouldn't have to care that it grabs a mutex internally. What I'd like to know is what it observably does and which threads it can be called from.

This method is private, so I'm describing implementation details.

> This doesn't seem shared. Could it be a UniquePtr instead? Makes for a clearer ownership situation.

Indeed, again something from an earlier design where I was passing the enumerator down.

> This seems unnecessary, the for loop below should guard against this.

Yes.

> Looks like you removed the whitelisting but not the comment.

Fixed.

> If we assert that it must be Some here I really don't understand when Nothing is allowed.

It's now a normal member.

> This XXX needs to be resolved like I mentioned in the previous review pass.

Yes, I'll do a sort in an other patch that will be specifically about the policy.

> *Why* can we lie here? Lying sounds like a bad thing.

It's been some times that the UUID are useless, I just don't want to change everything at once.

> Shouldn't we handle or assert the error returned here?

Yes.

> Shouldn't we handle or assert the error returned here?

Yes.

> Can we reflect that this is an out parameter in its name? `aOutDevices`?

Ok.

> There's also an implicit `operator bool()` on Maybe: https://searchfox.org/mozilla-central/rev/2aa42f2cab4a110edf21dd7281ac23a1ea8901f9/mfbt/Maybe.h#313
> 
> Though I'm OK with explicit.

It's now a normal member.
(In reply to Andreas Pehrson [:pehrsons] from comment #29)
> Comment on attachment 8972012 [details]
> Bug 1404977 - Part 5 - Allow querying the number of input channel from a
> WebRTCAudioDataListener.
> 
> https://reviewboard.mozilla.org/r/240754/#review246924
> 
> ::: commit-message-a6649:1
> (Diff revision 1)
> > +Bug 1404977 - Part 5 - Allow querying the number of input channel from a WebRTCAudioDataListener. r?pehrsons
> 
> s/channel/channels/
> 
> ::: dom/media/webrtc/MediaEngineWebRTC.h:187
> (Diff revision 1)
> >                         const AudioDataValue* aBuffer,
> >                         size_t aFrames,
> >                         TrackRate aRate,
> >                         uint32_t aChannels) override;
> >  
> > +  uint32_t InputChannelCount() override;
> 
> This seems to only be called from `MSGImpl::AudioInputChannelCount()`.
> 
> Am I correct in that this can never change?
> 
> Then it would make more sense to be passed on the side of the listener to
> `OpenAudioInput()`. Or for a compromise, pass it to the ctor and make it
> const for immutability, keep the method and drop the lock.

This can change, when we call `applySetting`. Having a lock is a problem indeed, but this patch set is not about removing shared mutable state, and we'll address this after (when we'll go back to a message passing approach instead of shared mutable state protected by locks).
Comment on attachment 8979576 [details]
Bug 1404977 - Part 7 - Make each MediaEngineWebRTCMicrophoneSource independent.

https://reviewboard.mozilla.org/r/245714/#review253310
Comment on attachment 8979577 [details]
Bug 1404977 - Part 8 - Tell the MSG the MediaEngineAudioSource are now independent and that we can have multiple of them, cleanup the MSG-side API for managing them.

https://reviewboard.mozilla.org/r/245716/#review252132

> This seems like it should be defined in CubebUtils where AudioDeviceID is defined.
> 
> Also it's called "Default" but it's used in CloseAudioInput like it was a "InvalidDeviceID" or "AnyDeviceID".
> 
> Which device IDs are special needs to be clear and well defined.

I've removed this and implemented your suggestion (using `Maybe`).

> So higher up in the file we had a definition saying that `nullptr` meant the default devie.
> 
> Now `nullptr` apparently means two things.
> 
> It seems to me like this method needs to take a `Maybe<AudioDeviceID>` where `Nothing()` means "look it up from the listener". This info should then go in a comment where the method is declared.

You're right, I've used `Maybe` here.

> Needs to state the thread it can be called on, like the others.

It can be called on different threads depending on the platform, I've clarified the situation.
I'm seeing issues with my automated tests here failing, am looking into them more, hold off on review.

I'm also seeing issues with some of the manual tests. Since the bug is quite long, here are the pens I'm using again: multiple gums in one page[0] and multiple iframe gums[1].

Env:
- Linux Mint 18.3 64bit VM (VMWare)
- Debug-opt Firefox built locally with clang
- Laptop microphone fed through to the VM by VMWare is default device. A monitor of my speakers and a monitor of null (the automated test device) are also present.

Issue:
> Assertion failure: aStream->GraphImpl()->IterationEnd() > mAllocations[i].mLastCallbackAppendTime, at /home/b/projects/mozilla/mozilla-central/dom/media/webrtc/MediaEngineWebRTCAudio.cpp:855

STR:
- Load either codepen mentioned in this comment
- Create a gum stream via UI with default settings
- Apply AEC (or other setting) to the stream via the UI
- Assert it hit
OR
- Load either codepen mentioned in this comment
- Create a gum stream via UI with AEC (or other setting applied) settings
- Assert it hit
OR
- Load either codepen mentioned in this comment
- Create a gum stream via UI with default settings + 2 channels
- Lower channel to 1 for that stream and apply via UI (possible intermittent)
- Assert it hit

Issue:
> Assertion failure: mState == kAllocated || mState == kStarted, at /home/b/projects/mozilla/mozilla-central/dom/media/webrtc/MediaEngineWebRTCAudio.cpp:644

STR:
- Load either codepen mentioned in this comment
- Create a gum stream via UI with default settings
- Refresh the page
- Assert is hit

Following these issues my browser is often left in a state where I cannot complete further gUM calls and need to kill it and zombie children and restart before I can test again. Log spam in this case include the following (least they're helpful).

Before killing main process:
> [Child 7331, Chrome_ChildThread] WARNING: pipe error: Bad file descriptor: file /home/b/projects/mozilla/mozilla-central/ipc/chromium/src/chrome/common/ipc_channel_posix.cc, line 726

After killing main process:
> [Child 7392, MediaStreamGrph] WARNING: No audio stream!: 'mDriver->mAudioStream || aOperation == INIT', file /home/b/projects/mozilla/mozilla-central/dom/media/GraphDriver.cpp, line 475


[0]: https://codepen.io/SingingTree/pen/yjaRzv
[1]: https://codepen.io/SingingTree/pen/BxWePB
Attachment #8979570 - Attachment is obsolete: true
Attachment #8979571 - Attachment is obsolete: true
Attachment #8979572 - Attachment is obsolete: true
Attachment #8979573 - Attachment is obsolete: true
Attachment #8979574 - Attachment is obsolete: true
Attachment #8979575 - Attachment is obsolete: true
Attachment #8979576 - Attachment is obsolete: true
Attachment #8979577 - Attachment is obsolete: true
Attachment #8979578 - Attachment is obsolete: true
Attachment #8979579 - Attachment is obsolete: true
Comment on attachment 8968567 [details]
Bug 1404977 - Part 1 - Add missing lock to the PREF_CUBEB_BACKEND branch of the pref callback in CubebUtils.cpp.

https://reviewboard.mozilla.org/r/237254/#review254342

Like I said in Alex' version of this patch, if we're not going to use the pref in any of the patches here anymore, we should move this patch to the bug where we start using it too.

See this thread for background: https://reviewboard.mozilla.org/r/237254/#comment316180
Attachment #8968567 - Flags: review?(apehrson) → review-
Comment on attachment 8968568 [details]
Bug 1404977 - Part 2 - Augment AudioDeviceInfo with a cubeb device id.

https://reviewboard.mozilla.org/r/237256/#review254344

Much cleaner now, thank you! r=me with the nits below fixed

::: dom/media/AudioDeviceInfo.h:38
(Diff revision 3)
> -                           uint32_t aMaxRate,
> +                  uint32_t aMaxRate,
> -                           uint32_t aMinRate,
> +                  uint32_t aMinRate,
> -                           uint32_t aMaxLatency,
> +                  uint32_t aMaxLatency,
> -                           uint32_t aMinLatency);
> +                  uint32_t aMinLatency);
>  
> +  explicit AudioDeviceInfo(cubeb_device_info* aInfo);

Perhaps group this with the other ctor above instead of the methods below.

::: dom/media/AudioDeviceInfo.cpp:16
(Diff revision 3)
>  
> -AudioDeviceInfo::AudioDeviceInfo(const nsAString& aName,
> +using namespace mozilla;
> +using namespace mozilla::CubebUtils;
> +
> +AudioDeviceInfo::AudioDeviceInfo(cubeb_device_info* aInfo)
> +: AudioDeviceInfo(aInfo->devid,

Still needs to be indented.

::: dom/media/AudioDeviceInfo.cpp:89
(Diff revision 3)
> +AudioDeviceID
> +AudioDeviceInfo::DeviceID() const
> +{
> +  return mDeviceId;
> +}
> +

For consistency, remove this newline or add newlines between other methods

::: dom/media/CubebUtils.h:11
(Diff revision 3)
>  
>  #if !defined(CubebUtils_h_)
>  #define CubebUtils_h_
>  
>  #include "cubeb/cubeb.h"
> -#include "mozilla/dom/AudioDeviceInfo.h"
> +#include <nsString.h>

Should use '"' for our own includes, and '<', '>' for std.

Is nsString.h needed here though? This file doesn't mention any strings.

::: dom/media/CubebUtils.h:13
(Diff revision 3)
>  #define CubebUtils_h_
>  
>  #include "cubeb/cubeb.h"
> -#include "mozilla/dom/AudioDeviceInfo.h"
> +#include <nsString.h>
> +#include "mozilla/RefPtr.h"
>  #include "mozilla/Maybe.h"

Maybe.h is not needed now I reckon.
Attachment #8968568 - Flags: review?(apehrson) → review+
Comment on attachment 8979572 [details]
Bug 1404977 - Part 3 - Remove global statics, introduce an audio device enumerator

https://reviewboard.mozilla.org/r/245706/#review252086

> This method is private, so I'm describing implementation details.

Fair enough.

> It's been some times that the UUID are useless, I just don't want to change everything at once.

Fair enough.
Comment on attachment 8972010 [details]
Bug 1404977 - Part 3 - Remove global statics, introduce an audio device enumerator

https://reviewboard.mozilla.org/r/240750/#review254346

Thanks! Only nits now. And two old open issues on mEnumerator in the mic source and unit tests that I'd like at least replies/justification to. r=me with that.

::: dom/media/webrtc/MediaEngineWebRTC.h:132
(Diff revision 2)
> +class CubebDeviceEnumerator final
>  {
>  public:
> -  AudioInput() = default;
> -  // Threadsafe because it's referenced from an MicrophoneSource, which can
> -  // had references to it on other threads.
> +  CubebDeviceEnumerator();
> +  ~CubebDeviceEnumerator();
> +  // This method returns a list of all the input and output audio devices

To be really nitpicky it doesn't return it, it populates the array argument with them.

::: dom/media/webrtc/MediaEngineWebRTC.h:135
(Diff revision 2)
> -  AudioInput() = default;
> -  // Threadsafe because it's referenced from an MicrophoneSource, which can
> -  // had references to it on other threads.
> -  NS_INLINE_DECL_THREADSAFE_REFCOUNTING(AudioInput)
> -
> -  virtual int GetNumOfRecordingDevices(int& aDevices) = 0;
> +  CubebDeviceEnumerator();
> +  ~CubebDeviceEnumerator();
> +  // This method returns a list of all the input and output audio devices
> +  // available on this machine.
> +  // This method is safe to call from all threads.
> +  void EnumerateAudioInputDevices(nsTArray<RefPtr<AudioDeviceInfo>>& aDevices);

aOutDevices could be reflected here too

::: dom/media/webrtc/MediaEngineWebRTC.h:136
(Diff revision 2)
> -  virtual int GetRecordingDeviceName(int aIndex, char (&aStrNameUTF8)[128],
> -                                     char aStrGuidUTF8[128]) = 0;
> +  // From a cubeb device id, maybe return the info for this device, if it's
> +  // still a valid id, or nullptr otherwise.

"maybe" is superfluous with the "if...otherwise" that follows.

::: dom/media/webrtc/MediaEngineWebRTC.h:160
(Diff revision 2)
> -  void UpdateDeviceList();
> -
> -  // We have an array, which consists of indexes to the current mDevices
> -  // list.  This is updated on mDevices updates.  Many devices in mDevices
> -  // won't be included in the array (wrong type, etc), or if a device is
> -  // removed it will map to -1 (and opens of this device will need to check
> +  Mutex mMutex;
> +  nsTArray<RefPtr<AudioDeviceInfo>> mDevices;
> +  // If mManualInvalidation is true, then it is necessary to query the device
> +  // list each time instead of relying on automatic invalidation of the cache by
> +  // cubeb itself.
> +  bool mManualInvalidation;

This is set in the ctor so therefore reading can happen on any thread, correct? Or does it need protection by mMutex? Please clarify in comment.

::: dom/media/webrtc/MediaEngineWebRTC.h:462
(Diff revision 2)
>    nsCOMPtr<nsIThread> mThread;
>  
>    // gUM runnables can e.g. Enumerate from multiple threads
>    Mutex mMutex;
> -  RefPtr<mozilla::AudioInput> mAudioInput;
> +  UniquePtr<mozilla::CubebDeviceEnumerator> mEnumerator;
>    bool mFullDuplex;
>    bool mDelayAgnostic;
>    bool mExtendedFilter;
>    bool mHasTabVideoSource;

This needs info on what is protected by mMutex.

I'll assume the ones below. What about mThread?

::: dom/media/webrtc/MediaEngineWebRTC.cpp:179
(Diff revision 2)
>                                                nsTArray<RefPtr<MediaEngineSource> >* aSources)
>  {
>    mMutex.AssertCurrentThreadOwns();
>  
> -  if (!mAudioInput) {
> -    if (!SupportsDuplex()) {
> +  if (!mEnumerator) {
> +    mEnumerator.reset(new CubebDeviceEnumerator());

nitpicky: Could also use `mEnumerator = MakeUnique<CubebDeviceEnumerator>();`. Perhaps a bit more semantical than reset.

::: dom/media/webrtc/MediaEngineWebRTC.cpp:310
(Diff revision 2)
> +  int rv =
> +    cubeb_register_device_collection_changed(GetCubebContext(),
> +                                             CUBEB_DEVICE_TYPE_INPUT,
> +                                             &mozilla::CubebDeviceEnumerator::AudioDeviceListChanged_s,
> +                                             this);

We are more and more getting style like the below, I think this comes from clang-format. It would probably help here too.

```
int rv = cubeb_register_device_collection_changed(
  GetCubebContext(),
  CUBEB_DEVICE_TYPE_INPUT,
  &mozilla::CubebDeviceEnumerator::AudioDeviceListChanged_s,
  this);
```
Attachment #8972010 - Flags: review?(apehrson) → review+
Comment on attachment 8972012 [details]
Bug 1404977 - Part 5 - Allow querying the number of input channels from a WebRTCAudioDataListener.

https://reviewboard.mozilla.org/r/240754/#review254350

r- here until this issue on part 7 is addressed: https://reviewboard.mozilla.org/r/240758/#comment316458
Attachment #8972012 - Flags: review?(apehrson) → review-
Comment on attachment 8972014 [details]
Bug 1404977 - Part 7 - Make each MediaEngineWebRTCMicrophoneSource independent.

https://reviewboard.mozilla.org/r/240758/#review254352

Better, but still a couple of open older issues. The non-null non-guarantee and the InputChannelCount() back-query issues are both reasons for the r-.

::: dom/media/webrtc/MediaEngineWebRTCAudio.cpp:437
(Diff revision 2)
> -  // Check channelCount violation
> -  if (static_cast<int32_t>(maxChannels) < c.mChannelCount.mMin ||
> -      static_cast<int32_t>(maxChannels) > c.mChannelCount.mMax) {
> +  int32_t maxChannels = mDeviceInfo->MaxChannels();
> +
> +  // First, check channelCount violation wrt constraints. This fails in case of
> +  // error.
> +  if (maxChannels < c.mChannelCount.mMin ||
> +      maxChannels > c.mChannelCount.mMax) {

Why is this a problem?

If the device can do 1-2 channels (maxChannels == 2) and I request constraints `channelCount: {max: 1}` this would be considered a bad constraint but it seems OK to me.

::: dom/media/webrtc/MediaEngineWebRTCAudio.cpp:444
(Diff revision 2)
>      return NS_ERROR_FAILURE;
>    }
> -  // Clamp channelCount to a valid value
> +  // A pref can force the channel count to use. If the pref has a value of zero
> +  // or lower, it has no effect.
>    if (prefs.mChannels <= 0) {
> -    prefs.mChannels = static_cast<int32_t>(maxChannels);
> +    prefs.mChannels = static_cast<int32_t>(mDeviceInfo->MaxChannels());

s/static_cast<int32_t>(mDeviceInfo->MaxChannels())/maxChannels/

::: dom/media/webrtc/MediaEngineWebRTCAudio.cpp:451
(Diff revision 2)
> +
> +  // Get the number of channels asked for by content, and clamp it between the
> +  // pref and the maximum number of channels that the device supports.
>    prefs.mChannels = c.mChannelCount.Get(std::min(prefs.mChannels,
> -                                        static_cast<int32_t>(maxChannels)));
> -  // Clamp channelCount to a valid value
> +                                        maxChannels));
> +

Remove this empty line to make it clear that the comment above applies to both statements.

::: dom/media/webrtc/MediaEngineWebRTCAudio.cpp:484
(Diff revision 2)
>      default:
> -      LOG(("Audio device %d in ignored state %d", mCapIndex, mState));
> +      LOG(("Audio device %s in ignored state %d", NS_ConvertUTF16toUTF8(mDeviceInfo->Name()).get(), mState));
>        break;
>    }
>  
> -  if (MOZ_LOG_TEST(GetMediaManagerLog(), LogLevel::Debug)) {
> +  if (mState == kAllocated) {

This doesn't cover kStarted or kStopped.

::: dom/media/webrtc/MediaEngineWebRTCAudio.cpp:532
(Diff revision 2)
> +  {
> +    if (mAllocations.IsEmpty()) {
> +      return;

indentation
Attachment #8972014 - Flags: review?(apehrson) → review-
Comment on attachment 8972015 [details]
Bug 1404977 - Part 8 - Tell the MSG the MediaEngineAudioSource are now independent and that we can have multiple of them, cleanup the MSG-side API for managing them.

https://reviewboard.mozilla.org/r/240760/#review247100

> Redusing the number of calls to `AudioInputChannelCount()` it's easy, but I guess this is not the important part of your comment. Close and then Open will not reduce the number of locks inside `AudioInputChannelCount()`. It is also create more work that I am not sure we need here (there is a simiral comment on part7 too).

It's not about more work but rechanneling the work already done to a simpler solution, which is what I think matters most. With a solution like I mention in a couple places on avoiding the back-query into the listener, we'd also get rid of AudioInputChannelCount() completely, so yes, that would reduce its number of locks :-)
Comment on attachment 8979577 [details]
Bug 1404977 - Part 8 - Tell the MSG the MediaEngineAudioSource are now independent and that we can have multiple of them, cleanup the MSG-side API for managing them.

https://reviewboard.mozilla.org/r/245716/#review252132

> It can be called on different threads depending on the platform, I've clarified the situation.

This situation is not threadsafe. See https://reviewboard.mozilla.org/r/240760/#comment316326 for one
Comment on attachment 8972015 [details]
Bug 1404977 - Part 8 - Tell the MSG the MediaEngineAudioSource are now independent and that we can have multiple of them, cleanup the MSG-side API for managing them.

https://reviewboard.mozilla.org/r/240760/#review254364

r- for old issues not addressed. These include some unsafe thread races and the InputChannelCount() back-query that I think should be simplified.

::: dom/media/MediaStreamGraph.cpp:987
(Diff revision 2)
> +void MediaStreamGraphImpl::DeviceChanged()
> +{
> +  MOZ_ASSERT(!OnGraphThread());
> +  if (!mInputDeviceID) {
> +    return;
> +  }
> +  nsTArray<RefPtr<AudioDataListener>>* listeners = mInputDeviceUsers.GetValue(mInputDeviceID);
> +  for (auto& listener : *listeners) {
> +    listener->DeviceChanged();
> +  }
> +}

If this is called with the msg lock (monitor?) held, it should assert so.

Is `mInputDeviceUsers` guarded by the monitor? Doesn't look like it to me.

Would it be simpler if this was dispatched to the graph thread first? Other uses of `mInputDeviceUsers` seem to be on the graph thread.

This seems like a serious threading issue.
Attachment #8972015 - Flags: review?(apehrson) → review-
Comment on attachment 8972016 [details]
Bug 1404977 - Part 9 - Propagate the changes to the GraphDrivers, simplifying them, and brokering all access through the MSG.

https://reviewboard.mozilla.org/r/240762/#review254376

r=me with the four outstanding open issues fixed (1 here, 3 on Alex' version of this).
Attachment #8972016 - Flags: review?(apehrson) → review+
Comment on attachment 8968569 [details]
Bug 1404977 - Part 10 - Make logging uniform, and consistently use GraphImpl() to access the graph impl.

https://reviewboard.mozilla.org/r/237258/#review254380

r=me with nits fixed (including https://reviewboard.mozilla.org/r/245720/)

::: dom/media/GraphDriver.cpp:220
(Diff revision 3)
>  
>  void
>  ThreadedDriver::Start()
>  {
>    LOG(LogLevel::Debug,
> -      ("Starting thread for a SystemClockDriver  %p", mGraphImpl));
> +      ("%p: Starting thread for a SystemClockDriver %p", GraphImpl(), this)); 

trailing whitespace
Comment on attachment 8981425 [details]
Bug 1404977 - Part 11 - Make sure the default device is the first element in the list.

https://reviewboard.mozilla.org/r/247542/#review254382

Looks good, thanks. Just need clarification/fix on one minor issue.

::: dom/media/AudioDeviceInfo.cpp:107
(Diff revision 1)
>  uint32_t AudioDeviceInfo::State() const
>  {
>    return mState;
>  }
>  
> +bool AudioDeviceInfo::Preferred() const

Are there some docs on "preferred" for an audio device somewhere?

Can there only be at most one preferred device per system? If yes, could we assert so? If no, how do we handle more than one?
Attachment #8981425 - Flags: review?(apehrson) → review+
(In reply to Bryce Van Dyk (:bryce) from comment #111)
> I'm seeing issues with my automated tests here failing, am looking into them
> more, hold off on review.
> 
> I'm also seeing issues with some of the manual tests. Since the bug is quite
> long, here are the pens I'm using again: multiple gums in one page[0] and
> multiple iframe gums[1].
> 
> Env:
> - Linux Mint 18.3 64bit VM (VMWare)
> - Debug-opt Firefox built locally with clang
> - Laptop microphone fed through to the VM by VMWare is default device. A
> monitor of my speakers and a monitor of null (the automated test device) are
> also present.
> 
> Issue:
> > Assertion failure: aStream->GraphImpl()->IterationEnd() > mAllocations[i].mLastCallbackAppendTime, at /home/b/projects/mozilla/mozilla-central/dom/media/webrtc/MediaEngineWebRTCAudio.cpp:855
> 
> STR:
> - Load either codepen mentioned in this comment
> - Create a gum stream via UI with default settings
> - Apply AEC (or other setting) to the stream via the UI
> - Assert it hit
> OR
> - Load either codepen mentioned in this comment
> - Create a gum stream via UI with AEC (or other setting applied) settings
> - Assert it hit
> OR
> - Load either codepen mentioned in this comment
> - Create a gum stream via UI with default settings + 2 channels
> - Lower channel to 1 for that stream and apply via UI (possible intermittent)
> - Assert it hit
> 
> Issue:
> > Assertion failure: mState == kAllocated || mState == kStarted, at /home/b/projects/mozilla/mozilla-central/dom/media/webrtc/MediaEngineWebRTCAudio.cpp:644
> 
> STR:
> - Load either codepen mentioned in this comment
> - Create a gum stream via UI with default settings
> - Refresh the page
> - Assert is hit
> 
> Following these issues my browser is often left in a state where I cannot
> complete further gUM calls and need to kill it and zombie children and
> restart before I can test again. Log spam in this case include the following
> (least they're helpful).
> 
> Before killing main process:
> > [Child 7331, Chrome_ChildThread] WARNING: pipe error: Bad file descriptor: file /home/b/projects/mozilla/mozilla-central/ipc/chromium/src/chrome/common/ipc_channel_posix.cc, line 726
> 
> After killing main process:
> > [Child 7392, MediaStreamGrph] WARNING: No audio stream!: 'mDriver->mAudioStream || aOperation == INIT', file /home/b/projects/mozilla/mozilla-central/dom/media/GraphDriver.cpp, line 475
> 
> 
> [0]: https://codepen.io/SingingTree/pen/yjaRzv
> [1]: https://codepen.io/SingingTree/pen/BxWePB

Let's look into this more when the current issues (incl. thread races) of the patches have been sorted, IMHO.

If you want me to hold off the reviews you can unassign me in mozreview.
(In reply to Andreas Pehrson [:pehrsons] from comment #124)
> 
> If you want me to hold off the reviews you can unassign me in mozreview.

Having looked again, I think the tests are correct. Since they were passing and are now not, I don't know if targets have shifted, but have at them for review if you desire.
(In reply to Andreas Pehrson [:pehrsons] from comment #123)
> Comment on attachment 8981425 [details]
> Part 11 - Make sure the default device is the first element in the list.
> 
> https://reviewboard.mozilla.org/r/247542/#review254382
> 
> Looks good, thanks. Just need clarification/fix on one minor issue.
> 
> ::: dom/media/AudioDeviceInfo.cpp:107
> (Diff revision 1)
> >  uint32_t AudioDeviceInfo::State() const
> >  {
> >    return mState;
> >  }
> >  
> > +bool AudioDeviceInfo::Preferred() const
> 
> Are there some docs on "preferred" for an audio device somewhere? 

It's a word we use instead of "default" because on Windows, there are two kinds of "default devices":
- the default communication device
- the default device

and we don't honor this at the minute, we use the default device as the preferred devices.

> Can there only be at most one preferred device per system? If yes, could we
> assert so? If no, how do we handle more than one?

Yes, I'm going to add an assert for this.
I've added some patches that makes things better.

I don't want to store the requested channel count at multiple location (it's unnecessary, since it already resides on the MSG thread), so I've tried another approach, more in line with what we want to do in the longer term: split the massive MediaEngineWebRTC*Source into a processing object that resides on the MSG, and a facade object that is sends messages to the processing object.

It's not split yet, but at least the listener is only accessed on the MSG thread.
Comment on attachment 8968567 [details]
Bug 1404977 - Part 1 - Add missing lock to the PREF_CUBEB_BACKEND branch of the pref callback in CubebUtils.cpp.

https://reviewboard.mozilla.org/r/237254/#review256142

Still need to address previous comments.

Relevant backstory: https://reviewboard.mozilla.org/r/237254/#comment316180
It led to removal of usage of the pref, so either move this entire patch to wherever the usage ends up, or resolve the original concern in that thread.
Attachment #8968567 - Flags: review?(apehrson) → review-
Comment on attachment 8972012 [details]
Bug 1404977 - Part 5 - Allow querying the number of input channels from a WebRTCAudioDataListener.

https://reviewboard.mozilla.org/r/240754/#review256144

::: dom/media/webrtc/MediaEngineWebRTCAudio.cpp
(Diff revisions 2 - 3)
> - MutexAutoLock lock(mMutex);
> - if (mAudioSource) {
> +  if (mAudioSource) {
> -   return mAudioSource->InputChannelCount();
> +    return mAudioSource->InputChannelCount();

I don't see the original concern being addressed. Instead this now looks unsafe.
Attachment #8972012 - Flags: review?(apehrson) → review-
Comment on attachment 8972014 [details]
Bug 1404977 - Part 7 - Make each MediaEngineWebRTCMicrophoneSource independent.

https://reviewboard.mozilla.org/r/240758/#review256146

The thread safety concern is still open (r-). I added comments in a couple more places where it's a problem.
There are outstanding unfixed nits too.

::: dom/media/webrtc/MediaEngineWebRTCAudio.cpp:470
(Diff revisions 2 - 3)
>        LOG(("Audio device %s %s", NS_ConvertUTF16toUTF8(mDeviceInfo->Name()).get()
>                                 , mState == kStarted ? "started" : "stopped"));

Deceiving log as this is not where the state transition happens.

::: dom/media/webrtc/MediaEngineWebRTCAudio.cpp:503
(Diff revision 3)
> +bool
> +MediaEngineWebRTCMicrophoneSource::PassThrough() const
> +{
> +  MOZ_ASSERT(!mAllocations.IsEmpty() &&
> +             mAllocations[0].mStream &&
> +             mAllocations[0].mStream->GraphImpl()->CurrentDriver()->OnThread());

Please explain in a comment how this guarantees that
1) mAllocations is non-empty, and that
2) mAllocations[0] has the mStream member set

::: dom/media/webrtc/MediaEngineWebRTCAudio.cpp:514
(Diff revision 3)
> +  if (mAllocations.IsEmpty()) {
> +    return;
> +  }
> +  MOZ_ASSERT(!mAllocations.IsEmpty() &&
> +             mAllocations[0].mStream &&
> +             mAllocations[0].mStream->GraphImpl()->CurrentDriver()->OnThread());

Please explain in a comment how this guarantees that mAllocations[0] has the mStream member set

::: dom/media/webrtc/MediaEngineWebRTCAudio.cpp:523
(Diff revision 3)
> +uint32_t
> +MediaEngineWebRTCMicrophoneSource::GetRequestedInputChannelCount()
> +{
> +  MOZ_ASSERT(!mAllocations.IsEmpty() &&
> +             mAllocations[0].mStream &&
> +             mAllocations[0].mStream->GraphImpl()->CurrentDriver()->OnThread());

Please explain in a comment how this guarantees that
1) mAllocations is non-empty, and that
2) mAllocations[0] has the mStream member set
Attachment #8972014 - Flags: review?(apehrson) → review-
Comment on attachment 8972015 [details]
Bug 1404977 - Part 8 - Tell the MSG the MediaEngineAudioSource are now independent and that we can have multiple of them, cleanup the MSG-side API for managing them.

https://reviewboard.mozilla.org/r/240760/#review256148

Needs to address open issues!

::: dom/media/MediaStreamGraph.cpp:2755
(Diff revision 3)
> -SourceMediaStream::OpenAudioInput(int aID,
> +SourceMediaStream::OpenAudioInput(CubebUtils::AudioDeviceID aID,
>                                    AudioDataListener *aListener)

s/`AudioDataListener *`/`AudioDataListener*`/

::: dom/media/MediaStreamGraphImpl.h:386
(Diff revision 3)
> -  /**
> -   * No more data will be forthcoming for aStream. The stream will end
> -   * at the current buffer end point. The StreamTracks's tracks must be
> -   * explicitly set to finished by the caller.
> -   */
> -  void OpenAudioInputImpl(int aID,
> +  /* Runs off a message on the graph thread when something requests audio from
> +   * an input audio device of ID aID, and delivers the input audio frames to
> +   * aListener. */
> +  void OpenAudioInputImpl(CubebUtils::AudioDeviceID aID,
> +                          AudioDataListener* aListener);
> +  /* Called on the main thread when something requests audio from an input

Untrue. It allows bouncing to main thread when called off main thread.
Attachment #8972015 - Flags: review?(apehrson) → review-
Comment on attachment 8983490 [details]
Bug 1404977 - Part 12 - Make DeviceChanged() notification thread safe by using the MSG message queue.

https://reviewboard.mozilla.org/r/249340/#review256154

Looks good if you fix the off-main-thread case, but I'd really appreciate if this was incorporated into the patch where the DeviceChanged concerns were commented on. Make the reviewer's life easier.

::: dom/media/MediaStreamGraph.cpp:999
(Diff revision 1)
> +  // This is safe to be called from any thread: this message comes from an
> +  // underlying platform API, and we don't have much guarantees. If it is not
> +  // called from the main thread (and it probably will rarely be), it will post
> +  // itself to the main thread, and the actual device change message will be ran
> +  // and acted upon on the graph thread.
> +  if (!NS_IsMainThread()) {
> +    RefPtr<nsIRunnable> runnable =
> +      WrapRunnable(this,
> +                   &MediaStreamGraphImpl::DeviceChanged);
> +    mAbstractMainThread->Dispatch(runnable.forget());
> +  }

I guess we don't have tests for this because this will fail. If it's called off main-thread we dispatch to main-thread *and* try to AppendMessage() (which will fail an assert).
Attachment #8983490 - Flags: review?(apehrson) → review+
Comment on attachment 8983491 [details]
Bug 1404977 - Part 13 - Remove useless mutex, and assert why they were useless, in WebRTCAudioDataListeners.

https://reviewboard.mozilla.org/r/249342/#review256190

Sorry, this doesn't look safe.

::: dom/media/webrtc/MediaEngineWebRTCAudio.cpp
(Diff revision 1)
>  }
>  
>  void
>  WebRTCAudioDataListener::DeviceChanged()
>  {
> -  MutexAutoLock lock(mMutex);

Needs threading assert.

::: dom/media/webrtc/MediaEngineWebRTCAudio.cpp:89
(Diff revision 1)
>  WebRTCAudioDataListener::InputChannelCount()
>  {
>    if (mAudioSource) {
>      return mAudioSource->InputChannelCount();

Needs threading assert.

::: dom/media/webrtc/MediaEngineWebRTCAudio.cpp:98
(Diff revision 1)
>  WebRTCAudioDataListener::Shutdown()
>  {
> -  MutexAutoLock lock(mMutex);
>    mAudioSource = nullptr;

Needs threading assert.

Especially since it doesn't look at all safe...
https://searchfox.org/mozilla-central/rev/cf464eabfeba64e866c1fa36b9fefd674dca9c51/dom/media/webrtc/MediaEngineWebRTCAudio.cpp#1234

::: dom/media/webrtc/MediaEngineWebRTCAudio.cpp:1224
(Diff revision 1)
> +  MOZ_ASSERT(!mAllocations.IsEmpty() &&
> +             mAllocations[0].mStream &&
> +             mAllocations[0].mStream->GraphImpl()->CurrentDriver()->OnThread());

`!mAllocations.IsEmpty()` and `mAllocations[0].mStream` are not invariants that can generally be relied upon.

Please prove that they cannot fail for any call to `DeviceChanged()`.
Attachment #8983491 - Flags: review?(apehrson) → review-
Comment on attachment 8983492 [details]
Bug 1404977 - Part 14 - Consistently pass the MSG to the AudioDataListener methods to be able to write thread asserts.

https://reviewboard.mozilla.org/r/249344/#review256194

This is fine in itself, but please merge it into part 13 so I don't have to strike down on 13 for removal of safety that gets re-added here.

Also note that 13 is still unsafe (`Shutdown()`).
Attachment #8983492 - Flags: review?(apehrson) → review+
Comment on attachment 8983493 [details]
Bug 1404977 - Part 15 - Disconnect the MediaManager and MediaStreamGraph part of MediaEngineWebRTCMicrophoneSource on the MSG thread.

https://reviewboard.mozilla.org/r/249346/#review256200

This looks OK, and it fixes most concerns of part 13. But I think it'd be easier for everyone if this was merged into part 13 as well.

r=me with nits fixed

::: dom/media/webrtc/MediaEngineWebRTC.h:386
(Diff revision 1)
> -  // Owning thread only.
> +  // mListener is created on the MediaManager thread, and then sent to the MSG
> +  // thread. On shutdown, we send this pointer to the MSG thread again, telling
> +  // it to clean up.
>    RefPtr<WebRTCAudioDataListener> mListener;

I agree with the first sentence, but not with the second.

It seems like we clear this on MediaManager thread first, and then let the graph clear its reference on the graph thread later.

At least it looks safe. But please make the comment accurate.

::: dom/media/webrtc/MediaEngineWebRTCAudio.cpp:505
(Diff revision 1)
> -  MOZ_ASSERT(!mAllocations.IsEmpty() &&
> +  MOZ_ASSERT(mAllocations.IsEmpty() ||
> +             (!mAllocations.IsEmpty() &&
>               mAllocations[0].mStream &&
> -             mAllocations[0].mStream->GraphImpl()->CurrentDriver()->OnThread());
> +             mAllocations[0].mStream->GraphImpl()->CurrentDriver()->OnThread()));
>    return mSkipProcessing;

Can I assume you ran into an issue where this was called and there was no allocations, or the first allocation had a null stream?

Well, this change doesn't make it safe.

Let's fix this where the check was added, in part 7. I guess what you need to do is change mAllocations to mAllocation since part 7 claims to make all mic sources independent. That makes it sound like it should now be illegal to Allocate() twice in a row without a Deallocate() in between.

If you do this, we don't have any shared sources any longer, and a bunch of things can be simplified. But let's do that in another bug (please file followup).
Attachment #8983493 - Flags: review?(apehrson) → review+
Comment on attachment 8972014 [details]
Bug 1404977 - Part 7 - Make each MediaEngineWebRTCMicrophoneSource independent.

https://reviewboard.mozilla.org/r/240758/#review256146

> Please explain in a comment how this guarantees that
> 1) mAllocations is non-empty, and that
> 2) mAllocations[0] has the mStream member set

I made it clearer by consolidating the early return that was present in functions downstream and explainning why there.
Marking this as a feature currently planned for 63, since we discussed it at the Nightly deep dive meeting today.
status-firefox63: --- → affected
Keywords: feature
Attachment #8981436 - Attachment is obsolete: true
Attachment #8981436 - Flags: review?(apehrson)
Attachment #8981437 - Attachment is obsolete: true
Attachment #8981437 - Flags: review?(apehrson)
Attachment #8983492 - Attachment is obsolete: true
Attachment #8983493 - Attachment is obsolete: true
Comment on attachment 8972014 [details]
Bug 1404977 - Part 7 - Make each MediaEngineWebRTCMicrophoneSource independent.

https://reviewboard.mozilla.org/r/240758/#review264664


Code analysis found 1 defect in this patch:
 - 1 defect found by clang-tidy

You can run this analysis locally with:
 - `./mach static-analysis check path/to/file.cpp` (C/C++)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: dom/media/webrtc/MediaEngineWebRTCAudio.cpp:123
(Diff revision 4)
> -    int aIndex,
> -    const char* aDeviceName,
> -    const char* aDeviceUUID,
> +    const nsString& aDeviceName,
> +    const nsCString& aDeviceUUID,
> +    uint32_t maxChannelCount,
>      bool aDelayAgnostic,
>      bool aExtendedFilter)
> -  : mAudioInput(aAudioInput)
> +  : mDeviceInfo(aInfo)

Warning: Parameter 'aInfo' is passed by value and only copied once; consider moving it to avoid unnecessary copies [clang-tidy: performance-unnecessary-value-param]

  : mDeviceInfo(aInfo)
                ^~~~~~
                std::move()
Comment on attachment 8983491 [details]
Bug 1404977 - Part 13 - Remove useless mutex, and assert why they were useless, in WebRTCAudioDataListeners.

https://reviewboard.mozilla.org/r/249342/#review264670


Code analysis found 1 defect in this patch:
 - 1 defect found by clang-tidy

You can run this analysis locally with:
 - `./mach static-analysis check path/to/file.cpp` (C/C++)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: dom/media/webrtc/MediaEngineWebRTCAudio.cpp:127
(Diff revision 2)
>      const nsString& aDeviceName,
>      const nsCString& aDeviceUUID,
>      uint32_t maxChannelCount,
>      bool aDelayAgnostic,
>      bool aExtendedFilter)
>    : mDeviceInfo(aInfo)

Warning: Parameter 'aInfo' is passed by value and only copied once; consider moving it to avoid unnecessary copies [clang-tidy: performance-unnecessary-value-param]

  : mDeviceInfo(aInfo)
                ^~~~~~
                std::move()
Comment on attachment 8992918 [details]
Bug 1404977 - Part 16 - Unit test CubebDeviceEnumerator.

https://reviewboard.mozilla.org/r/257748/#review264672


Code analysis found 1 defect in this patch:
 - 1 defect found by clang-tidy

You can run this analysis locally with:
 - `./mach static-analysis check path/to/file.cpp` (C/C++)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: dom/media/webrtc/MediaEngineWebRTCAudio.cpp:127
(Diff revision 1)
>      const nsString& aDeviceName,
>      const nsCString& aDeviceUUID,
>      uint32_t maxChannelCount,
>      bool aDelayAgnostic,
>      bool aExtendedFilter)
>    : mDeviceInfo(aInfo)

Warning: Parameter 'aInfo' is passed by value and only copied once; consider moving it to avoid unnecessary copies [clang-tidy: performance-unnecessary-value-param]

  : mDeviceInfo(aInfo)
                ^~~~~~
                std::move()
Comment on attachment 8992919 [details]
Bug 1404977 - Part 17 - Re-implement the workaround for the lack of input device enumeration on Android.

https://reviewboard.mozilla.org/r/257750/#review264674


Code analysis found 1 defect in this patch:
 - 1 defect found by clang-tidy

You can run this analysis locally with:
 - `./mach static-analysis check path/to/file.cpp` (C/C++)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: dom/media/webrtc/MediaEngineWebRTCAudio.cpp:127
(Diff revision 1)
>      const nsString& aDeviceName,
>      const nsCString& aDeviceUUID,
>      uint32_t maxChannelCount,
>      bool aDelayAgnostic,
>      bool aExtendedFilter)
>    : mDeviceInfo(aInfo)

Warning: Parameter 'aInfo' is passed by value and only copied once; consider moving it to avoid unnecessary copies [clang-tidy: performance-unnecessary-value-param]

  : mDeviceInfo(aInfo)
                ^~~~~~
                std::move()
Comment on attachment 8968567 [details]
Bug 1404977 - Part 1 - Add missing lock to the PREF_CUBEB_BACKEND branch of the pref callback in CubebUtils.cpp.

https://reviewboard.mozilla.org/r/237254/#review264970

Change looks good, but there seems to be issues in surrounding code that would be appropriate to fix here too. Either by following the docs or updating the docs to reflect reality, including a reason why the lock is not needed for those static members.

::: dom/media/CubebUtils.cpp:204
(Diff revision 5)
>      }
>    } else if (strcmp(aPref, PREF_CUBEB_LATENCY_PLAYBACK) == 0) {
>      // Arbitrary default stream latency of 100ms.  The higher this
>      // value, the longer stream volume changes will take to become
>      // audible.
>      sCubebPlaybackLatencyPrefSet = Preferences::HasUserValue(aPref);

This is unguarded but docs say it should be guarded.

::: dom/media/CubebUtils.cpp:209
(Diff revision 5)
>      sCubebPlaybackLatencyPrefSet = Preferences::HasUserValue(aPref);
>      uint32_t value = Preferences::GetUint(aPref, CUBEB_NORMAL_LATENCY_MS);
>      StaticMutexAutoLock lock(sMutex);
>      sCubebPlaybackLatencyInMilliseconds = std::min<uint32_t>(std::max<uint32_t>(value, 1), 1000);
>    } else if (strcmp(aPref, PREF_CUBEB_LATENCY_MSG) == 0) {
>      sCubebMSGLatencyPrefSet = Preferences::HasUserValue(aPref);

This is unguarded but docs say it should be guarded.
Attachment #8968567 - Flags: review?(apehrson) → review+
Comment on attachment 8972012 [details]
Bug 1404977 - Part 5 - Allow querying the number of input channels from a WebRTCAudioDataListener.

https://reviewboard.mozilla.org/r/240754/#review265002

My last comment still stands.
Attachment #8972012 - Flags: review?(apehrson) → review-
Comment on attachment 8972014 [details]
Bug 1404977 - Part 7 - Make each MediaEngineWebRTCMicrophoneSource independent.

https://reviewboard.mozilla.org/r/240758/#review264976

Mostly nits, but r- for unsafe mAllocations access.

And please address part 5 either by changes or discussion before having me review this again, thanks.

::: dom/media/webrtc/MediaEngineWebRTC.h:210
(Diff revision 4)
>    bool RequiresSharing() const override
>    {
>      return true;
>    }

If these are to be created independently by MediaEngineWebRTC, this needs to return false.

Ok, for mics we don't check this since part 3, but it would be confusing to still return true here.

::: dom/media/webrtc/MediaEngineWebRTC.h:386
(Diff revision 4)
> +  uint32_t GetRequestedInputChannelCount();
> +  void SetRequestedInputChannelCount(uint32_t aRequestedInputChannelCount);
>  
>    // Owning thread only.
>    RefPtr<WebRTCAudioDataListener> mListener;
> +  const RefPtr<AudioDeviceInfo> mDeviceInfo;

Is this also owning thread only? Since it's const and immutable it could be broader.

::: dom/media/webrtc/MediaEngineWebRTC.h:417
(Diff revision 4)
> +  // The number of channels asked for by content, after clamping to the range of
> +  // legal channel count for this particular device. This is the number of
> +  // channels of the input buffer received.
> +  uint32_t mRequestedInputChannelCount;

I'm a bit confused by the last sentence. "received"?

::: dom/media/webrtc/MediaEngineWebRTCAudio.cpp:508
(Diff revision 4)
> +  if (mAllocations.IsEmpty()) {
> +    return;
> +  }

When can this happen, and why can't it happen in the getter?

::: dom/media/webrtc/MediaEngineWebRTCAudio.cpp:508
(Diff revision 4)
> +  return mSkipProcessing;
> +}
> +void
> +MediaEngineWebRTCMicrophoneSource::SetPassThrough(bool aPassThrough)
> +{
> +  if (mAllocations.IsEmpty()) {

Unsafe access, see the docs for mAllocations.

::: dom/media/webrtc/MediaEngineWebRTCAudio.cpp:523
(Diff revision 4)
> +}
> +
> +uint32_t
> +MediaEngineWebRTCMicrophoneSource::GetRequestedInputChannelCount()
> +{
> +

Superfluous newline.

::: dom/media/webrtc/MediaEngineWebRTCAudio.cpp:524
(Diff revision 4)
> +  // This source has been released, and is waiting for collection. Simply return
> +  // 0, this source won't contribute to the channel count decision.

Put the comment inside the if block.

::: dom/media/webrtc/MediaEngineWebRTCAudio.cpp:530
(Diff revision 4)
> +  // Else, we have a source that is alive. mStream is always valid because it's
> +  // set right before ::Start is called.  SetPassThrough cannot be called before

It's set in SetTrack(), but what prevents this from being called between Allocate() and SetTrack()?

That's the info I want to see in the comment.

This is a public method so I don't see any such guarantees. If this is an intermediate state I could be ok with relying on the caller playing nice here, but please make this clear in the comment.

This goes for the other asserts like this too.

::: dom/media/webrtc/MediaEngineWebRTCAudio.cpp:531
(Diff revision 4)
> +  if (mState == kReleased) {
> +    return 0;
> +  }
> +
> +  // Else, we have a source that is alive. mStream is always valid because it's
> +  // set right before ::Start is called.  SetPassThrough cannot be called before

"SetPassThrough", relevance?

::: dom/media/webrtc/MediaEngineWebRTCAudio.cpp:533
(Diff revision 4)
> +  }
> +
> +  // Else, we have a source that is alive. mStream is always valid because it's
> +  // set right before ::Start is called.  SetPassThrough cannot be called before
> +  // that, because it's running on the graph thread, and this cannot happen
> +  // before it the source has been started.

s/it //

::: dom/media/webrtc/MediaEngineWebRTCAudio.cpp:534
(Diff revision 4)
> +
> +  // Else, we have a source that is alive. mStream is always valid because it's
> +  // set right before ::Start is called.  SetPassThrough cannot be called before
> +  // that, because it's running on the graph thread, and this cannot happen
> +  // before it the source has been started.
> +  MOZ_ASSERT(!mAllocations.IsEmpty() &&

These can now be changed to check that the length of mAllocations is always 1 (instead of non-empty), correct?

Because if not, mAllocations[0] could be another non-started allocation that doesn't have mStream set.

::: dom/media/webrtc/MediaEngineWebRTCAudio.cpp:544
(Diff revision 4)
> +  if (mAllocations.IsEmpty()) {
> +      return;
> +  }

When can this happen, and why can't it happen in the getter?

::: dom/media/webrtc/MediaEngineWebRTCAudio.cpp:544
(Diff revision 4)
> +
> +void
> +MediaEngineWebRTCMicrophoneSource::SetRequestedInputChannelCount(
> +  uint32_t aRequestedInputChannelCount)
> +{
> +  if (mAllocations.IsEmpty()) {

Unsafe access, see the docs on access to mAllocations.

::: dom/media/webrtc/MediaEngineWebRTCAudio.cpp:625
(Diff revision 4)
>      return rv;
>    }
>  
>    {
>      MutexAutoLock lock(mMutex);
>      mAllocations.AppendElement(Allocation(handle));

Can we assert here that mAllocations is empty?

::: dom/media/webrtc/MediaEngineWebRTCAudio.cpp:724
(Diff revision 4)
> +  // For now, we only allow opening a single audio input device per document,
> +  // because we can only have one MSG per document.

Put this inside the if block.
Attachment #8972014 - Flags: review?(apehrson) → review-
Comment on attachment 8972015 [details]
Bug 1404977 - Part 8 - Tell the MSG the MediaEngineAudioSource are now independent and that we can have multiple of them, cleanup the MSG-side API for managing them.

https://reviewboard.mozilla.org/r/240760/#review265008
Attachment #8972015 - Flags: review?(apehrson) → review-
Comment on attachment 8983491 [details]
Bug 1404977 - Part 13 - Remove useless mutex, and assert why they were useless, in WebRTCAudioDataListeners.

https://reviewboard.mozilla.org/r/249342/#review265014

::: dom/media/webrtc/MediaEngineWebRTCAudio.cpp:1255
(Diff revision 2)
> +  MOZ_ASSERT(!mAllocations.IsEmpty() &&
> +             mAllocations[0].mStream &&
> +             mAllocations[0].mStream->GraphImpl()->CurrentDriver()->OnThread());

Change this to use aGraph instead.
Attachment #8983491 - Flags: review?(apehrson) → review+
Comment on attachment 8992916 [details]
Bug 1404977 - Part 14 - Add a way to set the global cubeb* singleton at runtime, from a test.

https://reviewboard.mozilla.org/r/257744/#review265022

::: dom/media/CubebUtils.h:58
(Diff revision 1)
> +#ifdef ENABLE_SET_CUBEB_BACKEND
> +void
> +SetCubebContext(cubeb* aCubebContext);

Could we name this "force" or perhaps "override" instead of "set" to better show intent?
Attachment #8992916 - Flags: review?(apehrson) → review+
Comment on attachment 8992917 [details]
Bug 1404977 - Part 15 - Invalidate the device cache before re-enumerating devices when the cubeb backend does not support dynamic device collection invalidation.

https://reviewboard.mozilla.org/r/257746/#review265024
Attachment #8992917 - Flags: review?(apehrson) → review+
Comment on attachment 8992918 [details]
Bug 1404977 - Part 16 - Unit test CubebDeviceEnumerator.

https://reviewboard.mozilla.org/r/257748/#review265018

Looks good. Some of the manual labor would be nice to skip, but it looks good enough. I'm only giving r- because of the unrelated changes to MediaEngineWebRTCAudio.cpp. Please move them to the appropriate patch.

::: dom/media/gtest/TestAudioDeviceEnumerator.cpp:10
(Diff revision 1)
> +#define ENABLE_SET_CUBEB_BACKEND 1
> +#include "CubebUtils.h"

How can you be sure CubebUtils.h hasn't already been included by someone else?

Is it possible to define this in dom/media/gtest/moz.build instead?

::: dom/media/gtest/TestAudioDeviceEnumerator.cpp:18
(Diff revision 1)
> +
> +using namespace mozilla;
> +
> +const bool DEBUG_PRINTS = false;
> +
> +// Keep those and struct definition in sync with cubeb.h and cubeb-internal.h

look over "Keep those and struct definition"

::: dom/media/gtest/TestAudioDeviceEnumerator.cpp:37
(Diff revision 1)
> +  cubeb* context,
> +  cubeb_device_type devtype,
> +  cubeb_device_collection_changed_callback callback,
> +  void* user_ptr);
> +
> +struct cubeb_ops

I haven't verified this definition, but I assume that if it works we're good.

::: dom/media/gtest/TestAudioDeviceEnumerator.cpp:114
(Diff revision 1)
> +
> +// This class has two facets: it is both a fake cubeb backend that is intended
> +// to be used for testing, and passed to Gecko code that expects a normal
> +// backend, but is also controllable by the test code to decide what the backend
> +// should do, depending on what is being tested.
> +class MockCubeb

Did you consider gmock for mocking? I'm not sure whether it's feasible to use here but it can bring lots of convenience.

Like https://github.com/google/googletest/blob/master/googlemock/docs/ForDummies.md#actions-what-should-it-do

::: dom/media/gtest/TestAudioDeviceEnumerator.cpp:128
(Diff revision 1)
> +  {
> +  }
> +  // Cubeb backend implementation
> +  // This allows passing this class as a cubeb* instance.
> +  cubeb* AsCubebContext() { return reinterpret_cast<cubeb*>(this); }
> +  // Fill in the collection parameter with all the device of aType.

s/the device/devices/

::: dom/media/gtest/TestAudioDeviceEnumerator.cpp:144
(Diff revision 1)
> +    collection->device = new cubeb_device_info[count];
> +    collection->count = count;
> +
> +    uint32_t collection_index = 0;
> +    if (aType & CUBEB_DEVICE_TYPE_INPUT) {
> +      for (uint32_t i = 0; i < mInputDevices.Length(); i++) {

This could be a range-for over mInputDevices.

::: dom/media/gtest/TestAudioDeviceEnumerator.cpp:196
(Diff revision 1)
> +      MOZ_CRASH("bad device type when adding a device in mock cubeb backend");
> +    }
> +
> +    bool isInput = aDevice.type & CUBEB_DEVICE_TYPE_INPUT;
> +
> +    needToCall |=

This is a bitwise OR assignment, is that what you intended?

Likewise two lines below.

::: dom/media/gtest/TestAudioDeviceEnumerator.cpp:282
(Diff revision 1)
> +  {
> +    mSupportsDeviceCollectionChangedCallback = aSupports;
> +  }
> +
> +private:
> +  // This needs to have the exacte same memory layout as a real cubeb backed.

s/exacte/exact/

::: dom/media/gtest/TestAudioDeviceEnumerator.cpp:284
(Diff revision 1)
> +  }
> +
> +private:
> +  // This needs to have the exacte same memory layout as a real cubeb backed.
> +  // It's very important for this `ops` member to be the very first member of
> +  // the class, and to have not any virtual members (to avoid having a vtable).

s/have not/not have/

::: dom/media/gtest/TestAudioDeviceEnumerator.cpp:292
(Diff revision 1)
> +  cubeb_device_collection_changed_callback mDeviceCollectionChangeCallback;
> +  // For which device type to call the callback.
> +  cubeb_device_type mDeviceCollectionChangeType;
> +  // The pointer to pass in the callback.
> +  void* mDeviceCollectionChangeUserPtr;
> +  // Whether or not this backed supports device collection change notification

s/backed/backend/

::: dom/media/gtest/TestAudioDeviceEnumerator.cpp:442
(Diff revision 1)
> +DeviceTemplate(cubeb_devid aId)
> +{
> +  // A fake input device

Can we put "input" somewhere in the name?

::: dom/media/gtest/TestAudioDeviceEnumerator.cpp:534
(Diff revision 1)
> +  DeviceOperation operations[2] = { DeviceOperation::ADD,
> +                                    DeviceOperation::REMOVE };
> +
> +  for (DeviceOperation op : operations) {
> +    for (bool supports : supportsDeviceChangeCallback) {
> +      mock->ClearDevices(CUBEB_DEVICE_TYPE_INPUT);

You could use a test fixture to keep state and `void TearDown() override` to handle cleanup between tests.

https://github.com/google/googletest/blob/master/googletest/docs/primer.md#test-fixtures-using-the-same-data-configuration-for-multiple-tests

::: dom/media/gtest/moz.build:41
(Diff revision 1)
>      'TestMP4Demuxer.cpp',
>      'TestRust.cpp',
>      'TestVideoSegment.cpp',
>      'TestVideoUtils.cpp',
>      'TestVPXDecoding.cpp',
> -    'TestWebMBuffered.cpp',
> +    'TestWebMBuffered.cpp'

That comma was useful to avoid extra lines marked as changed when diffing.

::: dom/media/webrtc/MediaEngineWebRTCAudio.cpp:516
(Diff revision 1)
>  {
>    if (mAllocations.IsEmpty()) {
>      return;
>    }
> +
> +  MOZ_ASSERT(mAllocations.Length() <= 1);

This and all other changes to this file belong in another patch.
Attachment #8992918 - Flags: review?(apehrson) → review-
Comment on attachment 8992919 [details]
Bug 1404977 - Part 17 - Re-implement the workaround for the lack of input device enumeration on Android.

https://reviewboard.mozilla.org/r/257750/#review265056

r=me with nits addressed.

::: dom/media/gtest/TestAudioDeviceEnumerator.cpp:133
(Diff revision 1)
>    // Fill in the collection parameter with all the device of aType.
>    int EnumerateDevices(cubeb_device_type aType,
>                         cubeb_device_collection* collection)
>    {
> +#ifdef ANDROID
> +    EXPECT_TRUE(false) << "This is not to be called on Android.";

You could use FAIL() (or GTEST_FAIL() if it's not available) here. Or at least an `ASSERT_` instead of `EXPECT_`.

::: dom/media/webrtc/MediaEngineWebRTC.cpp:190
(Diff revision 1)
> +#ifndef ANDROID
>      MOZ_ASSERT(devices[i]->DeviceID());
> +#endif

Could we give the dummy device on android a dummy device id too, to avoid this?

::: dom/media/webrtc/MediaEngineWebRTC.cpp:353
(Diff revision 1)
>  {
> +  aOutDevices.Clear();
> +
> +#ifdef ANDROID
> +  // Bug 1473346: enumerating devices is not supported on Android in cubeb,
> +  // simply state there is a single mic, that is the default, and has a single

s/state there/state that there/
s/that is/that it is/

::: dom/media/webrtc/MediaEngineWebRTC.cpp:368
(Diff revision 1)
> +                                                     128,
> +                                                     410);

max latency 128 and min latency 410?

::: dom/media/webrtc/MediaEngineWebRTC.cpp:374
(Diff revision 1)
> +                                                     410);
> +  if (mDevices.IsEmpty()) {
> +    mDevices.AppendElement(info);
> +  }
> +  aOutDevices.AppendElements(mDevices);
> +  return;

no need for this return, or if you want to keep it, please add one to the other branch too for consistency.

::: dom/media/webrtc/MediaEngineWebRTCAudio.cpp:148
(Diff revision 1)
> +#ifndef ANDROID
>    MOZ_ASSERT(mDeviceInfo->DeviceID());
> +#endif

Not needed if we give the android dummy device a dummy id.
Attachment #8992919 - Flags: review?(apehrson) → review+
Comment on attachment 8992918 [details]
Bug 1404977 - Part 16 - Unit test CubebDeviceEnumerator.

https://reviewboard.mozilla.org/r/257748/#review265718
Comment on attachment 8972012 [details]
Bug 1404977 - Part 5 - Allow querying the number of input channels from a WebRTCAudioDataListener.

https://reviewboard.mozilla.org/r/240754/#review265002

I've added a lock. It's useless though, it's being removed later in the series.
Comment on attachment 8972014 [details]
Bug 1404977 - Part 7 - Make each MediaEngineWebRTCMicrophoneSource independent.

https://reviewboard.mozilla.org/r/240758/#review264976

Yes, done.

> Is this also owning thread only? Since it's const and immutable it could be broader.

Not really. This comment has been clarified later, and I've clarified this here.

> When can this happen, and why can't it happen in the getter?

On shutdown, it happens because we're still using a mix of message queue and mutex, because we haven't done the switch yet. It can happen in the getter, but it does not matter whether or not we're returning true or false there, so I'm not doing anything.

> It's set in SetTrack(), but what prevents this from being called between Allocate() and SetTrack()?
> 
> That's the info I want to see in the comment.
> 
> This is a public method so I don't see any such guarantees. If this is an intermediate state I could be ok with relying on the caller playing nice here, but please make this clear in the comment.
> 
> This goes for the other asserts like this too.

Of course it's an intermediate state, I'm locking down everything with asserts. It's technically public but it's clearly not usable in any other way than it is now, and having anybody but myself in my next set of patches modifying this would be catastrophic. I've made it clearer with a few comments and assert strings.

> "SetPassThrough", relevance?

Copy paste error, fixed.

> These can now be changed to check that the length of mAllocations is always 1 (instead of non-empty), correct?
> 
> Because if not, mAllocations[0] could be another non-started allocation that doesn't have mStream set.

No. I've made it clearer so that you understand (hopefully... I reckon the situation is less than optimal).

> When can this happen, and why can't it happen in the getter?

We're returning 0 already when this is the case.
Comment on attachment 8972014 [details]
Bug 1404977 - Part 7 - Make each MediaEngineWebRTCMicrophoneSource independent.

https://reviewboard.mozilla.org/r/240758/#review246930

> The lookup is no longer done but the member is now mutable.

This is now const.

> If this exits early, we won't get the log just below, which might be confusing when debugging.

Added a log statement here.

> MediaEngineWebRTCAudio shouldn't have to rely on its users to be nice so things are available.
> 
> Also, I don't see the same guarantee as you.
> Counter example:
> - The call to SetUserInputChannelCount() is in part 7.
> - It is called on the graph thread after a dispatch from the main thread after a dispatch from the media manager thread in ApplySettings, which is called from Reconfigure or Start.
> - Either Reconfigure() or Start() may be followed by Stop() and Deallocate() on the same allocation. If this was the only allocation, mAllocations is going to be empty.
> 
> Main thread in particular can take a long long time to run runnables. The graph thread dispatch further relies on main thread stable state so again there's a chance of a long wait.
> 
> It would be fairly easy to trigger this by `track.applySettings(blah).then(() => track.stop())` while keeping main thread relatively busy.
> 
> Sorry but this is happy path coding not acceptable for landing.

Right, added an early return for this, in the same fashion as the other methods. This is always called on the MSG thread, and it is possible that there are no allocations anymore here. This will be fixed later, it's rather clumsy but unnavoidable because we're mixing shared memory (to add/remove allocations) and message passing (for this call, for example).

> In part 5 I discuss this in a bit more detail. "The graph's record" is something we'd have to create in order to not call back into MediaEngineWebRTCMicrophoneSource synchronously. Again part 5 mentions where to do this.

I've decided to take a cleaner approach than to duplicate state, dropping this.

> You caught one but not the other.

Fixed the second one.
(In reply to Paul Adenot (:padenot) from comment #190)
> Comment on attachment 8972014 [details]
> Bug 1404977 - Part 7 - Make each MediaEngineWebRTCMicrophoneSource
> independent.
> 
> https://reviewboard.mozilla.org/r/240758/#review246930
> 
> > The lookup is no longer done but the member is now mutable.
> 
> This is now const.
> 
> > If this exits early, we won't get the log just below, which might be confusing when debugging.
> 
> Added a log statement here.
> 
> > MediaEngineWebRTCAudio shouldn't have to rely on its users to be nice so things are available.
> > 
> > Also, I don't see the same guarantee as you.
> > Counter example:
> > - The call to SetUserInputChannelCount() is in part 7.
> > - It is called on the graph thread after a dispatch from the main thread after a dispatch from the media manager thread in ApplySettings, which is called from Reconfigure or Start.
> > - Either Reconfigure() or Start() may be followed by Stop() and Deallocate() on the same allocation. If this was the only allocation, mAllocations is going to be empty.
> > 
> > Main thread in particular can take a long long time to run runnables. The graph thread dispatch further relies on main thread stable state so again there's a chance of a long wait.
> > 
> > It would be fairly easy to trigger this by `track.applySettings(blah).then(() => track.stop())` while keeping main thread relatively busy.
> > 
> > Sorry but this is happy path coding not acceptable for landing.
> 
> Right, added an early return for this, in the same fashion as the other
> methods. This is always called on the MSG thread, and it is possible that
> there are no allocations anymore here. This will be fixed later, it's rather
> clumsy but unnavoidable because we're mixing shared memory (to add/remove
> allocations) and message passing (for this call, for example).
> 
> > In part 5 I discuss this in a bit more detail. "The graph's record" is something we'd have to create in order to not call back into MediaEngineWebRTCMicrophoneSource synchronously. Again part 5 mentions where to do this.
> 
> I've decided to take a cleaner approach than to duplicate state, dropping
> this.
> 
> > You caught one but not the other.
> 
> Fixed the second one.

(This is something that should have been published much earlier, and got lost in reviewboard).
Comment on attachment 8992919 [details]
Bug 1404977 - Part 17 - Re-implement the workaround for the lack of input device enumeration on Android.

https://reviewboard.mozilla.org/r/257750/#review265056

> You could use FAIL() (or GTEST_FAIL() if it's not available) here. Or at least an `ASSERT_` instead of `EXPECT_`.

This is on purpose: FAIL and ASSSERT variants return an int. What we care about is to make the test orange in case of failure here, and EXPECT works.

> Could we give the dummy device on android a dummy device id too, to avoid this?

This would require too much changes from the existing code. We'll be able to do this later.

> max latency 128 and min latency 410?

This is all in the wrong order, good catch.

> Not needed if we give the android dummy device a dummy id.

I'm not doing that here. Also I'd rather spend time implementing enumeration on android instead of piling more hacks on top of this :-).
Comment on attachment 8972012 [details]
Bug 1404977 - Part 5 - Allow querying the number of input channels from a WebRTCAudioDataListener.

https://reviewboard.mozilla.org/r/240754/#review265002

I don't see this lock. Did you add it to the wrong patch?
Attachment #8972011 - Attachment is obsolete: true
Comment on attachment 8972012 [details]
Bug 1404977 - Part 5 - Allow querying the number of input channels from a WebRTCAudioDataListener.

https://reviewboard.mozilla.org/r/240754/#review267234
Attachment #8972012 - Flags: review?(apehrson) → review+
(In reply to Andreas Pehrson [:pehrsons] from comment #204)
> Comment on attachment 8972012 [details]
> Bug 1404977 - Part 5 - Allow querying the number of input channels from a
> WebRTCAudioDataListener.
> 
> https://reviewboard.mozilla.org/r/240754/#review265002
> 
> I don't see this lock. Did you add it to the wrong patch?

I don't understand what happened, but I had it locally. I just re-pushed what I have and it appeared.
Comment on attachment 8972014 [details]
Bug 1404977 - Part 7 - Make each MediaEngineWebRTCMicrophoneSource independent.

https://reviewboard.mozilla.org/r/240758/#review264976

> No. I've made it clearer so that you understand (hopefully... I reckon the situation is less than optimal).

Please change this (and the other) asserts to check `mAllocations.Length() == 1` instead of `!mAllocations.IsEmpty()` so we can be sure the array is sane during this intermediate state.

Also, the array access in the assert is unsafe. This needs to be fixed before landing. Locking in debug-only would be sad for anyone running a debug build, but better than unsafe access.
Comment on attachment 8972014 [details]
Bug 1404977 - Part 7 - Make each MediaEngineWebRTCMicrophoneSource independent.

https://reviewboard.mozilla.org/r/240758/#review267226

r- for unsafe mState and mAllocations (debug-only) access.

::: dom/media/webrtc/MediaEngineWebRTCAudio.cpp:501
(Diff revision 7)
> +  MOZ_ASSERT(!mAllocations.IsEmpty() &&
> +             mAllocations[0].mStream &&
> +             mAllocations[0].mStream->GraphImpl()->CurrentDriver()->OnThread(),

This might be debug-only, but it's still unsafe to access mAllocations without lock off the owning thread.

::: dom/media/webrtc/MediaEngineWebRTCAudio.cpp:512
(Diff revision 7)
> +    // This can be the case, for now, because we're mixing mutable shared state
> +    // and linearization via message queue. This is temporary.

nit: Inside the if block please.

::: dom/media/webrtc/MediaEngineWebRTCAudio.cpp:521
(Diff revision 7)
> +  MOZ_ASSERT(!mAllocations.IsEmpty() &&
> +             mAllocations[0].mStream &&
> +             mAllocations[0].mStream->GraphImpl()->CurrentDriver()->OnThread(),

This might be debug-only, but it's still unsafe to access mAllocations without lock off the owning thread.

::: dom/media/webrtc/MediaEngineWebRTCAudio.cpp:532
(Diff revision 7)
> +
> +uint32_t
> +MediaEngineWebRTCMicrophoneSource::GetRequestedInputChannelCount()
> +{
> +
> +  if (mState == kReleased) {

Accessing mState without locking the mutex is unsafe on the graph thread.

::: dom/media/webrtc/MediaEngineWebRTCAudio.cpp:543
(Diff revision 7)
> +  MOZ_ASSERT(!mAllocations.IsEmpty() &&
> +             mAllocations[0].mStream &&
> +             mAllocations[0].mStream->GraphImpl()->CurrentDriver()->OnThread(),

Unsafe to access mAllocations without lock off the owning thread.

::: dom/media/webrtc/MediaEngineWebRTCAudio.cpp:1222
(Diff revision 7)
> +  // This can happen because mAllocations is not yet using message passing, and
> +  // is access both on the media manager thread and the MSG thread. This is to
> +  // be fixed soon.
> +  // When deallocating, the listener is removed via message passing, while the
> +  // allocation is removed immediately, so there can be a few iterations where
> +  // we need to return early here.

nit: Put this in the if block below.

Here it reads to me like "This method can be called because..."
Attachment #8972014 - Flags: review?(apehrson) → review-
Comment on attachment 8972015 [details]
Bug 1404977 - Part 8 - Tell the MSG the MediaEngineAudioSource are now independent and that we can have multiple of them, cleanup the MSG-side API for managing them.

https://reviewboard.mozilla.org/r/240760/#review246934

> First of all that threading information has to go in a comment on the method. Preferably in the method too since there's no meaningful assert we can put.
> 
> Buuuut, how is this thread safe then?
> I'm looking at the `nsDataHashtable` that is used both here and on the graph thread (NotifyInputData is the closest example).

Note to self: Part 12 makes this thread safe.
Comment on attachment 8983490 [details]
Bug 1404977 - Part 12 - Make DeviceChanged() notification thread safe by using the MSG message queue.

https://reviewboard.mozilla.org/r/249340/#review267304

::: dom/media/MediaStreamGraph.cpp:1013
(Diff revision 5)
> +    explicit Message(MediaStreamGraph* aGraph)
> +      : ControlMessage(nullptr) {}

mGraphImpl is not initiated.

::: dom/media/MediaStreamGraph.cpp:1019
(Diff revision 5)
> +    void RunDuringShutdown() override
> +    {
> +      // Don't bother doing anything here, we're shutting down
> +    }

nit: There's an empty default impl for RunDuringShutdown: https://searchfox.org/mozilla-central/rev/033d45ca70ff32acf04286244644d19308c359d5/dom/media/MediaStreamGraphImpl.h#77
Comment on attachment 8992918 [details]
Bug 1404977 - Part 16 - Unit test CubebDeviceEnumerator.

https://reviewboard.mozilla.org/r/257748/#review267492
Attachment #8992918 - Flags: review?(apehrson) → review+
Comment on attachment 8972015 [details]
Bug 1404977 - Part 8 - Tell the MSG the MediaEngineAudioSource are now independent and that we can have multiple of them, cleanup the MSG-side API for managing them.

https://reviewboard.mozilla.org/r/240760/#review267510

Only nits now, thanks!

::: dom/media/MediaStreamGraph.cpp:803
(Diff revision 8)
>  
>  void
> -MediaStreamGraphImpl::OpenAudioInputImpl(int aID,
> -                                         AudioDataListener *aListener)
> +MediaStreamGraphImpl::OpenAudioInputImpl(CubebUtils::AudioDeviceID aID,
> +                                         AudioDataListener* aListener)
>  {
> -  MOZ_ASSERT(OnGraphThread());
> +  MOZ_ASSERT(OnGraphThreadOrNotRunning());

This reverts the change from bug 1460346. Intended?

::: dom/media/MediaStreamGraph.cpp:2797
(Diff revision 8)
>    mInputListener = nullptr;
>  }
>  
>  void
>  SourceMediaStream::DestroyImpl()
> -{
> + {

Indentation

::: dom/media/MediaStreamGraphImpl.h:390
(Diff revision 8)
> -   */
> -  void OpenAudioInputImpl(int aID,
> -                          AudioDataListener *aListener);
> -  virtual nsresult OpenAudioInput(int aID,
> -                                  AudioDataListener *aListener) override;
> -  void CloseAudioInputImpl(AudioDataListener *aListener);
> +                          AudioDataListener* aListener);
> +  /* Called on the main thread when something requests audio from an input
> +   * audio device aID. */
> +  virtual nsresult OpenAudioInput(CubebUtils::AudioDeviceID aID,
> +                                  AudioDataListener* aListener) override;
> +  /* Runs off a message on the graph when a input audio from aID is not needed

s/a input audio/input audio/

::: dom/media/MediaStreamGraphImpl.h:413
(Diff revision 8)
> +  /* Called on the graph thread when there is new input data for listeners. This
> +   * is the raw audio input for this MediaStreamGraph. */
> +  void NotifyInputData(const AudioDataValue* aBuffer, size_t aFrames,
> +                       TrackRate aRate, uint32_t aChannels);
> +  /* Called every time there are changes to input/output audio devices like
> +   * plug/unplug etc.  Depending on the platform, this can be called on

s/etc.  /etc. /

::: dom/media/MediaStreamGraphImpl.h:414
(Diff revision 8)
> +   * is the raw audio input for this MediaStreamGraph. */
> +  void NotifyInputData(const AudioDataValue* aBuffer, size_t aFrames,
> +                       TrackRate aRate, uint32_t aChannels);
> +  /* Called every time there are changes to input/output audio devices like
> +   * plug/unplug etc.  Depending on the platform, this can be called on
> +   * different thread. This function is called with the MSG lock held. */

s/different thread./different threads./

::: dom/media/MediaStreamGraphImpl.h:454
(Diff revision 8)
> -  uint32_t AudioChannelCount() const
> +  uint32_t AudioOutputChannelCount() const
>    {
>      return mOutputChannels;

AudioInputChannelCount has a threading assert now, and it looks like this could use one too.

::: dom/media/MediaStreamGraphImpl.h:462
(Diff revision 8)
>    }
>  
> +  /**
> +   * The audio input channel count for a MediaStreamGraph is the max of all the
> +   * channel counts requested by the listeners. The max channel count is
> +   * delivered to the listener themselves, and they take care of downmixing.

s/listener themselves/listeners themselves/

::: dom/media/MediaStreamGraphImpl.h:484
(Diff revision 8)
> +    nsTArray<RefPtr<AudioDataListener>>* listeners =
> +      mInputDeviceUsers.GetValue(mInputDeviceID);
> +    MOZ_ASSERT(listeners);
> +    for (const auto& listener : *listeners) {
> +      maxInputChannels =
> +        std::max(maxInputChannels, listener->InputChannelCount());

I'd like to see the name `RequestedInputChannelCount` propagated all the way to here.

Alas, s/InputChannelCount/RequestedInputChannelCount/
Attachment #8972015 - Flags: review?(apehrson) → review+
Comment on attachment 8972014 [details]
Bug 1404977 - Part 7 - Make each MediaEngineWebRTCMicrophoneSource independent.

https://reviewboard.mozilla.org/r/240758/#review267516

I still see unsafe mAllocations access. r-

::: dom/media/webrtc/MediaEngineWebRTC.h:261
(Diff revisions 7 - 8)
>  
>    void DeviceChanged() override;
>  
>    uint32_t InputChannelCount() override
>    {
> -    return GetRequestedInputChannelCount();
> +    return GetRequestedInputChannelCount(aGraph);

No aGraph here.

::: dom/media/webrtc/MediaEngineWebRTC.h:406
(Diff revisions 7 - 8)
>    // the owning thread. Accessed under one of the two.
>    nsTArray<Allocation> mAllocations;
>  
> -  // Current state of the shared resource for this source.
> -  // Set under mMutex on the owning thread. Accessed under one of the two
> -  MediaEngineSourceState mState = kReleased;
> +  // Current state of the shared resource for this source. Written on the
> +  // owning thread, read on either the owning thread or the MSG thread.
> +  std::atomic<MediaEngineSourceState> mState;

s/std::atomic/Atomic/

::: dom/media/webrtc/MediaEngineWebRTCAudio.cpp:152
(Diff revisions 7 - 8)
>    mSettings->mAutoGainControl.Construct(0);
>    mSettings->mNoiseSuppression.Construct(0);
>    mSettings->mChannelCount.Construct(0);
> +
> +  mState = kReleased;
>    // We'll init lazily as needed

I guess this comment belongs with mSettings above?

::: dom/media/webrtc/MediaEngineWebRTC.h:204
(Diff revision 8)
> -                                    int aIndex,
> -                                    const char* name,
> -                                    const char* uuid,
> +                                    const nsString& name,
> +                                    const nsCString& uuid,
> +                                    uint32_t maxChannelCount,

Let's make these follow the style guide.

::: dom/media/webrtc/MediaEngineWebRTCAudio.cpp:503
(Diff revision 8)
> +  MOZ_ASSERT(mAllocations.Length() == 1&&
> +             mAllocations[0].mStream &&
> +             mAllocations[0].mStream->GraphImpl()->CurrentDriver()->OnThread(),

Unsafe mAllocations access.

::: dom/media/webrtc/MediaEngineWebRTCAudio.cpp:520
(Diff revision 8)
> +      // This can be the case, for now, because we're mixing mutable shared state
> +      // and linearization via message queue. This is temporary.
> +      return;
> +    }
> +
> +    MOZ_ASSERT(mAllocations.Length() <= 1);

If this was 0 we've already returned, and we check that it's 1 on the next line.

::: dom/media/webrtc/MediaEngineWebRTCAudio.cpp:535
(Diff revision 8)
> +
> +

Superfluous newlines.

::: dom/media/webrtc/MediaEngineWebRTCAudio.cpp:551
(Diff revision 8)
> +  if (mAllocations.IsEmpty()) {
> +      return;
> +  }
> +  MOZ_ASSERT(mAllocations.Length() == 1 &&
> +             mAllocations[0].mStream &&
> +             mAllocations[0].mStream->GraphImpl()->CurrentDriver()->OnThread(),
> +             "Wrong calling pattern, don't call this before ::SetTrack.");
> +  mRequestedInputChannelCount = aRequestedInputChannelCount;
> +  mAllocations[0].mStream->GraphImpl()->ReevaluateInputDevice();

Unsafe mAllocations access.

::: dom/media/webrtc/MediaEngineWebRTCAudio.cpp:831
(Diff revision 8)
>                                          const PrincipalHandle& aPrincipalHandle)
>  {
>    TRACE_AUDIO_CALLBACK_COMMENT("SourceMediaStream %p track %i",
>                                 aStream.get(), aTrackID);
>    StreamTime delta;
> +  LOG_FRAMES(("NotifyPull, desired = %" PRId64, (int64_t) aDesiredTime));

This is already logged below.
Attachment #8972014 - Flags: review?(apehrson) → review-
Comment on attachment 8972014 [details]
Bug 1404977 - Part 7 - Make each MediaEngineWebRTCMicrophoneSource independent.

https://reviewboard.mozilla.org/r/240758/#review267546
Attachment #8972014 - Flags: review?(apehrson) → review+
Comment on attachment 8996344 [details]
Bug 1404977 - Tests P1: Add test to ensure multiple, concurrent gUM calls in separate iframes succeed.

https://reviewboard.mozilla.org/r/260476/#review267568

Looks good. Nothing fundamentally wrong but enough minor issues that I'd like a second look at all the fixes.

::: dom/media/tests/mochitest/mochitest.ini:90
(Diff revision 1)
>  [test_getUserMedia_mediaStreamClone.html]
>  skip-if = android_version == '18' # android(Bug 1189784, timeouts on 4.3 emulator)
>  [test_getUserMedia_mediaStreamConstructors.html]
>  [test_getUserMedia_mediaStreamTrackClone.html]
>  skip-if = android_version == '18' # android(Bug 1189784, timeouts on 4.3 emulator)
> +[test_getUserMedia_multipleIframesDifferentConstraints.html]

We have similar tests in test_getUserMedia_audioConstraints.html. Continuing off this naming scheme would be useful IMHO.

Perhaps test_getUserMedia_audioConstraints_iframe.html? Or something about multiple or simultaneous?

::: dom/media/tests/mochitest/test_getUserMedia_multipleIframesDifferentConstraints.html:9
(Diff revision 1)
> +  createHTML({
> +    title: "getUserMedia in Multiple Iframes",
> +    bug: "1404977"
> +  });
> +  /**
> +   * Verify that we can successfully call getUserMedia in multiple iframes. Kinda hacky because of

Let's skip the `<script>` indentation when we have this much js code. There seems to be precendece in existing tests.

::: dom/media/tests/mochitest/test_getUserMedia_multipleIframesDifferentConstraints.html:10
(Diff revision 1)
> +</head>
> +<body>
> +<pre id="test">
> +<script type="application/javascript">
> +  createHTML({
> +    title: "getUserMedia in Multiple Iframes",

s/I/i/

::: dom/media/tests/mochitest/test_getUserMedia_multipleIframesDifferentConstraints.html:10
(Diff revision 1)
> +</head>
> +<body>
> +<pre id="test">
> +<script type="application/javascript">
> +  createHTML({
> +    title: "getUserMedia in Multiple Iframes",

Let's call it what it is, audio capture in multiple iframes with different constraint requests.

::: dom/media/tests/mochitest/test_getUserMedia_multipleIframesDifferentConstraints.html:14
(Diff revision 1)
> +   * Verify that we can successfully call getUserMedia in multiple iframes. Kinda hacky because of
> +   * how the test interacts with iframes: the iframes end up eating some of the harness output.
> +   * E.g. the gUM calls made in the iFrames have asserts from head.js which are swallowed.

Please mention how the test does this.

I'd like to know that it tests all audio constraint (echo/gain/noise) combinations in separate iframes at the same time without having to read code.

::: dom/media/tests/mochitest/test_getUserMedia_multipleIframesDifferentConstraints.html:16
(Diff revision 1)
> +    bug: "1404977"
> +  });
> +  /**
> +   * Verify that we can successfully call getUserMedia in multiple iframes. Kinda hacky because of
> +   * how the test interacts with iframes: the iframes end up eating some of the harness output.
> +   * E.g. the gUM calls made in the iFrames have asserts from head.js which are swallowed.

are the asserts swallowed, or is it just the output from them? does this impact the test result, the interpretation of the test result, or both?

::: dom/media/tests/mochitest/test_getUserMedia_multipleIframesDifferentConstraints.html:43
(Diff revision 1)
> +    }
> +
> +    // We need a real device to get a MediaEngine supporting constraints
> +    let audioDevice = SpecialPowers.getCharPref("media.audio_loopback_dev", "");
> +    if (!audioDevice) {
> +      ok(false, "No device set by framework. Try --use-test-media-devices");

This will fail when run with default configuration locally on linux. We have precedence for `todo()` [1] in this case which I think provides better semantics.


[1] https://searchfox.org/mozilla-central/rev/196560b95f191b48ff7cba7c2ba9237bba6b5b6a/dom/media/tests/mochitest/test_getUserMedia_basicAudio_loopback.html#21

::: dom/media/tests/mochitest/test_getUserMedia_multipleIframesDifferentConstraints.html:47
(Diff revision 1)
> +    if (!audioDevice) {
> +      ok(false, "No device set by framework. Try --use-test-media-devices");
> +      return;
> +    }
> +
> +    let requestedConstraints = [

sorry for bikeshedding your naming here, but since this is an array of constraints I think we should let the name reflect this.

How about "constraintCombinations"?

::: dom/media/tests/mochitest/test_getUserMedia_multipleIframesDifferentConstraints.html:48
(Diff revision 1)
> +      { echoCancellation: false, autoGainControl: false, noiseSuppression: false },
> +      { echoCancellation: true, autoGainControl: false, noiseSuppression: false },
> +      { echoCancellation: false, autoGainControl: true, noiseSuppression: false },
> +      { echoCancellation: false, autoGainControl: false, noiseSuppression: true },
> +      { echoCancellation: true, autoGainControl: true, noiseSuppression: false },
> +      { echoCancellation: true, autoGainControl: true, noiseSuppression: true },

What about {false, true, true} and {true, false, true} ?

::: dom/media/tests/mochitest/test_getUserMedia_multipleIframesDifferentConstraints.html:48
(Diff revision 1)
> +      { echoCancellation: false, autoGainControl: false, noiseSuppression: false },
> +      { echoCancellation: true, autoGainControl: false, noiseSuppression: false },
> +      { echoCancellation: false, autoGainControl: true, noiseSuppression: false },
> +      { echoCancellation: false, autoGainControl: false, noiseSuppression: true },
> +      { echoCancellation: true, autoGainControl: true, noiseSuppression: false },
> +      { echoCancellation: true, autoGainControl: true, noiseSuppression: true },

You might want to re-use `egn` from test_gUM_audioConstraints.html here for simpler overview of combinations, and simpler typo checking.

https://searchfox.org/mozilla-central/source/dom/media/tests/mochitest/test_getUserMedia_audioConstraints.html#65-69

::: dom/media/tests/mochitest/test_getUserMedia_multipleIframesDifferentConstraints.html:60
(Diff revision 1)
> +    let iframes = [];
> +    let iframeLoadedPromises = [];
> +    let gumStreams = [];
> +    let playMediaPromises = [];
> +
> +    for (let i = 0; i < requestedConstraints.length; i++) {

Since you're not using the `i`, let's make this a for...of, https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/for...of.

::: dom/media/tests/mochitest/test_getUserMedia_multipleIframesDifferentConstraints.html:64
(Diff revision 1)
> +      \<html\>
> +        \<script type="application/javascript" src="mediaStreamPlayback.js"\>
> +        \<\/script\>
> +      \<\/html\>`;

Are you sure you need all these escapes? My simple browser console test indicates no.

::: dom/media/tests/mochitest/test_getUserMedia_multipleIframesDifferentConstraints.html:71
(Diff revision 1)
> +        \<\/script\>
> +      \<\/html\>`;
> +      let loadPromise = new Promise((resolve, reject) => {
> +        iframe.onload = () => { resolve(); };
> +      });
> +      document.documentElement.appendChild(iframe);

This will make your document
```
<html>
...
<body>...</body>
<iframe></iframe>
<iframe></iframe>
<iframe></iframe>
</html>
```

Is that really what you want?

Appending to `document.body` seems more appropriate.

::: dom/media/tests/mochitest/test_getUserMedia_multipleIframesDifferentConstraints.html:73
(Diff revision 1)
> +      iframes.push(iframe);
> +      iframeLoadedPromises.push(loadPromise);

I'd consider creating js-objects with constraints, iframe and loadPromise as members instead. Seems like better semantics. This way you also avoid the cross-referencing (and possible bugs related to it, like if the arrays have different lengths) across the arrays when iterating over one of them.

Array.map is good for this, and/or destructuring assignments.

::: dom/media/tests/mochitest/test_getUserMedia_multipleIframesDifferentConstraints.html:76
(Diff revision 1)
> +    is(iframes.length,
> +      requestedConstraints.length,
> +      "Should have created an iframe for each constraint");

Then this wouldn't be necessary since you have created an iframe for each constraint already.

::: dom/media/tests/mochitest/test_getUserMedia_multipleIframesDifferentConstraints.html:85
(Diff revision 1)
> +      try {
> +        let stream = await iframes[i].contentWindow.getUserMedia({audio: requestedConstraints[i]});
> +        gumStreams.push(stream);
> +        let differenceString = getConstraintDifferenceString(
> +          requestedConstraints[i], stream.getAudioTracks()[0].getSettings());
> +        ok(!differenceString,
> +          `Stream at index ${i} should have the same constraints as were ` +
> +          `requested from gUM. Differences: ${differenceString}`);
> +      } catch (e) {
> +        ok(false, `getUserMedia calls should not fail! Failed in iframe#: ${i} with: ${e}!`);
> +      }

I'd probably use a .catch instead of await on the gUM call to re-wrap the error message, and then use await on the outer promise instead.

If the gUM then fails the whole test will fail without the use of try/catch.

::: dom/media/tests/mochitest/test_getUserMedia_multipleIframesDifferentConstraints.html:99
(Diff revision 1)
> +    // Once all streams are collected, make sure make sure the constraints
> +    // haven't been mutated by other gUM call
> +    for (let i = 0; i < gumStreams.length; i++) {
> +      let differenceString = getConstraintDifferenceString(
> +        requestedConstraints[i], gumStreams[i].getAudioTracks()[0].getSettings());
> +      ok(!differenceString,
> +        `Stream at index ${i} should not have had contraints altered after ` +
> +        `all gUM calls are done. Differences: ${differenceString}`);
> +    }

When we have tests for checking audio content for different constraint settings we should probably re-use those here.

Can you link to those bugs in a comment here?

Bug 1406372, bug 1406376 and bug 1406377.

::: dom/media/tests/mochitest/test_getUserMedia_multipleIframesDifferentConstraints.html:112
(Diff revision 1)
> +    }
> +
> +    for (let i = 0; i < gumStreams.length; i++) {
> +      let testAudio = createMediaElement("audio", `testAudio${i}`);
> +      let playback = new LocalMediaStreamPlayback(testAudio, gumStreams[i]);
> +      playMediaPromises.push(playback.playMedia(false));

I think we'd rather want to `playMediaWithoutStoppingTracks(false)` to ensure all elements are playing while all tracks are active (so not element A passes, stops track A, which triggers some audio flow in track B, making element B stop, starting track C, and so on...).

Then we'll need to manually also stop all tracks during final cleanup.
Attachment #8996344 - Flags: review?(apehrson) → review-
Comment on attachment 8996345 [details]
Bug 1404977 - Tests P2: Add test to ensure multiple, concurrent gUM calls in a single window succeed.

https://reviewboard.mozilla.org/r/260478/#review267574

Like P1 I'd like to see the review fixes before approving.

::: dom/media/tests/mochitest/mochitest.ini:92
(Diff revision 1)
>  [test_getUserMedia_mediaStreamConstructors.html]
>  [test_getUserMedia_mediaStreamTrackClone.html]
>  skip-if = android_version == '18' # android(Bug 1189784, timeouts on 4.3 emulator)
>  [test_getUserMedia_multipleIframesDifferentConstraints.html]
>  skip-if = os == 'mac' || os == 'win' || toolkit == 'android' # Bug 1404995, no loopback devices on some platforms
> +[test_getUserMedia_multipleStreamsDifferentConstraints.html]

Let's tag along on the name test_getUserMedia_audioConstraints.html but with something about multiple/simultaneous/streams to distinguish them.

::: dom/media/tests/mochitest/test_getUserMedia_multipleStreamsDifferentConstraints.html:9
(Diff revision 1)
> +  createHTML({
> +    title: "getUserMedia Multiple Times with Different Constraints",
> +    bug: "1404977"
> +  });
> +  /**
> +   * Verify that multiple gUM calls with different constaints can be performed

Let's remove the indentation from the script tag here.

::: dom/media/tests/mochitest/test_getUserMedia_multipleStreamsDifferentConstraints.html:10
(Diff revision 1)
> +</head>
> +<body>
> +<pre id="test">
> +<script type="application/javascript">
> +  createHTML({
> +    title: "getUserMedia Multiple Times with Different Constraints",

Let's add that this is audio-only and that the "multiple times" are also simultaneously running streams.

::: dom/media/tests/mochitest/test_getUserMedia_multipleStreamsDifferentConstraints.html:14
(Diff revision 1)
> +  createHTML({
> +    title: "getUserMedia Multiple Times with Different Constraints",
> +    bug: "1404977"
> +  });
> +  /**
> +   * Verify that multiple gUM calls with different constaints can be performed

No need to repeat from the createHTML title if you're not adding more information.

::: dom/media/tests/mochitest/test_getUserMedia_multipleStreamsDifferentConstraints.html:41
(Diff revision 1)
> +    }
> +
> +    // We need a real device to get a MediaEngine supporting constraints
> +    let audioDevice = SpecialPowers.getCharPref("media.audio_loopback_dev", "");
> +    if (!audioDevice) {
> +      ok(false, "No device set by framework. Try --use-test-media-devices");

s/ok/todo/

I'll let this issue be the last that repeats something from P1. Consider the issues on P1 here too where applicable.
Attachment #8996345 - Flags: review?(apehrson) → review-
Repeating from IRC here for visibility, posterity: I'm hitting `Assertion failure: aStream->GraphImpl()->IterationEnd() > mAllocations[i].mLastCallbackAppendTime, at /home/b/projects/mozilla/mozilla-central/dom/media/webrtc/MediaEngineWebRTCAudio.cpp:900` when I test with either codepen from comment #111, have any processing enabled, and am running a debug Linux build.

When I'd looked into this previously it appeared that `PacketizeAndProcess` was setting the last callback time[0] such that the `aStream->GraphImpl()->IterationEnd() > mAllocations[i].mLastCallbackAppendTime` check in the assert failed due to those values being equal.

[0]: https://searchfox.org/mozilla-central/rev/196560b95f191b48ff7cba7c2ba9237bba6b5b6a/dom/media/webrtc/MediaEngineWebRTCAudio.cpp#1056
Comment on attachment 8981425 [details]
Bug 1404977 - Part 11 - Make sure the default device is the first element in the list.

https://reviewboard.mozilla.org/r/247542/#review267708

::: dom/media/webrtc/MediaEngineWebRTC.cpp:219
(Diff revision 8)
> -          mDelayAgnostic,
> +            mDelayAgnostic,
> -          mExtendedFilter);
> +            mExtendedFilter);
> -      aDevices->AppendElement(source);
> +      RefPtr<MediaDevice> device = MakeRefPtr<MediaDevice>(
> +                                     source,
> +                                     source->GetName(),
> +                                     NS_ConvertUTF8toUTF16(source->GetUUID()));

I'm failing assertions when testing with linux loopback devices that this input is not UTF-8.
Comment on attachment 8981425 [details]
Bug 1404977 - Part 11 - Make sure the default device is the first element in the list.

https://reviewboard.mozilla.org/r/247542/#review267708

> I'm failing assertions when testing with linux loopback devices that this input is not UTF-8.

So the UUID in this case is NS_LossyConvertUTF16ToASCII of the name, which for a non-ascii char does something.. undefined? It's at least not UTF8.

So the easy fix might be to change that convert to NS_ConvertUTF16ToUTF8.

But seeing that all users of GetUUID() converts it back to UTF16, should we just store the UUID as UTF16 instead?

Either case needs a deeper look at what impact having a UUID string with non-ascii chars will have.
Attachment #8968567 - Attachment is obsolete: true
Attachment #8968568 - Attachment is obsolete: true
Attachment #8972010 - Attachment is obsolete: true
Attachment #8972012 - Attachment is obsolete: true
Attachment #8972013 - Attachment is obsolete: true
Attachment #8972014 - Attachment is obsolete: true
Attachment #8972015 - Attachment is obsolete: true
Attachment #8972016 - Attachment is obsolete: true
Attachment #8968569 - Attachment is obsolete: true
Attachment #8983490 - Attachment is obsolete: true
Attachment #8983491 - Attachment is obsolete: true
Attachment #8992916 - Attachment is obsolete: true
Attachment #8992917 - Attachment is obsolete: true
Attachment #8992918 - Attachment is obsolete: true
Attachment #8992919 - Attachment is obsolete: true
Comment on attachment 8996767 [details]
Bug 1404977 - Part 1 - Add missing lock to the PREF_CUBEB_BACKEND branch of the pref callback in CubebUtils.cpp.

https://reviewboard.mozilla.org/r/260808/#review267834

You cleared all the history on all the patches all right!
Attachment #8996767 - Flags: review?(apehrson) → review+
Comment on attachment 8996768 [details]
Bug 1404977 - Part 2 - Augment AudioDeviceInfo with a cubeb device id.

https://reviewboard.mozilla.org/r/260810/#review267836
Attachment #8996768 - Flags: review?(apehrson) → review+
Comment on attachment 8996769 [details]
Bug 1404977 - Part 3 - Remove global statics, introduce an audio device enumerator

https://reviewboard.mozilla.org/r/260812/#review267838
Attachment #8996769 - Flags: review?(apehrson) → review+
Comment on attachment 8996770 [details]
Bug 1404977 - Part 5 - Allow querying the number of input channels from a WebRTCAudioDataListener.

https://reviewboard.mozilla.org/r/260814/#review267840
Attachment #8996770 - Flags: review?(apehrson) → review+
Comment on attachment 8996772 [details]
Bug 1404977 - Part 7 - Make each MediaEngineWebRTCMicrophoneSource independent.

https://reviewboard.mozilla.org/r/260818/#review267844
Attachment #8996772 - Flags: review?(apehrson) → review+
Comment on attachment 8996771 [details]
Bug 1404977 - Part 6 - Remove unused include for lock-free FIFO.

https://reviewboard.mozilla.org/r/260816/#review267842
Attachment #8996771 - Flags: review?(apehrson) → review+
Comment on attachment 8996773 [details]
Bug 1404977 - Part 8 - Tell the MSG the MediaEngineAudioSource are now independent and that we can have multiple of them, cleanup the MSG-side API for managing them.

https://reviewboard.mozilla.org/r/260820/#review267846
Attachment #8996773 - Flags: review?(apehrson) → review+
Comment on attachment 8996774 [details]
Bug 1404977 - Part 9 - Propagate the changes to the GraphDrivers, simplifying them, and brokering all access through the MSG.

https://reviewboard.mozilla.org/r/260822/#review267848
Attachment #8996774 - Flags: review?(apehrson) → review+
Comment on attachment 8996775 [details]
Bug 1404977 - Part 10 - Make logging uniform, and consistently use GraphImpl() to access the graph impl.

https://reviewboard.mozilla.org/r/260824/#review267850
Attachment #8996775 - Flags: review?(apehrson) → review+
Comment on attachment 8996776 [details]
Bug 1404977 - Part 12 - Make DeviceChanged() notification thread safe by using the MSG message queue.

https://reviewboard.mozilla.org/r/260826/#review267854
Attachment #8996776 - Flags: review?(apehrson) → review+
Comment on attachment 8996777 [details]
Bug 1404977 - Part 13 - Remove useless mutex, and assert why they were useless, in WebRTCAudioDataListeners.

https://reviewboard.mozilla.org/r/260828/#review267852
Attachment #8996777 - Flags: review?(apehrson) → review+
Attachment #8996778 - Flags: review?(apehrson) → review+
Comment on attachment 8996778 [details]
Bug 1404977 - Part 14 - Add a way to set the global cubeb* singleton at runtime, from a test.

https://reviewboard.mozilla.org/r/260830/#review267856
Comment on attachment 8996779 [details]
Bug 1404977 - Part 15 - Invalidate the device cache before re-enumerating devices when the cubeb backend does not support dynamic device collection invalidation.

https://reviewboard.mozilla.org/r/260832/#review267858
Attachment #8996779 - Flags: review?(apehrson) → review+
Comment on attachment 8996780 [details]
Bug 1404977 - Part 16 - Unit test CubebDeviceEnumerator.

https://reviewboard.mozilla.org/r/260834/#review267860
Attachment #8996780 - Flags: review?(apehrson) → review+
Comment on attachment 8996781 [details]
Bug 1404977 - Part 17 - Re-implement the workaround for the lack of input device enumeration on Android.

https://reviewboard.mozilla.org/r/260836/#review267862
Attachment #8996781 - Flags: review?(apehrson) → review+
Comment on attachment 8996777 [details]
Bug 1404977 - Part 13 - Remove useless mutex, and assert why they were useless, in WebRTCAudioDataListeners.

https://reviewboard.mozilla.org/r/260828/#review267852
Depends on: 1480161
Depends on: 1480162
Depends on: 1471922
Comment on attachment 8996344 [details]
Bug 1404977 - Tests P1: Add test to ensure multiple, concurrent gUM calls in separate iframes succeed.

https://reviewboard.mozilla.org/r/260476/#review267568

> are the asserts swallowed, or is it just the output from them? does this impact the test result, the interpretation of the test result, or both?

My understanding is they are completely swallowed and will not be seen in, or impact the "normal" test harness in our document. Comment updated to reflect this.

> What about {false, true, true} and {true, false, true} ?

Done. Had originally intended for this to not be exhaustive as I don't think it adds value in the tests current incarnation. But given that it's only two cases off, and is more valuable if we add further verificaiton to processing later this makes sense.

> You might want to re-use `egn` from test_gUM_audioConstraints.html here for simpler overview of combinations, and simpler typo checking.
> 
> https://searchfox.org/mozilla-central/source/dom/media/tests/mochitest/test_getUserMedia_audioConstraints.html#65-69

Done. I prefer the explicit style, but prefer consistency across our tests more. Something to note, if we enable the linter on this dir it will let us use extra whitespace to align properties, but it currently doesn't like extra whitespace to align args as in the linked example.

> Since you're not using the `i`, let's make this a for...of, https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/for...of.

This gets fixed as part of using a testcase object to associate other objects. My original thinking: we currently don't lint this dir,  but the usage of for...of upsets the linter as we won't use the var. I opted for the current style to avoid issue if we turn on the linter (despite it being ugly in its own way).
Depends on: 1480348
Comment on attachment 8996344 [details]
Bug 1404977 - Tests P1: Add test to ensure multiple, concurrent gUM calls in separate iframes succeed.

https://reviewboard.mozilla.org/r/260476/#review267568

> This gets fixed as part of using a testcase object to associate other objects. My original thinking: we currently don't lint this dir,  but the usage of for...of upsets the linter as we won't use the var. I opted for the current style to avoid issue if we turn on the linter (despite it being ugly in its own way).

You say "for...of upsets the linter as we won't use the var" but I don't see any place where we don't use the var. On the other hand I haven't run the linter so maybe I'm misunderstanding. Creating new issues for any remaining cases.
Comment on attachment 8996344 [details]
Bug 1404977 - Tests P1: Add test to ensure multiple, concurrent gUM calls in separate iframes succeed.

https://reviewboard.mozilla.org/r/260476/#review268050

Thank you! r=me with nits.

::: dom/media/tests/mochitest/test_getUserMedia_audioConstraints_concurrentIframes.html:21
(Diff revision 2)
> +  * This test is hacky because of how the test interacts with iframes: each
> +  * iframe includes a copy of head.js, but assertions in the iframes are
> +  * not seen by the test harness in our original document. This means we should
> +  * not rely on the implicit checks done by helpers in head.js for getUserMedia
> +  * and in this test, as they are swallowed by the iframes and will not be seen
> +  * in, nor impact the test result.

Should we then omit including mediaStreamPlayback.js in the iframes and instead manually call gUM on each iframe's navigator object?

Or do we get anything useful out of the gUM wrapper?

Then any and all checks would be up to us here.

::: dom/media/tests/mochitest/test_getUserMedia_audioConstraints_concurrentIframes.html:77
(Diff revision 2)
> +  // to only two calls at a time. The while and the splice lines can be removed,
> +  // and allConstraintCombinations can be renamed to constraintCombinations once
> +  // this issue is resolved.

Mention bug 1480348.

Or perhaps file a bug X, make X depend on 1480348 and mention X here, for best effect.

::: dom/media/tests/mochitest/test_getUserMedia_audioConstraints_concurrentIframes.html:109
(Diff revision 2)
> +
> +    // Wait for all iframes to be loaded
> +    await Promise.all(testCases.map(tc => tc.iframeLoadedPromise));
> +
> +    // One by one see if we can grab a gUM stream per iframe
> +    for (let i = 0; i < testCases.length; i++) {

for...of

::: dom/media/tests/mochitest/test_getUserMedia_audioConstraints_concurrentIframes.html:112
(Diff revision 2)
> +        .catch(e => {
> +          // Log faliure a rethrow to kill test
> +          ok(false, `getUserMedia calls should not fail! Failed in iframe of testcase#: ${i} with: ${e}!`);
> +          throw e;
> +        });

This should be
```
.catch(e => Promise.reject(new Error(`getUserMedia calls should not fail! Failed in iframe of testcase#: ${i} with: ${e}!`)));
```

::: dom/media/tests/mochitest/test_getUserMedia_audioConstraints_concurrentIframes.html:114
(Diff revision 2)
> +    for (let i = 0; i < testCases.length; i++) {
> +      testCases[i].gumStream =
> +        await testCases[i].iframe.contentWindow.getUserMedia({audio: testCases[i].requestedConstraints})
> +        .catch(e => {
> +          // Log faliure a rethrow to kill test
> +          ok(false, `getUserMedia calls should not fail! Failed in iframe of testcase#: ${i} with: ${e}!`);

You don't need `i` for logging here if you give each `testCase` an identifier. Then the identifier could also be a short string showing which egn-combination it's testing.

::: dom/media/tests/mochitest/test_getUserMedia_audioConstraints_concurrentIframes.html:125
(Diff revision 2)
> +      ok(!differenceString,
> +        `gUM stream for testcase at index ${i} should have the same constraints as were ` +
> +        `requested from gUM. Differences: ${differenceString}`);
> +    }
> +
> +    // Once all streams are collected, make sure make sure the constraints

s/make sure //

::: dom/media/tests/mochitest/test_getUserMedia_audioConstraints_concurrentIframes.html:126
(Diff revision 2)
> +        `gUM stream for testcase at index ${i} should have the same constraints as were ` +
> +        `requested from gUM. Differences: ${differenceString}`);
> +    }
> +
> +    // Once all streams are collected, make sure make sure the constraints
> +    // haven't been mutated by other gUM call

s/other/another/

::: dom/media/tests/mochitest/test_getUserMedia_audioConstraints_concurrentIframes.html:127
(Diff revision 2)
> +        `requested from gUM. Differences: ${differenceString}`);
> +    }
> +
> +    // Once all streams are collected, make sure make sure the constraints
> +    // haven't been mutated by other gUM call
> +    for (let i = 0; i < testCases.length; i++) {

for...of

::: dom/media/tests/mochitest/test_getUserMedia_audioConstraints_concurrentIframes.html:137
(Diff revision 2)
> +        `gUM stream for testcase at index ${i} should not have had contraints altered after ` +
> +        `all gUM calls are done. Differences: ${differenceString}`);
> +    }
> +
> +    // We do not currently have tests to verify the behaviour of the different
> +    // constraints. Once we do we should do further verificaiton here. See

s/verificaiton/verification/

::: dom/media/tests/mochitest/test_getUserMedia_audioConstraints_concurrentIframes.html:140
(Diff revision 2)
> +
> +    // We do not currently have tests to verify the behaviour of the different
> +    // constraints. Once we do we should do further verificaiton here. See
> +    // bug 1406372, bug 1406376, and bug 1406377.
> +
> +    for (let i = 0; i < testCases.length; i++) {

for...of

::: dom/media/tests/mochitest/test_getUserMedia_audioConstraints_concurrentIframes.html:143
(Diff revision 2)
> +    // bug 1406372, bug 1406376, and bug 1406377.
> +
> +    for (let i = 0; i < testCases.length; i++) {
> +      let testAudio = createMediaElement("audio", `testAudio${i}`);
> +      let playback = new LocalMediaStreamPlayback(testAudio, testCases[i].gumStream);
> +      testCases[i].playMediaPromise = playback.playMediaWithoutStoppingTracks(false);

You could use an await here to make the playback tests sequential (then drop my issue below).

It shouldn't take much more time and it should help with reading the log output if there's a failure on try.

::: dom/media/tests/mochitest/test_getUserMedia_audioConstraints_concurrentIframes.html:149
(Diff revision 2)
> +    }
> +
> +    await Promise.all(testCases.map(tc => tc.playMediaPromise));
> +
> +    // Stop the tracks for each stream, we left them running above via
> +    // playMediaWithoutStoppingTracks to make sure they can play sequentially.

s/sequentially/concurrently/ no?

::: dom/media/tests/mochitest/test_getUserMedia_audioConstraints_concurrentIframes.html:150
(Diff revision 2)
> +    // Remove iframes to get rid of tone generators from head.js, otherwise the
> +    // test will keep making noise on results page.

Let's not use tone generators in each iframe as they serve no purpose.

If one is needed you can set it up manually here instead, but I doubt it.
Attachment #8996344 - Flags: review?(apehrson) → review+
Comment on attachment 8996345 [details]
Bug 1404977 - Tests P2: Add test to ensure multiple, concurrent gUM calls in a single window succeed.

https://reviewboard.mozilla.org/r/260478/#review268056

Thanks! r=me with the issues here, and all issues on P1 for this rev where applicable, fixed.

::: dom/media/tests/mochitest/test_getUserMedia_audioConstraints_concurrentStreams.html:14
(Diff revision 2)
> +createHTML({
> +  title: "getUserMedia multiple times, concurrently, and with different constraints",
> +  bug: "1404977"
> +});
> +/**
> +  * Verify that we can successfully call getUserMedia multiple times,

add "for the same device"

::: dom/media/tests/mochitest/test_getUserMedia_audioConstraints_concurrentStreams.html:16
(Diff revision 2)
> +  bug: "1404977"
> +});
> +/**
> +  * Verify that we can successfully call getUserMedia multiple times,
> +  * concurrently. This is checked by calling getUserMedia a number of times
> +  * with different constraints. We verify the stream returned by that call has

s/verify the/verify that the/

::: dom/media/tests/mochitest/test_getUserMedia_audioConstraints_concurrentStreams.html:67
(Diff revision 2)
> +  // TODO: We would like to be able to perform an arbitrary number of gUM calls
> +  // at once, but issues with pulse and audio IPC mean on some systems we're
> +  // limited to as few as 2 concurrent calls. To avoid issues we chunk test runs
> +  // to only two calls at a time. The while and the splice lines can be removed,
> +  // and allConstraintCombinations can be renamed to constraintCombinations once
> +  // this issue is resolved.

Have you checked that this test actually needs chunking? It was ok locally for me.

I'd rather see chunking avoided so please verify this on try before landing.
Attachment #8996345 - Flags: review?(apehrson) → review+
Status: NEW → ASSIGNED
Comment on attachment 8996344 [details]
Bug 1404977 - Tests P1: Add test to ensure multiple, concurrent gUM calls in separate iframes succeed.

https://reviewboard.mozilla.org/r/260476/#review268050

> Should we then omit including mediaStreamPlayback.js in the iframes and instead manually call gUM on each iframe's navigator object?
> 
> Or do we get anything useful out of the gUM wrapper?
> 
> Then any and all checks would be up to us here.

We could avoid doing so. We'd need to manually manage the tone generation for loopback devices, which is normally handled by head.js. I don't like that we'd have some tests where the calls go via the head.js helper and some that don't.

> for...of

I think we should keep this one so we have a simple naming/id scheme for the media elements we're creating on line 141.

> You could use an await here to make the playback tests sequential (then drop my issue below).
> 
> It shouldn't take much more time and it should help with reading the log output if there's a failure on try.

I've left this as is so we can make sure all the streams are playing at once. I don't think we're doing much verificaiton here either way, but figure concurrent streams is the slightly harder test. In future if we end up verifying the constraints we may need to change this depending on how that verificaiton is done.

> Let's not use tone generators in each iframe as they serve no purpose.
> 
> If one is needed you can set it up manually here instead, but I doubt it.

This depends on if we want to work around using head.js in each of the frames. Leaving open for now.
Comment on attachment 8996344 [details]
Bug 1404977 - Tests P1: Add test to ensure multiple, concurrent gUM calls in separate iframes succeed.

https://reviewboard.mozilla.org/r/260476/#review268050

> We could avoid doing so. We'd need to manually manage the tone generation for loopback devices, which is normally handled by head.js. I don't like that we'd have some tests where the calls go via the head.js helper and some that don't.

Well it's designed to by default create a loopback tone, but to allow for manual overrides.

In this case we don't even need a loopback tone to pass the test, but the twoliner `let tone = new LoopbackTone(TEST_AUDIO_FREQ); tone.start();` is not a big deal if we'd like to test audio content one day.

If ever debugging this test I'd prefer having 8 MSGs getting only the input over each of them also running an AudioContext and playing that to speakers.

> I think we should keep this one so we have a simple naming/id scheme for the media elements we're creating on line 141.

You resolved the issue where I suggested giving each testCase an identifier. Why not use that?

> I've left this as is so we can make sure all the streams are playing at once. I don't think we're doing much verificaiton here either way, but figure concurrent streams is the slightly harder test. In future if we end up verifying the constraints we may need to change this depending on how that verificaiton is done.

Once the streams are running through an MSG there's not much impact by them being played or not. The main cost is in running the MSG and the processing on the input side.

> This depends on if we want to work around using head.js in each of the frames. Leaving open for now.

There shouldn't be anything to work around. Vanilla gUM should just work, no?
A bug is showing up in [1]. I have it in rr.

MediaManager thread:
> SourceListener::StopTrack -> MediaStreamGraphImpl::CloseAudioInput -> Dispatch to main thread for MSG::AppendMessage

Main thread:
> MediaStreamGraphImpl::CloseAudioInput -> MSG::AppendMessage

Graph thread:
> AudioCallbackDriver::DataCallback -> MediaStreamGraphImpl::RunMessagesInQueue -> MediaStreamGraphImpl::CloseAudioInputImpl -> mInputDeviceID = nullptr
... next iteration:
> AudioCallbackDriver::DataCallback -> MediaStreamGraphImpl::NotifyInputData -> listeners = mInputDeviceUsers.GetValue(mInputDeviceID)
Since mInputDeviceID was null, `listeners` becomes null too. And note that just above this is a debug-only assert `MOZ_ASSERT(mInputDeviceID || CurrentDriver()->Switching());` that is true because we're switching, but the switching hasn't happened yet.

The switching assert succeeding indicates this case is OK. Furthermore, NotifyOutputData has a guard for when mInputDeviceID is null. It seems to me that NotifyInputData needs one too.



[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=1ab0f605986b86920f17bd8ed561a6bca2dfddf6&selectedJob=191447591
Comment on attachment 8996344 [details]
Bug 1404977 - Tests P1: Add test to ensure multiple, concurrent gUM calls in separate iframes succeed.

https://reviewboard.mozilla.org/r/260476/#review268236

::: dom/media/tests/mochitest/test_getUserMedia_audioConstraints_concurrentIframes.html:99
(Diff revision 3)
> +      testCase.iframe = document.createElement("iframe");
> +      // Stick test scripts into the iframes so they can call the gUM helper
> +      testCase.iframe.srcdoc = `
> +      <html>
> +        <script type="application/javascript" src="mediaStreamPlayback.js">
> +        <\/script>

s/\//
Comment on attachment 8997405 [details]
Bug 1404977 - Exit NotifyInputData early if there's no input listener.

https://reviewboard.mozilla.org/r/261188/#review268238

::: dom/media/MediaStreamGraph.cpp:979
(Diff revision 1)
>      // this iteration, and we're switching back to an output-only driver next
>      // iteration.
>      MOZ_ASSERT(mInputDeviceID || CurrentDriver()->Switching());
>    }
>  #endif
> +  if (!mInputDeviceID) {

According to the comment above mInputDeviceID expected to be 0 when we are switching. Is the comment accurate?   Can you replace this check by checking if we are switching driver?
Attachment #8997405 - Flags: review?(achronop) → review+
Comment on attachment 8997405 [details]
Bug 1404977 - Exit NotifyInputData early if there's no input listener.

https://reviewboard.mozilla.org/r/261188/#review268238

> According to the comment above mInputDeviceID expected to be 0 when we are switching. Is the comment accurate?   Can you replace this check by checking if we are switching driver?

Well the comment says that a device ID of 0 is OK if we're switching, it's also OK if it's not 0.

I can't change the check to look at Switching() instead because that requires locking the monitor and we don't want that.

It's fine though, since we lock the monitor and do the check in debug builds.
Comment on attachment 8996344 [details]
Bug 1404977 - Tests P1: Add test to ensure multiple, concurrent gUM calls in separate iframes succeed.

https://reviewboard.mozilla.org/r/260476/#review268236

> s/\//

Without the escape we end up having an unterminated script end tag here, which I think is okay in Firefox, but will upset the linter. I'll rework the iframe behaviour such that we don't have a script in them.
Comment on attachment 8996345 [details]
Bug 1404977 - Tests P2: Add test to ensure multiple, concurrent gUM calls in a single window succeed.

https://reviewboard.mozilla.org/r/260478/#review268056

> Have you checked that this test actually needs chunking? It was ok locally for me.
> 
> I'd rather see chunking avoided so please verify this on try before landing.

I'm seeing some issues here sans chunking: https://treeherder.mozilla.org/#/jobs?repo=try&revision=9cb688b4dfbeef51eed57b76be836266b63934d2&selectedJob=191641627

I only partially kept up with the discussion around what the underlying issue was, so let me know if that issue is no related to the chunking.
Comment on attachment 8996345 [details]
Bug 1404977 - Tests P2: Add test to ensure multiple, concurrent gUM calls in a single window succeed.

https://reviewboard.mozilla.org/r/260478/#review268056

> I'm seeing some issues here sans chunking: https://treeherder.mozilla.org/#/jobs?repo=try&revision=9cb688b4dfbeef51eed57b76be836266b63934d2&selectedJob=191641627
> 
> I only partially kept up with the discussion around what the underlying issue was, so let me know if that issue is no related to the chunking.

Have dropped the chunking from the concurrentStream test.
https://hg.mozilla.org/integration/mozilla-inbound/rev/127bff24bfc8fa5d0e619ea727ab4a0c522c4497
Bug 1404977 - Part 1 - Add missing lock to the PREF_CUBEB_BACKEND branch of the pref callback in CubebUtils.cpp. r=pehrsons

https://hg.mozilla.org/integration/mozilla-inbound/rev/3e1bbbc9047f732251966666c1d5a80265c3593f
Bug 1404977 - Part 2 - Augment AudioDeviceInfo with a cubeb device id. r=pehrsons

https://hg.mozilla.org/integration/mozilla-inbound/rev/72fd73a9d4e3180fe0c16479b2da5f891addb312
Bug 1404977 - Part 3 - Remove global statics, introduce an audio device enumerator r=pehrsons

https://hg.mozilla.org/integration/mozilla-inbound/rev/16671b7c328ac8fe27fef7906aac7afef9dc7a3d
Bug 1404977 - Part 5 - Allow querying the number of input channels from a WebRTCAudioDataListener. r=pehrsons

https://hg.mozilla.org/integration/mozilla-inbound/rev/700fadf31c3a79c98679f0986900c06d494e0f37
Bug 1404977 - Part 6 - Remove unused include for lock-free FIFO. r=pehrsons

https://hg.mozilla.org/integration/mozilla-inbound/rev/c63f8579e0dbc3b3401897db3095b51ef4af3c0e
Bug 1404977 - Part 7 - Make each MediaEngineWebRTCMicrophoneSource independent. r=pehrsons

https://hg.mozilla.org/integration/mozilla-inbound/rev/507d5c269d25aee79fe081b1b02a4970ddcc1fb0
Bug 1404977 - Part 8 - Tell the MSG the MediaEngineAudioSource are now independent and that we can have multiple of them, cleanup the MSG-side API for managing them. r=pehrsons

https://hg.mozilla.org/integration/mozilla-inbound/rev/20a567fbf561dd88c059439f2b34e1024c58ca2c
Bug 1404977 - Part 9 - Propagate the changes to the GraphDrivers, simplifying them, and brokering all access through the MSG. r=pehrsons

https://hg.mozilla.org/integration/mozilla-inbound/rev/d09741d7dab28b401e4a1b753078f93be6a2eea3
Bug 1404977 - Part 10 - Make logging uniform, and consistently use GraphImpl() to access the graph impl. r=pehrsons

https://hg.mozilla.org/integration/mozilla-inbound/rev/d28f054ca88e4a441d0df0febc32227f005f97a3
Bug 1404977 - Part 11 - Make sure the default device is the first element in the list. r=pehrsons

https://hg.mozilla.org/integration/mozilla-inbound/rev/eba12c9748f882d9a1d84038cf9b3ae15ebe0987
Bug 1404977 - Part 12 - Make DeviceChanged() notification thread safe by using the MSG message queue. r=pehrsons

https://hg.mozilla.org/integration/mozilla-inbound/rev/dc0f442f208cec6baa8d2675faff00a5adedb143
Bug 1404977 - Part 13 - Remove useless mutex, and assert why they were useless, in WebRTCAudioDataListeners. r=pehrsons

https://hg.mozilla.org/integration/mozilla-inbound/rev/3259ebfa7dea7816ce60dbd0814908e2d66031d4
Bug 1404977 - Part 14 - Add a way to set the global cubeb* singleton at runtime, from a test. r=pehrsons

https://hg.mozilla.org/integration/mozilla-inbound/rev/ecbae0557eaae616491c2c87fe42591bb64d7b48
Bug 1404977 - Part 15 - Invalidate the device cache before re-enumerating devices when the cubeb backend does not support dynamic device collection invalidation. r=pehrsons

https://hg.mozilla.org/integration/mozilla-inbound/rev/edc7a43bb7592c5407564151feeeb38c3684281f
Bug 1404977 - Part 16 - Unit test CubebDeviceEnumerator. r=pehrsons

https://hg.mozilla.org/integration/mozilla-inbound/rev/d3fc55ebb24ab7117d5bbe740653af807dfe05b0
Bug 1404977 - Part 17 - Re-implement the workaround for the lack of input device enumeration on Android. r=pehrsons

https://hg.mozilla.org/integration/mozilla-inbound/rev/acbf4a97ecca6ff4a0b5f2c08115f91523e41461
Bug 1404977 - Exit NotifyInputData early if there's no input listener. r=achronop

https://hg.mozilla.org/integration/mozilla-inbound/rev/c2d350fb074fb0c59eb922973ee33fc8ceeec0a5
Bug 1404977 - Tests P1: Add test to ensure multiple, concurrent gUM calls in separate iframes succeed. r=pehrsons

https://hg.mozilla.org/integration/mozilla-inbound/rev/67c4ed3c73d7bb770edcbdffebfb5688a3822a5b
Bug 1404977 - Tests P2: Add test to ensure multiple, concurrent gUM calls in a single window succeed. r=pehrsons
Depends on: 1481152
Depends on: 1481957
Depends on: 1483926
Blocks: 1487057
Blocks: 1492847
Depends on: 1502313
No longer depends on: 1502313
Regressions: 1562626
See Also: → 1603439
No longer regressions: 1689741
Regressions: 1691899
Regressions: 1699233
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: