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

Attempt to update RTCRtpContributingSource objects at playout time. #1098

Merged
merged 1 commit into from
Apr 13, 2017

Conversation

taylor-b
Copy link
Contributor

Fixes #1085.

May not be the correct terminology, but the intention is that the
contributing source objects are updated at playout time, such that if
an application is using them to drive an audio level UI, that UI will be
in sync with the audio played out by the browser.

@stefhak
Copy link
Contributor

stefhak commented Mar 30, 2017

I would like someone that has been involved in the discussion about CSRC to review.

@burnburn
Copy link
Contributor

burnburn commented Apr 4, 2017

As I said on the call, I believe this is a slippery slope that we will regret, since in general we have no control over when any MST will play out; it may be duplicated, sent elsewhere, delayed via other APIs, etc. I'm not going to officially object since I don't have a good alternative to propose, but I reserve the right to say 'I told you so' :)

@taylor-b
Copy link
Contributor Author

taylor-b commented Apr 5, 2017

I understand the concern. Maybe calling it "playout time" is the mistake, but what we ultimately need is some well-defined, post-jitter-buffer point of time, which the application has a way of correlating to playout time.

Even if the application does something with the MediaStreamTrack that introduces extra delay, the original problem is solved as long as the application has a way of estimating the additional delay and factoring it in when updating its audio level UI.

@alvestrand
Copy link
Contributor

We may get implementor feedback on this item, but it clearly reflects WG consensus.
Can be merged once conflicts are resolved.

Fixes w3c#1085.

May not be the correct terminology, but the intention is that the
contributing source objects are updated at playout time, such that if
an application is using them to drive an audio level UI, that UI will be
in sync with the audio played out by the browser.
@taylor-b taylor-b force-pushed the issue_1085_audio_level_sync branch from 7f381f0 to 81705d4 Compare April 6, 2017 20:12
@taylor-b
Copy link
Contributor Author

taylor-b commented Apr 6, 2017

Merge conflicts resolved.

@vr000m
Copy link

vr000m commented Apr 12, 2017

What is the motivation of accessing audioLevel on the getContributingSources (I suppose this is the value from the RtpHeader extension instead of the actual audio level).

Clarifying: are we adding audioLevel to both the webrtc-pc and webrtc-stats APIs? I feel in this case, having the audio levels accessible in two places is just confusing.

See w3c/webrtc-stats#194

@taylor-b
Copy link
Contributor Author

What is the motivation of accessing audioLevel on the getContributingSources (I suppose this is the value from the RtpHeader extension instead of the actual audio level).

For CSRCs, it's the value from the RFC6465 extension, yes. The motivation is that applications may want information about the speakers contributing to a mixed audio stream, which may be reflected in the application's UI.

The original reason to put the API on RTCRtpReceiver and not getStats was that we didn't want applications to be reliant on getStats for basic WebRTC functionality, since getStats calls may be expensive and time-consuming.

And I suggested adding the contributing sources to getStats as well, because we can put more granular information there, and it could provide value to services (like callstats.io?) that only have access to stats, and not individual RTCRtpReceivers. An example of it being more granular: the working group decided that even if the RTCRtpContributingSources are updated "post jitter buffer", the stats objects should be updated as soon as packets are received, which is consistent with other things in the stats.

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

Successfully merging this pull request may close these issues.

None yet

6 participants