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

Additional description of audioLevel #288

Merged
merged 2 commits into from
Jan 17, 2018
Merged

Conversation

alvestrand
Copy link
Contributor

Fixes #239 (by answering "yes").

Fixes #239 (by answering "yes").
a <a>volume</a> setting of 1.0, the audio level is
expected to be the same as the audio level of the
source SSRC, while if the <a>volume</a> setting is
0.5, the audioLevel is expected to be half that
Copy link
Contributor

Choose a reason for hiding this comment

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

Doesn't this go against @vr000m's recommendation?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure which recommendation you're referring to.

Copy link
Contributor

Choose a reason for hiding this comment

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

My expectation was that input audio level and output level would match. And if someone did not hear anything then the volume stat being 0 would identify the problem.

I am trying to see now if we compare the input audio level with the audio output level and the audio output takes the volume into account, how do I diagnose if the issue is with the post decoding filter...?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You ask for a solution to #271

</p>
<p>
The audioLevel is averaged over some small
interval, using the algortihm described
Copy link
Contributor

Choose a reason for hiding this comment

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

This "small interval" part is new; what was the assumption before? That it was just the last decoded/sent packet?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The previous assumption was that it wasn't mentioned; I think the code averaged over a packet. This leaves it specified as "implementation defined", but the averaging has to use the sum-of-squares method rather than just averaging (averaging gives a completely bogus number).

<p>
The <a>audioLevel</a> represents the output audio
level of the track; thus, if the track is sourced
from an <a>RTCReceiver</a>, does no audio
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: RTCRtpReceiver

Copy link
Contributor

Choose a reason for hiding this comment

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

did this metric not report on the input and an output audio level?

Copy link
Contributor

Choose a reason for hiding this comment

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

Or at least there should be a section on input audio level as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@vr000m vr000m self-requested a review January 3, 2018 14:15
Copy link
Contributor

@vr000m vr000m left a comment

Choose a reason for hiding this comment

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

some comments inline

<p>
The <a>audioLevel</a> represents the output audio
level of the track; thus, if the track is sourced
from an <a>RTCReceiver</a>, does no audio
Copy link
Contributor

Choose a reason for hiding this comment

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

did this metric not report on the input and an output audio level?

<p>
The <a>audioLevel</a> represents the output audio
level of the track; thus, if the track is sourced
from an <a>RTCReceiver</a>, does no audio
Copy link
Contributor

Choose a reason for hiding this comment

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

Or at least there should be a section on input audio level as well.

a <a>volume</a> setting of 1.0, the audio level is
expected to be the same as the audio level of the
source SSRC, while if the <a>volume</a> setting is
0.5, the audioLevel is expected to be half that
Copy link
Contributor

Choose a reason for hiding this comment

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

My expectation was that input audio level and output level would match. And if someone did not hear anything then the volume stat being 0 would identify the problem.

I am trying to see now if we compare the input audio level with the audio output level and the audio output takes the volume into account, how do I diagnose if the issue is with the post decoding filter...?

@alvestrand
Copy link
Contributor Author

Need to add text saying that for outgoing tracks, it's the audio level transmitted into the PeerConnection.

Copy link
Contributor

@taylor-b taylor-b left a comment

Choose a reason for hiding this comment

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

Are we sure that the "volume" constraint actually is intended to affect remote audio tracks? Also, it can be specified as a range; how is it even expected to be interpreted in that case?

@alvestrand
Copy link
Contributor Author

That volume is a range is an accident of the constraint syntax. It should mean, formally, that the UA is going to pick a number within that range; it would be rather silly of the UA to not pick the middle.
There's no allowance in the mediacapture-main spec to say that volume only applies to certain types of track; I don't see any powerful reason to treat remote audio tracks differently.

@vr000m vr000m merged commit 8584b7d into master Jan 17, 2018
Get to CR automation moved this from In progress to Done Jan 17, 2018
@vr000m vr000m deleted the audio-level-is-output-239 branch January 17, 2018 14:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Get to CR
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

4 participants