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?

Makes sense indeed.

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)

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

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

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.

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

* * * 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/

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

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

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

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

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 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 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 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 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 on attachment 8705677 [details] MozReview Request: [mq]: audioinput Review request updated; see interdiff: https://reviewboard.mozilla.org/r/30113/diff/1-2/

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 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 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 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 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 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 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 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 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 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 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 on attachment 8705673 [details] MozReview Request: Bug 1221587: patch 4 - Rename MediaStreamGraphShutdownThreadRunnable2 r=padenot https://reviewboard.mozilla.org/r/30105/#review26859

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 on attachment 8705680 [details] MozReview Request: imported patch switching_update https://reviewboard.mozilla.org/r/30119/#review26867

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 on attachment 8705687 [details] MozReview Request: audiounit: Make full duplex work with gecko https://reviewboard.mozilla.org/r/30133/#review26871

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

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

Comment on attachment 8705680 [details] MozReview Request: imported patch switching_update Already reviewed this. See comment 25 for nits which still should be addressed.

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

(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

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

> ::: 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 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 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 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 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 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 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 on attachment 8705677 [details] MozReview Request: [mq]: audioinput Review request updated; see interdiff: https://reviewboard.mozilla.org/r/30113/diff/2-3/

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 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 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 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 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 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 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 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 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 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/

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

Ok, I can't get mozreview to believe me, but patch 9 is still r? to 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 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 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 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 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 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 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

(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

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

> ::: 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 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 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 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 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 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 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 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 on attachment 8705677 [details] MozReview Request: [mq]: audioinput Review request updated; see interdiff: https://reviewboard.mozilla.org/r/30113/diff/3-4/

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 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 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 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 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 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 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 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 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 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 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/

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

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 on attachment 8705687 [details] MozReview Request: audiounit: Make full duplex work with gecko WTF did mozreview do here???

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

(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 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 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/ ?