If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

WebRTC packetloss telemetry should be rate, not total packets lost

RESOLVED FIXED in Firefox 41

Status

()

Core
WebRTC
P1
normal
Rank:
19
RESOLVED FIXED
2 years ago
2 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

2 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

2 years ago
Depends on: 1189099
(Assignee)

Updated

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

Comment 1

2 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

2 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+

Comment 3

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/bd0b693e8c8f
https://hg.mozilla.org/mozilla-central/rev/bd0b693e8c8f
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox43: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
(Assignee)

Comment 5

2 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

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

Updated

2 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+
https://hg.mozilla.org/releases/mozilla-aurora/rev/cfcd91bd5aa2
status-firefox42: affected → fixed
https://hg.mozilla.org/releases/mozilla-beta/rev/cc10bb0cfab3
status-firefox41: affected → fixed
You need to log in before you can comment on or make changes to this bug.