Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

RTCRemoteInboundRtpStreamStats - plus alignment with outbound #32657

Merged
merged 31 commits into from
Aug 2, 2024

Conversation

hamishwillee
Copy link
Collaborator

This adds docs for RTCRemoteInboundRtpStreamStats , a stats object/dictionary you can get by iterating a RTCStatsReport.

It is not very exciting. Largely following the same patterns as the other docs.

This shares some common properties with RTCRemoteOutoundRtpStreamStats. There have been some changes to align those.

This is part of the ongoing work to finished RTC stats, which was started in #27028

@github-actions github-actions bot added Content:WebAPI Web API docs size/m [PR only] 51-500 LoC changed labels Mar 12, 2024
Copy link
Contributor

github-actions bot commented Mar 12, 2024

Preview URLs (25 pages)
Flaws (6)

Note! 20 documents with no flaws that don't need to be listed. 🎉

URL: /en-US/docs/Web/API/RTCRemoteOutboundRtpStreamStats/localId
Title: RTCRemoteOutboundRtpStreamStats: localId property
Flaw count: 2

  • macros:
    • /en-US/docs/Web/API/RTCSentRtpStreamStats/packetsSent does not exist
    • /en-US/docs/Web/API/RTCSentRtpStreamStats/bytesSent does not exist

URL: /en-US/docs/Web/API/RTCRemoteOutboundRtpStreamStats/kind
Title: RTCRemoteOutboundRtpStreamStats: kind property
Flaw count: 1

  • macros:
    • /en-US/docs/Web/API/RTCCodecStats/codec does not exist

URL: /en-US/docs/Web/API/RTCRemoteInboundRtpStreamStats/packetsLost
Title: RTCRemoteInboundRtpStreamStats: packetsLost property
Flaw count: 1

  • macros:
    • /en-US/docs/Web/API/RTCInboundRtpStreamStats/packetsLost does not exist

URL: /en-US/docs/Web/API/RTCRemoteInboundRtpStreamStats/packetsReceived
Title: RTCRemoteInboundRtpStreamStats: packetsReceived property
Flaw count: 1

  • macros:
    • /en-US/docs/Web/API/RTCInboundRtpStreamStats/packetsReceived does not exist

URL: /en-US/docs/Web/API/RTCRemoteInboundRtpStreamStats/jitter
Title: RTCRemoteInboundRtpStreamStats: jitter property
Flaw count: 1

  • macros:
    • /en-US/docs/Web/API/RTCInboundRtpStreamStats/jitter does not exist
External URLs (2)

URL: /en-US/docs/Web/API/RTCRemoteInboundRtpStreamStats/fractionLost
Title: RTCRemoteInboundRtpStreamStats: fractionLost property


URL: /en-US/docs/Glossary/Jitter
Title: Jitter

(comment last updated: 2024-08-02 00:45:52)

@hamishwillee hamishwillee force-pushed the rtcremoteinboundrtpstreamstats branch from ad6442b to 788f114 Compare March 15, 2024 02:52
@github-actions github-actions bot added the Content:Glossary Glossary entries label Mar 15, 2024

## Value

A positive integer value indicating the total number of received RTP packets at the remote endpoint.
Copy link
Collaborator Author

@hamishwillee hamishwillee Mar 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NOTE, I am confused here. For context RTCRemoteInboundRtpStreamStats are statistics from the remote system about inbound stream stats. In other words, the local end sent some information to the remote end. The remote end has reported some information back to say what it received.

So for remote inbound stats my understanding is that you're getting the packetsReceived as measured at the remote end. This will simply be all received packets since the beginning of the session including retransmissions.
This contrasts with RTCInboundRtpStreamStats.packetsRecieved which is the local end calculating stats for the inbound stream.

But the spec states for packetsReceived:

packetsReceived of type unsigned long long

Total number of RTP packets received for this SSRC. This includes retransmissions. At the receiving endpoint, this is calculated as defined in [RFC3550] section 6.4.1. At the sending endpoint the packetsReceived is estimated by subtracting the Cumulative Number of Packets Lost from the Extended Highest Sequence Number Received, both reported in the RTCP Receiver Report, and then subtracting the initial Extended Sequence Number that was sent to this SSRC in a RTCP Sender Report and then adding one, to mirror what is discussed in Appendix A.3 in [RFC3550], but for the sender side. If no RTCP Receiver Report has been received yet, then return 0.

IF we assume that by "receiving endpoint" we mean "whatever is calculating the packets received" then this works because a receiver just counts what packets it got - and that is then way we got back from the remote end in this report.

I THINK the bit about "at the sending endpoint ..." is irrelevant. I.e. you might calculate the packetsRecieved in that way at the sender, but that has nothing to do with this data we are reporting.

@hamishwillee hamishwillee force-pushed the rtcremoteinboundrtpstreamstats branch from d111915 to 137ef8a Compare March 19, 2024 00:16
@github-actions github-actions bot added size/l [PR only] 501-1000 LoC changed and removed size/m [PR only] 51-500 LoC changed labels Mar 19, 2024
Comment on lines 13 to 17
The value provides a quick and convenient measure of the link quality.
It is the packet loss as a fraction, scaled up by 256.
To convert to a percentage, divide by 256 and multiply by 100.
For example, a value of 20 would equal a 7.8% packet loss.
Note that the packet loss can be negative if more packets are received than expected, and in this case the fraction is set to zero.
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that all of this is my "guess/evaluation" based on the RFC info and appending linked in the next section. All the spec says is that it is calculated as per those spec links. I have interpreted that to mean that the property contains the 8 bit unsigned integer in the RR/SR "fraction loss" field. That is calculated by 8 bit shifting a number so the fraction is now the number.

It all sounds rational, but I can't prove it. I've tried a bunch of examples out and they all give me a value of zero :-( (which is what you get if there is no loss OR if the loss is negative).

@hamishwillee hamishwillee marked this pull request as ready for review March 19, 2024 05:38
@hamishwillee hamishwillee requested review from a team as code owners March 19, 2024 05:38
@hamishwillee hamishwillee requested review from Elchi3 and pepelsbey and removed request for a team March 19, 2024 05:38
@hamishwillee
Copy link
Collaborator Author

@wbamberg Yet another one to look at. I will probably continue working on it to update RTCRemoteOutoundRtpStreamStats if you don't review it soon, as they have more common behaviour. But if you do start reviewing I'll stop that.

@wbamberg
Copy link
Collaborator

ah right, so the quicker I review it the smaller it'll be? :)

@hamishwillee
Copy link
Collaborator Author

hamishwillee commented Mar 19, 2024

@wbamberg Yes :-). I'll be pausing if you review this earlier so that neither of us have a moving target for the "common" changes that are in this PR. But I'm not back until Friday. No huge urgency.

@wbamberg
Copy link
Collaborator

wbamberg commented Apr 6, 2024

@hamishwillee , I have started on this.

@pepelsbey
Copy link
Member

@hamishwillee hey! Do you think we can still pull this through?

@hamishwillee
Copy link
Collaborator Author

hamishwillee commented Jul 9, 2024

@pepelsbey I've been putting this off as "not a priority". I've merged most of the suggestions but I want to have another look at it on Friday. Will is on holiday, so it is not urgent unless you want to review it?

@hamishwillee hamishwillee force-pushed the rtcremoteinboundrtpstreamstats branch from a5f1644 to 41bb603 Compare July 9, 2024 00:45
@pepelsbey
Copy link
Member

Will is on holiday, so it is not urgent unless you want to review it?

Thank you! As you said, it’s not a priority, so I’d leave it to Will since he’s already familiar with it.

@hamishwillee hamishwillee force-pushed the rtcremoteinboundrtpstreamstats branch from ac40751 to 70661f0 Compare July 30, 2024 03:54
@hamishwillee
Copy link
Collaborator Author

@wbamberg I have accepted your suggestions and addressed all the issues you raised. No urgency, but this is ready for a new review when you are.

Copy link
Collaborator

@wbamberg wbamberg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Hamish!

3 typo suggestions, but approving anyway so you can merge if you accept them :).

@hamishwillee
Copy link
Collaborator Author

Thanks very much @wbamberg - appreciate your patience and help with these. One day I may be finished with all of these and we can both have a deep sign of relief.

@hamishwillee hamishwillee merged commit cd49415 into mdn:main Aug 2, 2024
9 checks passed
@wbamberg
Copy link
Collaborator

wbamberg commented Aug 2, 2024

ha yes I admit to a feeling of dread when I see one of these 🤣

@hamishwillee hamishwillee deleted the rtcremoteinboundrtpstreamstats branch August 2, 2024 02:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Content:Glossary Glossary entries Content:WebAPI Web API docs size/l [PR only] 501-1000 LoC changed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants