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

RTCOutboundRtpStreamStats.headerBytesSent/totalPacketSendDelay and RTX seems underspecified #747

Open
docfaraday opened this issue Mar 9, 2023 · 6 comments

Comments

@docfaraday
Copy link

Spec explicitly says that we should be totaling up packetsSent, bytesSent, retransmittedPacketsSent, and retransmittedBytesSent. But what about headerBytesSent/totalPacketSendDelay?

headerBytesSent

It seems pretty clear that this should include RTX, since spec says "headerBytesSent + bytesSent equals the number of bytes sent as payload over the transport.", and bytesSent accounts for RTX.

totalPacketSendDelay

This probably ought to account for RTX as well. The only way I can see this metric being useful is by comparing it to packetsSent (over some time window), and packetsSent includes RTX packets.

@vr000m
Copy link
Contributor

vr000m commented Mar 15, 2023

retransmittedPacketsSent and retransmittedBytesSent are already included in packetsSent and bytesSent. So they should be include retransmissions. Unless I am missing something, the definitions are correct, however, based on the ticket, a clarification would help the implementor. Would the following improvements clarify the definitions:

headerBytesSent of type unsigned long long

Total number of RTP header and padding bytes sent for this SSRC. This does not include the size of transport layer headers such as IP or UDP. headerBytesSent includes retransmissions. Hence, headerBytesSent + bytesSent equals the number of bytes sent as payload over the transport.

For totalPacketSendDelay says the measurement is added each time the packetSent is incremented. And the packetSent explicitly states that it includes retransmissions. Would the following clarification help?

totalPacketSendDelay of type double

The total number of seconds that packets have spent buffered locally before being transmitted onto the network. The time is measured from when a packet is emitted from the RTP packetizer until it is handed over to the OS network socket. This measurement is added to totalPacketSendDelay when packetsSent is incremented. packetSent includes retransmitted packets, therefore, this measurement should be include send delays for retransmitted packets.

@docfaraday
Copy link
Author

Yeah, I think being explicit here is necessary.

@henbos
Copy link
Collaborator

henbos commented Mar 15, 2023

Will you provide a clarification PR @vr000m ? (I think we can merge such PRs even if #746 has yet to be resolved)

@fippo
Copy link
Contributor

fippo commented Apr 8, 2023

How is this kind of stuff testable?

@fippo
Copy link
Contributor

fippo commented Apr 20, 2023

I've checked what libWebRTC does as part of implementing RTX stats.

It does not include the payload of RTX packets, so the accurate thing to say would be:

headerBytesSent includes the header bytes of retransmissions

@vr000m
Copy link
Contributor

vr000m commented Apr 22, 2023

Agree that the clarification is appropriate. Header bytes of retransmissions.

Can you confirm if the retransmission bytes are added to bytesSent?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants