Teach Gecko how to use full-duplex cubeb streams

RESOLVED FIXED in Firefox 46

Status

()

Core
WebRTC: Audio/Video
P1
normal
Rank:
7
RESOLVED FIXED
2 years ago
6 months ago

People

(Reporter: padenot, Assigned: jesup)

Tracking

(Depends on: 2 bugs)

unspecified
mozilla46
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox46 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(24 attachments, 16 obsolete attachments)

58 bytes, text/x-review-board-request
Details | Review
58 bytes, text/x-review-board-request
Details | Review
58 bytes, text/x-review-board-request
jesup
: review+
Details | Review
58 bytes, text/x-review-board-request
jesup
: review+
Details | Review
58 bytes, text/x-review-board-request
padenot
: review+
Details | Review
58 bytes, text/x-review-board-request
padenot
: review+
Details | Review
58 bytes, text/x-review-board-request
padenot
: review+
Details | Review
58 bytes, text/x-review-board-request
padenot
: review+
Details | Review
58 bytes, text/x-review-board-request
padenot
: review+
Details | Review
58 bytes, text/x-review-board-request
padenot
: review+
Details | Review
58 bytes, text/x-review-board-request
padenot
: review+
Details | Review
58 bytes, text/x-review-board-request
jesup
: review+
Details | Review
58 bytes, text/x-review-board-request
jesup
: review+
Details | Review
58 bytes, text/x-review-board-request
jesup
: review+
Details | Review
58 bytes, text/x-review-board-request
jesup
: review+
Details | Review
58 bytes, text/x-review-board-request
Details | Review
58 bytes, text/x-review-board-request
Details | Review
58 bytes, text/x-review-board-request
Details | Review
58 bytes, text/x-review-board-request
Details | Review
11.45 KB, patch
padenot
: review+
Details | Diff | Splinter Review
58 bytes, text/x-review-board-request
jesup
: review+
Details | Review
12.84 KB, patch
padenot
: review+
Details | Diff | Splinter Review
994 bytes, patch
padenot
: review+
Details | Diff | Splinter Review
3.69 KB, patch
pehrsons
: review+
Details | Diff | Splinter Review
(Reporter)

Description

2 years ago
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
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?
Assignee: nobody → padenot
backlog: --- → webrtc/webaudio+
Rank: 15
Priority: -- → P1
(Reporter)

Comment 2

2 years ago
Makes sense indeed.
(Assignee)

Comment 3

2 years ago
Created attachment 8695122 [details] [diff] [review]
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)
(Assignee)

Updated

2 years ago
Assignee: padenot → rjesup
Status: NEW → ASSIGNED
(Assignee)

Comment 4

2 years ago
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
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
Created attachment 8697918 [details] [diff] [review]
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).
Created attachment 8697919 [details] [diff] [review]
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.
Created attachment 8697920 [details] [diff] [review]
Rename MediaStreamGraphShutdownThreadRunnable2 to MediaStreamGraphShutdownThreadRunnable now that it's available

This is not really needed, but for cleanliness' sake.
Attachment #8697920 - Attachment is patch: true
Rank: 15 → 10
(Assignee)

Updated

2 years ago
Rank: 10 → 7
(Assignee)

Comment 11

2 years ago
Created attachment 8704919 [details] [diff] [review]
patch 12 - add per-platform prefs to control full-duplex cubeb input
Attachment #8704919 - Flags: review?(jib)
(Assignee)

Comment 12

2 years ago
Created attachment 8705010 [details] [diff] [review]
patch 11 - Block attempts to open two mics at once until supported in full-duplex
Attachment #8705010 - Flags: review?(jib)
(Assignee)

Comment 13

2 years ago
Created attachment 8705064 [details] [diff] [review]
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
Attachment #8705064 - Flags: review?(padenot)
(Assignee)

Comment 14

2 years ago
Created attachment 8705065 [details] [diff] [review]
Patch 3 - Add base devid support to cubeb api

I suspect we may need to fix the tests before landing this
Attachment #8705065 - Flags: review?(padenot)
(Assignee)

Comment 15

2 years ago
Created attachment 8705068 [details] [diff] [review]
patch 4 - Rename MediaStreamGraphShutdownThreadRunnable2
Attachment #8705068 - Flags: review?(padenot)
(Assignee)

Comment 16

2 years ago
Created attachment 8705070 [details] [diff] [review]
patch 5 - Base update of the MSG API for full-duplex
Attachment #8705070 - Flags: review?(padenot)
(Assignee)

Comment 17

2 years ago
Created attachment 8705071 [details] [diff] [review]
patch 6 - allow getUserMedia to use full-duplex cubeb streams
Attachment #8705071 - Flags: review?(padenot)
(Assignee)

Comment 18

2 years ago
Created attachment 8705072 [details] [diff] [review]
patch 7 - change listeners for full-duplex audio
Attachment #8705072 - Flags: review?(padenot)
(Assignee)

Comment 19

2 years ago
Created attachment 8705073 [details] [diff] [review]
patch 8 - use cubeb devids to select input devices
Attachment #8705073 - Flags: review?(padenot)
(Assignee)

Comment 20

2 years ago
Created attachment 8705075 [details] [diff] [review]
patch 9 - Implement switching of AudioCallbackDrivers for full-duplex
Attachment #8705075 - Flags: review?(padenot)
(Assignee)

Comment 21

2 years ago
Created attachment 8705076 [details] [diff] [review]
patch 10 - Improve logging of callback driver/switching
Attachment #8705076 - Flags: review?(padenot)
(Assignee)

Updated

2 years ago
Attachment #8705010 - Attachment description: Block attempts to open two mics at once until supported in full-duplex → patch 11 - Block attempts to open two mics at once until supported in full-duplex
(Assignee)

Updated

2 years ago
Attachment #8704919 - Attachment description: add per-platform prefs to control full-duplex cubeb input → patch 12 - add per-platform prefs to control full-duplex cubeb input
(Assignee)

Updated

2 years ago
Attachment #8697920 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Attachment #8695122 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Attachment #8697918 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Attachment #8697919 - Attachment is obsolete: true
(Reporter)

Comment 22

2 years ago
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 ?
Attachment #8705065 - Flags: review?(padenot) → review+
(Reporter)

Updated

2 years ago
Attachment #8705068 - Flags: review?(padenot) → review+
(Reporter)

Comment 23

2 years ago
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.
Attachment #8705076 - Flags: review?(padenot) → review+
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?
Attachment #8704919 - Flags: review?(jib) → review+
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.
Attachment #8705010 - Flags: review?(jib) → review+
(Assignee)

Comment 26

2 years ago
Created attachment 8705372 [details] [diff] [review]
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
Attachment #8705372 - Flags: review?(padenot)
(Assignee)

Updated

2 years ago
Depends on: 1237794
(Reporter)

Comment 27

2 years ago
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?
Attachment #8705070 - Flags: review?(padenot)
(Reporter)

Comment 28

2 years ago
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.
Attachment #8705071 - Flags: review?(padenot)
(Reporter)

Comment 29

2 years ago
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.
Attachment #8705072 - Flags: review?(padenot)
(Reporter)

Comment 30

2 years ago
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 ?
Attachment #8705073 - Flags: review?(padenot)
(Reporter)

Comment 31

2 years ago
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.
Attachment #8705075 - Flags: review?(padenot)
(Reporter)

Updated

2 years ago
Attachment #8705372 - Flags: review?(padenot) → review+
(Reporter)

Comment 32

2 years ago
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.
Attachment #8705064 - Flags: review?(padenot)
(Assignee)

Comment 33

2 years ago
Created attachment 8705670 [details]
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/
(Assignee)

Comment 34

2 years ago
Created attachment 8705671 [details]
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/
(Assignee)

Comment 35

2 years ago
Created attachment 8705672 [details]
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/
(Assignee)

Comment 36

2 years ago
Created attachment 8705673 [details]
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/
(Assignee)

Comment 37

2 years ago
Created attachment 8705674 [details]
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/
(Assignee)

Comment 38

2 years ago
Created attachment 8705675 [details]
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/
(Assignee)

Comment 39

2 years ago
Created attachment 8705676 [details]
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/
(Assignee)

Comment 40

2 years ago
Created attachment 8705677 [details]
MozReview Request: [mq]: audioinput

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

Comment 41

2 years ago
Created attachment 8705678 [details]
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/
(Assignee)

Comment 42

2 years ago
Created attachment 8705679 [details]
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/
(Assignee)

Comment 43

2 years ago
Created attachment 8705680 [details]
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/
(Assignee)

Comment 44

2 years ago
Created attachment 8705681 [details]
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/
(Assignee)

Comment 45

2 years ago
Created 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 commit: https://reviewboard.mozilla.org/r/30123/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/30123/
(Assignee)

Comment 46

2 years ago
Created attachment 8705683 [details]
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/
(Assignee)

Comment 47

2 years ago
Created attachment 8705684 [details]
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/
(Assignee)

Comment 48

2 years ago
Created attachment 8705685 [details]
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/
(Assignee)

Comment 49

2 years ago
Created attachment 8705686 [details]
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/
(Assignee)

Comment 50

2 years ago
Created attachment 8705687 [details]
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/
(Assignee)

Updated

2 years ago
Attachment #8705672 - Flags: review?(padenot)
(Assignee)

Updated

2 years ago
Attachment #8705673 - Flags: review?(padenot)
(Assignee)

Updated

2 years ago
Attachment #8705674 - Flags: review?(padenot)
(Assignee)

Updated

2 years ago
Attachment #8705675 - Flags: review?(padenot)
(Assignee)

Updated

2 years ago
Attachment #8705676 - Flags: review?(padenot)
(Assignee)

Updated

2 years ago
Attachment #8705677 - Flags: review?(padenot)
(Assignee)

Updated

2 years ago
Attachment #8705678 - Flags: review?(padenot)
(Assignee)

Updated

2 years ago
Attachment #8705679 - Flags: review?(padenot)
(Assignee)

Updated

2 years ago
Attachment #8705680 - Flags: review?(jib)
(Assignee)

Updated

2 years ago
Attachment #8705681 - Flags: review?(jib)
(Assignee)

Updated

2 years ago
Attachment #8705687 - Flags: review?(padenot)
(Assignee)

Updated

2 years ago
Attachment #8705672 - Attachment description: MozReview Request: Bug 1221587: Patch 3 - Add base devid support to cubeb api → MozReview Request: Bug 1221587: Patch 3 - Add base devid support to cubeb api r=padenot
(Assignee)

Comment 51

2 years ago
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/
(Assignee)

Comment 52

2 years ago
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/
Attachment #8705673 - Attachment description: MozReview Request: Bug 1221587: patch 4 - Rename MediaStreamGraphShutdownThreadRunnable2 → MozReview Request: Bug 1221587: patch 4 - Rename MediaStreamGraphShutdownThreadRunnable2 r=padenot
(Assignee)

Comment 53

2 years ago
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/
Attachment #8705674 - Attachment description: MozReview Request: Bug 1221587: patch 5 - Base update of the MSG API for full-duplex → MozReview Request: Bug 1221587: patch 5 - Base update of the MSG API for full-duplex r?padenot
(Assignee)

Updated

2 years ago
Attachment #8705675 - Attachment description: MozReview Request: Bug 1221587: patch 6 - allow getUserMedia to use full-duplex cubeb streams → MozReview Request: Bug 1221587: patch 6 - allow getUserMedia to use full-duplex cubeb streams r?padenot
(Assignee)

Comment 54

2 years ago
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/
(Assignee)

Comment 55

2 years ago
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/
Attachment #8705676 - Attachment description: MozReview Request: Bug 1221587: patch 7 - change listeners for full-duplex audio → MozReview Request: Bug 1221587: patch 7 - change listeners for full-duplex audio r?padenot
(Assignee)

Comment 56

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/30113/diff/1-2/
Attachment #8705677 - Attachment description: MozReview Request: Bug 1221587: patch 8 - use cubeb devids to select input devices → MozReview Request: Bug 1221587: patch 8 - use cubeb devids to select input devices r?padenot
(Assignee)

Comment 57

2 years ago
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/
Attachment #8705678 - Attachment description: MozReview Request: Bug 1221587: patch 9 - Implement switching of AudioCallbackDrivers for full-duplex → MozReview Request: Bug 1221587: patch 9 - Implement switching of AudioCallbackDrivers for full-duplex r?padenot
(Assignee)

Comment 58

2 years ago
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/
Attachment #8705679 - Attachment description: MozReview Request: Bug 1221587: patch 10 - Improve logging of callback driver/switching → MozReview Request: Bug 1221587: patch 10 - Improve logging of callback driver/switching r=padenot
(Assignee)

Comment 59

2 years ago
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/
Attachment #8705680 - Attachment description: MozReview Request: Bug 1221587: patch 11 - Block attempts to open two mics at once until supported in full-duplex → MozReview Request: Bug 1221587: patch 11 - Block attempts to open two mics at once until supported in full-duplex r=jib
(Assignee)

Comment 60

2 years ago
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/
Attachment #8705681 - Attachment description: MozReview Request: Bug 1221587: add per-platform prefs to control full-duplex cubeb input → MozReview Request: Bug 1221587: add per-platform prefs to control full-duplex cubeb input r=jib
(Assignee)

Comment 61

2 years ago
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/
(Assignee)

Comment 62

2 years ago
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/
(Assignee)

Comment 63

2 years ago
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/
(Assignee)

Comment 64

2 years ago
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/
(Assignee)

Comment 65

2 years ago
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/
(Assignee)

Comment 66

2 years ago
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/
Attachment #8705687 - Attachment description: MozReview Request: Bug 1221587: patch 12: Clear SharedThreadPool before shutdown-threads, and don't switch from SystemClockDriver to SystemClockDriver → MozReview Request: Bug 1221587: patch 13: Clear SharedThreadPool before shutdown-threads, and don't switch from SystemClockDriver to SystemClockDriver r=padenot
(Assignee)

Comment 67

2 years ago
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
Attachment #8705672 - Flags: review+
(Assignee)

Comment 68

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

https://reviewboard.mozilla.org/r/30105/#review26859
Attachment #8705673 - Flags: review+
(Assignee)

Comment 69

2 years ago
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
Attachment #8705679 - Flags: review+
(Assignee)

Updated

2 years ago
Attachment #8705680 - Flags: review+
(Assignee)

Comment 70

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

https://reviewboard.mozilla.org/r/30119/#review26867
(Assignee)

Updated

2 years ago
Attachment #8705681 - Flags: review+
(Assignee)

Comment 71

2 years ago
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
(Assignee)

Comment 72

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

https://reviewboard.mozilla.org/r/30133/#review26871
Attachment #8705687 - Flags: review+
(Assignee)

Updated

2 years ago
Depends on: 1238038
(Assignee)

Comment 73

2 years ago
> @@ +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.
(Assignee)

Comment 74

2 years ago
> @@ +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 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.
Attachment #8705681 - Flags: review?(jib) → review+
Comment on attachment 8705680 [details]
MozReview Request: imported patch switching_update

Already reviewed this. See comment 25 for nits which still should be addressed.
Attachment #8705680 - Flags: review?(jib) → review+
(Assignee)

Comment 77

2 years ago
(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.
(Assignee)

Comment 78

2 years ago
(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
(Assignee)

Comment 79

2 years ago
> >  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.
(Assignee)

Comment 80

2 years ago
> ::: 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.
(Assignee)

Updated

2 years ago
Attachment #8705672 - Flags: review?(padenot)
(Assignee)

Updated

2 years ago
Attachment #8705673 - Flags: review?(padenot)
(Assignee)

Comment 81

2 years ago
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/
(Assignee)

Comment 82

2 years ago
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/
Attachment #8705672 - Flags: review?(padenot)
(Assignee)

Updated

2 years ago
Attachment #8705673 - Flags: review?(padenot)
(Assignee)

Comment 83

2 years ago
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/
(Assignee)

Comment 84

2 years ago
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/
(Assignee)

Comment 85

2 years ago
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/
(Assignee)

Comment 86

2 years ago
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/
(Assignee)

Comment 87

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/30113/diff/2-3/
Attachment #8705677 - Attachment description: MozReview Request: Bug 1221587: patch 8 - use cubeb devids to select input devices r?padenot → MozReview Request: [mq]: audioinput
(Assignee)

Comment 88

2 years ago
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/
Attachment #8705678 - Attachment description: MozReview Request: Bug 1221587: patch 9 - Implement switching of AudioCallbackDrivers for full-duplex r?padenot → MozReview Request: Bug 1221587: patch 8 - use cubeb devids to select input devices r?padenot
(Assignee)

Comment 89

2 years ago
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/
Attachment #8705679 - Attachment description: MozReview Request: Bug 1221587: patch 10 - Improve logging of callback driver/switching r=padenot → MozReview Request: Bug 1221587: patch 9 - Implement switching of AudioCallbackDrivers for full-duplex r?padenot
(Assignee)

Comment 90

2 years ago
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/
Attachment #8705680 - Attachment description: MozReview Request: Bug 1221587: patch 11 - Block attempts to open two mics at once until supported in full-duplex r=jib → MozReview Request: Bug 1221587: patch 10 - Improve logging of callback driver/switching r=padenot
Attachment #8705680 - Flags: review+ → review?(padenot)
(Assignee)

Comment 91

2 years ago
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/
Attachment #8705681 - Attachment description: MozReview Request: Bug 1221587: add per-platform prefs to control full-duplex cubeb input r=jib → MozReview Request: Bug 1221587: patch 11 - Block attempts to open two mics at once until supported in full-duplex r=jib
Attachment #8705681 - Flags: review+ → review?(jib)
(Assignee)

Updated

2 years ago
Attachment #8705682 - Attachment description: MozReview Request: Bug 1237414: Switch AsyncCubebOperation to a SharedThreadPool → MozReview Request: Bug 1221587: add per-platform prefs to control full-duplex cubeb input r=jib
Attachment #8705682 - Flags: review?(jib)
(Assignee)

Comment 92

2 years ago
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/
(Assignee)

Updated

2 years ago
Attachment #8705683 - Attachment description: MozReview Request: imported patch linux_pulse_import → MozReview Request: Bug 1237414: Switch AsyncCubebOperation to a SharedThreadPool
(Assignee)

Comment 93

2 years ago
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/
(Assignee)

Updated

2 years ago
Attachment #8705684 - Attachment description: MozReview Request: imported patch cubeb-pulse-improvement → MozReview Request: imported patch linux_pulse_import
(Assignee)

Comment 94

2 years ago
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/
(Assignee)

Comment 95

2 years ago
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/
Attachment #8705685 - Attachment description: MozReview Request: audiounit: Make full duplex work with gecko → MozReview Request: imported patch cubeb-pulse-improvement
(Assignee)

Comment 96

2 years ago
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/
Attachment #8705686 - Attachment description: MozReview Request: Bug 1237794: Extend ClearOnShutdown() to allow specifying the shutdown phase → MozReview Request: audiounit: Make full duplex work with gecko
(Assignee)

Comment 97

2 years ago
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/
Attachment #8705687 - Attachment description: MozReview Request: Bug 1221587: patch 13: Clear SharedThreadPool before shutdown-threads, and don't switch from SystemClockDriver to SystemClockDriver r=padenot → MozReview Request: Bug 1237794: Extend ClearOnShutdown() to allow specifying the shutdown phase
(Assignee)

Comment 98

2 years ago
Created attachment 8705847 [details]
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/
Attachment #8705847 - Flags: review?(padenot)
(Assignee)

Updated

2 years ago
Attachment #8705672 - Flags: review?(padenot)
(Assignee)

Updated

2 years ago
Attachment #8705673 - Flags: review?(padenot)
(Assignee)

Updated

2 years ago
Attachment #8705679 - Flags: review?(padenot)
(Assignee)

Updated

2 years ago
Attachment #8705680 - Flags: review?(padenot)
(Assignee)

Updated

2 years ago
Attachment #8705681 - Flags: review?(jib)
(Assignee)

Updated

2 years ago
Attachment #8705682 - Flags: review?(jib) → review+
(Assignee)

Updated

2 years ago
Attachment #8705687 - Flags: review?(padenot)
(Assignee)

Updated

2 years ago
Attachment #8705847 - Flags: review?(padenot) → review+
(Assignee)

Updated

2 years ago
Attachment #8705679 - Flags: review+ → review?(padenot)
(Assignee)

Comment 100

2 years ago
Ok, I can't get mozreview to believe me, but patch 9 is still r? to padenot
(Reporter)

Comment 101

2 years ago
(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.
(Reporter)

Comment 102

2 years ago
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 ?
Attachment #8705677 - Flags: review?(padenot)
(Reporter)

Comment 103

2 years ago
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 ?
Attachment #8705678 - Flags: review?(padenot)
(Reporter)

Updated

2 years ago
Attachment #8705679 - Flags: review?(padenot)
(Reporter)

Comment 104

2 years ago
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 ?
(Reporter)

Comment 105

2 years ago
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 ?
Attachment #8705674 - Flags: review?(padenot) → review+
(Reporter)

Comment 106

2 years ago
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.
Attachment #8705675 - Flags: review?(padenot) → review+
(Reporter)

Comment 107

2 years ago
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
Attachment #8705676 - Flags: review?(padenot) → review+
(Assignee)

Comment 108

2 years ago
Created attachment 8706978 [details] [diff] [review]
patch 14 - move AudioDataListener to refcounting and a separate allocation
Attachment #8706978 - Flags: review?(padenot)
(Assignee)

Comment 109

2 years ago
(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
(Assignee)

Comment 110

2 years ago
(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.
(Assignee)

Comment 111

2 years ago
> ::: 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
(Assignee)

Comment 112

2 years ago
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/
(Assignee)

Comment 113

2 years ago
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/
(Assignee)

Comment 114

2 years ago
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/
Attachment #8705672 - Flags: review?(padenot)
(Assignee)

Comment 115

2 years ago
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/
Attachment #8705673 - Flags: review?(padenot)
(Assignee)

Comment 116

2 years ago
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/
(Assignee)

Comment 117

2 years ago
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/
(Assignee)

Comment 118

2 years ago
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/
(Assignee)

Updated

2 years ago
Attachment #8705677 - Flags: review?(padenot)
(Assignee)

Comment 119

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

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

Comment 120

2 years ago
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/
Attachment #8705678 - Flags: review?(padenot)
(Assignee)

Comment 121

2 years ago
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/
Attachment #8705679 - Flags: review?(rjesup)
Attachment #8705679 - Flags: review?(padenot)
(Assignee)

Comment 122

2 years ago
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/
Attachment #8705680 - Attachment description: MozReview Request: Bug 1221587: patch 10 - Improve logging of callback driver/switching r=padenot → MozReview Request: imported patch switching_update
Attachment #8705680 - Flags: review?(padenot)
(Assignee)

Updated

2 years ago
Attachment #8705681 - Attachment description: MozReview Request: Bug 1221587: patch 11 - Block attempts to open two mics at once until supported in full-duplex r=jib → MozReview Request: Bug 1221587: patch 10 - Improve logging of callback driver/switching r=padenot
Attachment #8705681 - Flags: review?(padenot)
(Assignee)

Comment 123

2 years ago
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/
(Assignee)

Comment 124

2 years ago
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/
Attachment #8705682 - Attachment description: MozReview Request: Bug 1221587: add per-platform prefs to control full-duplex cubeb input r=jib → MozReview Request: Bug 1221587: patch 11 - Block attempts to open two mics at once until supported in full-duplex r=jib
Attachment #8705682 - Flags: review+ → review?(jib)
(Assignee)

Comment 125

2 years ago
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/
Attachment #8705683 - Attachment description: MozReview Request: Bug 1237414: Switch AsyncCubebOperation to a SharedThreadPool → MozReview Request: Bug 1221587: add per-platform prefs to control full-duplex cubeb input r=jib
Attachment #8705683 - Flags: review?(jib)
(Assignee)

Comment 126

2 years ago
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/
Attachment #8705684 - Attachment description: MozReview Request: imported patch linux_pulse_import → MozReview Request: Bug 1237414: Switch AsyncCubebOperation to a SharedThreadPool
(Assignee)

Comment 127

2 years ago
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/
Attachment #8705685 - Attachment description: MozReview Request: imported patch cubeb-pulse-improvement → MozReview Request: imported patch linux_pulse_import
(Assignee)

Comment 128

2 years ago
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/
Attachment #8705686 - Attachment description: MozReview Request: audiounit: Make full duplex work with gecko → MozReview Request: imported patch cubeb-pulse-improvement
(Assignee)

Comment 129

2 years ago
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/
Attachment #8705687 - Attachment description: MozReview Request: Bug 1237794: Extend ClearOnShutdown() to allow specifying the shutdown phase → MozReview Request: audiounit: Make full duplex work with gecko
Attachment #8705687 - Flags: review?(padenot)
(Assignee)

Updated

2 years ago
Attachment #8705847 - Attachment description: MozReview Request: Bug 1221587: patch 13: Clear SharedThreadPool before shutdown-threads, and don't switch from SystemClockDriver to SystemClockDriver r=padenot → MozReview Request: Bug 1237794: Extend ClearOnShutdown() to allow specifying the shutdown phase
Attachment #8705847 - Flags: review?(padenot)
(Assignee)

Comment 130

2 years ago
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/
(Assignee)

Comment 131

2 years ago
Created attachment 8707308 [details]
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/
Attachment #8707308 - Flags: review?(padenot)
(Assignee)

Comment 132

2 years ago
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
Attachment #8705682 - Flags: review?(jib) → review+
(Assignee)

Updated

2 years ago
Attachment #8704919 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Attachment #8705010 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Attachment #8705064 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Attachment #8705065 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Attachment #8705068 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Attachment #8705070 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Attachment #8705071 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Attachment #8705072 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Attachment #8705073 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Attachment #8705075 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Attachment #8705076 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Attachment #8705372 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Attachment #8705672 - Flags: review?(padenot)
(Assignee)

Updated

2 years ago
Attachment #8705673 - Flags: review?(padenot)
(Assignee)

Updated

2 years ago
Attachment #8705679 - Flags: review?(rjesup)
(Assignee)

Updated

2 years ago
Attachment #8705680 - Flags: review+
(Assignee)

Updated

2 years ago
Attachment #8705681 - Flags: review?(padenot)
(Assignee)

Updated

2 years ago
Attachment #8705683 - Flags: review?(jib) → review+
(Assignee)

Comment 133

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

WTF did mozreview do here???
Attachment #8705687 - Flags: review?(padenot)
Attachment #8705687 - Flags: review+
(Assignee)

Updated

2 years ago
Attachment #8705847 - Flags: review?(padenot)
Attachment #8705847 - Flags: review+
(Assignee)

Updated

2 years ago
Attachment #8707308 - Flags: review?(padenot) → review+
(Assignee)

Updated

2 years ago
Attachment #8705684 - Flags: review+
(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.
(Assignee)

Comment 135

2 years ago
(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.
(Reporter)

Comment 136

2 years ago
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.
Attachment #8706978 - Flags: review?(padenot) → review+
(Reporter)

Comment 137

2 years ago
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/ ?
Attachment #8705677 - Flags: review?(padenot) → review+
(Reporter)

Comment 138

2 years ago
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
Attachment #8705678 - Flags: review?(padenot) → review+
(Reporter)

Comment 139

2 years ago
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
Attachment #8705679 - Flags: review?(padenot) → review+
(Reporter)

Comment 140

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

https://reviewboard.mozilla.org/r/30119/#review27517
Attachment #8705680 - Flags: review?(padenot) → review+
(Assignee)

Comment 141

2 years ago
Created attachment 8710395 [details] [diff] [review]
Update for API changes in cubeb
Attachment #8710395 - Flags: review?(padenot)
(Assignee)

Comment 142

2 years ago
Created attachment 8710396 [details] [diff] [review]
break cycles in full_duplex gecko code to avoid leak
Attachment #8710396 - Flags: review?(padenot)
(Reporter)

Updated

2 years ago
Attachment #8710395 - Flags: review?(padenot) → review+
(Reporter)

Updated

2 years ago
Attachment #8710396 - Flags: review?(padenot) → review+
(Assignee)

Updated

2 years ago
Attachment #8705671 - Flags: review?(padenot)
(Assignee)

Updated

2 years ago
Depends on: 1241476
(Assignee)

Comment 146

2 years ago
Created attachment 8710895 [details] [diff] [review]
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)
Attachment #8710895 - Flags: review?(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?
Attachment #8710895 - Flags: review?(pehrsons) → review+
(Assignee)

Updated

2 years ago
Blocks: 1242061
Depends on: 1241628
(Reporter)

Comment 151

2 years ago
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)
Attachment #8705671 - Flags: review?(padenot)
(Assignee)

Updated

2 years ago
Depends on: 1258894
Depends on: 1302231
(Assignee)

Updated

10 months ago
Depends on: 1360334
(Assignee)

Updated

6 months ago
Depends on: 1389941
You need to log in before you can comment on or make changes to this bug.