Closed Bug 1355220 Opened 3 years ago Closed 3 years ago

Add sender.getStats() and receiver.getStats()

Categories

(Core :: WebRTC, enhancement, P1)

enhancement

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: jib, Assigned: ng)

References

(Blocks 1 open bug, )

Details

(Keywords: dev-doc-complete)

Attachments

(1 file)

The spec now has 

    interface RTCRtpSender {
        ...
        Promise<RTCStatsReport> getStats();
    };

    interface RTCRtpReceiver {
        ...
        Promise<RTCStatsReport> getStats();
    };

because sender.getStats() is superior to pc.getStats(sender),
  and receiver.getStats() is superior to pc.getStats(receiver).

This should be fairly simple to implement. I'd say we could do it entirely in PeerConnection.js, except there's a subtle difference between a sender/receiver and its track in edge-cases where the same track is used in multiple places. So we should probably wire the difference down to the c++ level.

If we do it that way, remember to not accidentally allow sender and receiver as selector arguments (they should still be disallowed from content JS).

[1] (github tip) https://rawgit.com/w3c/webrtc-pc/7f66f98a1c5a2f8668a4c9018adaec89ee254215/webrtc.html#idl-def-rtcrtpsender
Assignee: nobody → na-g
Comment on attachment 8861702 [details]
Bug 1355220 add RTCRtpSender/Receiver.getStats;

https://reviewboard.mozilla.org/r/133680/#review136552

::: dom/media/tests/mochitest/test_peerConnection_sender_and_receiver_stats.html:74
(Diff revision 1)
> +
> +  // This MUST be run after PC_*_WAIT_FOR_MEDIA_FLOW to ensure that we have RTP
> +  // before checking for RTCP.
> +  // It will throw UnsyncedRtcpError if it times out waiting for sync.
> +  var waitForSyncedRtcp = async pc => {
> +    

Zapped

::: dom/media/tests/mochitest/test_peerConnection_sender_and_receiver_stats.html:77
(Diff revision 1)
> +  // It will throw UnsyncedRtcpError if it times out waiting for sync.
> +  var waitForSyncedRtcp = async pc => {
> +    
> +    // Ensures that RTCP is present
> +    let ensureSyncedRtcp = async () => {
> +      

Zapped
Comment on attachment 8861702 [details]
Bug 1355220 add RTCRtpSender/Receiver.getStats;

https://reviewboard.mozilla.org/r/133682/#review136982

Code looks good. Mostly comments on the test, which I think could benefit from reorganizing a bit.

r- because I don't think we should be testing anything about the makeup of the stats ids, or that they match remotely, as they won't AFAIK, and it's not part of the spec.

::: dom/media/PeerConnection.js:1667
(Diff revision 1)
>      return this._pc._getParameters(this);
>    }
> +
> +  getStats() {
> +    return this._pc._async(
> +      async () => this._pc._getStats(this.__DOM_IMPL__.track));

Shouldn't need __DOM_IMPL__ here.

::: dom/media/PeerConnection.js:1683
(Diff revision 1)
>      Object.assign(this, { _pc: pc, track });
>    }
> +
> +  getStats() {
> +    return this._pc._async(
> +      async () => this._pc.getStats(this.__DOM_IMPL__.track));

same here

::: dom/media/tests/mochitest/test_peerConnection_sender_and_receiver_stats.html:16
(Diff revision 1)
> +    title: "RTCRtpSender.getStats() and RTCRtpReceiver.getStats()",
> +    visible: true
> +  });
> +
> +  var test;
> +  var pushPrefs = (...p) => SpecialPowers.pushPrefEnv({set: p});

unused

::: dom/media/tests/mochitest/test_peerConnection_sender_and_receiver_stats.html:18
(Diff revision 1)
> +  var checkStats = (sndReports, rcvReports) => {
> +    // Check that number of report sets is correct
> +    is(sndReports.length, 2, "Correct number of reports from RTPSenders.");
> +    is(rcvReports.length, 2, "Correct number of reports from RTPReceivers.");

This function seems to need only one sender report and one receiver report to do its job. I think that would be a useful simplification.

We could then drop the plural and remove a level of nesting, which I think would help.

Also the length == 2 check belongs a level up I think, where there's hopefully context explaining this invariant.

::: dom/media/tests/mochitest/test_peerConnection_sender_and_receiver_stats.html:23
(Diff revision 1)
> +    let getRemoteIds = (reportSet, idPrefix) => {
> +      let ids = [];
> +      reportSet.forEach(report => {
> +        report.forEach(statBlock => {

I'd recommend s/reportSet/reports/ and s/statBlock/stat/ to simplify the terms.

::: dom/media/tests/mochitest/test_peerConnection_sender_and_receiver_stats.html:38
(Diff revision 1)
> +    let sRemoteIds = getRemoteIds(sndReports, /^outbound/);
> +    let rRemoteIds = getRemoteIds(rcvReports, /^inbound/);
> +    is(sRemoteIds.join("|"), rRemoteIds.join("|"),
> +        "Sender and receiver remoteIds match.");

This part seems wrong. AFAIK there's no guarantee that these ids match up on the two sides if we e.g. send multiple tracks (I think I've even seen them not match up, i.e. switched). 

As far as the spec is concerned, I believe the ids can be total gibberish and need be unique only to the single report.

Instead we should check that the ssrc's match. Also, any patch without regexp is a better patch imho. ;)

::: dom/media/tests/mochitest/test_peerConnection_sender_and_receiver_stats.html:50
(Diff revision 1)
> +  var getSenderStatsReports = async pc => {
> +    return await Promise.all(
> +        [...pc.getSenders().map(async s => s.getStats())]);
> +  };
> +
> +  var getReceiverStatsReports = async pc => {
> +    return await Promise.all(
> +        [...pc.getReceivers().map(async s => s.getStats())]);
> +  };

I'd inline these. They appear to merely replace crisp JS with concatenated English.

Also, getSenders() already returns an array, so [...] is redundant. Lastly, I'd s/async// inside the .map() since it's not needed. E.g.:

    let senderReports = await Promise.all(pc.getSenders().map(s => s.getStats())]);

But see comment elsewhere about avoiding bundling.

::: dom/media/tests/mochitest/test_peerConnection_sender_and_receiver_stats.html:60
(Diff revision 1)
> +  // Error that indicates that RTCP isn't synced between the local and remote,
> +  //  senders and receivers.
> +  function UnsyncedRtcpError(message) {
> +    this.name = 'UnsyncedRtcpError';
> +    this.message = message;
> +    this.stack = (new Error()).stack;
> +  }
> +  UnsyncedRtcpError.prototype = Object.create(Error.prototype);
> +  UnsyncedRtcpError.prototype.constructor = UnsyncedRtcpError;

See comment below on why I think this is too much, but if it stays I'd prefer new class extends JS syntax.

::: dom/media/tests/mochitest/test_peerConnection_sender_and_receiver_stats.html:73
(Diff revision 1)
> +  var waitForSyncedRtcp = async pc => {
> +    
> +    // Ensures that RTCP is present
> +    let ensureSyncedRtcp = async () => {
> +      
> +      let senderStatsReports = await getSenderStatsReports(pc);
> +      let receiverStatsReports = await getReceiverStatsReports(pc);
> +      let reports = [...senderStatsReports, ...receiverStatsReports];
> +      for (let stats of reports) {
> +        for (let [k, v] of stats) {
> +          if (v.type.endsWith("bound-rtp") && !v.remoteId) {
> +            throw new UnsyncedRtcpError(v.id + " is missing remoteId: "
> +              + JSON.stringify(v));
> +          }
> +          if (v.type == "inbound-rtp" && v.isRemote == true
> +              && v.roundTripTime === undefined) {
> +            throw new UnsyncedRtcpError(v.id + " is missing roundTripTime: "
> +              + JSON.stringify(v));
> +          }
> +        }
> +      }
> +      return [senderStatsReports, receiverStatsReports];
> +    }

Bulking retrieval of all sender and receiver stats too early seems like a missed opportunity for code reuse. Can waiting for all these instead be done higher up?

Then this seems to reduce down to mostly a rehash of waitForSyncedRtcp from test_peerConnection_stats.html [1] which I'd love to see moved to pc.js and reused to avoid duplicating this already rather non-trivial helper. E.g. (assuming we did audio and video individually, there's only two), we could call it like this for each:

    let [sr, rr] = await Promise.all([waitForSyncedRtcpStats(() => sender.getStats()),
                                      waitForSyncedRtcpStats(() => receiver.getStats()]);
    checkStats(sr, rr);

Thoughts?

[1] https://dxr.mozilla.org/mozilla-central/rev/f229b7e5d91eb70d23d3e31db7caff9d69a2ef04/dom/media/tests/mochitest/test_peerConnection_stats.html#376

::: dom/media/tests/mochitest/test_peerConnection_sender_and_receiver_stats.html:74
(Diff revision 1)
> +    
> +    // Ensures that RTCP is present
> +    let ensureSyncedRtcp = async () => {
> +      

trailing spaces

::: dom/media/tests/mochitest/test_peerConnection_sender_and_receiver_stats.html:99
(Diff revision 1)
> +      try {
> +        return await ensureSyncedRtcp();
> +      } catch (e) {
> +        // Only handle UnsyncedRtcpError, everything else should percolate up
> +        if (!(e instanceof UnsyncedRtcpError)) {
> +          error(e.stack);
> +          throw e;
> +        }

I dislike overloading errors (using errors to pass information), especially for no reason, like here. Why not have ensureSyncedRtcp() return null instead of throwing?

IMHO errors should be exceptional and not part of the API or normal flow.

::: dom/media/tests/mochitest/test_peerConnection_sender_and_receiver_stats.html:115
(Diff revision 1)
> +  var PC_LOCAL_TEST_LOCAL_STATS = test => {
> +    return waitForSyncedRtcp(test.pcLocal).then(
> +      ([senderReports, receiverReports]) => {
> +        checkStats(senderReports, receiverReports);
> +    });
> +  }
> +
> +  var PC_REMOTE_TEST_REMOTE_STATS = test => {
> +    return waitForSyncedRtcp(test.pcRemote).then(
> +      ([senderReports, receiverReports]) => {
> +        checkStats(senderReports, receiverReports);
> +    });
> +  }
> +
> +  var test;
> +  runNetworkTest(function (options) {
> +    test = new PeerConnectionTest(options);
> +
> +    test.chain.insertAfter("PC_LOCAL_WAIT_FOR_MEDIA_FLOW",
> +      [PC_LOCAL_TEST_LOCAL_STATS]);
> +
> +    test.chain.insertAfter("PC_REMOTE_WAIT_FOR_MEDIA_FLOW",
> +      [PC_REMOTE_TEST_REMOTE_STATS]);

I'd inline these as well using named function declarations. Also, [] is redundant with insertAfter(). We can also use async here as well. E.g.

    test.chain.insertAfter("PC_LOCAL_WAIT_FOR_MEDIA_FLOW",
      async function PC_LOCAL_TEST_LOCAL_STATS(test) {
          
      });
Attachment #8861702 - Flags: review?(jib) → review-
Also don't forget to add a DOM reviewer, and give them the URL link to github tip.
Blocks: 1359872
Flags: needinfo?(na-g)
Comment on attachment 8861702 [details]
Bug 1355220 add RTCRtpSender/Receiver.getStats;

https://reviewboard.mozilla.org/r/133682/#review136982

> Bulking retrieval of all sender and receiver stats too early seems like a missed opportunity for code reuse. Can waiting for all these instead be done higher up?
> 
> Then this seems to reduce down to mostly a rehash of waitForSyncedRtcp from test_peerConnection_stats.html [1] which I'd love to see moved to pc.js and reused to avoid duplicating this already rather non-trivial helper. E.g. (assuming we did audio and video individually, there's only two), we could call it like this for each:
> 
>     let [sr, rr] = await Promise.all([waitForSyncedRtcpStats(() => sender.getStats()),
>                                       waitForSyncedRtcpStats(() => receiver.getStats()]);
>     checkStats(sr, rr);
> 
> Thoughts?
> 
> [1] https://dxr.mozilla.org/mozilla-central/rev/f229b7e5d91eb70d23d3e31db7caff9d69a2ef04/dom/media/tests/mochitest/test_peerConnection_stats.html#376

waitForSyncedRtcp has been moved to pc.js. After moving to 1 send only and 1 receive only PC, I am seeing some unexpected behavior. I am investigating.
Comment on attachment 8861702 [details]
Bug 1355220 add RTCRtpSender/Receiver.getStats;

https://reviewboard.mozilla.org/r/133682/#review144096

Looks great! Hope you find the unexpected behavior. 

Does this mean we can get rid of waitForSyncedRtcp in test_peerConnection_stats.html?

Misc nits.

::: dom/media/PeerConnection.js:1689
(Diff revision 2)
> +  getStats() {
> +    return this._pc._async(
> +      async () => this._pc.getStats(this.__DOM_IMPL__.track));

Don't need __DOM_IMPL__ here.

::: dom/media/tests/mochitest/pc.js:1518
(Diff revision 2)
> +          return null;
> +        }
> +      }
> +      return report;
> +    }
> +    let failures = 0;

s/failures/retries/ ?

::: dom/media/tests/mochitest/pc.js:1523
(Diff revision 2)
> +      try {
> +        let syncedStats = await ensureSyncedRtcp();
> +        if (syncedStats) {
> +          return syncedStats;
> +        }
> +      } catch (e) {
> +          info(e);
> +          info(e.stack);
> +          throw e;
> +      }

The try{}catch{} here should be unnecessary now.

::: dom/media/tests/mochitest/pc.js:1534
(Diff revision 2)
> +          info(e);
> +          info(e.stack);
> +          throw e;
> +      }
> +      failures += 1;
> +      info("waitForSyncedRtcp FAILURE #" + failures + ", retrying.\n");

They're not really failures anymore. s/FAILURE/no RTCP/ ?

::: dom/media/tests/mochitest/test_peerConnection_sender_and_receiver_stats.html:18
(Diff revision 2)
> +  });
> +
> +  var test;
> +
> +  var checkStats = (sndReport, rcvReport, mediaType) => {
> +    // Returns remote IDs with the expected prefix removed

update comment

::: dom/media/tests/mochitest/test_peerConnection_sender_and_receiver_stats.html:21
(Diff revision 2)
> +      report.forEach(stat => {
> +        if (stat.type.endsWith("bound-rtp")) {
> +          is(stat.mediaType, kind, "mediaType of " + stat.id + " is "
> +             + stat.mediaType + ", expected type is " + kind);

is() already outputs the expected type if there's a discrepancy.

We could also use filter() and map() instead of forEach() here if you want.

::: dom/media/tests/mochitest/test_peerConnection_sender_and_receiver_stats.html:46
(Diff revision 2)
> +        return Promise.all([test.pcLocal.waitForSyncedRtcp(),
> +                       test.pcRemote.waitForSyncedRtcp()])
> +                  .then(async () =>
> +          {

funny indents

::: dom/media/tests/mochitest/test_peerConnection_sender_and_receiver_stats.html:55
(Diff revision 2)
> +            // TODO check stats here
> +            let senders = test.pcLocal.getSenders();
> +            let receivers = test.pcRemote.getReceivers();
> +            is(senders.length, 2, "Have exactly two senders.");
> +            is(receivers.length, 2, "Have exactly two receivers.");
> +            for( type of ["audio", "video"]) {

This sets window.type. Use let.

::: dom/media/tests/mochitest/test_peerConnection_sender_and_receiver_stats.html:59
(Diff revision 2)
> +            is(receivers.length, 2, "Have exactly two receivers.");
> +            for( type of ["audio", "video"]) {
> +              let senderStats =
> +                  await senders.find(s => s.track.kind == type).getStats();
> +              is(senders.filter(s => s.track.kind == type).length, 1,
> +                  "Exactl 1 sender of kind " + type);

Exactly

::: dom/media/tests/mochitest/test_peerConnection_sender_and_receiver_stats.html:63
(Diff revision 2)
> +              is(senders.filter(s => s.track.kind == type).length, 1,
> +                  "Exactl 1 sender of kind " + type);
> +              let receiverStats =
> +                  await receivers.find(r => r.track.kind == type).getStats();
> +              is(receivers.filter(r => r.track.kind == type).length, 1,
> +                  "Exactl 1 receiver of kind " + type);

Exactly
Attachment #8861702 - Flags: review?(jib) → review+
Comment on attachment 8861702 [details]
Bug 1355220 add RTCRtpSender/Receiver.getStats;

https://reviewboard.mozilla.org/r/133682/#review147050

r+ for the webidl
Attachment #8861702 - Flags: review+
Pushed by na-g@nostrum.com:
https://hg.mozilla.org/integration/autoland/rev/cc7b19b52df4
add RTCRtpSender/Receiver.getStats;r=jib,smaug
https://hg.mozilla.org/mozilla-central/rev/cc7b19b52df4
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Flags: needinfo?(na-g)
Need MDN docs for all three variants, on sender, receiver and pc.
Keywords: dev-doc-needed
Courtesy of jib, this is now listed on Firefox 55 for developers, but as he says above, WebRTC stats are not currently documented very much. That should start changing before long, but can't give any guarantees when.

Leaving dev-doc-needed on this until it's fully handled.
You need to log in before you can comment on or make changes to this bug.