Bug 1531803 - Part 5: Rework local track checking in our mochitests to ignore track ids, and decruft. r=jib
authorByron Campen [:bwc] <docfaraday@gmail.com>
Mon, 29 Apr 2019 15:52:00 +0000
changeset 530752 6bf3fd241bb89e14d9535b04085b2949000e6329
parent 530751 4864459fde540d61387aa5bb4ec8b05b607d3b4d
child 530753 d2ba7da778f0ec24b08c80c870ed0f86b42e6758
push id11265
push userffxbld-merge
push dateMon, 13 May 2019 10:53:39 +0000
treeherdermozilla-beta@77e0fe8dbdd3 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersjib
bugs1531803
milestone68.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1531803 - Part 5: Rework local track checking in our mochitests to ignore track ids, and decruft. r=jib Differential Revision: https://phabricator.services.mozilla.com/D25798
dom/media/tests/mochitest/pc.js
dom/media/tests/mochitest/templates.js
dom/media/tests/mochitest/test_peerConnection_remoteReofferRollback.html
dom/media/tests/mochitest/test_peerConnection_remoteRollback.html
--- a/dom/media/tests/mochitest/pc.js
+++ b/dom/media/tests/mochitest/pc.js
@@ -784,19 +784,18 @@ function PeerConnectionWrapper(label, co
   this.localRequiresTrickleIce = false;
   this.remoteRequiresTrickleIce = false;
   this.localMediaElements = [];
   this.remoteMediaElements = [];
   this.audioElementsOnly = false;
 
   this._sendStreams = [];
 
-  this.expectedLocalTrackInfoById = {};
-  this.expectedSignalledTrackInfoById = {};
-  this.observedRemoteTrackInfoById = {};
+  this.expectedLocalTrackInfo = [];
+  this.remoteStreamsByTrackId = new Map();
 
   this.disableRtpCountChecking = false;
 
   this.iceConnectedResolve;
   this.iceConnectedReject;
   this.iceConnected = new Promise((resolve, reject) => {
     this.iceConnectedResolve = resolve;
     this.iceConnectedReject = reject;
@@ -970,27 +969,22 @@ PeerConnectionWrapper.prototype = {
    *        MediaStream to use as container for `track` on remote side
    */
   attachLocalTrack : function(track, stream) {
     info("Got a local " + track.kind + " track");
 
     this.expectNegotiationNeeded();
     var sender = this._pc.addTrack(track, stream);
     is(sender.track, track, "addTrack returns sender");
+    is(this._pc.getSenders().pop(), sender, "Sender should be the last element in getSenders()");
 
     ok(track.id, "track has id");
     ok(track.kind, "track has kind");
     ok(stream.id, "stream has id");
-    this.expectedLocalTrackInfoById[track.id] = {
-      type: track.kind,
-      streamId: stream.id,
-    };
-    this.expectedSignalledTrackInfoById[track.id] =
-      this.expectedLocalTrackInfoById[track.id];
-
+    this.expectedLocalTrackInfo.push({track, sender, streamId: stream.id});
     this.addSendStream(stream);
 
     // This will create one media element per track, which might not be how
     // we set up things with the RTCPeerConnection. It's the only way
     // we can ensure all sent tracks are flowing however.
     this.ensureMediaElement(track, "local");
 
     return this.observedNegotiationNeeded;
@@ -1029,42 +1023,40 @@ PeerConnectionWrapper.prototype = {
       });
     }
 
     this.addSendStream(stream);
 
     stream.getTracks().forEach(track => {
       ok(track.id, "track has id");
       ok(track.kind, "track has kind");
-      this.expectedLocalTrackInfoById[track.id] = {
-          type: track.kind,
-          streamId: stream.id
-        };
-      this.expectedSignalledTrackInfoById[track.id] =
-        this.expectedLocalTrackInfoById[track.id];
+      const sender = this._pc.getSenders().find(s => s.track == track);
+      ok(sender, "track has a sender");
+      this.expectedLocalTrackInfo.push({track, sender, streamId: stream.id});
       this.ensureMediaElement(track, "local");
     });
 
     return this.observedNegotiationNeeded;
   },
 
   removeSender : function(index) {
     var sender = this._pc.getSenders()[index];
-    delete this.expectedLocalTrackInfoById[sender.track.id];
+    this.expectedLocalTrackInfo =
+      this.expectedLocalTrackInfo.filter(i => i.sender != sender);
     this.expectNegotiationNeeded();
     this._pc.removeTrack(sender);
     return this.observedNegotiationNeeded;
   },
 
   senderReplaceTrack : function(sender, withTrack, stream) {
-    delete this.expectedLocalTrackInfoById[sender.track.id];
-    this.expectedLocalTrackInfoById[withTrack.id] = {
-        type: withTrack.kind,
-        streamId: stream.id
-      };
+    const info = this.expectedLocalTrackInfo.find(i => i.sender == sender);
+    if (!info) {
+      return; // replaceTrack on a null track, probably
+    }
+    info.track = withTrack;
     this.addSendStream(stream);
     this.ensureMediaElement(withTrack, 'local');
     return sender.replaceTrack(withTrack);
   },
 
   getUserMedia : async function(constraints) {
     var stream = await getUserMedia(constraints);
     if (constraints.audio) {
@@ -1260,73 +1252,90 @@ PeerConnectionWrapper.prototype = {
         ok(signalingStateTransitions[oldstate].includes(newstate), this + ": legal signaling state transition from " + oldstate + " to " + newstate);
       } else {
         ok(false, this + ": old signaling state " + oldstate + " missing in signaling transition array");
       }
       this.signalingStateLog.push(newstate);
     });
   },
 
-  /**
-   * Checks whether a given track is expected, has not been observed yet, and
-   * is of the correct type. Then, moves the track from
-   * |expectedTrackInfoById| to |observedTrackInfoById|.
-   */
-  checkTrackIsExpected : function(trackId,
-                                  kind,
-                                  expectedTrackInfoById,
-                                  observedTrackInfoById) {
-    ok(expectedTrackInfoById[trackId], "track id " + trackId + " was expected");
-    ok(!observedTrackInfoById[trackId], "track id " + trackId + " was not yet observed");
-    var observedKind = kind;
-    var expectedKind = expectedTrackInfoById[trackId].type;
-    is(observedKind, expectedKind,
-        "track id " + trackId + " was of kind " +
-        observedKind + ", which matches " + expectedKind);
-    observedTrackInfoById[trackId] = expectedTrackInfoById[trackId];
-  },
-
   isTrackOnPC: function(track) {
     return !!this.getStreamForRecvTrack(track);
   },
 
   allExpectedTracksAreObserved: function(expected, observed) {
     return Object.keys(expected).every(trackId => observed[trackId]);
   },
 
-  getWebrtcTrackId: function(receiveTrack) {
-    let matchingTransceiver = this._pc.getTransceivers().find(
-        transceiver => transceiver.receiver.track == receiveTrack);
-    if (!matchingTransceiver) {
-      return null;
-    }
+  setupStreamEventHandlers: function(stream) {
+    const myTrackIds = new Set(stream.getTracks().map(t => t.id));
 
-    return matchingTransceiver.getRemoteTrackId();
+    stream.addEventListener('addtrack', ({track}) => {
+      ok(!myTrackIds.has(track.id), "Duplicate addtrack callback: "
+        + `stream id=${stream.id} track id=${track.id}`);
+      myTrackIds.add(track.id);
+      // addtrack events happen before track events, so the track callback hasn't
+      // heard about this yet.
+      let streams = this.remoteStreamsByTrackId.get(track.id);
+      ok(!streams || !streams.has(stream.id),
+        `In addtrack for stream id=${stream.id}` +
+        `there should not have been a track event for track id=${track.id} ` +
+        " containing this stream yet.");
+      ok(stream.getTracks().includes(track), "In addtrack, stream id=" +
+        `${stream.id} should already contain track id=${track.id}`);
+    });
+
+    stream.addEventListener('removetrack', ({track}) => {
+      ok(myTrackIds.has(track.id), "Duplicate removetrack callback: "
+        + `stream id=${stream.id} track id=${track.id}`);
+      myTrackIds.delete(track.id);
+      // Also remove the association from remoteStreamsByTrackId
+      const streams = this.remoteStreamsByTrackId.get(track.id);
+      ok(streams, `In removetrack for stream id=${stream.id}, track id=` +
+        `${track.id} should have had a track callback for the stream.`);
+      streams.delete(stream.id);
+      ok(!stream.getTracks().includes(track), "In removetrack, stream id=" +
+        `${stream.id} should not contain track id=${track.id}`);
+    });
   },
 
   setupTrackEventHandler: function() {
-    this._pc.addEventListener('track', event => {
-      info(this + ": 'ontrack' event fired for " + event.track.id +
-                  "(SDP msid is " + this.getWebrtcTrackId(event.track) +
-                  ")");
+    this._pc.addEventListener('track', ({track, streams}) => {
+      info(`${this}: 'ontrack' event fired for ${track.id}`);
+      ok(this.isTrackOnPC(track), `Found track ${track.id}`);
+
+      let gratuitousEvent = true;
+      let streamsContainingTrack = this.remoteStreamsByTrackId.get(track.id);
+      if (!streamsContainingTrack) {
+        gratuitousEvent = false; // Told us about a new track
+        this.remoteStreamsByTrackId.set(track.id, new Set());
+        streamsContainingTrack = this.remoteStreamsByTrackId.get(track.id);
+      }
+
+      for (const stream of streams) {
+        ok(stream.getTracks().includes(track),
+          `In track event, track id=${track.id}` +
+          ` should already be in stream id=${stream.id}`);
 
-      // TODO(bug 1403238): Checking for remote tracks needs to be completely
-      // reworked, because with the latest spec the identifiers aren't the same
-      // as they are on the other end. Ultimately, what we need to check is
-      // whether the _transceivers_ are in line with what is expected, and
-      // whether the callbacks are consistent with the transceivers.
-      let trackId = this.getWebrtcTrackId(event.track);
-      ok(!this.observedRemoteTrackInfoById[trackId],
-         "track id " + trackId + " was not yet observed");
-      this.observedRemoteTrackInfoById[trackId] = {
-        type: event.track.kind
-      };
-      ok(this.isTrackOnPC(event.track), "Found track " + event.track.id);
+        if (!streamsContainingTrack.has(stream.id)) {
+          gratuitousEvent = false; // Told us about a new stream
+          streamsContainingTrack.add(stream.id);
+          this.setupStreamEventHandlers(stream);
+        }
+      }
 
-      this.ensureMediaElement(event.track, 'remote');
+      ok(!gratuitousEvent, "track event told us something new")
+
+      // So far, we've verified consistency between the current state of the
+      // streams, addtrack/removetrack events on the streams, and track events
+      // on the peerconnection. We have also verified that we have not gotten
+      // any gratuitous events. We have not done anything to verify that the
+      // current state of affairs matches what we were expecting it to.
+
+      this.ensureMediaElement(track, 'remote');
     });
   },
 
   /**
    * Either adds a given ICE candidate right away or stores it to be added
    * later, depending on the state of the PeerConnection.
    *
    * @param {object} candidate
@@ -1443,64 +1452,74 @@ PeerConnectionWrapper.prototype = {
 
       ok(typeof anEvent.candidate.sdpMLineIndex === 'number', "SDP MLine Index needs to exist");
       this._local_ice_candidates.push(anEvent.candidate);
       candidateHandler(this.label, anEvent.candidate);
     };
   },
 
   checkLocalMediaTracks : function() {
-    var observed = {};
-    info(this + " Checking local tracks " + JSON.stringify(this.expectedLocalTrackInfoById));
-    this._pc.getSenders().forEach(sender => {
-      if (sender.track) {
-        this.checkTrackIsExpected(sender.track.id,
-                                  sender.track.kind,
-                                  this.expectedLocalTrackInfoById,
-                                  observed);
-      }
+    info(`${this}: Checking local tracks ${JSON.stringify(this.expectedLocalTrackInfo)}`);
+    const sendersWithTrack = this._pc.getSenders().filter(({track}) => track);
+    is(sendersWithTrack.length, this.expectedLocalTrackInfo.length,
+      "The number of senders with a track should be equal to the number of " +
+      "expected local tracks.");
+
+    // expectedLocalTrackInfo is in the same order that the tracks were added, and
+    // so should the output of getSenders.
+    this.expectedLocalTrackInfo.forEach((info, i) => {
+      const sender = sendersWithTrack[i];
+      is(sender, info.sender, `Sender ${i} should match`);
+      is(sender.track, info.track, `Track ${i} should match`);
     });
-
-    Object.keys(this.expectedLocalTrackInfoById).forEach(
-        id => ok(observed[id], this + " local id " + id + " was observed"));
   },
 
   /**
    * Checks that we are getting the media tracks we expect.
    */
   checkMediaTracks : function() {
     this.checkLocalMediaTracks();
   },
 
-  checkMsids: function() {
-    var checkSdpForMsids = (desc, expectedTrackInfo, side) => {
-      Object.keys(expectedTrackInfo).forEach(trackId => {
-        var streamId = expectedTrackInfo[trackId].streamId;
-        ok(desc.sdp.match(new RegExp("a=msid:" + streamId + " " + trackId)),
-           this + ": " + side + " SDP contains stream " + streamId +
-           " and track " + trackId );
-      });
+  checkLocalMsids: function() {
+    const sdp = this.localDescription.sdp;
+    const msections = sdputils.getMSections(sdp);
+    const expectedStreamIdCounts = new Map();
+    for (const {track, sender, streamId} of this.expectedLocalTrackInfo) {
+      const transceiver = this._pc.getTransceivers().find(t => t.sender == sender);
+      ok(transceiver, "There should be a transceiver for each sender");
+      if (transceiver.mid) {
+        const midFinder = new RegExp(`^a=mid:${transceiver.mid}$`, "m");
+        const msection = msections.find(m => m.match(midFinder));
+        ok(msection, `There should be a media section for mid = ${transceiver.mid}`);
+        ok(msection.startsWith(`m=${track.kind}`),
+          `Media section should be of type ${track.kind}`);
+        const msidFinder = new RegExp(`^a=msid:${streamId} \\S+$`, "m");
+        ok(msection.match(msidFinder),
+          `Should find a=msid:${streamId} in media section`
+          + " (with any track id for now)");
+        const count = expectedStreamIdCounts.get(streamId) || 0;
+        expectedStreamIdCounts.set(streamId, count + 1);
+      }
     };
 
-    checkSdpForMsids(this.localDescription, this.expectedSignalledTrackInfoById,
-                     "local");
-  },
-
-  markRemoteTracksAsNegotiated: function() {
-    Object.values(this.observedRemoteTrackInfoById).forEach(
-        trackInfo => trackInfo.negotiated = true);
-  },
-
-  rollbackRemoteTracksIfNotNegotiated: function() {
-    Object.keys(this.observedRemoteTrackInfoById).forEach(
-        id => {
-          if (!this.observedRemoteTrackInfoById[id].negotiated) {
-            delete this.observedRemoteTrackInfoById[id];
-          }
-        });
+    // Check for any unexpected msids.
+    const allMsids = sdp.match(new RegExp("^a=msid:\\S+", "mg"));
+    if (!allMsids) {
+      return;
+    }
+    const allStreamIds =
+      allMsids.map(msidAttr => msidAttr.replace('a=msid:', ''));
+    allStreamIds.forEach(id => {
+      const count = expectedStreamIdCounts.get(id);
+      ok(count, `Unexpected stream id ${id} found in local description.`);
+      if (count) {
+        expectedStreamIdCounts.set(id, count - 1);
+      }
+    });
   },
 
   /**
    * Check that media flow is present for the given media element by checking
    * that it reaches ready state HAVE_ENOUGH_DATA and progresses time further
    * than the start of the check.
    *
    * This ensures, that the stream being played is producing
@@ -1591,34 +1610,22 @@ PeerConnectionWrapper.prototype = {
                (t.currentDirection != "inactive") &&
                (t.currentDirection != "sendonly");
       })
       .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);
   },
 
   getExpectedSendTracks : function() {
-    return Object.keys(this.expectedLocalTrackInfoById)
-              .map(id => this.findSendTrackByWebrtcId(id));
-  },
-
-  findReceiveTrackByWebrtcId : function(webrtcId) {
-    return this._pc.getReceivers().map(receiver => receiver.track)
-              .find(track => this.getWebrtcTrackId(track) == webrtcId);
-  },
-
-  // Send tracks use the same identifiers that go in the signaling
-  findSendTrackByWebrtcId : function(webrtcId) {
-    return this._pc.getSenders().map(sender => sender.track)
-              .filter(track => track) // strip out null
-              .find(track => track.id == webrtcId);
+    return this._pc.getSenders().map(s => s.track).filter(t => t);
   },
 
   /**
    * Wait for presence of video flow on all media elements and rtp flow on
    * all sending and receiving track involved in this test.
    *
    * @returns {Promise}
    *        A promise that resolves when media flows for all elements and tracks
@@ -1868,17 +1875,17 @@ PeerConnectionWrapper.prototype = {
     }
 
     var nin = this._pc.getTransceivers()
       .filter(t => {
         return !t.stopped &&
                (t.currentDirection != "inactive") &&
                (t.currentDirection != "sendonly");
       }).length;
-    var nout = Object.keys(this.expectedLocalTrackInfoById).length;
+    const nout = Object.keys(this.expectedLocalTrackInfo).length;
     var ndata = this.dataChannels.length;
 
     // TODO(Bug 957145): Restore stronger inbound-rtp test once Bug 948249 is fixed
     //is((counters["inbound-rtp"] || 0), nin, "Have " + nin + " inbound-rtp stat(s)");
     ok((counters["inbound-rtp"] || 0) >= nin, "Have at least " + nin + " inbound-rtp stat(s) *");
 
     is(counters["outbound-rtp"] || 0, nout, "Have " + nout + " outbound-rtp stat(s)");
 
--- a/dom/media/tests/mochitest/templates.js
+++ b/dom/media/tests/mochitest/templates.js
@@ -192,30 +192,16 @@ var commandsPeerConnectionOfferAnswer = 
   function PC_LOCAL_SETUP_ICE_HANDLER(test) {
     test.pcLocal.setupIceCandidateHandler(test);
   },
 
   function PC_REMOTE_SETUP_ICE_HANDLER(test) {
     test.pcRemote.setupIceCandidateHandler(test);
   },
 
-  function PC_LOCAL_STEEPLECHASE_SIGNAL_EXPECTED_LOCAL_TRACKS(test) {
-    if (test.testOptions.steeplechase) {
-      send_message({"type": "local_expected_tracks",
-                    "expected_tracks": test.pcLocal.expectedLocalTrackInfoById});
-    }
-  },
-
-  function PC_REMOTE_STEEPLECHASE_SIGNAL_EXPECTED_LOCAL_TRACKS(test) {
-    if (test.testOptions.steeplechase) {
-      send_message({"type": "remote_expected_tracks",
-                    "expected_tracks": test.pcRemote.expectedLocalTrackInfoById});
-    }
-  },
-
   function PC_LOCAL_CREATE_OFFER(test) {
     return test.createOffer(test.pcLocal).then(offer => {
       is(test.pcLocal.signalingState, STABLE,
          "Local create offer does not change signaling state");
     });
   },
 
   function PC_LOCAL_STEEPLECHASE_SIGNAL_OFFER(test) {
@@ -296,18 +282,17 @@ var commandsPeerConnectionOfferAnswer = 
       });
   },
 
   function PC_REMOTE_SET_LOCAL_DESCRIPTION(test) {
     return test.setLocalDescription(test.pcRemote, test.originalAnswer, STABLE)
       .then(() => {
         is(test.pcRemote.signalingState, STABLE,
            "signalingState after remote setLocalDescription is 'stable'");
-      })
-      .then(() => test.pcRemote.markRemoteTracksAsNegotiated());
+      });
   },
 
   function PC_LOCAL_GET_ANSWER(test) {
     if (!test.testOptions.steeplechase) {
       test._remote_answer = test.originalAnswer;
       test._answer_constraints = test.pcRemote.constraints;
       return Promise.resolve();
     }
@@ -319,18 +304,17 @@ var commandsPeerConnectionOfferAnswer = 
     });
   },
 
   function PC_LOCAL_SET_REMOTE_DESCRIPTION(test) {
     return test.setRemoteDescription(test.pcLocal, test._remote_answer, STABLE)
       .then(() => {
         is(test.pcLocal.signalingState, STABLE,
            "signalingState after local setRemoteDescription is 'stable'");
-      })
-      .then(() => test.pcLocal.markRemoteTracksAsNegotiated());
+      });
   },
 
   function PC_REMOTE_SANE_LOCAL_SDP(test) {
     test.pcRemote.localRequiresTrickleIce =
       sdputils.verifySdp(test._remote_answer, "answer",
                          test._offer_constraints, test._offer_options,
                          test.testOptions);
   },
@@ -410,20 +394,20 @@ var commandsPeerConnectionOfferAnswer = 
 
   function PC_REMOTE_CHECK_ICE_CONNECTIONS(test) {
     return test.pcRemote.getStats().then(stats => {
       test.pcRemote.checkStatsIceConnections(stats, test.testOptions);
     });
   },
 
   function PC_LOCAL_CHECK_MSID(test) {
-    return test.pcLocal.checkMsids();
+    return test.pcLocal.checkLocalMsids();
   },
   function PC_REMOTE_CHECK_MSID(test) {
-    return test.pcRemote.checkMsids();
+    return test.pcRemote.checkLocalMsids();
   },
 
   function PC_LOCAL_CHECK_TRACK_STATS(test) {
     return checkAllTrackStats(test.pcLocal);
   },
   function PC_REMOTE_CHECK_TRACK_STATS(test) {
     return checkAllTrackStats(test.pcRemote);
   },
--- a/dom/media/tests/mochitest/test_peerConnection_remoteReofferRollback.html
+++ b/dom/media/tests/mochitest/test_peerConnection_remoteReofferRollback.html
@@ -31,18 +31,17 @@
             test.pcLocal.endOfTrickleIce.then(() => {
               send_message({"type": "end_of_trickle_ice"});
             });
           }
         },
 
         function PC_REMOTE_ROLLBACK(test) {
           return test.setRemoteDescription(test.pcRemote, { type: "rollback" },
-                                           STABLE)
-            .then(() => test.pcRemote.rollbackRemoteTracksIfNotNegotiated());
+                                           STABLE);
         },
 
         function PC_LOCAL_ROLLBACK(test) {
           // We haven't negotiated the new stream yet.
           test.pcLocal.expectNegotiationNeeded();
           return test.setLocalDescription(
               test.pcLocal,
               new RTCSessionDescription({ type: "rollback", sdp: ""}),
--- a/dom/media/tests/mochitest/test_peerConnection_remoteRollback.html
+++ b/dom/media/tests/mochitest/test_peerConnection_remoteRollback.html
@@ -16,18 +16,17 @@
     test = new PeerConnectionTest(options);
     test.setMediaConstraints([{audio: true}], [{audio: true}]);
     test.chain.removeAfter('PC_REMOTE_CHECK_CAN_TRICKLE_SYNC');
     test.chain.append([
       function PC_REMOTE_ROLLBACK(test) {
         // We still haven't negotiated the tracks
         test.pcRemote.expectNegotiationNeeded();
         return test.setRemoteDescription(test.pcRemote, { type: "rollback" },
-                                         STABLE)
-          .then(() => test.pcRemote.rollbackRemoteTracksIfNotNegotiated());
+                                         STABLE);
       },
 
       function PC_REMOTE_CHECK_CAN_TRICKLE_REVERT_SYNC(test) {
         is(test.pcRemote._pc.canTrickleIceCandidates, null,
            "Remote canTrickleIceCandidates is reverted to null");
       },
 
       function PC_LOCAL_ROLLBACK(test) {