1221587 - Teach Gecko how to use full-duplex cubeb streams

Status: RESOLVED FIXED

(Core :: WebRTC: Audio/Video, defect, P1)

Description

[Paul Adenot (:padenot)]

This involves:
- Wiring up device enumeration to MediaManager.cpp
- Teaching the MSG to get its mic data from the callback or the SourceMediaStream listener so we can roll-out full-duplex per-platform
- Wiring up the AEC using the low-level AudioProcessing interface

Comment 1

[Maire Reavy [:mreavy]]

Paul -- I think it makes the most sense for you to take this one since I believe this is the toughest piece remaining and the long pole for landing full duplex support on Window.  Do you agree?

Comment 2

[Paul Adenot (:padenot)]

Makes sense indeed.

Comment 3

[Randell Jesup [:jesup] (needinfo me)]

Attachment: WIP patch to allow getUserMedia to use full-duplex cubeb streams

builds and runs (in webrtc.org capture mode, hard-coded).  Cubeb code is stubbed, with partial implementation of enumeration and audio input #if'd out.  Needs actual interfacing to cubeb and per-platform prefs to enable/disable it, plus some other cleanup.  Design is an encapsulation of the VoEHW interface (or rather a small subset of the interface)

Comment 4

[Randell Jesup [:jesup] (needinfo me)]

I have full-duplex mostly working on Windows in Firefox.  http://hg.mozilla.org/users/rjesup_wgate.com/full_duplex
Enumeration is partly stubbed out, and it doesn't support actually selecting a mic (for the same reason), so it grabs the mic associated with the output stream

Comment 5

[Andreas Pehrson [:pehrsons]]

I got full duplex working on Mac with [1] and the three patches I'll be posting shortly. If you could check the sanity of those patches and take them over Randell, I'll be more than happy!

[1] https://github.com/pehrsons/cubeb/tree/wip/fullduplex

Comment 6

[Andreas Pehrson [:pehrsons]]

Attachment: Make sure we remove mixer callbacks properly when we try to switch GraphDriver multiple times per iteration

This came from a UAF I showed padenot in Orlando. Turned out we tried to switch driver twice before the next iteration and both times we'd just remove mCurrentDriver and set mNextDriver to the new one. I.e., we removed (the same) mCurrentDriver twice and also effectively did `mNextDriver = driver1; mNextDriver = driver2;`. Both driver1 and driver2 had been added to the list of mixer callbacks but driver1 was never removed (but it was destroyed since mNextDriver was the only ref to it).

Comment 7

[Andreas Pehrson [:pehrsons]]

Attachment: Make sure a new audio driver is inited after the previous one has been shut down

This makes sure that we INIT a new audio driver _after_ the previous audio driver has been SHUTDOWN.

On Mac we can't have two AudioOutputUnits for the same device at the same time. The second one just doesn't see its callback called.

Comment 8

[Andreas Pehrson [:pehrsons]]

Attachment: Rename MediaStreamGraphShutdownThreadRunnable2 to MediaStreamGraphShutdownThreadRunnable now that it's available

This is not really needed, but for cleanliness' sake.

Comment 9

[Alex Chronopoulos [:achronop]]

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

Comment 10

[Alex Chronopoulos [:achronop]]

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

Comment 11

[Randell Jesup [:jesup] (needinfo me)]

Attachment: patch 12 - add per-platform prefs to control full-duplex cubeb input

Comment 12

[Randell Jesup [:jesup] (needinfo me)]

Attachment: patch 11 - Block attempts to open two mics at once until supported in full-duplex

Comment 13

[Randell Jesup [:jesup] (needinfo me)]

Attachment: Patch 2 - Win32 fixes for cubeb full-duplex patch

Very likely these are moot and your current version of the cubeb full-duplex code covers this

Comment 14

[Randell Jesup [:jesup] (needinfo me)]

Attachment: Patch 3 - Add base devid support to cubeb api

I suspect we may need to fix the tests before landing this

Comment 15

[Randell Jesup [:jesup] (needinfo me)]

Attachment: patch 4 - Rename MediaStreamGraphShutdownThreadRunnable2

Comment 16

[Randell Jesup [:jesup] (needinfo me)]

Attachment: patch 5 - Base update of the MSG API for full-duplex

Comment 17

[Randell Jesup [:jesup] (needinfo me)]

Attachment: patch 6 - allow getUserMedia to use full-duplex cubeb streams

Comment 18

[Randell Jesup [:jesup] (needinfo me)]

Attachment: patch 7 - change listeners for full-duplex audio

Comment 19

[Randell Jesup [:jesup] (needinfo me)]

Attachment: patch 8 - use cubeb devids to select input devices

Comment 20

[Randell Jesup [:jesup] (needinfo me)]

Attachment: patch 9 - Implement switching of AudioCallbackDrivers for full-duplex

Comment 21

[Randell Jesup [:jesup] (needinfo me)]

Attachment: patch 10 - Improve logging of callback driver/switching

Comment 22

[Paul Adenot (:padenot)]

Comment on attachment 8705065 [details] [diff] [review]
Patch 3 - Add base devid support to cubeb api

Review of attachment 8705065 [details] [diff] [review]:
-----------------------------------------------------------------

::: media/libcubeb/include/cubeb.h
@@ +176,5 @@
>  } cubeb_device_pref;
>  
> +/** Stream format initialization parameters. */
> +typedef struct {
> +  cubeb_devid devid;          /* Device identifier handle -- NULL means default */

This is going to be landed either by myself for WASAPI or from Alex's or Andreas' patch, in the form of an upstream cubeb pull request.

::: media/libcubeb/moz.build
@@ +4,5 @@
>  # License, v. 2.0. If a copy of the MPL was not distributed with this
>  # file, You can obtain one at http://mozilla.org/MPL/2.0/.
>  
>  DIRS += ['include', 'src']
> +#TEST_DIRS += ['tests']

Hm ? Just so that it compiles ?

Comment 23

[Paul Adenot (:padenot)]

Comment on attachment 8705076 [details] [diff] [review]
patch 10 - Improve logging of callback driver/switching

Review of attachment 8705076 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/media/GraphDriver.cpp
@@ +95,5 @@
>    LIFECYCLE_LOG("Switching to new driver: %p (%s)",
>        aNextDriver, aNextDriver->AsAudioCallbackDriver() ?
>        "AudioCallbackDriver" : "SystemClockDriver");
> + if (mNextDriver &&
> +       mNextDriver != GraphImpl()->CurrentDriver()) {

nit: indentation.

Comment 24

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

Comment on attachment 8704919 [details] [diff] [review]
patch 12 - add per-platform prefs to control full-duplex cubeb input

Review of attachment 8704919 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/media/MediaManager.cpp
@@ +1523,5 @@
>      }
>    }
> +  LOG(("%s: default prefs: %dx%d @%dfps (min %d), %dHz test tones, %sfull-duplex", __FUNCTION__,
> +       mPrefs.mWidth, mPrefs.mHeight, mPrefs.mFPS, mPrefs.mMinFPS, mPrefs.mFreq,
> +       mPrefs.mFullDuplex ? "" : "not "));

half-duplex?

::: dom/media/webrtc/MediaEngineWebRTC.cpp
@@ +286,5 @@
>      } else {
>        mAudioInput = new mozilla::AudioInputWebRTC(mVoiceEngine);
>      }
>    }
>    

You're it.

::: modules/libpref/init/all.js
@@ +458,5 @@
>  #else
>  // *BSD, others - merely a guess for now
>  pref("media.peerconnection.capture_delay", 50);
>  pref("media.getusermedia.playout_delay", 50);
> +pref("media.navigator.audio.full_duplex", false);

nit: Instead of 5 of the same, why not put it outside the #if/else?

Comment 25

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

Comment on attachment 8705010 [details] [diff] [review]
patch 11 - Block attempts to open two mics at once until supported in full-duplex

Review of attachment 8705010 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/media/webrtc/MediaEngineWebRTC.h
@@ +246,2 @@
>    }
>    

nip existing whitespace.

@@ +247,5 @@
>    
>    virtual int SetRecordingDevice(int index)
>    {
>      int32_t devindex = DeviceIndex(index);
> +    // Bug XXXXXX: block attempts to capture from more than one mic at a time

Please file bug and add bug #

@@ +260,5 @@
>  private:
>    nsTArray<int> mDeviceIndexes;
>    int mSelectedDevice;
>    static cubeb_device_collection *mDevices;
> +  static bool mInUse;

This patch seems simple enough. Just a comment on surrounding code, which I gather is from the other 12 patches, that I find it perhaps an odd choice to see a singleton (I hope?) with static members, yet virtual rather than, say, static methods? I'm sure there's a good reason, just wanted to mention it.

Comment 26

[Randell Jesup [:jesup] (needinfo me)]

Attachment: patch 12: Clear SharedThreadPool before shutdown-threads, and don't switch from SystemClockDriver to SystemClockDriver

fixes (with bug 1237794 for ClearOnShutdown with phase support) the shutdown hang, and also the bug I hit trying to rr it - where we switched from SystemClockDriver to SystemClockDriver on CloseAudioInput, because UpdateStreamOrder had already switched us

Comment 27

[Paul Adenot (:padenot)]

Comment on attachment 8705070 [details] [diff] [review]
patch 5 - Base update of the MSG API for full-duplex

Review of attachment 8705070 [details] [diff] [review]:
-----------------------------------------------------------------

I think I'll need to look at those patches again, it's pretty big. Would you mind using mozreview next time ?

::: dom/media/GraphDriver.cpp
@@ +896,5 @@
> +
> +  // Process mic data if any/needed -- after inserting far-end data for AEC!
> +  if (aInputBuffer) {
> +    if (mAudioInput) { // for this specific input-only or full-duplex stream
> +      mAudioInput->NotifyInputData(mGraphImpl, aInputBuffer,

Hm, we're not using the AEC-ed data here are we ? (Edit: I see this is fixed in a later patch).

@@ +898,5 @@
> +  if (aInputBuffer) {
> +    if (mAudioInput) { // for this specific input-only or full-duplex stream
> +      mAudioInput->NotifyInputData(mGraphImpl, aInputBuffer,
> +                                   static_cast<size_t>(aFrames),
> +                                   ChannelCount);

Typo: you want `ChannelCount()`

::: dom/media/GraphDriver.h
@@ +198,5 @@
>  
> +  // XXX Thread-safety! Do these via commands to avoid TSAN issues
> +  // and crashes!!!
> +  virtual void SetInputListener(MediaStreamListener *aListener) {
> +    mAudioInput = aListener;

Add a threading assert.

@@ +200,5 @@
> +  // and crashes!!!
> +  virtual void SetInputListener(MediaStreamListener *aListener) {
> +    mAudioInput = aListener;
> +  }
> +  // XXX do we need the param?  probably no

Until we have multiple inputs so we know which one to remove.

@@ +202,5 @@
> +    mAudioInput = aListener;
> +  }
> +  // XXX do we need the param?  probably no
> +  virtual void RemoveInputListener(MediaStreamListener *aListener) {
> +    mAudioInput = nullptr;

Add a threading assert.

@@ +500,5 @@
>     * driver back to a SystemClockDriver).
>     * This is synchronized by the Graph's monitor.
>     * */
>    bool mStarted;
> +  /* listener for mic input, if any */

Nit: Capital L and stop at the end of the comment.

::: dom/media/MediaStreamGraph.cpp
@@ +936,5 @@
> +
> +nsresult
> +MediaStreamGraphImpl::OpenAudioInput(char *aName, MediaStreamListener *aListener)
> +{
> +  // XXX So, so, so annoying.  Can't AppendMessage except on Mainthread

We can probably change this if really needed. I'm sure it'll come in handy for AudioWorklets or other advanced use of the MSG.

@@ +940,5 @@
> +  // XXX So, so, so annoying.  Can't AppendMessage except on Mainthread
> +  if (!NS_IsMainThread()) {
> +    NS_DispatchToMainThread(WrapRunnable(this,
> +                                         &MediaStreamGraphImpl::OpenAudioInput,
> +                                         aName, aListener)); // XXX Fix! string need to copied

We can just remove the string? It does not seem too useful.

::: dom/media/MediaStreamGraph.h
@@ +181,5 @@
>     * are also notified of atomically to MediaStreamListeners.
>     */
>    virtual void NotifyFinishedTrackCreation(MediaStreamGraph* aGraph) {}
> +
> +  /* These are for cubeb audio input streams: */

input and output, right?

Comment 28

[Paul Adenot (:padenot)]

Comment on attachment 8705071 [details] [diff] [review]
patch 6 - allow getUserMedia to use full-duplex cubeb streams

Review of attachment 8705071 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/media/GraphDriver.cpp
@@ +599,2 @@
>    if (cubeb_stream_init(CubebUtils::GetCubebContext(), &stream,
> +                        "AudioCallbackDriver", &params, &params, latency,

This means we're opening a stereo mic ? The mic is probably mono and cubeb will likely upmix it to stereo, but it's not very useful.

::: dom/media/webrtc/MediaEngineWebRTC.h
@@ +133,5 @@
> +                                     char strGuidUTF8[128]) = 0;
> +  virtual int GetRecordingDeviceStatus(bool& isAvailable) = 0;
> +  virtual void StartRecording(MediaStreamGraph *graph) = 0;
> +  virtual void StopRecording(MediaStreamGraph *graph) = 0;
> +  virtual int SetRecordingDevice(int index) = 0;

aArgument for arguments.

@@ +136,5 @@
> +  virtual void StopRecording(MediaStreamGraph *graph) = 0;
> +  virtual int SetRecordingDevice(int index) = 0;
> +
> +protected:
> +  webrtc::VoiceEngine* mVoiceEngine;

Is a raw ptr okay? What is the lifetime for this?

@@ +167,5 @@
> +      if (mDevices->device[i]->type == CUBEB_DEVICE_TYPE_INPUT && // paranoia
> +          mDevices->device[i]->state == CUBEB_DEVICE_STATE_ENABLED)
> +      {
> +        devices++;
> +        // XXX to support device changes, we need to identify by name/UUID not index

You can do that now, there is enough API to do so? Maybe you meant that as a follow-up though.

@@ +180,5 @@
> +    if (!mDevices) {
> +      return 1;
> +    }
> +    int devindex = index == -1 ? 0 : index;
> +    snprintf(strNameUTF8, 128, "%s%s", index == -1 ? "default: " : "",

Put all the occurrences of this 128 in a constant somewhere.

Also, should "default: " be localizable ?

@@ +188,5 @@
> +  }
> +
> +  virtual int GetRecordingDeviceStatus(bool& isAvailable)
> +  {
> +    isAvailable = true; // XXX Cheating

Can you open a bug for that? Or just implement it, this is available in `cubeb_device_info.state`.

@@ +199,5 @@
> +    ptrVoERender = webrtc::VoEExternalMedia::GetInterface(mVoiceEngine);
> +    if (ptrVoERender) {
> +      ptrVoERender->SetExternalRecordingStatus(true);
> +    }
> +    graph->OpenAudioInput(nullptr, this);

We should have a name here, right ? We don't use the name anyway, is there a reason to have it? Maybe in a future iteration ?

@@ +209,5 @@
> +  }
> +
> +  virtual int SetRecordingDevice(int index)
> +  {
> +    return 1;

Comment? I have no clue what this means.

@@ +213,5 @@
> +    return 1;
> +  }
> +
> +private:
> +  cubeb_device_collection *mDevices;

nit: star next to the type.

@@ +246,5 @@
> +  }
> +
> +  virtual int GetRecordingDeviceStatus(bool& isAvailable)
> +  {
> +    isAvailable = true; // XXX Cheating

Same as above.

@@ +415,5 @@
>  
>    // gUM runnables can e.g. Enumerate from multiple threads
>    Mutex mMutex;
>    webrtc::VoiceEngine* mVoiceEngine;
> +  mozilla::AudioInput* mAudioInput;

Are raw pointers really okay here ?

::: dom/media/webrtc/MediaEngineWebRTCAudio.cpp
@@ +321,5 @@
>        return NS_ERROR_FAILURE;
>      }
>  
>      mState = kReleased;
> +    //XXX assert that we're stopped

File a bug for this? Do you need a cubeb API ?

::: media/libcubeb/src/cubeb_alsa.c
@@ -305,5 @@
>    p = calloc(1, snd_pcm_frames_to_bytes(stm->pcm, avail));
>    assert(p);
>  
>    pthread_mutex_unlock(&stm->mutex);
> -  got = stm->data_callback(stm, stm->user_ptr, p, avail);

As said on IRC, I have a patch somewhere that does all this for all backends.

::: media/libcubeb/src/cubeb_pulse.c
@@ -197,5 @@
>      assert(r == 0);
>      assert(size > 0);
>      assert(size % frame_size == 0);
>  
> -    got = stm->data_callback(stm, stm->user_ptr, buffer, size / frame_size);

Ditto.

Comment 29

[Paul Adenot (:padenot)]

Comment on attachment 8705072 [details] [diff] [review]
patch 7 - change listeners for full-duplex audio

Review of attachment 8705072 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/media/GraphDriver.h
@@ +198,4 @@
>      mAudioInput = aListener;
>    }
>    // XXX do we need the param?  probably no
> +  virtual void RemoveInputListener(AudioDataListener *aListener) {

Better name indeed.

@@ +497,5 @@
>     * This is synchronized by the Graph's monitor.
>     * */
>    bool mStarted;
>    /* listener for mic input, if any */
> +  AudioDataListener* mAudioInput;

Why removing the RefPtr ?

::: dom/media/MediaStreamGraph.h
@@ +1310,5 @@
>     * at construction.
>     */
>    TrackRate mSampleRate;
>  
> +  nsTArray<AudioDataListener*> mAudioInputs; // XXX ownership?

Having raw pointers and thread everywhere is asking for trouble :-)

::: dom/media/webrtc/MediaEngineDefault.h
@@ +143,5 @@
>                       "MediaEngineDefaultAudioSource data underrun");
>  #endif
>    }
>  
> +  void NotifySpeakerData(MediaStreamGraph* aGraph,

Can we call this `NotifyOutputData` ?

::: dom/media/webrtc/MediaEngineWebRTCAudio.cpp
@@ +453,5 @@
> +                                                     uint32_t aChannels)
> +{
> +}
> +
> +// XXX Assumes number of channels doesn't change!

Changing the number of channels here should work since my patch set on stereo PeerConnection.

@@ +454,5 @@
> +{
> +}
> +
> +// XXX Assumes number of channels doesn't change!
> +// Called back on GraphDriver thread (internal or external)

I don't understand the "internal or external" part.

Comment 30

[Paul Adenot (:padenot)]

Comment on attachment 8705073 [details] [diff] [review]
patch 8 - use cubeb devids to select input devices

Review of attachment 8705073 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/media/GraphDriver.cpp
@@ +595,5 @@
>      return;
>    }
>  
> +  input = output;
> +  input.devid = mGraphImpl->mInputDeviceID;

You need to change the channel count here.

@@ +596,5 @@
>    }
>  
> +  input = output;
> +  input.devid = mGraphImpl->mInputDeviceID;
> +  

nit: trailing space.

::: dom/media/MediaStreamGraph.cpp
@@ +923,5 @@
>    }
>  }
>  
>  void
> +MediaStreamGraphImpl::OpenAudioInputImpl(void *aID, AudioDataListener *aListener)

I don't really like the void* in the signature, can we either use the cubeb type or typedef it to something so we move an opaque type around with more info on what it is ?

@@ +965,5 @@
>  }
>  
>  void
>  MediaStreamGraphImpl::CloseAudioInputImpl(AudioDataListener *aListener)
>  {

Add a threading assert on all the *Impl methods.

::: dom/media/MediaStreamGraphImpl.h
@@ +589,5 @@
> +  bool mInputWanted;
> +  cubeb_devid mInputDeviceID;
> +  bool mOutputWanted;
> +  cubeb_devid mOutputDeviceID;
> +  

nit: trailing space.

::: dom/media/webrtc/MediaEngineWebRTC.cpp
@@ +43,5 @@
>  #define LOG(args) MOZ_LOG(GetUserMediaLog(), mozilla::LogLevel::Debug, args)
>  
>  namespace mozilla {
>  
> +cubeb_device_collection *AudioInputCubeb::mDevices = nullptr;

nit: * next to the type.

@@ +320,5 @@
>      if (mAudioSources.Get(uuid, getter_AddRefs(aSource))) {
>        // We've already seen this device, just append.
>        aASources->AppendElement(aSource.get());
>      } else {
> +      AudioInput *audioinput = mAudioInput;

Same.

::: dom/media/webrtc/MediaEngineWebRTC.h
@@ +188,5 @@
> +            (mDevices->device[i]->state == CUBEB_DEVICE_STATE_ENABLED ||
> +             mDevices->device[i]->state == CUBEB_DEVICE_STATE_UNPLUGGED))
> +        {
> +          mDeviceIndexes.AppendElement(i);
> +          // XXX to support device changes, we need to identify by name/devid not index

I don't really get this translation thing. We have an order for

@@ +256,5 @@
>  
>  private:
> +  nsTArray<int> mDeviceIndexes;
> +  int mSelectedDevice;
> +  static cubeb_device_collection *mDevices;

How are we supporting plugging/unplugging devices ? It appears the notification callback is not implemented for some backends (at least WASAPI), we should fix this.

Is this thread safe, how is it synchronized ?

Comment 31

[Paul Adenot (:padenot)]

Comment on attachment 8705075 [details] [diff] [review]
patch 9 - Implement switching of AudioCallbackDrivers for full-duplex

Review of attachment 8705075 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/media/GraphDriver.cpp
@@ +69,2 @@
>    MOZ_ASSERT(!PreviousDriver());
> +  MOZ_ASSERT(aPreviousDriver);

Those two line are weird.

::: dom/media/GraphDriver.h
@@ +516,2 @@
>    dom::AudioChannel mAudioChannel;
> +  /* used to queue us to add the mixer callback on first run */

nit: capital and full stop.

::: dom/media/MediaStreamGraph.cpp
@@ +923,5 @@
>  
>  void
>  MediaStreamGraphImpl::OpenAudioInputImpl(void *aID, AudioDataListener *aListener)
>  {
> + // Bug XXXXXX Need support for multiple mics at once

Make sure to put the bug number here.

@@ +966,5 @@
>      }
>      MediaStreamGraphImpl *mGraph;
> +    // aID is a cubeb_devid, and we assume that opaque ptr is valid until
> +    // we close cubeb.
> +    void *mID;

I think it can be invalidated if we remove or add a device maybe ? Also void* here is not ideal.

@@ +983,3 @@
>    mAudioInputs.RemoveElement(aListener);
> +
> +  // Switch Drivers since we're adding removing input (to nothing/system or output only)

s/adding removing/adding or removing an/

@@ +1008,5 @@
> +    } else {
> +      STREAM_LOG(LogLevel::Debug, ("CloseInput: no output present (SystemClockCallback)"));
> +
> +      driver = new SystemClockDriver(this);
> +      mMixer.RemoveCallback(CurrentDriver()->AsAudioCallbackDriver());

Can we put the RemoveCallback in the AudioCallbackDriver/SystemClockDriver as well? It would be better/safer I think.

Comment 32

[Paul Adenot (:padenot)]

Comment on attachment 8705064 [details] [diff] [review]
Patch 2 - Win32 fixes for cubeb full-duplex patch

Review of attachment 8705064 [details] [diff] [review]:
-----------------------------------------------------------------

This is done in my patch as well.

Comment 33

[Randell Jesup [:jesup] (needinfo me)]

Attachment: MozReview Request: imported patch cubeb_duplex_wip.patch

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

Comment 34

[Randell Jesup [:jesup] (needinfo me)]

Attachment: MozReview Request: Bug 1221587: Patch 2 - Win32 fixes for cubeb full-duplex patch

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

Comment 35

[Randell Jesup [:jesup] (needinfo me)]

Attachment: MozReview Request: Bug 1221587: Patch 3 - Add base devid support to cubeb api r=padenot

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

Comment 36

[Randell Jesup [:jesup] (needinfo me)]

Attachment: MozReview Request: Bug 1221587: patch 4 - Rename MediaStreamGraphShutdownThreadRunnable2 r=padenot

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

Comment 37

[Randell Jesup [:jesup] (needinfo me)]

Attachment: MozReview Request: Bug 1221587: patch 5 - Base update of the MSG API for full-duplex r?padenot

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

Comment 38

[Randell Jesup [:jesup] (needinfo me)]

Attachment: MozReview Request: Bug 1221587: patch 6 - allow getUserMedia to use full-duplex cubeb streams r?padenot

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

Comment 39

[Randell Jesup [:jesup] (needinfo me)]

Attachment: MozReview Request: Bug 1221587: patch 7 - change listeners for full-duplex audio r?padenot

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

Comment 40

[Randell Jesup [:jesup] (needinfo me)]

Attachment: MozReview Request: [mq]: audioinput

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

Comment 41

[Randell Jesup [:jesup] (needinfo me)]

Attachment: MozReview Request: Bug 1221587: patch 8 - use cubeb devids to select input devices r?padenot

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

Comment 42

[Randell Jesup [:jesup] (needinfo me)]

Attachment: MozReview Request: Bug 1221587: patch 9 - Implement switching of AudioCallbackDrivers for full-duplex r?padenot

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

Comment 43

[Randell Jesup [:jesup] (needinfo me)]

Attachment: MozReview Request: imported patch switching_update

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

Comment 44

[Randell Jesup [:jesup] (needinfo me)]

Attachment: MozReview Request: Bug 1221587: patch 10 - Improve logging of callback driver/switching r=padenot

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

Comment 45

[Randell Jesup [:jesup] (needinfo me)]

Attachment: MozReview Request: Bug 1221587: patch 11 - Block attempts to open two mics at once until supported in full-duplex r=jib

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

Comment 46

[Randell Jesup [:jesup] (needinfo me)]

Attachment: MozReview Request: Bug 1221587: add per-platform prefs to control full-duplex cubeb input r=jib

* * *
Clean up previous audio driver shutdown when starting new audio
From dcc558daf6d8e8123b9480f9ba67b9d11a81bc73 Mon Sep 17 00:00:00 2001
 driver

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

Comment 47

[Randell Jesup [:jesup] (needinfo me)]

Attachment: MozReview Request: Bug 1237414: Switch AsyncCubebOperation to a SharedThreadPool

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

Comment 48

[Randell Jesup [:jesup] (needinfo me)]

Attachment: MozReview Request: imported patch linux_pulse_import

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

Comment 49

[Randell Jesup [:jesup] (needinfo me)]

Attachment: MozReview Request: imported patch cubeb-pulse-improvement

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

Comment 50

[Randell Jesup [:jesup] (needinfo me)]

Attachment: MozReview Request: audiounit: Make full duplex work with gecko

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

Comment 51

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8705672 [details]
MozReview Request: Bug 1221587: Patch 3 - Add base devid support to cubeb api r=padenot

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

Comment 52

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8705673 [details]
MozReview Request: Bug 1221587: patch 4 - Rename MediaStreamGraphShutdownThreadRunnable2 r=padenot

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

Comment 53

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8705674 [details]
MozReview Request: Bug 1221587: patch 5 - Base update of the MSG API for full-duplex r?padenot

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

Comment 54

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8705675 [details]
MozReview Request: Bug 1221587: patch 6 - allow getUserMedia to use full-duplex cubeb streams r?padenot

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

Comment 55

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8705676 [details]
MozReview Request: Bug 1221587: patch 7 - change listeners for full-duplex audio r?padenot

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

Comment 56

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8705677 [details]
MozReview Request: [mq]: audioinput

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

Comment 57

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8705678 [details]
MozReview Request: Bug 1221587: patch 8 - use cubeb devids to select input devices r?padenot

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

Comment 58

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8705679 [details]
MozReview Request: Bug 1221587: patch 9 - Implement switching of AudioCallbackDrivers for full-duplex r?padenot

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

Comment 59

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8705680 [details]
MozReview Request: imported patch switching_update

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

Comment 60

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8705681 [details]
MozReview Request: Bug 1221587: patch 10 - Improve logging of callback driver/switching r=padenot

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

Comment 61

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8705682 [details]
MozReview Request: Bug 1221587: patch 11 - Block attempts to open two mics at once until supported in full-duplex r=jib

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

Comment 62

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8705683 [details]
MozReview Request: Bug 1221587: add per-platform prefs to control full-duplex cubeb input r=jib

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

Comment 63

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8705684 [details]
MozReview Request: Bug 1237414: Switch AsyncCubebOperation to a SharedThreadPool

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

Comment 64

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8705685 [details]
MozReview Request: imported patch linux_pulse_import

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

Comment 65

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8705686 [details]
MozReview Request: imported patch cubeb-pulse-improvement

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

Comment 66

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8705687 [details]
MozReview Request: audiounit: Make full duplex work with gecko

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

Comment 67

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8705672 [details]
MozReview Request: Bug 1221587: Patch 3 - Add base devid support to cubeb api r=padenot

https://reviewboard.mozilla.org/r/30103/#review26857

Comment 68

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8705673 [details]
MozReview Request: Bug 1221587: patch 4 - Rename MediaStreamGraphShutdownThreadRunnable2 r=padenot

https://reviewboard.mozilla.org/r/30105/#review26859

Comment 69

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8705679 [details]
MozReview Request: Bug 1221587: patch 9 - Implement switching of AudioCallbackDrivers for full-duplex r?padenot

https://reviewboard.mozilla.org/r/30117/#review26863

Comment 70

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8705680 [details]
MozReview Request: imported patch switching_update

https://reviewboard.mozilla.org/r/30119/#review26867

Comment 71

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8705681 [details]
MozReview Request: Bug 1221587: patch 10 - Improve logging of callback driver/switching r=padenot

https://reviewboard.mozilla.org/r/30121/#review26869

Comment 72

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8705687 [details]
MozReview Request: audiounit: Make full duplex work with gecko

https://reviewboard.mozilla.org/r/30133/#review26871

Comment 73

[Randell Jesup [:jesup] (needinfo me)]

> @@ +260,5 @@
> >  private:
> >    nsTArray<int> mDeviceIndexes;
> >    int mSelectedDevice;
> >    static cubeb_device_collection *mDevices;
> > +  static bool mInUse;
> 
> This patch seems simple enough. Just a comment on surrounding code, which I
> gather is from the other 12 patches, that I find it perhaps an odd choice to
> see a singleton (I hope?) with static members, yet virtual rather than, say,
> static methods? I'm sure there's a good reason, just wanted to mention it.

It's not a singleton... mSelectedDevice is specific to each mic/AudioSource they're attached to.  Perhaps not a wonderful design, but it works.

Comment 74

[Randell Jesup [:jesup] (needinfo me)]

> @@ +898,5 @@
> > +  if (aInputBuffer) {
> > +    if (mAudioInput) { // for this specific input-only or full-duplex stream
> > +      mAudioInput->NotifyInputData(mGraphImpl, aInputBuffer,
> > +                                   static_cast<size_t>(aFrames),
> > +                                   ChannelCount);
> 
> Typo: you want `ChannelCount()`

ChannelCount is a static int, not a method.

> ::: dom/media/GraphDriver.h
> @@ +198,5 @@
> >  
> > +  // XXX Thread-safety! Do these via commands to avoid TSAN issues
> > +  // and crashes!!!
> > +  virtual void SetInputListener(MediaStreamListener *aListener) {
> > +    mAudioInput = aListener;
> 
> Add a threading assert.

Following patch moves these to MSG commands, removing the issue and the comment.

> > +  // XXX So, so, so annoying.  Can't AppendMessage except on Mainthread
> 
> We can probably change this if really needed. I'm sure it'll come in handy
> for AudioWorklets or other advanced use of the MSG.

This has been a pain before as well. And bouncing off MainThread to do realtime actions is bad.

> @@ +940,5 @@
> > +  // XXX So, so, so annoying.  Can't AppendMessage except on Mainthread
> > +  if (!NS_IsMainThread()) {
> > +    NS_DispatchToMainThread(WrapRunnable(this,
> > +                                         &MediaStreamGraphImpl::OpenAudioInput,
> > +                                         aName, aListener)); // XXX Fix! string need to copied
> 
> We can just remove the string? It does not seem too useful.

Later patch replaces aName with aID (devid)

Comment 75

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

Comment on attachment 8705681 [details]
MozReview Request: Bug 1221587: patch 10 - Improve logging of callback driver/switching r=padenot

Already reviewed this. See comment 24 for nits which still should be addressed.

Comment 76

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

Comment on attachment 8705680 [details]
MozReview Request: imported patch switching_update

Already reviewed this. See comment 25 for nits which still should be addressed.

Comment 77

[Randell Jesup [:jesup] (needinfo me)]

(In reply to Paul Adenot (:padenot) from comment #28)
> Comment on attachment 8705071 [details] [diff] [review]
> patch 6 - allow getUserMedia to use full-duplex cubeb streams
> 
> Review of attachment 8705071 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/media/GraphDriver.cpp
> @@ +599,2 @@
> >    if (cubeb_stream_init(CubebUtils::GetCubebContext(), &stream,
> > +                        "AudioCallbackDriver", &params, &params, latency,
> 
> This means we're opening a stereo mic ? The mic is probably mono and cubeb
> will likely upmix it to stereo, but it's not very useful.

Hmmm.  Let me look.  The right thing to do here is a bit unclear, depending on the usage, but mono probably makes more sense. I'll set it to mono for now.

> 
> ::: dom/media/webrtc/MediaEngineWebRTC.h
> @@ +133,5 @@
> > +                                     char strGuidUTF8[128]) = 0;
> > +  virtual int GetRecordingDeviceStatus(bool& isAvailable) = 0;
> > +  virtual void StartRecording(MediaStreamGraph *graph) = 0;
> > +  virtual void StopRecording(MediaStreamGraph *graph) = 0;
> > +  virtual int SetRecordingDevice(int index) = 0;
> 
> aArgument for arguments.

Yeah... I copied the method signatures from the code I'm wrapping.  Done.

> @@ +136,5 @@
> > +  virtual void StopRecording(MediaStreamGraph *graph) = 0;
> > +  virtual int SetRecordingDevice(int index) = 0;
> > +
> > +protected:
> > +  webrtc::VoiceEngine* mVoiceEngine;
> 
> Is a raw ptr okay? What is the lifetime for this?

Yes.  The BlahEngines use an internal ref/Release setup, and also the VoiceEngine is held open both by the AudioSource these are attached to, and by the MediaEngine itself (MediaEngineWebRTC).  This is in keeping with how we handle it already.

> @@ +167,5 @@
> > +      if (mDevices->device[i]->type == CUBEB_DEVICE_TYPE_INPUT && // paranoia
> > +          mDevices->device[i]->state == CUBEB_DEVICE_STATE_ENABLED)
> > +      {
> > +        devices++;
> > +        // XXX to support device changes, we need to identify by name/UUID not index
> 
> You can do that now, there is enough API to do so? Maybe you meant that as a
> follow-up though.

Yes, this is an existing issue I'm just echoing here.

> @@ +180,5 @@
> > +    if (!mDevices) {
> > +      return 1;
> > +    }
> > +    int devindex = index == -1 ? 0 : index;
> > +    snprintf(strNameUTF8, 128, "%s%s", index == -1 ? "default: " : "",
> 
> Put all the occurrences of this 128 in a constant somewhere.

Unfortunately the interface we're mirroring here uses a raw 128 also.  We can #define it, but really it has to track the underlying interface in the engine for now.

> Also, should "default: " be localizable ?

Should? yes.  This mirrors what's done in webrtc.org code we're replacing.  We can move to remove it and have the UI insert it for the default entry, localized.  (Actually: webrtc.org inserts it for Pulse; I think the Mac OS inserts it; and we don't get it at all currently on Windows - there's a bug on file for that to add it.)

> @@ +188,5 @@
> > +  }
> > +
> > +  virtual int GetRecordingDeviceStatus(bool& isAvailable)
> > +  {
> > +    isAvailable = true; // XXX Cheating
> 
> Can you open a bug for that? Or just implement it, this is available in
> `cubeb_device_info.state`.

For cubeb, this is always true (we don't expose devices that aren't input-able).  For old-style, I made it call the underlying webrtc.org interface as it used to.

> @@ +199,5 @@
> > +    ptrVoERender = webrtc::VoEExternalMedia::GetInterface(mVoiceEngine);
> > +    if (ptrVoERender) {
> > +      ptrVoERender->SetExternalRecordingStatus(true);
> > +    }
> > +    graph->OpenAudioInput(nullptr, this);
> 
> We should have a name here, right ? We don't use the name anyway, is there a
> reason to have it? Maybe in a future iteration ?

Later patch for devid support replaces this with a devid.

> @@ +415,5 @@
> >  
> >    // gUM runnables can e.g. Enumerate from multiple threads
> >    Mutex mMutex;
> >    webrtc::VoiceEngine* mVoiceEngine;
> > +  mozilla::AudioInput* mAudioInput;
> 
> Are raw pointers really okay here ?

For AudioInput, probably not.  Made it refcounting and use RefPtrs

> ::: dom/media/webrtc/MediaEngineWebRTCAudio.cpp
> @@ +321,5 @@
> >        return NS_ERROR_FAILURE;
> >      }
> >  
> >      mState = kReleased;
> > +    //XXX assert that we're stopped
> 
> File a bug for this? Do you need a cubeb API ?

Not needed; we error out if not Stopped above this.  Removed the comments.
 
> ::: media/libcubeb/src/cubeb_alsa.c
> @@ -305,5 @@
> >    p = calloc(1, snd_pcm_frames_to_bytes(stm->pcm, avail));
> >    assert(p);
> >  
> >    pthread_mutex_unlock(&stm->mutex);
> > -  got = stm->data_callback(stm, stm->user_ptr, p, avail);
> 
> As said on IRC, I have a patch somewhere that does all this for all backends.
> 
> ::: media/libcubeb/src/cubeb_pulse.c
> @@ -197,5 @@
> >      assert(r == 0);
> >      assert(size > 0);
> >      assert(size % frame_size == 0);
> >  
> > -    got = stm->data_callback(stm, stm->user_ptr, buffer, size / frame_size);
> 
> Ditto.

I'll remove these from here and add a temp patch to my queue.

Comment 78

[Randell Jesup [:jesup] (needinfo me)]

(In reply to Paul Adenot (:padenot) from comment #29)
> Comment on attachment 8705072 [details] [diff] [review]
> patch 7 - change listeners for full-duplex audio
> 
> Review of attachment 8705072 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/media/GraphDriver.h
> @@ +198,4 @@
> >      mAudioInput = aListener;
> >    }
> >    // XXX do we need the param?  probably no
> > +  virtual void RemoveInputListener(AudioDataListener *aListener) {
> 
> Better name indeed.
> 
> @@ +497,5 @@
> >     * This is synchronized by the Graph's monitor.
> >     * */
> >    bool mStarted;
> >    /* listener for mic input, if any */
> > +  AudioDataListener* mAudioInput;
> 
> Why removing the RefPtr ?

For a while this was an AudioInput* which wasn't refcounted; with this patch as it is now it's an AudioSource which is.  I've re-instated it.

> ::: dom/media/MediaStreamGraph.h
> @@ +1310,5 @@
> >     * at construction.
> >     */
> >    TrackRate mSampleRate;
> >  
> > +  nsTArray<AudioDataListener*> mAudioInputs; // XXX ownership?
> 
> Having raw pointers and thread everywhere is asking for trouble :-)

Ditto previous.
 
> ::: dom/media/webrtc/MediaEngineDefault.h
> @@ +143,5 @@
> >                       "MediaEngineDefaultAudioSource data underrun");
> >  #endif
> >    }
> >  
> > +  void NotifySpeakerData(MediaStreamGraph* aGraph,
> 
> Can we call this `NotifyOutputData` ?

Sure

Comment 79

[Randell Jesup [:jesup] (needinfo me)]

> >  void
> > +MediaStreamGraphImpl::OpenAudioInputImpl(void *aID, AudioDataListener *aListener)
> 
> I don't really like the void* in the signature, can we either use the cubeb
> type or typedef it to something so we move an opaque type around with more
> info on what it is ?

typedef cubeb_devid AudioDeviceID;
 
> @@ +965,5 @@
> >  }
> >  
> >  void
> >  MediaStreamGraphImpl::CloseAudioInputImpl(AudioDataListener *aListener)
> >  {
> 
> Add a threading assert on all the *Impl methods.

If you want I can (AssertOnGraphThreadOrNotRunning() probably) -- none of the (many) other ControlMessage BlahImpl()'s do it.

> ::: dom/media/webrtc/MediaEngineWebRTC.h
> @@ +188,5 @@
> > +            (mDevices->device[i]->state == CUBEB_DEVICE_STATE_ENABLED ||
> > +             mDevices->device[i]->state == CUBEB_DEVICE_STATE_UNPLUGGED))
> > +        {
> > +          mDeviceIndexes.AppendElement(i);
> > +          // XXX to support device changes, we need to identify by name/devid not index
> 
> I don't really get this translation thing. We have an order for

You cut off your comment ... but the issue was that webrtc.org code assumes you can identify a device reliably over time by index, even if the array changes.  Existing bug filed upstream.  With using devids, we can move away from it once we drop webrtc.org input support.

> @@ +256,5 @@
> >  
> >  private:
> > +  nsTArray<int> mDeviceIndexes;
> > +  int mSelectedDevice;
> > +  static cubeb_device_collection *mDevices;
> 
> How are we supporting plugging/unplugging devices ? It appears the
> notification callback is not implemented for some backends (at least
> WASAPI), we should fix this.

Lets file a follow-on bug for that.  Right now that's a regression; a simple fix for it would be to grab an updated list on each Enumerate request (though that has the same sort of hole around indexes changing as before).  Best would be to a) update it on actual plug/unplug events, and b) use unique id's to match up sources with devices (not indexes).
 
> Is this thread safe, how is it synchronized ?

Only accessed from the MediaManager thread.

Comment 80

[Randell Jesup [:jesup] (needinfo me)]

> ::: dom/media/GraphDriver.cpp
> @@ +69,2 @@
> >    MOZ_ASSERT(!PreviousDriver());
> > +  MOZ_ASSERT(aPreviousDriver);
> 
> Those two line are weird.

But correct :-)

> ::: dom/media/GraphDriver.h
> @@ +516,2 @@
> >    dom::AudioChannel mAudioChannel;
> > +  /* used to queue us to add the mixer callback on first run */
> 
> nit: capital and full stop.
> 
> ::: dom/media/MediaStreamGraph.cpp
> @@ +923,5 @@
> >  
> >  void
> >  MediaStreamGraphImpl::OpenAudioInputImpl(void *aID, AudioDataListener *aListener)
> >  {
> > + // Bug XXXXXX Need support for multiple mics at once
> 
> Make sure to put the bug number here.
> 
> @@ +966,5 @@
> >      }
> >      MediaStreamGraphImpl *mGraph;
> > +    // aID is a cubeb_devid, and we assume that opaque ptr is valid until
> > +    // we close cubeb.
> > +    void *mID;
> 
> I think it can be invalidated if we remove or add a device maybe ? Also
> void* here is not ideal.

Now a typedef to cubeb_devid. devid's are allocated; we might need to copy and destroy them when we move to updating mDevices (where they come from, and the real lifetime they're tied to).  We can handle that in the plug/unplug patch.

> @@ +1008,5 @@
> > +    } else {
> > +      STREAM_LOG(LogLevel::Debug, ("CloseInput: no output present (SystemClockCallback)"));
> > +
> > +      driver = new SystemClockDriver(this);
> > +      mMixer.RemoveCallback(CurrentDriver()->AsAudioCallbackDriver());
> 
> Can we put the RemoveCallback in the AudioCallbackDriver/SystemClockDriver
> as well? It would be better/safer I think.

Yes, I was thinking that as well.

Comment 81

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8705671 [details]
MozReview Request: Bug 1221587: Patch 2 - Win32 fixes for cubeb full-duplex patch

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

Comment 82

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8705672 [details]
MozReview Request: Bug 1221587: Patch 3 - Add base devid support to cubeb api r=padenot

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

Comment 83

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8705673 [details]
MozReview Request: Bug 1221587: patch 4 - Rename MediaStreamGraphShutdownThreadRunnable2 r=padenot

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

Comment 84

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8705674 [details]
MozReview Request: Bug 1221587: patch 5 - Base update of the MSG API for full-duplex r?padenot

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

Comment 85

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8705675 [details]
MozReview Request: Bug 1221587: patch 6 - allow getUserMedia to use full-duplex cubeb streams r?padenot

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

Comment 86

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8705676 [details]
MozReview Request: Bug 1221587: patch 7 - change listeners for full-duplex audio r?padenot

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

Comment 87

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8705677 [details]
MozReview Request: [mq]: audioinput

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

Comment 88

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8705678 [details]
MozReview Request: Bug 1221587: patch 8 - use cubeb devids to select input devices r?padenot

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

Comment 89

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8705679 [details]
MozReview Request: Bug 1221587: patch 9 - Implement switching of AudioCallbackDrivers for full-duplex r?padenot

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

Comment 90

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8705680 [details]
MozReview Request: imported patch switching_update

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

Comment 91

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8705681 [details]
MozReview Request: Bug 1221587: patch 10 - Improve logging of callback driver/switching r=padenot

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

Comment 92

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8705682 [details]
MozReview Request: Bug 1221587: patch 11 - Block attempts to open two mics at once until supported in full-duplex r=jib

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

Comment 93

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8705683 [details]
MozReview Request: Bug 1221587: add per-platform prefs to control full-duplex cubeb input r=jib

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

Comment 94

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8705684 [details]
MozReview Request: Bug 1237414: Switch AsyncCubebOperation to a SharedThreadPool

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

Comment 95

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8705685 [details]
MozReview Request: imported patch linux_pulse_import

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

Comment 96

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8705686 [details]
MozReview Request: imported patch cubeb-pulse-improvement

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

Comment 97

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8705687 [details]
MozReview Request: audiounit: Make full duplex work with gecko

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

Comment 98

[Randell Jesup [:jesup] (needinfo me)]

Attachment: MozReview Request: Bug 1237794: Extend ClearOnShutdown() to allow specifying the shutdown phase

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

Comment 99

[Randell Jesup [:jesup] (needinfo me)]

https://reviewboard.mozilla.org/r/30213/#review26935

Comment 100

[Randell Jesup [:jesup] (needinfo me)]

Ok, I can't get mozreview to believe me, but patch 9 is still r? to padenot

Comment 101

[Paul Adenot (:padenot)]

(In reply to Randell Jesup [:jesup] from comment #74)
> > @@ +898,5 @@
> > > +  if (aInputBuffer) {
> > > +    if (mAudioInput) { // for this specific input-only or full-duplex stream
> > > +      mAudioInput->NotifyInputData(mGraphImpl, aInputBuffer,
> > > +                                   static_cast<size_t>(aFrames),
> > > +                                   ChannelCount);
> > 
> > Typo: you want `ChannelCount()`
> 
> ChannelCount is a static int, not a method.

Wow, that's not great, we should rename it as kChannelCount or something.

Comment 102

[Paul Adenot (:padenot)]

Comment on attachment 8705677 [details]
MozReview Request: [mq]: audioinput

https://reviewboard.mozilla.org/r/30113/#review27007

::: dom/media/GraphDriver.h:236
(Diff revision 3)
> -  RefPtr<AudioDataListener> mAudioInput;
> +  AudioDataListener *mAudioInput;

You're re-removing the refcounting here ?

::: dom/media/MediaStreamGraph.h:1317
(Diff revision 3)
> +   */ 

nit: trailing space.

::: dom/media/webrtc/MediaEngineWebRTC.h:224
(Diff revision 3)
> +  virtual ~AudioInputCubeb()

Maybe use something like:

https://dxr.mozilla.org/mozilla-central/source/dom/media/AudioStream.h?from=CubebDestroyPolicy#23

and 

https://dxr.mozilla.org/mozilla-central/source/dom/media/AudioStream.h?from=CubebDestroyPolicy#295

So that it's cleaner? Or we can simply do it when we'll add the device change notification callback ?

At some point we should consider centralizing input and output devices management with some sort of device manager, for the platform level, so that we can reduce the number of notification and other threading issues, but we'll likely need off-main-thread MSG control messages first so we can notify the various graphs.

::: dom/media/webrtc/MediaEngineWebRTC.h:289
(Diff revision 3)
> +  virtual ~AudioInputWebRTC() {}

I thought it did not compile anymore when you had a public dtor for a refcounted class? Did it compile before ?

Comment 103

[Paul Adenot (:padenot)]

Comment on attachment 8705678 [details]
MozReview Request: Bug 1221587: patch 8 - use cubeb devids to select input devices r?padenot

https://reviewboard.mozilla.org/r/30115/#review27005

::: dom/media/GraphDriver.cpp:599
(Diff revision 3)
> -  in_params.channels = 1; // change to support optional stereo capture
> +  input.channels = 1; // change to support optional stereo capture

Would there be a way to access the cubeb_device data structure here so we can decide to open stereo mics ?

It will be necessary for output device selection as well, and would be better than to expose ad-hoc properties to the graph.

Ideally, the graph should know close to nothing about cubeb (for now, it just knows the prefered sample rate), devices, etc., it should just know how many channels it has to output to.

::: dom/media/MediaStreamGraph.h:44
(Diff revision 3)
> +typedef cubeb_devid AudioDeviceID;

Maybe put this in CubebUtils.h ?

Comment 104

[Paul Adenot (:padenot)]

Comment on attachment 8705679 [details]
MozReview Request: Bug 1221587: patch 9 - Implement switching of AudioCallbackDrivers for full-duplex r?padenot

https://reviewboard.mozilla.org/r/30117/#review27003

::: dom/media/GraphDriver.cpp:698
(Diff revision 3)
> +AudioCallbackDriver::RemoveCallback()

Can you make this one private and automatically called when switching off an AudioCallbackDriver ?

::: dom/media/MediaStreamGraph.cpp:931
(Diff revision 3)
> +    return;

What will happen here from the point of view of a JS author ? An exception or something ?

::: dom/media/MediaStreamGraph.cpp:1018
(Diff revision 3)
> -  // XXX So, so, so annoying.  Can't AppendMessage except on Mainthread
> +  // So, so, so annoying.  Can't AppendMessage except on Mainthread

Mind to file a bug on this ?

Comment 105

[Paul Adenot (:padenot)]

Comment on attachment 8705674 [details]
MozReview Request: Bug 1221587: patch 5 - Base update of the MSG API for full-duplex r?padenot

https://reviewboard.mozilla.org/r/30107/#review27261

I think this one is good, but I'm having trouble following the changes accross patches, can you provide a rollup next time so I can check the final status ?

Comment 106

[Paul Adenot (:padenot)]

Comment on attachment 8705675 [details]
MozReview Request: Bug 1221587: patch 6 - allow getUserMedia to use full-duplex cubeb streams r?padenot

https://reviewboard.mozilla.org/r/30109/#review26997

::: dom/media/webrtc/MediaEngineWebRTC.h:132
(Diff revisions 2 - 3)
> -  virtual int GetRecordingDeviceName(int index, char strNameUTF8[128],
> +  

nit: trailing space.

Comment 107

[Paul Adenot (:padenot)]

Comment on attachment 8705676 [details]
MozReview Request: Bug 1221587: patch 7 - change listeners for full-duplex audio r?padenot

https://reviewboard.mozilla.org/r/30111/#review27263

Comment 108

[Randell Jesup [:jesup] (needinfo me)]

Attachment: patch 14 - move AudioDataListener to refcounting and a separate allocation

Comment 109

[Randell Jesup [:jesup] (needinfo me)]

(In reply to Paul Adenot (:padenot) from comment #102)
> Comment on attachment 8705677 [details]
> MozReview Request: [mq]: audioinput
> 
> https://reviewboard.mozilla.org/r/30113/#review27007
> 
> ::: dom/media/GraphDriver.h:236
> (Diff revision 3)
> > -  RefPtr<AudioDataListener> mAudioInput;
> > +  AudioDataListener *mAudioInput;
> 
> You're re-removing the refcounting here ?

Not anymore :-)

> ::: dom/media/webrtc/MediaEngineWebRTC.h:224
> (Diff revision 3)
> > +  virtual ~AudioInputCubeb()
> 
> Maybe use something like:
> 
> https://dxr.mozilla.org/mozilla-central/source/dom/media/AudioStream.
> h?from=CubebDestroyPolicy#23
> 
> and 
> 
> https://dxr.mozilla.org/mozilla-central/source/dom/media/AudioStream.
> h?from=CubebDestroyPolicy#295
> 
> So that it's cleaner? Or we can simply do it when we'll add the device
> change notification callback ?

Lets do it then; it's a bit painful.

> 
> At some point we should consider centralizing input and output devices
> management with some sort of device manager, for the platform level, so that
> we can reduce the number of notification and other threading issues, but
> we'll likely need off-main-thread MSG control messages first so we can
> notify the various graphs.

Sure

> ::: dom/media/webrtc/MediaEngineWebRTC.h:289
> (Diff revision 3)
> > +  virtual ~AudioInputWebRTC() {}
> 
> I thought it did not compile anymore when you had a public dtor for a
> refcounted class? Did it compile before ?

Fixed in later patch in queue

Comment 110

[Randell Jesup [:jesup] (needinfo me)]

(In reply to Paul Adenot (:padenot) from comment #103)
> Comment on attachment 8705678 [details]
> MozReview Request: Bug 1221587: patch 8 - use cubeb devids to select input
> devices r?padenot
> 
> https://reviewboard.mozilla.org/r/30115/#review27005
> 
> ::: dom/media/GraphDriver.cpp:599
> (Diff revision 3)
> > -  in_params.channels = 1; // change to support optional stereo capture
> > +  input.channels = 1; // change to support optional stereo capture
> 
> Would there be a way to access the cubeb_device data structure here so we
> can decide to open stereo mics ?

Sure, but we need to add a constraint/control to choose if we want stereo at getUserMedia() time I think.
 
> It will be necessary for output device selection as well, and would be
> better than to expose ad-hoc properties to the graph.
> 
> Ideally, the graph should know close to nothing about cubeb (for now, it
> just knows the prefered sample rate), devices, etc., it should just know how
> many channels it has to output to.

Lets do these in one of the followups; either device change, or output selection.
 
> ::: dom/media/MediaStreamGraph.h:44
> (Diff revision 3)
> > +typedef cubeb_devid AudioDeviceID;
> 
> Maybe put this in CubebUtils.h ?

Ok.

Comment 111

[Randell Jesup [:jesup] (needinfo me)]

> ::: dom/media/GraphDriver.cpp:698
> (Diff revision 3)
> > +AudioCallbackDriver::RemoveCallback()
> 
> Can you make this one private and automatically called when switching off an
> AudioCallbackDriver ?

That's handled in a later patch in the queue.

> 
> ::: dom/media/MediaStreamGraph.cpp:931
> (Diff revision 3)
> > +    return;
> 
> What will happen here from the point of view of a JS author ? An exception
> or something ?

This is handled in patch 11 (Block multi-mic); they'll see an error (though not distinct from other unlikely error cases).

> ::: dom/media/MediaStreamGraph.cpp:1018
> (Diff revision 3)
> > -  // XXX So, so, so annoying.  Can't AppendMessage except on Mainthread
> > +  // So, so, so annoying.  Can't AppendMessage except on Mainthread
> 
> Mind to file a bug on this ?

Bug 1239135

Comment 112

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8705670 [details]
MozReview Request: imported patch cubeb_duplex_wip.patch

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

Comment 113

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8705671 [details]
MozReview Request: Bug 1221587: Patch 2 - Win32 fixes for cubeb full-duplex patch

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

Comment 114

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8705672 [details]
MozReview Request: Bug 1221587: Patch 3 - Add base devid support to cubeb api r=padenot

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

Comment 115

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8705673 [details]
MozReview Request: Bug 1221587: patch 4 - Rename MediaStreamGraphShutdownThreadRunnable2 r=padenot

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

Comment 116

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8705674 [details]
MozReview Request: Bug 1221587: patch 5 - Base update of the MSG API for full-duplex r?padenot

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

Comment 117

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8705675 [details]
MozReview Request: Bug 1221587: patch 6 - allow getUserMedia to use full-duplex cubeb streams r?padenot

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

Comment 118

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8705676 [details]
MozReview Request: Bug 1221587: patch 7 - change listeners for full-duplex audio r?padenot

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

Comment 119

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8705677 [details]
MozReview Request: [mq]: audioinput

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

Comment 120

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8705678 [details]
MozReview Request: Bug 1221587: patch 8 - use cubeb devids to select input devices r?padenot

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

Comment 121

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8705679 [details]
MozReview Request: Bug 1221587: patch 9 - Implement switching of AudioCallbackDrivers for full-duplex r?padenot

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

Comment 122

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8705680 [details]
MozReview Request: imported patch switching_update

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

Comment 123

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8705681 [details]
MozReview Request: Bug 1221587: patch 10 - Improve logging of callback driver/switching r=padenot

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

Comment 124

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8705682 [details]
MozReview Request: Bug 1221587: patch 11 - Block attempts to open two mics at once until supported in full-duplex r=jib

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

Comment 125

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8705683 [details]
MozReview Request: Bug 1221587: add per-platform prefs to control full-duplex cubeb input r=jib

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

Comment 126

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8705684 [details]
MozReview Request: Bug 1237414: Switch AsyncCubebOperation to a SharedThreadPool

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

Comment 127

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8705685 [details]
MozReview Request: imported patch linux_pulse_import

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

Comment 128

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8705686 [details]
MozReview Request: imported patch cubeb-pulse-improvement

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

Comment 129

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8705687 [details]
MozReview Request: audiounit: Make full duplex work with gecko

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

Comment 130

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8705847 [details]
MozReview Request: Bug 1237794: Extend ClearOnShutdown() to allow specifying the shutdown phase

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

Comment 131

[Randell Jesup [:jesup] (needinfo me)]

Attachment: MozReview Request: Bug 1221587: patch 13: Clear SharedThreadPool before shutdown-threads, and don't switch from SystemClockDriver to SystemClockDriver r=padenot

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

Comment 132

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8705682 [details]
MozReview Request: Bug 1221587: patch 11 - Block attempts to open two mics at once until supported in full-duplex r=jib

Silly mozreview.... will be fixing a bunch of these

Comment 133

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8705687 [details]
MozReview Request: audiounit: Make full duplex work with gecko

WTF did mozreview do here???

Comment 134

[Andreas Pehrson [:pehrsons]]

(In reply to Randell Jesup [:jesup] from comment #133)
> Comment on attachment 8705687 [details]
> MozReview Request: audiounit: Make full duplex work with gecko
> 
> WTF did mozreview do here???

Looks like mozreview interpreted a patch as based on a different patch in your previous revision. This happens if it cannot properly track the previous revision of the patch with evolve metadata (it falls back on the order of your patches, so if something is inserted in the middle that'll offset the patches after it by 1 - they'll all be mapped wrong, unless evolve metadata is present).

I've run into this a few times when using git (painful), and I imagine you might be running into it if you're still using MQ with these patches. I'm pretty sure MQ's workflow of applying/unapplying patches breaks evolve. You should use bookmarks, histedit, rebase, etc. for the best mozreview experience.

Comment 135

[Randell Jesup [:jesup] (needinfo me)]

(In reply to Andreas Pehrson [:pehrsons] (Telenor) from comment #134)
> I've run into this a few times when using git (painful), and I imagine you
> might be running into it if you're still using MQ with these patches. I'm
> pretty sure MQ's workflow of applying/unapplying patches breaks evolve. You
> should use bookmarks, histedit, rebase, etc. for the best mozreview
> experience.

Yeah, not surprising - but a much bigger change to my workflow.  mq + bzexport did a darn good job of figuring out which patch to obsolete; thus provably mozreview could do better (especially given a full set instead of a single one).  Even simple metadata matching (patch name!) would do tons better than what it does.

Comment 136

[Paul Adenot (:padenot)]

Comment on attachment 8706978 [details] [diff] [review]
patch 14 - move AudioDataListener to refcounting and a separate allocation

Review of attachment 8706978 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/media/webrtc/MediaEngineWebRTC.h
@@ +398,5 @@
>      MOZ_ASSERT(aAudioInput);
>      mDeviceName.Assign(NS_ConvertUTF8toUTF16(name));
>      mDeviceUUID.Assign(uuid);
> +    mListener = new mozilla::WebRTCAudioDataListener(this);
> +   Init();

nit: indentation.

Comment 137

[Paul Adenot (:padenot)]

Comment on attachment 8705677 [details]
MozReview Request: [mq]: audioinput

https://reviewboard.mozilla.org/r/30113/#review27491

::: dom/media/webrtc/MediaEngineWebRTC.h:141
(Diff revisions 3 - 4)
> +  NS_INLINE_DECL_THREADSAFE_REFCOUNTING(AudioInput)

s/had/add/ ?

Comment 138

[Paul Adenot (:padenot)]

Comment on attachment 8705678 [details]
MozReview Request: Bug 1221587: patch 8 - use cubeb devids to select input devices r?padenot

https://reviewboard.mozilla.org/r/30115/#review27493

Comment 139

[Paul Adenot (:padenot)]

Comment on attachment 8705679 [details]
MozReview Request: Bug 1221587: patch 9 - Implement switching of AudioCallbackDrivers for full-duplex r?padenot

https://reviewboard.mozilla.org/r/30117/#review27495

Comment 140

[Paul Adenot (:padenot)]

Comment on attachment 8705680 [details]
MozReview Request: imported patch switching_update

https://reviewboard.mozilla.org/r/30119/#review27517

Comment 141

[Randell Jesup [:jesup] (needinfo me)]

Attachment: Update for API changes in cubeb

Comment 142

[Randell Jesup [:jesup] (needinfo me)]

Attachment: break cycles in full_duplex gecko code to avoid leak

Comment 143

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ff93a27168c4
https://hg.mozilla.org/integration/mozilla-inbound/rev/3bd606775747
https://hg.mozilla.org/integration/mozilla-inbound/rev/1c3afb2f433b
https://hg.mozilla.org/integration/mozilla-inbound/rev/765fa97d2407
https://hg.mozilla.org/integration/mozilla-inbound/rev/8af4dd12d47c
https://hg.mozilla.org/integration/mozilla-inbound/rev/83f16bb75412
https://hg.mozilla.org/integration/mozilla-inbound/rev/d063509e90f7
https://hg.mozilla.org/integration/mozilla-inbound/rev/67833fc7a708
https://hg.mozilla.org/integration/mozilla-inbound/rev/4b22bd7b7a73
https://hg.mozilla.org/integration/mozilla-inbound/rev/d31752600dfd
https://hg.mozilla.org/integration/mozilla-inbound/rev/ff326b0cc099

Comment 144

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

I had to back this (and everything else from this push) out for causing various crashes on WinXP. They all seem to mention "winmm_stream_init" or "cubeb_winmm.c".

https://treeherder.mozilla.org/logviewer.html#?job_id=20232505&repo=mozilla-inbound
https://hg.mozilla.org/integration/mozilla-inbound/rev/780537ebaeef
https://hg.mozilla.org/integration/mozilla-inbound/rev/a813dc549ac4
https://hg.mozilla.org/integration/mozilla-inbound/rev/7e7888cd175d
https://hg.mozilla.org/integration/mozilla-inbound/rev/eeef1aa20f9f
https://hg.mozilla.org/integration/mozilla-inbound/rev/3bd1d3dc69b7
https://hg.mozilla.org/integration/mozilla-inbound/rev/ab0a30768b68
https://hg.mozilla.org/integration/mozilla-inbound/rev/e03ee3abc555
https://hg.mozilla.org/integration/mozilla-inbound/rev/fd77d9587c81
https://hg.mozilla.org/integration/mozilla-inbound/rev/ababf79d0994
https://hg.mozilla.org/integration/mozilla-inbound/rev/d4ad9ac49be5
https://hg.mozilla.org/integration/mozilla-inbound/rev/97de89a62b97

Comment 145

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/93f83978cac6
https://hg.mozilla.org/integration/mozilla-inbound/rev/2d18cbea1800
https://hg.mozilla.org/integration/mozilla-inbound/rev/269a82e6f643
https://hg.mozilla.org/integration/mozilla-inbound/rev/6766d2a6b5a9
https://hg.mozilla.org/integration/mozilla-inbound/rev/cf179eb7e195
https://hg.mozilla.org/integration/mozilla-inbound/rev/ef761a32a969
https://hg.mozilla.org/integration/mozilla-inbound/rev/509a5c5e3677
https://hg.mozilla.org/integration/mozilla-inbound/rev/f57bb0eed773
https://hg.mozilla.org/integration/mozilla-inbound/rev/86eee3f50995
https://hg.mozilla.org/integration/mozilla-inbound/rev/e97c3cf17660

Comment 146

[Randell Jesup [:jesup] (needinfo me)]

Attachment: stall MSG final shutdown until AudioCallbackDriver shutdown has finished

this should fix an intermittent orange on Android/B2G in shutdown (crash in PR_Lock due to a null lock)

Comment 147

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8710895 [details] [diff] [review]
stall MSG final shutdown until AudioCallbackDriver shutdown has finished

Review of attachment 8710895 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/media/MediaStreamGraph.cpp
@@ +1440,5 @@
>        MOZ_ASSERT(!mGraph->mDriver->AsAudioCallbackDriver()->InCallback());
>      }
>  #endif
>  
> +    mGraph->mDriver->Shutdown(true); // wait until it's shut down since

This looks like the only caller. Can you skip the anonymous bool and just document why we're shutting down sync in GraphDriver?

Comment 148

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/444ae0c94211

Comment 149

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/93f83978cac6
https://hg.mozilla.org/mozilla-central/rev/2d18cbea1800
https://hg.mozilla.org/mozilla-central/rev/269a82e6f643
https://hg.mozilla.org/mozilla-central/rev/6766d2a6b5a9
https://hg.mozilla.org/mozilla-central/rev/cf179eb7e195
https://hg.mozilla.org/mozilla-central/rev/ef761a32a969
https://hg.mozilla.org/mozilla-central/rev/509a5c5e3677
https://hg.mozilla.org/mozilla-central/rev/f57bb0eed773
https://hg.mozilla.org/mozilla-central/rev/86eee3f50995
https://hg.mozilla.org/mozilla-central/rev/e97c3cf17660

Comment 150

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/444ae0c94211

Comment 151

[Paul Adenot (:padenot)]

Comment on attachment 8705671 [details]
MozReview Request: Bug 1221587: Patch 2 - Win32 fixes for cubeb full-duplex patch

https://reviewboard.mozilla.org/r/30101/#review29151

(clearing review request, this has landed)