Closed Bug 1188407 Opened 9 years ago Closed 9 years ago

WebRTC packetloss telemetry should be rate, not total packets lost

Categories

(Core :: WebRTC, defect, P1)

defect

Tracking

()

RESOLVED FIXED
mozilla43
Tracking Status
firefox41 --- fixed
firefox42 --- fixed
firefox43 --- fixed

People

(Reporter: jesup, Assigned: jesup)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

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.
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: 9 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+
No longer depends on: webrtc-telemetry
You need to log in before you can comment on or make changes to this bug.