Closed Bug 1509669 Opened 6 years ago Closed 3 years ago

Stats for packets received by remote looks wrong

Categories

(Core :: WebRTC: Audio/Video, defect, P2)

Product:
Core
Component:
WebRTC: Audio/Video
Type:
defect

Tracking

()

Status:
RESOLVED DUPLICATE of bug 1654112

People

(Reporter: bwc, Assigned: bwc)

References

Details

Attachments

(4 files)

Bug 1509669 - Part 1: Remove some bogus stats from webidl.
(deleted), text/x-phabricator-request
Details
Bug 1509669 - Part 2: Remove some bogus stats from our c++ code. r?ng
(deleted), text/x-phabricator-request
Details
Bug 1509669 - Part 3: Change the descriptions of some telemetry histograms to indicate that they were populated with garbage data.
(deleted), text/x-phabricator-request
Details
Bug 1509669 - Part 0: Remove some mochitest logic that was expecting the bogus stats.
(deleted), text/x-phabricator-request
Details 
I keep on running into mochitest failures on try where the stats for packets received by remote (which is supposed to come from RTCP RRs) is higher than the number of packets we have actually transmitted. Suspiciously, this erroneously high value happens to match the number of frames webrtc.org claims to have encoded. Even more suspiciously, the code that fetches this statistic seems to be using webrtc.org's internal count for packets _sent_!

https://searchfox.org/mozilla-central/source/media/webrtc/signaling/src/media-conduit/VideoConduit.cpp#375

It really looks like VideoConduit can't get the received packet count (according to RTCP RR), and is fudging the data by just using the packet count webrtc.org thinks it has sent!
The same seems to be true for bytesReceived.
Rank: 15
Priority: -- → P2
Type: enhancement → defect

Updated searchfox link: https://searchfox.org/mozilla-central/rev/44a212460990ffffecf50a8e972d3cbde2e7216b/media/webrtc/signaling/src/media-conduit/VideoConduit.cpp#347

@ng, is this something we can fix, given what webrtc.org gives us? If not, I think we need to disable this checking, and I would argue that we should not emit these misleading statistics in the first place.

Flags: needinfo?(na-g)

There is no received packet count in RTCP RRs. This value has never been real or knowable. The definition in the spec is bogus, as it doesn't take into account retransmission's or duplicate packets as it should. It is durtles all the way down.

Ok, then let's remove it from our implementation, and file a spec bug.

Flags: needinfo?(na-g)
Assignee: nobody → docfaraday

(In reply to Byron Campen [:bwc] from comment #4)

Ok, then let's remove it from our implementation, and file a spec bug.

Apologies, I keep running into comment glare.

I definitely would like to remove this stat. There is recognition on the Chrome team that this is number is bogus. I think the stat needs to be moved off of RTCReceivedRtpStreamStats and onto RTCPInboundRtpStreamStats. It may need to be moved to the legacy section, which would allow Chrome to continue to report it.

CC'ing :jib

Depends on D26340

I don't think we should land this until the spec work has gone through, as this field is in use in the wild.

(In reply to Nico Grunbaum [:ng] from comment #11)

I don't think we should land this until the spec work has gone through, as this field is in use in the wild.

Fair enough, we can land the mochitest changes first and then leave this bug open for the rest.

Attachment #9056172 - Attachment description: Bug 1509669 - Part 3: Remove some mochitest logic that was expecting the bogus stats. → Bug 1509669 - Part 0: Remove some mochitest logic that was expecting the bogus stats.
Attachment #9056168 - Attachment description: Bug 1509669 - Part 0: Remove some bogus stats from webidl. → Bug 1509669 - Part 1: Remove some bogus stats from webidl.
Attachment #9056169 - Attachment description: Bug 1509669 - Part 1: Remove some bogus stats from our c++ code. r?ng → Bug 1509669 - Part 2: Remove some bogus stats from our c++ code. r?ng
Attachment #9056171 - Attachment description: Bug 1509669 - Part 2: Change the descriptions of some telemetry histograms to indicate that they were populated with garbage data. → Bug 1509669 - Part 3: Change the descriptions of some telemetry histograms to indicate that they were populated with garbage data.

Would it be acceptable to review and land part 0, and leave this bug open for the rest once the spec is settled? We need to kill this intermittent somehow.

Flags: needinfo?(na-g)

Yes. Reviewed.

Flags: needinfo?(na-g)
Keywords: leave-open
Pushed by bcampen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/838848b7451d
Part 0: Remove some mochitest logic that was expecting the bogus stats. r=ng

Check up on these intermittents in a few days.

Flags: needinfo?(docfaraday)
No longer blocks: 1521731, 1530013, 1530113, 1530750, 1531619, 1532515, 1532543, 1538740
Flags: needinfo?(docfaraday)

The leave-open keyword is there and there is no activity for 6 months.
:bwc, maybe it's time to close this bug?

Flags: needinfo?(docfaraday)

Nope. We're waiting on spec work to finish this bug off. Is this spec work done?

Flags: needinfo?(docfaraday) → needinfo?(na-g)

TLDR; bytesReceived can be nixed, packetsReceived stays

bytesReceived has been moved to: RTCInboundRtpStreamStats https://w3c.github.io/webrtc-stats/#dom-rtcinboundrtpstreamstats

packetsReceived however is still explicitly overloaded as an estimate on the receive side. The libwebrtc implementation in tree does match the spec, but I should note the I don't think libwebrtc stats have handled RTP sequence number rollover correctly in the past.

Flags: needinfo?(na-g)

The leave-open keyword is there and there is no activity for 6 months.
:bwc, maybe it's time to close this bug?

Flags: needinfo?(docfaraday)

Where are we on the spec here?

Flags: needinfo?(docfaraday) → needinfo?(na-g)

Nothing has changed in the spec, I think we can fairly safely call the spec stable at this point.

Flags: needinfo?(na-g)

The leave-open keyword is there and there is no activity for 6 months.
:bwc, maybe it's time to close this bug?

Flags: needinfo?(docfaraday)

This will be fixed in bug 1654112, I am leaving this open until the merge lands.

Depends on: 1654112
Flags: needinfo?(docfaraday)

The leave-open keyword is there and there is no activity for 6 months.
:bwc, maybe it's time to close this bug?

Flags: needinfo?(docfaraday)
Flags: needinfo?(docfaraday)

The leave-open keyword is there and there is no activity for 6 months.
:bwc, maybe it's time to close this bug?
For more information, please visit auto_nag documentation.

Flags: needinfo?(docfaraday)

packetsReceived was fixed in https://phabricator.services.mozilla.com/D108671 and https://phabricator.services.mozilla.com/D125709 (as much as possible; it is still a flawed estimate, but at least we're estimating it according to spec now). We do not seem to have re-implemented the testing in pc.js for packetsReceived though; that may need to be done, but that's a separate issue. As for bytesReceived, that has been removed from the spec for remote inbound stats.

Status: NEW → RESOLVED
Closed: 3 years ago
Flags: needinfo?(docfaraday)
Resolution: --- → DUPLICATE
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: