WebRTC packetloss telemetry should be rate, not total packets lost

RESOLVED FIXED in Firefox 41

Status

()

defect
P1
normal
Rank:
19
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: jesup, Assigned: jesup)

Tracking

(Depends on 1 bug)

Trunk
mozilla43
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox41 fixed, firefox42 fixed, firefox43 fixed)

Details

Attachments

(1 attachment)

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.
Depends on: 1189099
backlog: --- → webRTC+
Rank: 19
Priority: -- → P1
Assignee: nobody → rjesup
Status: NEW → ASSIGNED
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.
Attachment #8645825 - Flags: review?(jib) → review+
https://hg.mozilla.org/mozilla-central/rev/bd0b693e8c8f
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
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
Attachment #8645825 - Flags: approval-mozilla-aurora?
Attachment #8645825 - Flags: approval-mozilla-beta?
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.
Attachment #8645825 - Flags: approval-mozilla-beta?
Attachment #8645825 - Flags: approval-mozilla-beta+
Attachment #8645825 - Flags: approval-mozilla-aurora?
Attachment #8645825 - Flags: approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.