-
Notifications
You must be signed in to change notification settings - Fork 114
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
Synchronize MTI stats with webrtc-stats cleanup #2744
Conversation
Following their deprecation in w3c/webrtc-stats#637
remove supprt in remote-inbound-rtp per w3c/webrtc-stats@af5fb42
Reflecting removal from webrtc-stats per w3c/webrtc-stats@109aecf
@@ -15505,7 +15503,6 @@ <h3> | |||
{{RTCReceivedRtpStreamStats/packetsReceived}}, | |||
{{RTCReceivedRtpStreamStats/packetsLost}}, | |||
{{RTCReceivedRtpStreamStats/jitter}}, | |||
{{RTCReceivedRtpStreamStats/packetsDiscarded}}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
packetsDiscarded
seems well implemented.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's also still in the stats spec AFAICT
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the patch moves it to its new parent RTCInboundRtpStreamStats
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's implemented in inbound-rtp, not remote-inbound-rtp. The WPT tests both, one passes on all browsers and one fails on all browsers.
The PR moved it from RTCReceivedRtpStreamStats which implied both inbound-rtp and remote-inbound-rtp to only RTCInboundRtpStreamStats. This matches the implementation status
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for explaining! LGTM then
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AFAICT this is correct. Approved!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I noticed one thing we should change IMO
</td> | ||
<td rowspan="2" data-tests= | ||
"RTCPeerConnection-track-stats.https.html,RTCPeerConnection-mandatory-getStats.https.html"> | ||
{{RTCMediaHandlerStats/trackIdentifier}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh we should make it mandatory to implement RTCInboundRtpStreamStats.trackIdentifier. It got moved there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you provide it as a separate PR since that one got merged now? :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will do #2747
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done: #2748
* Align WebRTC MTI stats with specs see w3c/webrtc-pc#2744 w3c/webrtc-pc#2748 w3c/webrtc-stats#647
…tonly Automatic update from web-platform-tests Align WebRTC MTI stats with specs (#35703) * Align WebRTC MTI stats with specs see w3c/webrtc-pc#2744 w3c/webrtc-pc#2748 w3c/webrtc-stats#647 -- wpt-commits: ffb6a1e86161c3c449d8220700624bbc31fb0dcd wpt-pr: 35703
Preview | Diff