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

Add a demo for "Unprocessed volume" #993

Closed
wants to merge 10 commits into from
Closed

Conversation

alvestrand
Copy link
Contributor

Description
Add a demo that demonstrates the difference between processed and unprocessed audio volume.

This doesn't work right as of Chrome 63 (and probably 65); the PR
is created in order to get the implementation work going.

This doesn't work right as of Chrome 63 (and probably 65); the PR
is created in order to get the implementation work going.
@@ -0,0 +1,76 @@
<!DOCTYPE html>
<!--
* Copyright (c) 2015 The WebRTC project authors. All Rights Reserved.
Copy link
Contributor

Choose a reason for hiding this comment

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

Update copyright year.

@@ -0,0 +1,85 @@
/*
* Copyright (c) 2015 The WebRTC project authors. All Rights Reserved.
Copy link
Contributor

Choose a reason for hiding this comment

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

Update copyright

console.log('First track settings:',
JSON.stringify(stream.getAudioTracks()[0].getSettings()));
// Set up second track with audio processing disabled
constraints.audio = {echoCancellation: {exact: false}};
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do you have to use exact here?

Copy link
Member

Choose a reason for hiding this comment

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

I think exact is correct if we want two tracks, one with and one without echoCancellation, and demo the difference.

window.stream = stream;
var soundMeter = window.soundMeter = new SoundMeter(window.audioContext);
soundMeter.connectToSource(stream, function(e) {
if (e) {
Copy link
Contributor

Choose a reason for hiding this comment

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

You could expand e to error since you have space. Also not sure if alert is preferred anymore but not outputting text on the page might not be clear enough.

try {
window.AudioContext = window.AudioContext || window.webkitAudioContext;
window.audioContext = new AudioContext();
} catch (e) {
Copy link
Contributor

Choose a reason for hiding this comment

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

You are not printing the actual error, e is unused.

Copy link
Member

Choose a reason for hiding this comment

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

I believe you can do try { } catch { }.

soundMeter.slow.toFixed(2);
}, 200);
});
console.log('First track settings:',
Copy link
Contributor

Choose a reason for hiding this comment

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

Use trace instead?

Copy link
Member

Choose a reason for hiding this comment

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

+1

});
}

function handleError(error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

You could make a generic error handler here and pass it to connectToSource error callbacks as well.

@KaptenJansson
Copy link
Contributor

Any cycles to finish this?

@KaptenJansson
Copy link
Contributor

Ping

<p>Note that you will not hear your own voice; use the <a href="../audio">local audio rendering demo</a> for that.</p>
<p>The <code>audioContext</code>, <code>stream</code> and <code>soundMeter</code> variables are in global scope, so you can inspect them from the console.</p>

<a href="https://github.com/webrtc/samples/tree/gh-pages/src/content/getusermedia/volume" title="View source for this page on GitHub" id="viewSource">View source on GitHub</a>
Copy link
Collaborator

Choose a reason for hiding this comment

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

wrong link

@googlebot
Copy link

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again. If the bot doesn't comment, it means it doesn't think anything has changed.

@ErikHellman ErikHellman requested a review from henbos June 14, 2018 13:40
@ErikHellman
Copy link
Contributor

Fixed all the outstanding issues and converted all vars to const/let.

Seems like the CLA check doesn't like my other email. I'll fix this for future PRs, but they

try {
window.AudioContext = window.AudioContext || window.webkitAudioContext;
window.audioContext = new AudioContext();
} catch (e) {
Copy link
Member

Choose a reason for hiding this comment

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

I believe you can do try { } catch { }.

soundMeter.slow.toFixed(2);
}, 200);
});
console.log('First track settings:',
Copy link
Member

Choose a reason for hiding this comment

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

+1


// Put variables in global scope to make them available to the browser console.
let constraints = window.constraints = {
audio: {echoCancellation: true},
Copy link
Member

Choose a reason for hiding this comment

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

Should this be echoCancellation: {exact:true} so that we fail if it cannot be satisfied?

Copy link
Member

Choose a reason for hiding this comment

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

I find it confusing and error prone that the same constraints are setup up as a global variable, then used, then modified. If the demo were to have a reset button it would stop working unless this is explicitly reset. I've also not seen "let constraints = window.constraints = ...". Wouldn't "let constraints" work? Or if that didn't work, why not "var constraints"? I don't see this variable adding anything.

Can we just pass the desired constraints when we call getUserMedia? Not have a variable for it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ping @alvestrand. I didn't update this part. :)

console.log('First track settings:',
JSON.stringify(stream.getAudioTracks()[0].getSettings()));
// Set up second track with audio processing disabled
constraints.audio = {echoCancellation: {exact: false}};
Copy link
Member

Choose a reason for hiding this comment

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

I think exact is correct if we want two tracks, one with and one without echoCancellation, and demo the difference.

alert('Web Audio API not supported.');
}

// Put variables in global scope to make them available to the browser console.
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure what what this means.

Copy link
Collaborator

Choose a reason for hiding this comment

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

sam wrote this... but var at top scope have always been available in the console or window[]. No longer true for es6 const/let notably.

video: false
}

function handleSuccess(stream) {
Copy link
Member

Choose a reason for hiding this comment

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

Can we do async/await instead? Promise-chains with callbacks are harder to follow.

Copy link
Contributor

Choose a reason for hiding this comment

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

While I agree that it is easier to read, I'm not sure it's suitable for these samples. My guess is that most JavaScript developers are more familiar with promises than async/await.

Copy link
Collaborator

Choose a reason for hiding this comment

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

that is the reason there was no push for es6... stackoverflow is still full of people who use the callback API but we had to move the needle there

Copy link
Contributor

Choose a reason for hiding this comment

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

I've changed my mind on this. async/await is better in most cases than promise-chains. We should be good citizens and push the use of that. :)

@henbos
Copy link
Member

henbos commented Jun 15, 2018

Did I review the latest changes or did googlebot block you? I saw comments from February on the files I commented on

@ErikHellman
Copy link
Contributor

@alvestrand I need your approval on this so that the Google CLA gets happy again. :)

- Added missing semicolons
- Converted callbacks to prmoises (async/await)
console.log('First track settings:', audioSettings);

// Set up second track with audio processing disabled
constraints.audio = {echoCancellation: {exact: false}};
Copy link
Collaborator

Choose a reason for hiding this comment

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

any reason why this is {exact: false} and not false?

@alvestrand alvestrand dismissed KaptenJansson’s stale review August 29, 2018 10:44

He's away at the moment

@ErikHellman
Copy link
Contributor

Closing this for now as we're not ready to publish this yet.

@alvestrand alvestrand deleted the unprocessed-volume branch April 3, 2020 11:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants