From a2936f5009a0bdc9cd2ae32fe8407a9ecdf2550e Mon Sep 17 00:00:00 2001 From: Narcis Beleuzu Date: Wed, 23 Sep 2020 16:52:16 +0300 Subject: [PATCH] Backed out 4 changesets (bug 1654430) for mda failure on test_ondevicechange.html . CLOSED TREE Backed out changeset 10a70b4fad32 (bug 1654430) Backed out changeset 495576ebc999 (bug 1654430) Backed out changeset 0f1db7f155cb (bug 1654430) Backed out changeset 788a6c7c52f1 (bug 1654430) --- dom/media/MediaManager.cpp | 84 +++------ dom/media/MediaManager.h | 18 +- dom/media/tests/mochitest/head.js | 4 - dom/media/tests/mochitest/mochitest.ini | 1 - dom/media/tests/mochitest/pc.js | 164 +++++++++--------- dom/media/tests/mochitest/templates.js | 75 ++++---- .../tests/mochitest/test_ondevicechange.html | 72 ++++---- ...on_addAudioTrackToExistingVideoStream.html | 3 +- dom/media/webrtc/MediaEngine.h | 4 +- 9 files changed, 182 insertions(+), 243 deletions(-) diff --git a/dom/media/MediaManager.cpp b/dom/media/MediaManager.cpp index 4441b3f6c9ad..4ac9f6084020 100644 --- a/dom/media/MediaManager.cpp +++ b/dom/media/MediaManager.cpp @@ -2217,60 +2217,32 @@ void MediaManager::DeviceListChanged() { } mDeviceListChangeEvent.Notify(); - // Wait 200 ms, because - // A) on some Windows machines, if we call EnumerateRawDevices immediately - // after receiving devicechange event, we'd get an outdated devices list. - // B) Waiting helps coalesce multiple calls on us into one, which can happen - // if a device with both audio input and output is attached or removed. - // We want to react & fire a devicechange event only once in that case. - - if (mDeviceChangeTimer) { - mDeviceChangeTimer->Cancel(); - } else { - mDeviceChangeTimer = MakeRefPtr(); - } - RefPtr self = this; + // On some Windows machines, if we call EnumerateRawDevices immediately after + // receiving devicechange event, we would get an outdated devices list. + PR_Sleep(PR_MillisecondsToInterval(200)); auto devices = MakeRefPtr(); - mDeviceChangeTimer->WaitFor(TimeDuration::FromMilliseconds(200), __func__) + EnumerateRawDevices(0, MediaSourceEnum::Camera, MediaSourceEnum::Microphone, + MediaSinkEnum::Speaker, DeviceEnumerationType::Normal, + DeviceEnumerationType::Normal, false, devices) ->Then( GetCurrentSerialEventTarget(), __func__, - [self, this, devices] { - if (!MediaManager::GetIfExists()) { - return MgrPromise::CreateAndReject( - MakeRefPtr(MediaMgrError::Name::AbortError, - u"In shutdown"_ns), - __func__); - } - return EnumerateRawDevices( - 0, MediaSourceEnum::Camera, MediaSourceEnum::Microphone, - MediaSinkEnum::Speaker, DeviceEnumerationType::Normal, - DeviceEnumerationType::Normal, false, devices); - }, - []() { - // Timer was canceled by us, or we're in shutdown. - return MgrPromise::CreateAndReject( - MakeRefPtr(MediaMgrError::Name::AbortError), - __func__); - }) - ->Then( - GetCurrentSerialEventTarget(), __func__, - [self, this, devices](bool) { + [self = RefPtr(this), this, devices](bool) { if (!MediaManager::GetIfExists()) { return; } - MediaManager::DeviceIdSet deviceIDs; + nsTArray deviceIDs; for (auto& device : *devices) { nsString id; device->GetId(id); - MOZ_ALWAYS_TRUE(deviceIDs.put(std::move(id))); + if (!deviceIDs.Contains(id)) { + deviceIDs.AppendElement(id); + } } - // For any real removed cameras, microphones or speakers, notify - // their listeners cleanly that the source has stopped, so JS knows - // and usage indicators update. - for (auto iter = mDeviceIDs.iter(); !iter.done(); iter.next()) { - const auto& id = iter.get(); - if (deviceIDs.has(id)) { + // For any removed devices, notify their listeners cleanly that the + // source has stopped, so JS knows and usage indicators update. + for (auto& id : mDeviceIDs) { + if (deviceIDs.Contains(id)) { // Device has not been removed continue; } @@ -3131,8 +3103,7 @@ RefPtr MediaManager::EnumerateDevicesImpl( }) ->Then( GetMainThreadSerialEventTarget(), __func__, - [aWindowId, originKey, aVideoInputEnumType, aAudioInputEnumType, - aOutDevices](bool) { + [aWindowId, originKey, aOutDevices](bool) { // Only run if window is still on our active list. MediaManager* mgr = MediaManager::GetIfExists(); if (!mgr || !mgr->IsWindowStillActive(aWindowId)) { @@ -3141,17 +3112,12 @@ RefPtr MediaManager::EnumerateDevicesImpl( __func__); } + mgr->mDeviceIDs.Clear(); for (auto& device : *aOutDevices) { - if (device->mKind == MediaDeviceKind::Audiooutput || - (device->mKind == MediaDeviceKind::Audioinput && - aAudioInputEnumType != DeviceEnumerationType::Fake && - device->GetMediaSource() == MediaSourceEnum::Microphone) || - (device->mKind == MediaDeviceKind::Videoinput && - aVideoInputEnumType != DeviceEnumerationType::Fake && - device->GetMediaSource() == MediaSourceEnum::Camera)) { - nsString id; - device->GetId(id); - MOZ_ALWAYS_TRUE(mgr->mDeviceIDs.put(std::move(id))); + nsString id; + device->GetId(id); + if (!mgr->mDeviceIDs.Contains(id)) { + mgr->mDeviceIDs.AppendElement(id); } } @@ -3652,12 +3618,6 @@ void MediaManager::Shutdown() { #endif } - if (mDeviceChangeTimer) { - mDeviceChangeTimer->Cancel(); - // Drop ref to MediaTimer early to avoid blocking SharedThreadPool shutdown - mDeviceChangeTimer = nullptr; - } - { // Close off any remaining active windows. @@ -3680,7 +3640,7 @@ void MediaManager::Shutdown() { mActiveCallbacks.Clear(); mCallIds.Clear(); mPendingGUMRequest.Clear(); - mDeviceIDs.clear(); + mDeviceIDs.Clear(); #ifdef MOZ_WEBRTC StopWebRtcLog(); #endif diff --git a/dom/media/MediaManager.h b/dom/media/MediaManager.h index 1659e55b7c2b..ee23c67bf97e 100644 --- a/dom/media/MediaManager.h +++ b/dom/media/MediaManager.h @@ -41,7 +41,6 @@ class nsIPrefBranch; namespace mozilla { class TaskQueue; -class MediaTimer; namespace dom { struct MediaStreamConstraints; struct MediaTrackConstraints; @@ -344,7 +343,6 @@ class MediaManager final : public nsIMediaManagerService, public nsIObserver { nsRefPtrHashtable mActiveCallbacks; nsClassHashtable> mCallIds; nsTArray> mPendingGUMRequest; - RefPtr mDeviceChangeTimer; bool mCamerasMuted = false; bool mMicrophonesMuted = false; @@ -358,21 +356,7 @@ class MediaManager final : public nsIMediaManagerService, public nsIObserver { static StaticRefPtr sSingleton; static StaticMutex sSingletonMutex; - struct nsStringHasher { - using Key = nsString; - using Lookup = nsString; - - static HashNumber hash(const Lookup& aLookup) { - return HashString(aLookup.get()); - } - - static bool match(const Key& aKey, const Lookup& aLookup) { - return aKey == aLookup; - } - }; - - using DeviceIdSet = HashSet; - DeviceIdSet mDeviceIDs; + nsTArray mDeviceIDs; // Connect/Disconnect on media thread only MediaEventListener mDeviceListChangeListener; diff --git a/dom/media/tests/mochitest/head.js b/dom/media/tests/mochitest/head.js index d84fcee66470..b6230e440fa5 100644 --- a/dom/media/tests/mochitest/head.js +++ b/dom/media/tests/mochitest/head.js @@ -418,10 +418,6 @@ function pushPrefs(...p) { return SpecialPowers.pushPrefEnv({ set: p }); } -function popPrefs() { - return SpecialPowers.popPrefEnv(); -} - function setupEnvironment() { var defaultMochitestPrefs = { set: [ diff --git a/dom/media/tests/mochitest/mochitest.ini b/dom/media/tests/mochitest/mochitest.ini index 8766fb954c03..3824ea1f41e6 100644 --- a/dom/media/tests/mochitest/mochitest.ini +++ b/dom/media/tests/mochitest/mochitest.ini @@ -59,7 +59,6 @@ skip-if = os != 'linux' # the only platform with real devices skip-if = os != 'linux' # the only platform with real devices [test_setSinkId_preMutedElement.html] [test_ondevicechange.html] -run-sequentially = sets prefs that may disrupt other tests [test_getUserMedia_active_autoplay.html] [test_getUserMedia_audioCapture.html] skip-if = toolkit == 'android' || (os == "win" && processor == "aarch64") # android(Bug 1189784, timeouts on 4.3 emulator), android(Bug 1264333), aarch64 due to 1538359 diff --git a/dom/media/tests/mochitest/pc.js b/dom/media/tests/mochitest/pc.js index 92bf4ad07857..8ef3f6a8714d 100644 --- a/dom/media/tests/mochitest/pc.js +++ b/dom/media/tests/mochitest/pc.js @@ -1787,88 +1787,91 @@ PeerConnectionWrapper.prototype = { * @returns {Promise} * Returns a promise which yields a StatsReport object with RTP stats. */ - async _waitForRtpFlow(target, rtpType) { - const { track } = target; - info(`_waitForRtpFlow(${track.id}, ${rtpType})`); - const packets = `packets${rtpType == "outbound-rtp" ? "Sent" : "Received"}`; + async waitForRtpFlow(track) { + info("waitForRtpFlow(" + track.id + ")"); + let hasFlow = (stats, retries) => { + const dict = JSON.stringify([...stats.entries()]); + info( + `Checking for stats in ${dict} for ${track.kind} track ${track.id}` + + `retry number ${retries}` + ); + const rtp = [...stats.values()].find(({ type }) => + ["inbound-rtp", "outbound-rtp"].includes(type) + ); + if (!rtp) { + return false; + } + info("Should have RTP stats for track " + track.id); + info("RTP stats: " + JSON.stringify(rtp)); + let nrPackets = + rtp[rtp.type == "outbound-rtp" ? "packetsSent" : "packetsReceived"]; + info( + "Track " + + track.id + + " has " + + nrPackets + + " " + + rtp.type + + " RTP packets." + ); + return nrPackets > 0; + }; - const retryInterval = 500; // Time between stats checks - const timeout = 30000; // Timeout in ms - const retries = timeout / retryInterval; - - for (let i = 0; i < retries; i++) { - info(`Checking ${rtpType} for ${track.kind} track ${track.id} try ${i}`); - for (const rtp of (await target.getStats()).values()) { - if (rtp.type != rtpType) { - continue; - } - if (rtp.kind != track.kind) { - continue; - } - - const numPackets = rtp[packets]; - info(`Track ${track.id} has ${numPackets} ${packets}.`); - if (!numPackets) { - continue; - } - - ok(true, `RTP flowing for ${track.kind} track ${track.id}`); - return; + // Time between stats checks + const retryInterval = 500; + // Timeout in ms + const timeout = 30000; + let retry = 0; + // Check hasFlow at a reasonable interval + for (let remaining = timeout; remaining >= 0; remaining -= retryInterval) { + let stats = await this._pc.getStats(track); + if (hasFlow(stats, retry++)) { + ok(true, "RTP flowing for " + track.kind + " track " + track.id); + return stats; } await wait(retryInterval); } throw new Error( - `Checking stats for track ${track.id} timed out after ${timeout} ms` + "Timeout checking for stats for track " + + track.id + + " after at least" + + timeout + + "ms" ); }, - /** - * Wait for inbound RTP packet flow for the given MediaStreamTrack. - * - * @param {object} receiver - * An RTCRtpReceiver to wait for data flow on. - * @returns {Promise} - * Returns a promise that resolves once data is flowing. - */ - async waitForInboundRtpFlow(receiver) { - return this._waitForRtpFlow(receiver, "inbound-rtp"); - }, - - /** - * Wait for outbound RTP packet flow for the given MediaStreamTrack. - * - * @param {object} sender - * An RTCRtpSender to wait for data flow on. - * @returns {Promise} - * Returns a promise that resolves once data is flowing. - */ - async waitForOutboundRtpFlow(sender) { - return this._waitForRtpFlow(sender, "outbound-rtp"); - }, - - getExpectedActiveReceivers() { + getExpectedActiveReceiveTracks() { return this._pc .getTransceivers() - .filter( - t => + .filter(t => { + return ( !t.stopped && t.currentDirection && t.currentDirection != "inactive" && t.currentDirection != "sendonly" - ) - .filter(({ receiver }) => receiver.track) - .map(({ mid, currentDirection, receiver }) => { - info( - `Found transceiver that should be receiving RTP: mid=${mid}` + - ` currentDirection=${currentDirection}` + - ` kind=${receiver.track.kind} track-id=${receiver.track.id}` ); - return receiver; - }); + }) + .map(t => { + info( + "Found transceiver that should be receiving RTP: mid=" + + t.mid + + " currentDirection=" + + t.currentDirection + + " kind=" + + t.receiver.track.kind + + " track-id=" + + t.receiver.track.id + ); + return t.receiver.track; + }) + .filter(t => t); }, - getExpectedSenders() { - return this._pc.getSenders().filter(({ track }) => track); + getExpectedSendTracks() { + return this._pc + .getSenders() + .map(s => s.track) + .filter(t => t); }, /** @@ -1879,21 +1882,24 @@ PeerConnectionWrapper.prototype = { * A promise that resolves when media flows for all elements and tracks */ waitForMediaFlow() { - const receivers = this.getExpectedActiveReceivers(); - return Promise.all([ - ...this.localMediaElements.map(el => this.waitForMediaElementFlow(el)), - ...this.remoteMediaElements - .filter(({ srcObject }) => - receivers.some(({ track }) => - srcObject.getTracks().some(t => t == track) + return Promise.all( + [].concat( + this.localMediaElements.map(element => + this.waitForMediaElementFlow(element) + ), + this.remoteMediaElements + .filter(elem => + this.getExpectedActiveReceiveTracks().some(track => + elem.srcObject.getTracks().some(t => t == track) + ) ) - ) - .map(el => this.waitForMediaElementFlow(el)), - ...receivers.map(receiver => this.waitForInboundRtpFlow(receiver)), - ...this.getExpectedSenders().map(sender => - this.waitForOutboundRtpFlow(sender) - ), - ]); + .map(elem => this.waitForMediaElementFlow(elem)), + this.getExpectedActiveReceiveTracks().map(track => + this.waitForRtpFlow(track) + ), + this.getExpectedSendTracks().map(track => this.waitForRtpFlow(track)) + ) + ); }, async waitForSyncedRtcp() { diff --git a/dom/media/tests/mochitest/templates.js b/dom/media/tests/mochitest/templates.js index dcb0cfab4b9b..44ca50a684e0 100644 --- a/dom/media/tests/mochitest/templates.js +++ b/dom/media/tests/mochitest/templates.js @@ -153,43 +153,48 @@ function waitForAnIceCandidate(pc) { }); } -async function checkTrackStats(pc, track, outbound) { - const audio = track.kind == "audio"; - const msg = - `${pc} stats ${outbound ? "outbound " : "inbound "}` + - `${audio ? "audio" : "video"} rtp track id ${track.id}`; - const stats = await pc.getStats(track); - ok( - pc.hasStat(stats, { - type: outbound ? "outbound-rtp" : "inbound-rtp", - kind: audio ? "audio" : "video", - }), - `${msg} - found expected stats` - ); - ok( - !pc.hasStat(stats, { - type: outbound ? "inbound-rtp" : "outbound-rtp", - }), - `${msg} - did not find extra stats with wrong direction` - ); - ok( - !pc.hasStat(stats, { - kind: audio ? "video" : "audio", - }), - `${msg} - did not find extra stats with wrong media type` - ); +function checkTrackStats(pc, track, outbound) { + var audio = track.kind == "audio"; + var msg = + pc + + " stats " + + (outbound ? "outbound " : "inbound ") + + (audio ? "audio" : "video") + + " rtp track id " + + track.id; + return pc.getStats(track).then(stats => { + ok( + pc.hasStat(stats, { + type: outbound ? "outbound-rtp" : "inbound-rtp", + kind: audio ? "audio" : "video", + }), + msg + " - found expected stats" + ); + ok( + !pc.hasStat(stats, { + type: outbound ? "inbound-rtp" : "outbound-rtp", + }), + msg + " - did not find extra stats with wrong direction" + ); + ok( + !pc.hasStat(stats, { + kind: audio ? "video" : "audio", + }), + msg + " - did not find extra stats with wrong media type" + ); + }); } -function checkAllTrackStats(pc) { - return Promise.all([ - ...pc - .getExpectedActiveReceivers() - .map(({ track }) => checkTrackStats(pc, track, false)), - ...pc - .getExpectedSenders() - .map(({ track }) => checkTrackStats(pc, track, true)), - ]); -} +var checkAllTrackStats = pc => { + return Promise.all( + [].concat( + pc + .getExpectedActiveReceiveTracks() + .map(track => checkTrackStats(pc, track, false)), + pc.getExpectedSendTracks().map(track => checkTrackStats(pc, track, true)) + ) + ); +}; // Commands run once at the beginning of each test, even when performing a // renegotiation test. diff --git a/dom/media/tests/mochitest/test_ondevicechange.html b/dom/media/tests/mochitest/test_ondevicechange.html index 4d3613145677..b002085535be 100644 --- a/dom/media/tests/mochitest/test_ondevicechange.html +++ b/dom/media/tests/mochitest/test_ondevicechange.html @@ -15,10 +15,23 @@ createHTML({ const RESPONSE_WAIT_TIME_MS = 3000; -async function maybeReceiveDevicechangeEvent() { +function OnDeviceChangeEvent() { + return new Promise(resolve => navigator.mediaDevices.ondevicechange = resolve); +} + +function OnDeviceChangeEventReceived() { return Promise.race([ - new Promise(r => navigator.mediaDevices.ondevicechange = () => r(true)), - wait(RESPONSE_WAIT_TIME_MS).then(() => false) + OnDeviceChangeEvent(), + wait(RESPONSE_WAIT_TIME_MS).then(() => + Promise.reject(new Error("Timed out waiting for devicechange"))), + ]); +} + +function OnDeviceChangeEventNotReceived() { + return Promise.race([ + OnDeviceChangeEvent().then(() => + Promise.reject(new Error("devicechange fired unexpectedly"))), + wait(RESPONSE_WAIT_TIME_MS), ]); } @@ -28,51 +41,30 @@ runTest(async () => { await pushPrefs( // Ensure there are continuous fake devicechange events throughout this test ["media.ondevicechange.fakeDeviceChangeEvent.enabled", true], + // Enforce gUM permission prompts initially + ["media.navigator.permission.disabled", false], // Make fake devices count as real, permission-wise, or devicechange events // won't be exposed ["media.navigator.permission.fake", true], - // Ensure this precondition to the below tests - ["media.navigator.permission.disabled", true] ); - try { - { - const stream = await getUserMedia({video: true, fake: true}); - const [track] = stream.getVideoTracks(); - await pushPrefs(["media.navigator.permission.disabled", false]); - try { - ok(await maybeReceiveDevicechangeEvent(), - "devicechange event is fired when gUM is in use without permanent " + - "permission granted"); - } finally { - track.stop(); - await popPrefs(); - } - } + info("assure devicechange event is NOT fired when gUM is NOT in use and permanent permission is NOT granted"); + await OnDeviceChangeEventNotReceived(); + ok(true, "devicechange event is NOT fired when gUM is NOT in use and permanent permission is NOT granted"); - { - await pushPrefs(["media.navigator.permission.disabled", false]); - try { - ok(!await maybeReceiveDevicechangeEvent(), - "devicechange event is NOT fired when gUM is NOT in use and " + - "permanent permission is NOT granted"); - } finally { - await popPrefs(); - } - } + await pushPrefs(['media.navigator.permission.disabled', true]); - { - ok(await maybeReceiveDevicechangeEvent(), - "devicechange event is fired when gUM is NOT in use and permanent "+ - "permission is granted"); - } - } finally { - await popPrefs(); - } + info("assure devicechange event is fired when gUM is NOT in use and permanent permission is granted"); + await OnDeviceChangeEventReceived(); + ok(true, "devicechange event is fired when gUM is NOT in use and permanent permission is granted"); - // Let some time pass to allow fake ended events to finish firing on other - // documents before proceeding with subsequent tests to not distrupt them. - await maybeReceiveDevicechangeEvent(); + info("assure devicechange event is fired when gUM is in use"); + const st = await getUserMedia({video: true}); + const videoTracks = st.getVideoTracks(); + await pushPrefs(['media.navigator.permission.disabled', false]); + await OnDeviceChangeEventReceived(); + ok(true, "devicechange event is fired when gUM is in use"); + videoTracks.forEach(track => track.stop()); }); diff --git a/dom/media/tests/mochitest/test_peerConnection_addAudioTrackToExistingVideoStream.html b/dom/media/tests/mochitest/test_peerConnection_addAudioTrackToExistingVideoStream.html index 22ed0bc30475..0d1ea10b623d 100644 --- a/dom/media/tests/mochitest/test_peerConnection_addAudioTrackToExistingVideoStream.html +++ b/dom/media/tests/mochitest/test_peerConnection_addAudioTrackToExistingVideoStream.html @@ -1,4 +1,4 @@ - + @@ -12,7 +12,6 @@ }); runNetworkTest(function (options) { - SimpleTest.requestCompleteLog(); var test = new PeerConnectionTest(options); test.chain.replace("PC_LOCAL_GUM", [ diff --git a/dom/media/webrtc/MediaEngine.h b/dom/media/webrtc/MediaEngine.h index 75b39e843324..efce5b4a341b 100644 --- a/dom/media/webrtc/MediaEngine.h +++ b/dom/media/webrtc/MediaEngine.h @@ -47,9 +47,7 @@ class MediaEngine { virtual void Shutdown() = 0; - virtual void SetFakeDeviceChangeEventsEnabled(bool aEnable) { - MOZ_DIAGNOSTIC_ASSERT(false, "Fake events may not have started/stopped"); - } + virtual void SetFakeDeviceChangeEventsEnabled(bool aEnable) {} virtual MediaEventSource& DeviceListChangeEvent() = 0;