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.

Sign up for GitHub

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

Jump to bottom

Replace RTCVideo{Sender,Receiver}Stats::keyFrames{Sent,Received} with RTC{Out,In}boundRtpStreamStats::keyFrames{En,De}coded #447

Merged
merged 3 commits into from
Jun 26, 2019
Merged

Replace RTCVideo{Sender,Receiver}Stats::keyFrames{Sent,Received} with RTC{Out,In}boundRtpStreamStats::keyFrames{En,De}coded #447

merged 3 commits into from
Jun 26, 2019

Conversation

rasmusbrandt
Copy link
Contributor

@rasmusbrandt rasmusbrandt commented Jun 20, 2019 •
edited
Loading

Moving keyFramesSent on sender to keyFramesEncoded on outbound-rtp, as per offline discussion with @henbos.

@rasmusbrandt
Replace RTCVideoSenderStats::keyFramesSent with RTCOutboundRtpStreamS…
576ba0e 
…tats::keyFramesEncoded.

Bug: webrtc-stats:402
@rasmusbrandt rasmusbrandt changed the title WIP: Replace RTCVideoSenderStats::keyFramesSent with RTCOutboundRtpStreamStats::keyFramesEncoded Replace RTCVideoSenderStats::keyFramesSent with RTCOutboundRtpStreamStats::keyFramesEncoded Jun 20, 2019
@rasmusbrandt
Copy link
Contributor Author

This is ready for a first pass, @henbos ptal.

@rasmusbrandt rasmusbrandt marked this pull request as ready for review June 20, 2019 09:56
@rasmusbrandt
Copy link
Contributor Author

Tests are failing on HEAD too, at least in my local checkout.

henbos
henbos approved these changes Jun 24, 2019
View reviewed changes
Copy link
Collaborator

@henbos henbos left a comment

Choose a reason for hiding this comment

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

This is part of the #402 work, moving it should be done!
With the regards to the definition change from "sent" to "encoded" I don't really have an opinion (FWIW the framesSent etc metrics should also move as part of #402 ).

LGTM. @vr000m are you OK with merging this?

@rasmusbrandt
Copy link
Contributor Author

rasmusbrandt commented Jun 26, 2019 •
edited
Loading

ping @vr000m :)

Re. the definition change: for most cases, keyFramesEncoded should be the same as keyFramesSent. keyFramesReceived and keyFramesDecoded could differ though, depending on far behind a stream is in the jitter buffer. My main motivation for changing the definition, however, was for consistency with the existing framesEncoded and framesDecoded.

@alvestrand
Copy link
Contributor

Formalistic: You need to add RTCVideoSenderStats::keyFramesSent to "obsolete stats", with a note saying whether or not there are implementations known.
It is really important for traceability that we document what names have been used in previous versions of the spec, so that we don't ever reuse them with a different definition.

@henbos
Copy link
Collaborator

henbos commented Jun 26, 2019

Formalistic: You need to add RTCVideoSenderStats::keyFramesSent to "obsolete stats", with a note saying whether or not there are implementations known.

According to https://webrtc-stats.callstats.io/verify/, neither Chrome or Firefox implement these metrics. So I don't think the obsolete section is needed?

@henbos
Copy link
Collaborator

henbos commented Jun 26, 2019

So I don't think the obsolete section is needed?

I chatted with @alvestrand and he would like it in obsolete anyway. Can you fix?

@rasmusbrandt
Copy link
Contributor Author

Done. PTAL.

@rasmusbrandt
Copy link
Contributor Author

webrtc.org implementation is here: https://webrtc-review.googlesource.com/c/src/+/143683

@rasmusbrandt rasmusbrandt changed the title Replace RTCVideoSenderStats::keyFramesSent with RTCOutboundRtpStreamStats::keyFramesEncoded Replace RTCVideo{Sender,Receiver}Stats::keyFrames{Sent,Received} with RTC{Out,In}boundRtpStreamStats::keyFrames{En,De}coded Jun 26, 2019
@rasmusbrandt
Copy link
Contributor Author

Thanks for catching the inconsistency. Now I think I got it right.

@vr000m
Copy link
Contributor

vr000m commented Jun 26, 2019

LGTM

@vr000m vr000m merged commit 6219431 into w3c:master Jun 26, 2019
aarongable pushed a commit to chromium/chromium that referenced this pull request Jun 27, 2019
Add keyFrames{En,De}coded to peerconnection_getstats.js.
ef6d53e 
Standard-PR here: w3c/webrtc-stats#447

Bug: webrtc:7066
Change-Id: I52dd0a4cf8d5ea3402894293dcb74fe975c44b14
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1680254
Commit-Queue: Rasmus Brandt <brandtr@chromium.org>
Reviewed-by: Henrik Boström <hbos@chromium.org>
Cr-Commit-Position: refs/heads/master@{#672889}
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@henbos henbos henbos approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@rasmusbrandt @alvestrand @henbos @vr000m