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

Status: RESOLVED FIXED

(Core :: WebRTC, enhancement, P1)

Description

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

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

Comment 1

[Nico Grunbaum [:ng, @chew:mozilla.org]]

Attachment: Bug 1355220 add RTCRtpSender/Receiver.getStats;

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

Comment 2

[Nico Grunbaum [:ng, @chew:mozilla.org]]

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 3

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

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) {
          
      });

Comment 4

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Also don't forget to add a DOM reviewer, and give them the URL link to github tip.

Comment 5

[Nico Grunbaum [:ng, @chew:mozilla.org]]

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 6

[Nico Grunbaum [:ng, @chew:mozilla.org]]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/133682/diff/1-2/

Comment 7

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

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

Comment 8

[Nico Grunbaum [:ng, @chew:mozilla.org]]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/133682/diff/2-3/

Comment 9

[Nico Grunbaum [:ng, @chew:mozilla.org]]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/133682/diff/3-4/

Comment 10

[Nico Grunbaum [:ng, @chew:mozilla.org]]

WebIDL review notes
spec: https://rawgit.com/w3c/webrtc-pc/master/webrtc.html#idl-def-rtcrtpsender

Comment 11

[Olli Pettay [:smaug][[e-mail]]]

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

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

r+ for the webidl

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/cc7b19b52df4
add RTCRtpSender/Receiver.getStats;r=jib,smaug

Comment 13

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/cc7b19b52df4

Comment 14

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Need MDN docs for all three variants, on sender, receiver and pc.

Comment 15

[Eric Shepherd [:sheppy]]

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.

Comment 16

[Eric Shepherd [:sheppy]]

Documentation added:

https://developer.mozilla.org/en-US/docs/Web/API/RTCRtpSender/getStats
https://developer.mozilla.org/en-US/docs/Web/API/RTCRtpReceiver/getStats

Documentation updated:

https://developer.mozilla.org/en-US/docs/Web/API/RTCRtpSender (added getStats() to the method list)