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

Add audio capture stats, rename method to getStats. #97

Closed
wants to merge 3 commits into from
Closed

Add audio capture stats, rename method to getStats. #97

wants to merge 3 commits into from

Conversation

henbos
Copy link
Contributor

@henbos henbos commented Apr 20, 2023 •
edited by pr-preview bot
Loading

Fixes #96.

Preview | Diff

@henbos henbos requested a review from jan-ivar April 20, 2023 13:32
@henbos
Copy link
Contributor Author

henbos commented Apr 20, 2023

This is approximately just a move, but instead of talking about "audio samples" I've rephrased it to match the terminology that is used outside of webrtc-stats to say "audio frames" instead. I've also added a clarifying note.

jan-ivar
jan-ivar reviewed Apr 20, 2023
View reviewed changes
index.html Outdated Show resolved Hide resolved
@henbos @jan-ivar
Update index.html
2520b4f 
Co-authored-by: Jan-Ivar Bruaroey <jan-ivar@users.noreply.github.com>
jan-ivar
jan-ivar approved these changes Apr 20, 2023
View reviewed changes
Copy link
Member

@jan-ivar jan-ivar left a comment

Choose a reason for hiding this comment

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

LGTM in the sense that this is a move from stats, which I appreciate!

But I am not an audio expert, and would appreciate @padenot looking this over.

@jan-ivar
Copy link
Member

jan-ivar commented Apr 20, 2023 •
edited
Loading

Editors can integrate pending approval from @padenot.

jan-ivar
jan-ivar reviewed Apr 20, 2023
View reviewed changes
index.html Outdated Show resolved Hide resolved
@henbos
Copy link
Contributor Author

henbos commented Apr 20, 2023

If this merges we'll delete the corresponding ones from RTCPeerConnection.getStats and implement this instead

@jan-ivar jan-ivar self-requested a review April 20, 2023 16:06
@jan-ivar
Copy link
Member

@dontcallmedom can we add @padenot as a reviewer in this repo? I don't seem to have the powers.

@jan-ivar
Copy link
Member

Paul had valuable feedback and should be able to comment tomorrow.

@jan-ivar jan-ivar mentioned this pull request Apr 21, 2023
Closed
@jan-ivar
Copy link
Member

I've filed #98 with Paul's concerns.

@henbos
Copy link
Contributor Author

henbos commented Apr 22, 2023

#98 is about async versus sync, not about the metrics added in this PR. Can we follow up on the API shape separately from the addition of these metrics? Can I merge the PR?

@henbos
Copy link
Contributor Author

henbos commented Apr 25, 2023

#98 is about async versus sync, not about the metrics added in this PR. Can we follow up on the API shape separately from the addition of these metrics? Can I merge the PR?

Ping @jan-ivar

@chromium-wpt-export-bot chromium-wpt-export-bot mentioned this pull request Aug 14, 2023
Closed
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Aug 14, 2023
@henbos @chromium-wpt-export-bot
Implement MediaStreamTrack video frame stats.
8ea46eb 
This is a prototype implementation of the video side of track stats,
https://w3c.github.io/mediacapture-extensions/#mediastreamtrack-frame-stats,
including changes from PR
w3c/mediacapture-extensions#97.

Bug: None
Change-Id: I80a01fc52c5b6ea16b8bc37d050dfaac9d65c48a
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Aug 15, 2023
@henbos @chromium-wpt-export-bot
Implement MediaStreamTrack video frame stats.
c64fc8e 
This is a prototype implementation of the video side of track stats,
https://w3c.github.io/mediacapture-extensions/#mediastreamtrack-frame-stats,
including changes from PR
w3c/mediacapture-extensions#97.

Bug: None
Change-Id: I80a01fc52c5b6ea16b8bc37d050dfaac9d65c48a
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Aug 15, 2023
@henbos @chromium-wpt-export-bot
[Prototype] Implement MediaStreamTrack video frame stats.
1ca661e 
This is a prototype implementation of the video side of track stats,
https://w3c.github.io/mediacapture-extensions/#mediastreamtrack-frame-stats,
including changes from PR
w3c/mediacapture-extensions#97.

Bug: None
Change-Id: I80a01fc52c5b6ea16b8bc37d050dfaac9d65c48a
@henbos henbos added the Icebox Stuff that may be done one day, but not today. label Sep 15, 2023
@henbos
Copy link
Contributor Author

henbos commented Sep 15, 2023

This PR is obsolete. I will close it once I have a synchronous version of the proposal and other PRs have landed. Iceboxed in the meantime

@henbos
Copy link
Contributor Author

henbos commented Sep 29, 2023

Closing this one, will upload a new version which follows the new API pattern and rephrases things a bit

@henbos henbos closed this Sep 29, 2023
@henbos henbos mentioned this pull request Sep 29, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@youennf youennf youennf approved these changes

@jan-ivar jan-ivar Awaiting requested review from jan-ivar

Assignees
No one assigned
Labels
Icebox Stuff that may be done one day, but not today.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Migrate capture metrics from RTCAudioSourceStats to MediaStreamTrack method
3 participants
@henbos @jan-ivar @youennf