WebRTC packetloss telemetry should be rate, not total packets lost

RESOLVED FIXED in Firefox 41

Status

()

P1
normal
Rank:
19
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: jesup, Assigned: jesup)

Tracking

(Depends on: 1 bug)

Trunk
mozilla43
Points:
---

Firefox Tracking Flags

(firefox41 fixed, firefox42 fixed, firefox43 fixed)

Details

Attachments

(1 attachment)

(Assignee)

Description

3 years ago
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.
(Assignee)

Updated

3 years ago
Depends on: 1189099
(Assignee)

Updated

3 years ago
backlog: --- → webRTC+
Rank: 19
Priority: -- → P1
(Assignee)

Comment 1

3 years ago
Created attachment 8645825 [details] [diff] [review]
switch packetloss to a rate from total-packets-lost-per-update
Attachment #8645825 - Flags: review?(jib)
(Assignee)

Updated

3 years ago
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
Last Resolved: 3 years ago
status-firefox43: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
(Assignee)

Comment 5

3 years ago
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?
(Assignee)

Updated

3 years ago
Attachment #8645825 - Flags: approval-mozilla-beta?
(Assignee)

Updated

3 years ago
status-firefox41: --- → affected
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.