From a1777772d75b4992c5d2372478b54a039f4a6eb2 Mon Sep 17 00:00:00 2001 From: Paul Adenot Date: Thu, 31 May 2018 16:44:00 +0200 Subject: [PATCH] Bug 1404977 - Part 13 - Remove useless mutex, and assert why they were useless, in WebRTCAudioDataListeners. r=pehrsons MozReview-Commit-ID: 2Mb5WZXbYgS --HG-- extra : rebase_source : c4a7c8874919901eb11327cfb5f86d6b185be388 --- dom/media/MediaStreamGraph.cpp | 6 +- dom/media/MediaStreamGraph.h | 13 ++-- dom/media/MediaStreamGraphImpl.h | 2 +- dom/media/webrtc/MediaEngineWebRTC.h | 30 ++++---- dom/media/webrtc/MediaEngineWebRTCAudio.cpp | 80 +++++++++++++++------ 5 files changed, 90 insertions(+), 41 deletions(-) diff --git a/dom/media/MediaStreamGraph.cpp b/dom/media/MediaStreamGraph.cpp index 88e3e6e1719e..271c391cb3fa 100644 --- a/dom/media/MediaStreamGraph.cpp +++ b/dom/media/MediaStreamGraph.cpp @@ -878,6 +878,10 @@ MediaStreamGraphImpl::CloseAudioInputImpl(Maybe& aID, MOZ_ASSERT(listeners); DebugOnly wasPresent = listeners->RemoveElement(aListener); MOZ_ASSERT(wasPresent); + + // Breaks the cycle between the MSG and the listener. + aListener->Disconnect(this); + if (!listeners->IsEmpty()) { // There is still a consumer for this audio input device return; @@ -990,7 +994,7 @@ void MediaStreamGraphImpl::DeviceChangedImpl() nsTArray>* listeners = mInputDeviceUsers.GetValue(mInputDeviceID); for (auto& listener : *listeners) { - listener->DeviceChanged(); + listener->DeviceChanged(this); } } diff --git a/dom/media/MediaStreamGraph.h b/dom/media/MediaStreamGraph.h index ca0276e499ea..7d4b1c36508f 100644 --- a/dom/media/MediaStreamGraph.h +++ b/dom/media/MediaStreamGraph.h @@ -106,26 +106,31 @@ public: * cancellation. This is not guaranteed to be in any particular size * chunks. */ - virtual void NotifyOutputData(MediaStreamGraph* aGraph, + virtual void NotifyOutputData(MediaStreamGraphImpl* aGraph, AudioDataValue* aBuffer, size_t aFrames, TrackRate aRate, uint32_t aChannels) = 0; /** * Input data from a microphone (or other audio source. This is not * guaranteed to be in any particular size chunks. */ - virtual void NotifyInputData(MediaStreamGraph* aGraph, + virtual void NotifyInputData(MediaStreamGraphImpl* aGraph, const AudioDataValue* aBuffer, size_t aFrames, TrackRate aRate, uint32_t aChannels) = 0; /** * Number of audio input channels. */ - virtual uint32_t InputChannelCount() = 0; + virtual uint32_t RequestedInputChannelCount(MediaStreamGraphImpl* aGraph) = 0; /** * Called when the underlying audio device has changed. */ - virtual void DeviceChanged() = 0; + virtual void DeviceChanged(MediaStreamGraphImpl* aGraph) = 0; + + /** + * Called when the underlying audio device is being closed. + */ + virtual void Disconnect(MediaStreamGraphImpl* aGraph) = 0; }; class AudioDataListener : public AudioDataListenerInterface { diff --git a/dom/media/MediaStreamGraphImpl.h b/dom/media/MediaStreamGraphImpl.h index 71cad65e699c..3d753a11dda2 100644 --- a/dom/media/MediaStreamGraphImpl.h +++ b/dom/media/MediaStreamGraphImpl.h @@ -484,7 +484,7 @@ public: MOZ_ASSERT(listeners); for (const auto& listener : *listeners) { maxInputChannels = - std::max(maxInputChannels, listener->InputChannelCount()); + std::max(maxInputChannels, listener->RequestedInputChannelCount(this)); } return maxInputChannels; } diff --git a/dom/media/webrtc/MediaEngineWebRTC.h b/dom/media/webrtc/MediaEngineWebRTC.h index 84a8afe165e2..cea208750fcb 100644 --- a/dom/media/webrtc/MediaEngineWebRTC.h +++ b/dom/media/webrtc/MediaEngineWebRTC.h @@ -160,6 +160,8 @@ private: bool mManualInvalidation; }; +// This class is instantiated on the MediaManager thread, and is then sent and +// only ever access again on the MediaStreamGraph. class WebRTCAudioDataListener : public AudioDataListener { protected: @@ -168,31 +170,29 @@ protected: public: explicit WebRTCAudioDataListener(MediaEngineWebRTCMicrophoneSource* aAudioSource) - : mMutex("WebRTCAudioDataListener::mMutex") - , mAudioSource(aAudioSource) + : mAudioSource(aAudioSource) {} // AudioDataListenerInterface methods - void NotifyOutputData(MediaStreamGraph* aGraph, + void NotifyOutputData(MediaStreamGraphImpl* aGraph, AudioDataValue* aBuffer, size_t aFrames, TrackRate aRate, uint32_t aChannels) override; - void NotifyInputData(MediaStreamGraph* aGraph, + void NotifyInputData(MediaStreamGraphImpl* aGraph, const AudioDataValue* aBuffer, size_t aFrames, TrackRate aRate, uint32_t aChannels) override; - uint32_t InputChannelCount() override; + uint32_t RequestedInputChannelCount(MediaStreamGraphImpl* aGraph) override; - void DeviceChanged() override; + void DeviceChanged(MediaStreamGraphImpl* aGraph) override; - void Shutdown(); + void Disconnect(MediaStreamGraphImpl* aGraph) override; private: - Mutex mMutex; RefPtr mAudioSource; }; @@ -247,20 +247,22 @@ public: const PrincipalHandle& aPrincipalHandle) override; // AudioDataListenerInterface methods - void NotifyOutputData(MediaStreamGraph* aGraph, + void NotifyOutputData(MediaStreamGraphImpl* aGraph, AudioDataValue* aBuffer, size_t aFrames, TrackRate aRate, uint32_t aChannels) override; - void NotifyInputData(MediaStreamGraph* aGraph, + void NotifyInputData(MediaStreamGraphImpl* aGraph, const AudioDataValue* aBuffer, size_t aFrames, TrackRate aRate, uint32_t aChannels) override; - void DeviceChanged() override; + void DeviceChanged(MediaStreamGraphImpl* aGraph) override; uint32_t RequestedInputChannelCount(MediaStreamGraphImpl* aGraph) override { return GetRequestedInputChannelCount(aGraph); } + void Disconnect(MediaStreamGraphImpl* aGraph) override; + dom::MediaSourceEnum GetMediaSource() const override { return dom::MediaSourceEnum::Microphone; @@ -364,7 +366,7 @@ private: size_t aFrames, uint32_t aChannels); - void PacketizeAndProcess(MediaStreamGraph* aGraph, + void PacketizeAndProcess(MediaStreamGraphImpl* aGraph, const AudioDataValue* aBuffer, size_t aFrames, TrackRate aRate, @@ -381,7 +383,9 @@ private: uint32_t GetRequestedInputChannelCount(MediaStreamGraphImpl* aGraphImpl); void SetRequestedInputChannelCount(uint32_t aRequestedInputChannelCount); - // Owning thread only. + // mListener is created on the MediaManager thread, and then sent to the MSG + // thread. On shutdown, we send this pointer to the MSG thread again, telling + // it to clean up. RefPtr mListener; // Can be shared on any thread. diff --git a/dom/media/webrtc/MediaEngineWebRTCAudio.cpp b/dom/media/webrtc/MediaEngineWebRTCAudio.cpp index 7d2512146c42..b7a641306702 100644 --- a/dom/media/webrtc/MediaEngineWebRTCAudio.cpp +++ b/dom/media/webrtc/MediaEngineWebRTCAudio.cpp @@ -58,49 +58,52 @@ WebRTCAudioDataListener::NotifyOutputData(MediaStreamGraph* aGraph, TrackRate aRate, uint32_t aChannels) { - MutexAutoLock lock(mMutex); + MOZ_ASSERT(aGraph->CurrentDriver()->OnThread()); if (mAudioSource) { mAudioSource->NotifyOutputData(aGraph, aBuffer, aFrames, aRate, aChannels); } } void -WebRTCAudioDataListener::NotifyInputData(MediaStreamGraph* aGraph, +WebRTCAudioDataListener::NotifyInputData(MediaStreamGraphImpl* aGraph, const AudioDataValue* aBuffer, size_t aFrames, TrackRate aRate, uint32_t aChannels) { - MutexAutoLock lock(mMutex); + MOZ_ASSERT(aGraph->CurrentDriver()->OnThread()); if (mAudioSource) { mAudioSource->NotifyInputData(aGraph, aBuffer, aFrames, aRate, aChannels); } } void -WebRTCAudioDataListener::DeviceChanged() +WebRTCAudioDataListener::DeviceChanged(MediaStreamGraphImpl* aGraph) { - MutexAutoLock lock(mMutex); + MOZ_ASSERT(aGraph->CurrentDriver()->OnThread()); if (mAudioSource) { - mAudioSource->DeviceChanged(); + mAudioSource->DeviceChanged(aGraph); } } uint32_t -WebRTCAudioDataListener::InputChannelCount() +WebRTCAudioDataListener::RequestedInputChannelCount(MediaStreamGraphImpl* aGraph) { - MutexAutoLock lock(mMutex); + MOZ_ASSERT(aGraph->CurrentDriver()->OnThread()); if (mAudioSource) { - return mAudioSource->InputChannelCount(); + return mAudioSource->RequestedInputChannelCount(aGraph); } return 0; } void -WebRTCAudioDataListener::Shutdown() +WebRTCAudioDataListener::Disconnect(MediaStreamGraphImpl* aGraph) { - MutexAutoLock lock(mMutex); - mAudioSource = nullptr; + MOZ_ASSERT(aGraph->CurrentDriver()->OnThread()); + if (mAudioSource) { + mAudioSource->Disconnect(aGraph); + mAudioSource = nullptr; + } } /** @@ -529,6 +532,9 @@ MediaEngineWebRTCMicrophoneSource::SetPassThrough(bool aPassThrough) uint32_t MediaEngineWebRTCMicrophoneSource::GetRequestedInputChannelCount(MediaStreamGraphImpl* aGraphImpl) { + MOZ_ASSERT(aGraphImpl->CurrentDriver()->OnThread(), + "Wrong calling pattern, don't call this before ::SetTrack."); + if (mState == kReleased) { // This source has been released, and is waiting for collection. Simply // return 0, this source won't contribute to the channel count decision. @@ -544,6 +550,9 @@ MediaEngineWebRTCMicrophoneSource::SetRequestedInputChannelCount( uint32_t aRequestedInputChannelCount) { MutexAutoLock lock(mMutex); + + MOZ_ASSERT(mAllocations.Length() <= 1); + if (mAllocations.IsEmpty()) { return; } @@ -561,6 +570,12 @@ MediaEngineWebRTCMicrophoneSource::ApplySettings(const MediaEnginePrefs& aPrefs, { AssertIsOnOwningThread(); MOZ_DIAGNOSTIC_ASSERT(aGraph); +#ifdef DEBUG + { + MutexAutoLock lock(mMutex); + MOZ_ASSERT(mAllocations.Length() <= 1); + } +#endif RefPtr that = this; NS_DispatchToMainThread(media::NewRunnableFrom([that, graph = std::move(aGraph), aPrefs]() mutable { @@ -616,6 +631,12 @@ MediaEngineWebRTCMicrophoneSource::Allocate(const dom::MediaTrackConstraints &aC auto handle = MakeRefPtr(aConstraints, aPrincipalInfo, aDeviceId); +#ifdef DEBUG + { + MutexAutoLock lock(mMutex); + MOZ_ASSERT(mAllocations.Length() <= 1); + } +#endif LOG(("Mic source %p allocation %p Allocate()", this, handle.get())); nsresult rv = ReevaluateAllocation(handle, nullptr, aPrefs, aDeviceId, @@ -626,6 +647,7 @@ MediaEngineWebRTCMicrophoneSource::Allocate(const dom::MediaTrackConstraints &aC { MutexAutoLock lock(mMutex); + MOZ_ASSERT(mAllocations.IsEmpty(), "Only allocate once."); mAllocations.AppendElement(Allocation(handle)); } @@ -638,6 +660,8 @@ MediaEngineWebRTCMicrophoneSource::Deallocate(const RefPtr return NS_ERROR_NOT_AVAILABLE; } + MOZ_ASSERT(mAllocations.Length() == 1, "Only allocate once."); + size_t i = mAllocations.IndexOf(aHandle, 0, AllocationHandleComparator()); MOZ_DIAGNOSTIC_ASSERT(i != mAllocations.NoIndex); MOZ_ASSERT(!mAllocations[i].mStream); @@ -766,6 +793,7 @@ MediaEngineWebRTCMicrophoneSource::Start(const RefPtr& a nsresult MediaEngineWebRTCMicrophoneSource::Stop(const RefPtr& aHandle) { + MOZ_ASSERT(mAllocations.Length() <= 1); AssertIsOnOwningThread(); LOG(("Mic source %p allocation %p Stop()", this, aHandle.get())); @@ -774,6 +802,7 @@ MediaEngineWebRTCMicrophoneSource::Stop(const RefPtr& aH MOZ_DIAGNOSTIC_ASSERT(i != mAllocations.NoIndex, "Cannot stop track that we don't know about"); Allocation& allocation = mAllocations[i]; + MOZ_ASSERT(allocation.mStream, "SetTrack must have been called before ::Stop"); if (!allocation.mEnabled) { // Already stopped - this is allowed @@ -788,6 +817,7 @@ MediaEngineWebRTCMicrophoneSource::Stop(const RefPtr& aH CubebUtils::AudioDeviceID deviceID = mDeviceInfo->DeviceID(); Maybe id = Some(deviceID); allocation.mStream->CloseAudioInput(id, mListener); + mListener = nullptr; if (HasEnabledTrack()) { // Another track is keeping us from stopping @@ -798,12 +828,6 @@ MediaEngineWebRTCMicrophoneSource::Stop(const RefPtr& aH mState = kStopped; } - if (mListener) { - // breaks a cycle, since the WebRTCAudioDataListener has a RefPtr to us - mListener->Shutdown(); - mListener = nullptr; - } - return NS_OK; } @@ -884,12 +908,14 @@ MediaEngineWebRTCMicrophoneSource::Pull(const RefPtr& aH } void -MediaEngineWebRTCMicrophoneSource::NotifyOutputData(MediaStreamGraph* aGraph, +MediaEngineWebRTCMicrophoneSource::NotifyOutputData(MediaStreamGraphImpl* aGraph, AudioDataValue* aBuffer, size_t aFrames, TrackRate aRate, uint32_t aChannels) { + MOZ_ASSERT(aGraph->CurrentDriver()->OnThread()); + if (!mPacketizerOutput || mPacketizerOutput->PacketSize() != aRate/100u || mPacketizerOutput->Channels() != aChannels) { @@ -977,7 +1003,7 @@ MediaEngineWebRTCMicrophoneSource::NotifyOutputData(MediaStreamGraph* aGraph, // Only called if we're not in passthrough mode void -MediaEngineWebRTCMicrophoneSource::PacketizeAndProcess(MediaStreamGraph* aGraph, +MediaEngineWebRTCMicrophoneSource::PacketizeAndProcess(MediaStreamGraphImpl* aGraph, const AudioDataValue* aBuffer, size_t aFrames, TrackRate aRate, @@ -1202,12 +1228,13 @@ MediaEngineWebRTCMicrophoneSource::InsertInGraph(const T* aBuffer, // Called back on GraphDriver thread! // Note this can be called back after ::Shutdown() void -MediaEngineWebRTCMicrophoneSource::NotifyInputData(MediaStreamGraph* aGraph, +MediaEngineWebRTCMicrophoneSource::NotifyInputData(MediaStreamGraphImpl* aGraph, const AudioDataValue* aBuffer, size_t aFrames, TrackRate aRate, uint32_t aChannels) { + MOZ_ASSERT(aGraph->CurrentDriver()->OnThread()); TRACE_AUDIO_CALLBACK(); { @@ -1253,14 +1280,23 @@ do { \ } while(0) void -MediaEngineWebRTCMicrophoneSource::DeviceChanged() +MediaEngineWebRTCMicrophoneSource::DeviceChanged(MediaStreamGraphImpl* aGraph) { + MOZ_ASSERT(aGraph->CurrentDriver()->OnThread()); // Reset some processing ResetProcessingIfNeeded(gain_control); ResetProcessingIfNeeded(echo_cancellation); ResetProcessingIfNeeded(noise_suppression); } +void +MediaEngineWebRTCMicrophoneSource::Disconnect(MediaStreamGraphImpl* aGraph) +{ + // This method is just for asserts. + MOZ_ASSERT(aGraph->CurrentDriver()->OnThread()); + MOZ_ASSERT(!mListener); +} + void MediaEngineWebRTCMicrophoneSource::Shutdown() {