Stats for packets received by remote looks wrong
Categories
(Core :: WebRTC: Audio/Video, defect, P2)
Tracking
()
People
(Reporter: bwc, Assigned: bwc)
References
Details
Attachments
(4 files)
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!
Assignee | ||
Comment 1•6 years ago
|
||
The same seems to be true for bytesReceived.
Updated•6 years ago
|
Assignee | ||
Updated•6 years ago
|
Assignee | ||
Comment 2•6 years ago
|
||
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.
Assignee | ||
Updated•6 years ago
|
Comment 3•6 years ago
|
||
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.
Assignee | ||
Comment 4•6 years ago
|
||
Ok, then let's remove it from our implementation, and file a spec bug.
Assignee | ||
Updated•6 years ago
|
Comment 5•6 years ago
|
||
(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
Assignee | ||
Comment 6•6 years ago
|
||
Assignee | ||
Comment 7•6 years ago
|
||
Depends on D26340
Assignee | ||
Comment 8•6 years ago
|
||
Depends on D26341
Assignee | ||
Comment 9•6 years ago
|
||
Depends on D26343
Assignee | ||
Comment 10•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e79df3da345a25921d6129c8dba79e6edbff62e0
Comment 11•6 years ago
|
||
I don't think we should land this until the spec work has gone through, as this field is in use in the wild.
Assignee | ||
Comment 12•6 years ago
|
||
(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.
Assignee | ||
Updated•6 years ago
|
Updated•6 years ago
|
Updated•6 years ago
|
Updated•6 years ago
|
Updated•6 years ago
|
Assignee | ||
Comment 13•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d53d9399bb9d74e8a3637dc6da6ce0ad7fa1f49b
Assignee | ||
Comment 14•5 years ago
|
||
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.
Assignee | ||
Comment 16•5 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=9826205b9fa85c7e15d72fd2891002b5c16b8e27
Assignee | ||
Updated•5 years ago
|
Comment 17•5 years ago
|
||
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
Comment 18•5 years ago
|
||
bugherder |
Assignee | ||
Comment 19•5 years ago
|
||
Check up on these intermittents in a few days.
Assignee | ||
Updated•5 years ago
|
Comment hidden (Intermittent Failures Robot) |
Comment hidden (Intermittent Failures Robot) |
Comment hidden (Intermittent Failures Robot) |
Comment hidden (Intermittent Failures Robot) |
Comment hidden (Intermittent Failures Robot) |
Comment 33•5 years ago
|
||
The leave-open keyword is there and there is no activity for 6 months.
:bwc, maybe it's time to close this bug?
Assignee | ||
Comment 34•5 years ago
|
||
Nope. We're waiting on spec work to finish this bug off. Is this spec work done?
Comment 35•5 years ago
|
||
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.
Comment 36•4 years ago
|
||
The leave-open keyword is there and there is no activity for 6 months.
:bwc, maybe it's time to close this bug?
Assignee | ||
Comment 37•4 years ago
|
||
Where are we on the spec here?
Comment 38•4 years ago
|
||
Nothing has changed in the spec, I think we can fairly safely call the spec stable at this point.
Comment 39•4 years ago
|
||
The leave-open keyword is there and there is no activity for 6 months.
:bwc, maybe it's time to close this bug?
Comment 40•4 years ago
|
||
This will be fixed in bug 1654112, I am leaving this open until the merge lands.
Comment 41•3 years ago
|
||
The leave-open keyword is there and there is no activity for 6 months.
:bwc, maybe it's time to close this bug?
Assignee | ||
Updated•3 years ago
|
Comment 42•3 years ago
|
||
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.
Assignee | ||
Comment 43•3 years ago
|
||
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.
Assignee | ||
Updated•3 years ago
|
Updated•3 years ago
|
Description
•