1188407 - WebRTC packetloss telemetry should be rate, not total packets lost

Status: RESOLVED FIXED

(Core :: WebRTC, defect, P1)

Description

[Randell Jesup [:jesup] (needinfo me)]

Audio and video packetloss telemetry appears to be packets lost, not loss rate.  Also, audio inbound packetloss is always 0.

Best might be overall rate, and then a distribution-within-call telemetry (loss as measured every second).  First we need to fix the overall values though.

Comment 1

[Randell Jesup [:jesup] (needinfo me)]

Attachment: switch packetloss to a rate from total-packets-lost-per-update

Comment 2

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

Comment on attachment 8645825 [details] [diff] [review]
switch packetloss to a rate from total-packets-lost-per-update

Review of attachment 8645825 [details] [diff] [review]:
-----------------------------------------------------------------

lgtm.

::: media/webrtc/signaling/src/peerconnection/PeerConnectionCtx.cpp
@@ +220,5 @@
>        for (decltype(array.Length()) i = 0; i < array.Length(); i++) {
>          auto& s = array[i];
>          bool isAudio = (s.mId.Value().Find("audio") != -1);
> +        if (s.mPacketsLost.WasPassed() && s.mPacketsReceived.WasPassed() &&
> +            (s.mPacketsLost.Value()+s.mPacketsReceived.Value()) != 0) {

nit: space around +

@@ +231,5 @@
> +                           WEBRTC_VIDEO_QUALITY_INBOUND_PACKETLOSS_RATE;
> +          }
> +          // *1000 so we can read in 10's of a percent (permille)
> +          Accumulate(id,
> +                     (s.mPacketsLost.Value()*1000) /

space around *, and parenthesis are not technically needed. Up to you.

Comment 3

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/bd0b693e8c8f

Comment 4

[Ryan VanderMeulen [:RyanVM]]

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

Comment 5

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8645825 [details] [diff] [review]
switch packetloss to a rate from total-packets-lost-per-update

Approval Request Comment
[Feature/regressing bug #]: N/A

[User impact if declined]: Packet loss rate Telemetry not useful

[Describe test coverage new/current, TreeHerder]: N/A

[Risks and why]:  Exceedingly low risk

[String/UUID change made/needed]: none

Comment 6

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8645825 [details] [diff] [review]
switch packetloss to a rate from total-packets-lost-per-update

Early in the beta cycle, should be low risk, taking it.

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/cfcd91bd5aa2

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/cc10bb0cfab3