1404977 - Rework device enumeration

Status: RESOLVED FIXED

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

Description

[Paul Adenot (:padenot)]

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.

Comment 1

[Nils Ohlmeier [:drno]]

@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.

Comment 2

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Tech debt does not qualify as P1 under the new priority schema. Adjusting to top-rank P2.

Comment 3

[Paul Adenot (:padenot)]

Attachment: Bug 1404977 - Part 1 - Add missing lock to the PREF_CUBEB_BACKEND branch of the pref callback in CubebUtils.cpp.

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

Comment 4

[Paul Adenot (:padenot)]

Attachment: Bug 1404977 - Part 2 - Augment AudioDeviceInfo with a cubeb device id.

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

Comment 5

[Paul Adenot (:padenot)]

Attachment: Bug 1404977 - Part 10 - Make logging uniform, and consistently use GraphImpl() to access the graph impl.

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

Comment 6

[Paul Adenot (:padenot)]

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.

Comment 7

[Bryce Seager van Dyk [:bryce] (he/him) - Not reading bugmail]

Cheers, will do.

Comment 8

[Alex Chronopoulos [:achronop]]

This looks close to the work I am doing for Bug 1152401. How far have you get with it? Can I steal it?

Comment 9

[Paul Adenot (:padenot)]

Attachment: Bug 1404977 - Part 3 - Remove global statics, introduce an audio device enumerator

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

Comment 10

[Paul Adenot (:padenot)]

Attachment: Bug 1404977 - Part 4 - Format a comment and remove an unused and now useless method.

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

Comment 11

[Paul Adenot (:padenot)]

Attachment: Bug 1404977 - Part 5 - Allow querying the number of input channels from a WebRTCAudioDataListener.

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

Comment 12

[Paul Adenot (:padenot)]

Attachment: Bug 1404977 - Part 6 - Remove unused include for lock-free FIFO.

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

Comment 13

[Paul Adenot (:padenot)]

Attachment: Bug 1404977 - Part 7 - Make each MediaEngineWebRTCMicrophoneSource independent.

The concept of an "allocation" is not of any use anymore, now, but this is big
enough so that I want to clean it up later.

This allows having multiple stream for the same gUM call on the same device in
the same document, and being able to apply different constraints to each of
them.

Also removes the last bit of global static, so that it is also possible to have
multiple mics open in the same content process.

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

Comment 14

[Paul Adenot (:padenot)]

Attachment: 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.

The MSG now can feed microphone data to all its input listener. This paves the
way for multiple input device, if we feel it's needed at some point, but does
not implement it.

The method for adding/removing inputs are also cleaned up.

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

Comment 15

[Paul Adenot (:padenot)]

Attachment: Bug 1404977 - Part 9 - Propagate the changes to the GraphDrivers, simplifying them, and brokering all access through the MSG.

For an AudioCallbackDriver, the number of input channels is immutable, and
passed at construction, so that it's less necessary to rely on global state.

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

Comment 16

[Paul Adenot (:padenot)]

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.

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

Comment 17

[Paul Adenot (:padenot)]

Comment on attachment 8968568 [details]
Bug 1404977 - Part 2 - Augment AudioDeviceInfo with a cubeb device id.

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

Comment 18

[Paul Adenot (:padenot)]

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

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

Comment 19

[Paul Adenot (:padenot)]

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.

Comment 20

[Bryce Seager van Dyk [:bryce] (he/him) - Not reading bugmail]

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 21

[Andreas Pehrson [:pehrsons]]

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

Comment 22

[Andreas Pehrson [:pehrsons]]

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

Comment 23

[Andreas Pehrson [:pehrsons]]

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.

Comment 24

[Andreas Pehrson [:pehrsons]]

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

Comment 25

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8972013 [details]
Bug 1404977 - Part 6 - Remove unused include for lock-free FIFO.

https://reviewboard.mozilla.org/r/240756/#review246928

Comment 26

[Andreas Pehrson [:pehrsons]]

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.

Comment 27

[Andreas Pehrson [:pehrsons]]

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

Comment 28

[Bryce Seager van Dyk [:bryce] (he/him) - Not reading bugmail]

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

Comment 29

[Andreas Pehrson [:pehrsons]]

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?

Comment 30

[Andreas Pehrson [:pehrsons]]

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 31

[Andreas Pehrson [:pehrsons]]

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?

Comment 32

[Andreas Pehrson [:pehrsons]]

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/

Comment 33

[Bryce Seager van Dyk [:bryce] (he/him) - Not reading bugmail]

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

Comment 34

[Alex Chronopoulos [:achronop]]

Attachment: Bug 1404977 - Part 1 - Add a pref to force the output device by name, for testing.

This is a bit unrelated, but I'm touching a bunch of stuff in the area and it
was in the same queue.

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

Comment 35

[Alex Chronopoulos [:achronop]]

Attachment: Bug 1404977 - Part 2 - Augment AudioDeviceID with a cubeb device id.

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

Comment 36

[Alex Chronopoulos [:achronop]]

Attachment: Bug 1404977 - Part 3 - Remove global statics, introduce and audio device enumerator

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

Comment 37

[Alex Chronopoulos [:achronop]]

Attachment: Bug 1404977 - Part 4 - Format a comment and remove an unused and now useless method.

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

Comment 38

[Alex Chronopoulos [:achronop]]

Attachment: Bug 1404977 - Part 5 - Allow querying the number of input channel from a WebRTCAudioDataListener.

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

Comment 39

[Alex Chronopoulos [:achronop]]

Attachment: Bug 1404977 - Part 6 - Remove unused include for lock-free FIFO.

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

Comment 40

[Alex Chronopoulos [:achronop]]

Attachment: Bug 1404977 - Part 7 - Make each MediaEngineWebRTCMicrophoneSource independant.

The concept of an "allocation" is not of any use anymore, now, but this is big
enough so that I want to clean it up later.

This allows having multiple stream for the same gUM call on the same device in
the same document, and being able to apply different constraints to each of
them.

Also removes the last bit of global static, so that it is also possible to have
multiple mics open in the same content process.

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

Comment 41

[Alex Chronopoulos [:achronop]]

Attachment: 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.

The MSG now can feed microphone data to all its input listener. This paves the
way for multiple input device, if we feel it's needed at some point, but does
not implement it.

The method for adding/removing inputs are also cleaned up.

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

Comment 42

[Alex Chronopoulos [:achronop]]

Attachment: Bug 1404977 - Part 9 - Propagate the changes to the GraphDrivers, simplifying them, and brokering all access through the MSG.

For an AudioCallbackDriver, the number of input channels is immutable, and
passed at construction, so that it's less necessary to rely on global state.

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

Comment 43

[Alex Chronopoulos [:achronop]]

Attachment: Bug 1404977 - Part 10 - Make logging uniform, and consistently use GraphImpl() to access the graph impl.

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

Comment 44

[Alex Chronopoulos [:achronop]]

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 45

[Andreas Pehrson [:pehrsons]]

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

Comment 46

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8974942 [details]
Bug 1404977 - Part 2 - Augment AudioDeviceID with a cubeb device id.

https://reviewboard.mozilla.org/r/243320/#review249256

Comment 47

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8974943 [details]
Bug 1404977 - Part 3 - Remove global statics, introduce and audio device enumerator

https://reviewboard.mozilla.org/r/243322/#review249258

Comment 48

[Andreas Pehrson [:pehrsons]]

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

Comment 49

[Andreas Pehrson [:pehrsons]]

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

Comment 50

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8974946 [details]
Bug 1404977 - Part 6 - Remove unused include for lock-free FIFO.

https://reviewboard.mozilla.org/r/243328/#review249264

Comment 51

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8974947 [details]
Bug 1404977 - Part 7 - Make each MediaEngineWebRTCMicrophoneSource independant.

https://reviewboard.mozilla.org/r/243330/#review249266

Comment 52

[Andreas Pehrson [:pehrsons]]

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

Comment 53

[Andreas Pehrson [:pehrsons]]

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

Comment 54

[Andreas Pehrson [:pehrsons]]

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

Comment 55

[Alex Chronopoulos [:achronop]]

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 56

[Alex Chronopoulos [:achronop]]

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 57

[Alex Chronopoulos [:achronop]]

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 58

[Alex Chronopoulos [:achronop]]

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 59

[Alex Chronopoulos [:achronop]]

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.

Comment 60

[Bryce Seager van Dyk [:bryce] (he/him) - Not reading bugmail]

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 61

[Alex Chronopoulos [:achronop]]

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 62

[Alex Chronopoulos [:achronop]]

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 63

[Alex Chronopoulos [:achronop]]

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 64

[Alex Chronopoulos [:achronop]]

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

Comment 65

[Alex Chronopoulos [:achronop]]

Attachment: Bug 1404977 - Part 1 - Add a pref to force the output device by name, for testing.

This is a bit unrelated, but I'm touching a bunch of stuff in the area and it
was in the same queue.

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

Comment 66

[Alex Chronopoulos [:achronop]]

Attachment: Bug 1404977 - Part 2 - Augment AudioDeviceInfo with a cubeb device id.

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

Comment 67

[Alex Chronopoulos [:achronop]]

Attachment: Bug 1404977 - Part 3 - Remove global statics, introduce an audio device enumerator

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

Comment 68

[Alex Chronopoulos [:achronop]]

Attachment: Bug 1404977 - Part 4 - Format a comment and remove an unused and now useless method.

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

Comment 69

[Alex Chronopoulos [:achronop]]

Attachment: Bug 1404977 - Part 5 - Allow querying the number of input channels from a WebRTCAudioDataListener.

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

Comment 70

[Alex Chronopoulos [:achronop]]

Attachment: Bug 1404977 - Part 6 - Remove unused include for lock-free FIFO.

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

Comment 71

[Alex Chronopoulos [:achronop]]

Attachment: Bug 1404977 - Part 7 - Make each MediaEngineWebRTCMicrophoneSource independent.

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

Comment 72

[Alex Chronopoulos [:achronop]]

Attachment: 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.

The MSG now can feed microphone data to all its input listeners. This paves the
way for multiple input device, if we feel it's needed at some point, but does
not implement it.

The method for adding/removing inputs are also cleaned up.

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

Comment 73

[Alex Chronopoulos [:achronop]]

Attachment: Bug 1404977 - Part 9 - Propagate the changes to the GraphDrivers, simplifying them, and brokering all access through the MSG.

For an AudioCallbackDriver, the number of input channels is immutable, and
passed at construction, so that it's less necessary to rely on global state.

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

Comment 74

[Alex Chronopoulos [:achronop]]

Attachment: Bug 1404977 - Part 10 - Make logging uniform, and consistently use GraphImpl() to access the graph impl.

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

Comment 75

[Andreas Pehrson [:pehrsons]]

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.

Comment 76

[Andreas Pehrson [:pehrsons]]

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.

Comment 77

[Andreas Pehrson [:pehrsons]]

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 78

[Andreas Pehrson [:pehrsons]]

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.

Comment 79

[Andreas Pehrson [:pehrsons]]

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

Comment 80

[Andreas Pehrson [:pehrsons]]

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.

Comment 81

[Andreas Pehrson [:pehrsons]]

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 82

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8979575 [details]
Bug 1404977 - Part 6 - Remove unused include for lock-free FIFO.

https://reviewboard.mozilla.org/r/245712/#review252122

Comment 83

[Andreas Pehrson [:pehrsons]]

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.

Comment 84

[Andreas Pehrson [:pehrsons]]

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 85

[Andreas Pehrson [:pehrsons]]

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.

Comment 86

[Andreas Pehrson [:pehrsons]]

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 87

[Andreas Pehrson [:pehrsons]]

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?

Comment 88

[Andreas Pehrson [:pehrsons]]

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 89

[Andreas Pehrson [:pehrsons]]

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"));
>  
> +

?

Comment 90

[Paul Adenot (:padenot)]

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 91

[Paul Adenot (:padenot)]

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 92

[Paul Adenot (:padenot)]

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 93

[Paul Adenot (:padenot)]

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.

Comment 94

[Paul Adenot (:padenot)]

(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 95

[Paul Adenot (:padenot)]

Comment on attachment 8979576 [details]
Bug 1404977 - Part 7 - Make each MediaEngineWebRTCMicrophoneSource independent.

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

Comment 96

[Paul Adenot (:padenot)]

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.

Comment 97

[Paul Adenot (:padenot)]

Attachment: Bug 1404977 - Part 11 - Make sure the default device is the first element in the list.

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

Comment 98

[Paul Adenot (:padenot)]

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.

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

Comment 99

[Paul Adenot (:padenot)]

Comment on attachment 8968568 [details]
Bug 1404977 - Part 2 - Augment AudioDeviceInfo with a cubeb device id.

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

Comment 100

[Paul Adenot (:padenot)]

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

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

Comment 101

[Paul Adenot (:padenot)]

Comment on attachment 8972011 [details]
Bug 1404977 - Part 4 - Format a comment and remove an unused and now useless method.

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

Comment 102

[Paul Adenot (:padenot)]

Comment on attachment 8972012 [details]
Bug 1404977 - Part 5 - Allow querying the number of input channels from a WebRTCAudioDataListener.

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

Comment 103

[Paul Adenot (:padenot)]

Comment on attachment 8972013 [details]
Bug 1404977 - Part 6 - Remove unused include for lock-free FIFO.

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

Comment 104

[Paul Adenot (:padenot)]

Comment on attachment 8972014 [details]
Bug 1404977 - Part 7 - Make each MediaEngineWebRTCMicrophoneSource independent.

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

Comment 105

[Paul Adenot (:padenot)]

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.

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

Comment 106

[Paul Adenot (:padenot)]

Comment on attachment 8972016 [details]
Bug 1404977 - Part 9 - Propagate the changes to the GraphDrivers, simplifying them, and brokering all access through the MSG.

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

Comment 107

[Paul Adenot (:padenot)]

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

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

Comment 108

[Bryce Seager van Dyk [:bryce] (he/him) - Not reading bugmail]

Attachment: Bug 1404977 - Tests P1: Add test to check that multiple gums within iFrames succeed.

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

Comment 109

[Bryce Seager van Dyk [:bryce] (he/him) - Not reading bugmail]

Attachment: Bug 1404977 - Tests P2: Add test to ensure multiple gUM calls in a single window will work.

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

Comment 110

[Alex Chronopoulos [:achronop]]

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

Comment 111

[Bryce Seager van Dyk [:bryce] (he/him) - Not reading bugmail]

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

Comment 112

[Andreas Pehrson [:pehrsons]]

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

Comment 113

[Andreas Pehrson [:pehrsons]]

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.

Comment 114

[Andreas Pehrson [:pehrsons]]

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 115

[Andreas Pehrson [:pehrsons]]

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);
```

Comment 116

[Andreas Pehrson [:pehrsons]]

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

Comment 117

[Andreas Pehrson [:pehrsons]]

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

Comment 118

[Andreas Pehrson [:pehrsons]]

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 119

[Andreas Pehrson [:pehrsons]]

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 120

[Andreas Pehrson [:pehrsons]]

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.

Comment 121

[Andreas Pehrson [:pehrsons]]

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).

Comment 122

[Andreas Pehrson [:pehrsons]]

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 123

[Andreas Pehrson [:pehrsons]]

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?

Comment 124

[Andreas Pehrson [:pehrsons]]

(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.

Comment 125

[Bryce Seager van Dyk [:bryce] (he/him) - Not reading bugmail]

(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.

Comment 126

[Paul Adenot (:padenot)]

(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.

Comment 127

[Paul Adenot (:padenot)]

Attachment: Bug 1404977 - Part 12 - Make DeviceChanged() notification thread safe by using the MSG message queue.

This is slightly slower, especially if the main thread is busy, but it's cleaner
and actually safe.

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

Comment 128

[Paul Adenot (:padenot)]

Attachment: Bug 1404977 - Part 13 - Remove useless mutex, and assert why they were useless, in WebRTCAudioDataListeners.

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

Comment 129

[Paul Adenot (:padenot)]

Attachment: Bug 1404977 - Part 14 - Consistently pass the MSG to the AudioDataListener methods to be able to write thread asserts.

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

Comment 130

[Paul Adenot (:padenot)]

Attachment: Bug 1404977 - Part 15 - Disconnect the MediaManager and MediaStreamGraph part of MediaEngineWebRTCMicrophoneSource on the MSG thread.

This makes the lifetime and various thread access clearer.

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

Comment 131

[Paul Adenot (:padenot)]

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.

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

Comment 132

[Paul Adenot (:padenot)]

Comment on attachment 8968568 [details]
Bug 1404977 - Part 2 - Augment AudioDeviceInfo with a cubeb device id.

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

Comment 133

[Paul Adenot (:padenot)]

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

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

Comment 134

[Paul Adenot (:padenot)]

Comment on attachment 8972011 [details]
Bug 1404977 - Part 4 - Format a comment and remove an unused and now useless method.

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

Comment 135

[Paul Adenot (:padenot)]

Comment on attachment 8972012 [details]
Bug 1404977 - Part 5 - Allow querying the number of input channels from a WebRTCAudioDataListener.

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

Comment 136

[Paul Adenot (:padenot)]

Comment on attachment 8972013 [details]
Bug 1404977 - Part 6 - Remove unused include for lock-free FIFO.

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

Comment 137

[Paul Adenot (:padenot)]

Comment on attachment 8972014 [details]
Bug 1404977 - Part 7 - Make each MediaEngineWebRTCMicrophoneSource independent.

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

Comment 138

[Paul Adenot (:padenot)]

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.

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

Comment 139

[Paul Adenot (:padenot)]

Comment on attachment 8972016 [details]
Bug 1404977 - Part 9 - Propagate the changes to the GraphDrivers, simplifying them, and brokering all access through the MSG.

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

Comment 140

[Paul Adenot (:padenot)]

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

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

Comment 141

[Paul Adenot (:padenot)]

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

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

Comment 142

[Paul Adenot (:padenot)]

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 143

[Andreas Pehrson [:pehrsons]]

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.

Comment 144

[Andreas Pehrson [:pehrsons]]

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.

Comment 145

[Andreas Pehrson [:pehrsons]]

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

Comment 146

[Andreas Pehrson [:pehrsons]]

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.

Comment 147

[Andreas Pehrson [:pehrsons]]

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).

Comment 148

[Andreas Pehrson [:pehrsons]]

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()`.

Comment 149

[Andreas Pehrson [:pehrsons]]

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()`).

Comment 150

[Andreas Pehrson [:pehrsons]]

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).

Comment 151

[Paul Adenot (:padenot)]

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.

Comment 152

[Paul Adenot (:padenot)]

Changes are at https://hg.mozilla.org/users/paul_paul.cx/mozilla-unified/.

Comment 153

[Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)]

Marking this as a feature currently planned for 63, since we discussed it at the Nightly deep dive meeting today.

Comment 155

[Bryce Seager van Dyk [:bryce] (he/him) - Not reading bugmail]

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

Comment 156

[Bryce Seager van Dyk [:bryce] (he/him) - Not reading bugmail]

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

Comment 157

[Paul Adenot (:padenot)]

Attachment: Bug 1404977 - Part 14 - Add a way to set the global cubeb* singleton at runtime, from a test.

This is for testing purposes only. Defining ENABLE_SET_CUBEB_BACKEND before
including CubebUtils.h will expose the function. This is not to be set outside
of test files.

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

Comment 158

[Paul Adenot (:padenot)]

Attachment: Bug 1404977 - Part 15 - Invalidate the device cache before re-enumerating devices when the cubeb backend does not support dynamic device collection invalidation.

Also, clear the array that's been passed in before appending the new devices.

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

Comment 159

[Paul Adenot (:padenot)]

Attachment: Bug 1404977 - Part 16 - Unit test CubebDeviceEnumerator.

This is done by implementing a fake cubeb backend that implements the subset of
operations we need, while offering an API to be able to control what this
backend is doing.

Because we're reimplementing the private cubeb API, it is necessary to copy
part of a cubeb internal header, and mimick exactly how the vtable mechanism to
do the dynamic dispatch to the diffferent backends in cubeb works. This is not
ideal but works.

When the cubeb API functions are called (from deep in the Gecko process), we
re-bind the call to the mock cubeb backend object and behave exactly like a
normal backend (calling various callbacks and returning fake objects).

Finally, we inject this mock cubeb backend to the running Gecko process (in lieu
of the real one that would have been picked) by setting the global sCubebBackend
variable via a private API exposed only in the test in CubebUtils.h.

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

Comment 160

[Paul Adenot (:padenot)]

Attachment: Bug 1404977 - Part 17 - Re-implement the workaround for the lack of input device enumeration on Android.

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

Comment 161

[Paul Adenot (:padenot)]

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.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/237254/diff/4-5/

Comment 162

[Paul Adenot (:padenot)]

Comment on attachment 8968568 [details]
Bug 1404977 - Part 2 - Augment AudioDeviceInfo with a cubeb device id.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/237256/diff/4-5/

Comment 163

[Paul Adenot (:padenot)]

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

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

Comment 164

[Paul Adenot (:padenot)]

Comment on attachment 8972011 [details]
Bug 1404977 - Part 4 - Format a comment and remove an unused and now useless method.

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

Comment 165

[Paul Adenot (:padenot)]

Comment on attachment 8972012 [details]
Bug 1404977 - Part 5 - Allow querying the number of input channels from a WebRTCAudioDataListener.

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

Comment 166

[Paul Adenot (:padenot)]

Comment on attachment 8972013 [details]
Bug 1404977 - Part 6 - Remove unused include for lock-free FIFO.

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

Comment 167

[Paul Adenot (:padenot)]

Comment on attachment 8972014 [details]
Bug 1404977 - Part 7 - Make each MediaEngineWebRTCMicrophoneSource independent.

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

Comment 168

[Paul Adenot (:padenot)]

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.

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

Comment 169

[Paul Adenot (:padenot)]

Comment on attachment 8972016 [details]
Bug 1404977 - Part 9 - Propagate the changes to the GraphDrivers, simplifying them, and brokering all access through the MSG.

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

Comment 170

[Paul Adenot (:padenot)]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/237258/diff/4-5/

Comment 171

[Paul Adenot (:padenot)]

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

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

Comment 172

[Paul Adenot (:padenot)]

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

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

Comment 173

[Paul Adenot (:padenot)]

Comment on attachment 8983491 [details]
Bug 1404977 - Part 13 - Remove useless mutex, and assert why they were useless, in WebRTCAudioDataListeners.

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

Comment 174

[Code Review Bot [:reviewbot]]

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 175

[Code Review Bot [:reviewbot]]

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 176

[Code Review Bot [:reviewbot]]

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 177

[Code Review Bot [:reviewbot]]

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 178

[Andreas Pehrson [:pehrsons]]

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.

Comment 179

[Andreas Pehrson [:pehrsons]]

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.

Comment 180

[Andreas Pehrson [:pehrsons]]

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.

Comment 181

[Andreas Pehrson [:pehrsons]]

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

Comment 182

[Andreas Pehrson [:pehrsons]]

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.

Comment 183

[Andreas Pehrson [:pehrsons]]

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?

Comment 184

[Andreas Pehrson [:pehrsons]]

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

Comment 185

[Andreas Pehrson [:pehrsons]]

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.

Comment 186

[Andreas Pehrson [:pehrsons]]

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.

Comment 187

[Paul Adenot (:padenot)]

Comment on attachment 8992918 [details]
Bug 1404977 - Part 16 - Unit test CubebDeviceEnumerator.

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

Comment 188

[Paul Adenot (:padenot)]

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 189

[Paul Adenot (:padenot)]

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 190

[Paul Adenot (:padenot)]

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.

Comment 191

[Paul Adenot (:padenot)]

(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 192

[Paul Adenot (:padenot)]

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 193

[Paul Adenot (:padenot)]

Comment on attachment 8972014 [details]
Bug 1404977 - Part 7 - Make each MediaEngineWebRTCMicrophoneSource independent.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/240758/diff/4-5/

Comment 194

[Paul Adenot (:padenot)]

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.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/240760/diff/4-5/

Comment 195

[Paul Adenot (:padenot)]

Comment on attachment 8972016 [details]
Bug 1404977 - Part 9 - Propagate the changes to the GraphDrivers, simplifying them, and brokering all access through the MSG.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/240762/diff/4-5/

Comment 196

[Paul Adenot (:padenot)]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/237258/diff/5-6/

Comment 197

[Paul Adenot (:padenot)]

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

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

Comment 198

[Paul Adenot (:padenot)]

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

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

Comment 199

[Paul Adenot (:padenot)]

Comment on attachment 8983491 [details]
Bug 1404977 - Part 13 - Remove useless mutex, and assert why they were useless, in WebRTCAudioDataListeners.

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

Comment 200

[Paul Adenot (:padenot)]

Comment on attachment 8992916 [details]
Bug 1404977 - Part 14 - Add a way to set the global cubeb* singleton at runtime, from a test.

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

Comment 201

[Paul Adenot (:padenot)]

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.

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

Comment 202

[Paul Adenot (:padenot)]

Comment on attachment 8992918 [details]
Bug 1404977 - Part 16 - Unit test CubebDeviceEnumerator.

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

Comment 203

[Paul Adenot (:padenot)]

Comment on attachment 8992919 [details]
Bug 1404977 - Part 17 - Re-implement the workaround for the lack of input device enumeration on Android.

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

Comment 204

[Andreas Pehrson [:pehrsons]]

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?

Comment 205

[Paul Adenot (:padenot)]

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.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/237254/diff/5-6/

Comment 206

[Paul Adenot (:padenot)]

Comment on attachment 8968568 [details]
Bug 1404977 - Part 2 - Augment AudioDeviceInfo with a cubeb device id.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/237256/diff/5-6/

Comment 207

[Paul Adenot (:padenot)]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/240750/diff/4-5/

Comment 208

[Paul Adenot (:padenot)]

Comment on attachment 8972012 [details]
Bug 1404977 - Part 5 - Allow querying the number of input channels from a WebRTCAudioDataListener.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/240754/diff/4-5/

Comment 209

[Paul Adenot (:padenot)]

Comment on attachment 8972013 [details]
Bug 1404977 - Part 6 - Remove unused include for lock-free FIFO.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/240756/diff/4-5/

Comment 210

[Paul Adenot (:padenot)]

Comment on attachment 8972014 [details]
Bug 1404977 - Part 7 - Make each MediaEngineWebRTCMicrophoneSource independent.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/240758/diff/5-6/

Comment 211

[Paul Adenot (:padenot)]

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.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/240760/diff/5-6/

Comment 212

[Paul Adenot (:padenot)]

Comment on attachment 8972016 [details]
Bug 1404977 - Part 9 - Propagate the changes to the GraphDrivers, simplifying them, and brokering all access through the MSG.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/240762/diff/5-6/

Comment 213

[Paul Adenot (:padenot)]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/237258/diff/6-7/

Comment 214

[Paul Adenot (:padenot)]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/247542/diff/4-5/

Comment 215

[Paul Adenot (:padenot)]

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

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

Comment 216

[Paul Adenot (:padenot)]

Comment on attachment 8983491 [details]
Bug 1404977 - Part 13 - Remove useless mutex, and assert why they were useless, in WebRTCAudioDataListeners.

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