1509669 - Stats for packets received by remote looks wrong

Status: RESOLVED DUPLICATE of bug 1654112

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

Description

[Byron Campen [:bwc]]

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!

Comment 1

[Byron Campen [:bwc]]

The same seems to be true for bytesReceived.

Comment 2

[Byron Campen [:bwc]]

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.

Comment 3

[Nico Grunbaum [:ng, @chew:mozilla.org]]

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.

Comment 4

[Byron Campen [:bwc]]

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

Comment 5

[Nico Grunbaum [:ng, @chew:mozilla.org]]

(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

Comment 6

[Byron Campen [:bwc]]

Attachment: Bug 1509669 - Part 1: Remove some bogus stats from webidl.

Comment 7

[Byron Campen [:bwc]]

Attachment: Bug 1509669 - Part 2: Remove some bogus stats from our c++ code. r?ng

Depends on D26340

Comment 8

[Byron Campen [:bwc]]

Attachment: Bug 1509669 - Part 3: Change the descriptions of some telemetry histograms to indicate that they were populated with garbage data.

Depends on D26341

Comment 9

[Byron Campen [:bwc]]

Attachment: Bug 1509669 - Part 0: Remove some mochitest logic that was expecting the bogus stats.

Depends on D26343

Comment 10

[Byron Campen [:bwc]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e79df3da345a25921d6129c8dba79e6edbff62e0

Comment 11

[Nico Grunbaum [:ng, @chew:mozilla.org]]

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

Comment 12

[Byron Campen [:bwc]]

(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.

Comment 13

[Byron Campen [:bwc]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d53d9399bb9d74e8a3637dc6da6ce0ad7fa1f49b

Comment 14

[Byron Campen [:bwc]]

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.

Comment 15

[Nico Grunbaum [:ng, @chew:mozilla.org]]

Yes. Reviewed.

Comment 16

[Byron Campen [:bwc]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9826205b9fa85c7e15d72fd2891002b5c16b8e27

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/838848b7451d
Part 0: Remove some mochitest logic that was expecting the bogus stats. r=ng

Comment 18

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/838848b7451d

Comment 19

[Byron Campen [:bwc]]

Check up on these intermittents in a few days.

Comment 28

[Intermittent Failures Robot]

5 failures in 2673 pushes (0.002 failures/push) were associated with this bug in the last 7 days.

Repository breakdown:

• autoland: 4
• mozilla-beta: 1

Platform breakdown:

• linux64: 4
• linux64-qr: 1

For more details, see:
https://treeherder.mozilla.org/intermittent-failures.html#/bugdetails?bug=1509669&startday=2019-04-15&endday=2019-04-21&tree=all

Comment 29

[Intermittent Failures Robot]

2 failures in 3031 pushes (0.001 failures/push) were associated with this bug in the last 7 days.

Repository breakdown:

• mozilla-beta: 2

Platform breakdown:

• linux64-qr: 1
• linux32: 1

For more details, see:
https://treeherder.mozilla.org/intermittent-failures.html#/bugdetails?bug=1509669&startday=2019-04-22&endday=2019-04-28&tree=all

Comment 30

[Intermittent Failures Robot]

1 failures in 4705 pushes (0.0 failures/push) were associated with this bug in the last 7 days.

Repository breakdown:

• mozilla-beta: 1

Platform breakdown:

• linux64: 1

For more details, see:
https://treeherder.mozilla.org/intermittent-failures.html#/bugdetails?bug=1509669&startday=2019-05-06&endday=2019-05-12&tree=all

Comment 31

[Intermittent Failures Robot]

1 failures in 4337 pushes (0.0 failures/push) were associated with this bug in the last 7 days.

Repository breakdown:

• mozilla-release: 1

Platform breakdown:

• linux32: 1

For more details, see:
https://treeherder.mozilla.org/intermittent-failures.html#/bugdetails?bug=1509669&startday=2019-05-27&endday=2019-06-02&tree=all

Comment 32

[Intermittent Failures Robot]

1 failures in 2581 pushes (0.0 failures/push) were associated with this bug in the last 7 days.

Repository breakdown:

• mozilla-release: 1

Platform breakdown:

• linux64: 1

For more details, see:
https://treeherder.mozilla.org/intermittent-failures.html#/bugdetails?bug=1509669&startday=2019-06-17&endday=2019-06-23&tree=all

Comment 33

[BugBot [:suhaib / :marco/ :calixte]]

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

Comment 34

[Byron Campen [:bwc]]

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

Comment 35

[Nico Grunbaum [:ng, @chew:mozilla.org]]

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

[BugBot [:suhaib / :marco/ :calixte]]

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

Comment 37

[Byron Campen [:bwc]]

Where are we on the spec here?

Comment 38

[Nico Grunbaum [:ng, @chew:mozilla.org]]

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

Comment 39

[BugBot [:suhaib / :marco/ :calixte]]

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

[Nico Grunbaum [:ng, @chew:mozilla.org]]

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

Comment 41

[BugBot [:suhaib / :marco/ :calixte]]

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

Comment 42

[BugBot [:suhaib / :marco/ :calixte]]

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.

Comment 43

[Byron Campen [:bwc]]

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.