w3c / mediacapture-main

Media Capture and Streams specification (aka getUserMedia)
https://w3c.github.io/mediacapture-main/
Other
126 stars 62 forks source link

Add autoGainControl and noiseSuppression constraints. #444

Closed jan-ivar closed 7 years ago

jan-ivar commented 7 years ago

Chrome has googAutoGainControl and googNoiseSuppression, & Firefox has mozAutoGainControl and mozNoiseSuppression.

With two working implementations, we should standardize autoGainControl and noiseSuppression to avoid prefix pollution.

With this in the spec, my hope is Google would rethink moving googAutoGainControl and googNoiseSuppression into navigator.mediaDevices.getSupportedConstraints, and we would remove our mozAutoGainControl and mozNoiseSuppression mistakes.

foolip commented 7 years ago

That would be great! @guidou, have you done any editing in this spec?

guidou commented 7 years ago

No, I haven't edited this spec. I have reviewed a couple of changes related to things I an familiar with in Chromium.

stefhak commented 7 years ago

As noted in #445 we should take this to the list.

ShijunS commented 7 years ago

Could someone share any developer docs related to implementations in Chrome and Firefox? Maybe @jan-ivar, @guidou?

I hope they are not implemented specific for WebRTC - the control knobs should be in WebRTC otherwise. In addition, if you expect it will benefit WebAudio, it'd be helpful to get some feedback from some Audio WG experts.

jan-ivar commented 7 years ago

@ShijunS Not sure what you mean with docs. It's implemented solely in gUM. Try it in Firefox here.

ShijunS commented 7 years ago

@jan-ivar, I was hoping you'd have MDN pages to inform web developers how to use the constraints. For Edge, both AGC and NS are integrated internally with the RTC stack - it won't make sense for developers to turn them off. From FF perspective, what are the typical scenarios other than RTC where you would enable the functionality. It'd be nice to get a couple examples of actual major websites doing that.

Btw, I didn't see any signal from the jsfiddle test page while using Nightly, not sure what I have missed or should expect.

jan-ivar commented 7 years ago

@ShijunS MDN tries to document the web standard, not the firefox implementation. But given what MDN says for echoCancellation I wouldn't expect any MDN entry to say much more than this PR.

Odd that the fiddle didn't work. I tested it just now on both OSX and Windows 10 Pro (1607 - 14393.1066) in both FF release and Nightly. Would you mind double-checking that these are latest versions, trying it on a different system, different mic, and/or/ file a bug?

I'll explain what you'd see and hear (though I found hearing the difference myself most clarifying):

After giving mic permission, the checkboxes should be set like:

☑ echoCancellation ☑ noiseSuppression ☐ autoGainControl ☐ mute

because the first two are on by default in firefox. I recommend using a headset, and turn the volume up a bit, then flip the switches.

With ☐ noiseSuppression you should hear background noise noticeably appear. Then with ☑ autoGainControl you should hear the background noise amplified a bit. Finally, with ☑ noiseSuppression back on: amplification without the noise.

That's about it. AFAIK googNoiseSuppression and googGainControl work the same.

Of course, the effects on background noise are not the main point. The point is being able to record things other than talking heads. E.g. music performances, nature sounds, even ambient noise, or otherwise tailor recording to, say, a controlled environment, like an audio booth. The typical use case is to record with higher quality, since these filters sacrifice a lot fidelity, especially with stereo (some implementations even revert to mono when some of these filters are on today).

I expect people want to do these things even when the sink is a not peer connection, e.g. MediaRecorder, web audio (which may want the raw signal to work on) or in controlled environments maybe even local playback. Sure some want to send music over a peer connection as well, but putting these switches on gUM seems like the right place to cover all these use cases.

I'm not an audiophile myself, but @padenot may be able to answer more about existing uses and needs. Note that Chrome already turns off all audio processing in response to echoCancellation, I suspect as a kludge to support these cases.

ShijunS commented 7 years ago

I've been traveling in the past few days, will test on more systems when back to office. It could be totally a test problem on my end, so should not block the discussion.

@jan-ivar, what is your opinion on defining the default values of the constraints? Would they be different based consuming target objects? I'd like to make sure we don't break existing use case like WebRTC.

jan-ivar commented 7 years ago

@ShijunS Constraints don't have defaults, by design. Unconstrained == UA chooses. Constraints are externalized (app) requirements letting UAs change device properties as they sees fit within the net envelope of all constraints put on it.

So basically, as a user of the API, if I care about a property, I should constrain it. If I don't care, I shouldn't have expectations of what value it'll have.

Would they be different based consuming target objects?

They could be. The spec says:

"Implementations may choose to make such source-to-sink optimizations as long as they only do so within the constraints established by the application, as the next example demonstrates.

It is fairly obvious that changes to a given source will impact sink consumers. However, in some situations changes to a given sink may also cause implementations to adjust a source's settings."

I don't think anyone has implemented anything like that yet though, but maybe someday.

ShijunS commented 7 years ago

@jan-ivar, could you provide examples of actual major websites using the vendor-prefixed constraints? I hope it will be a strong supporting evidence for adding the new features to the spec.

BTW, I'm now back to office, and have verified the jsfiddle page with Nightly. Sorry for the false alarm a couple days ago.

jan-ivar commented 7 years ago

@ShijunS Usually we look at usage when we're deciding between standardizing a legacy API or deprecating it. The chromium issue I linked to earlier discusses the latter, and boils down to that they haven't added counters yet, and that usage in httparchive looks low.

But we're not trying to standardize googAutoGainControl, googNoiseSuppression, mozAutoGainControl or mozNoiseSuppression.

The PR adds autoGainControl and noiseSuppression which would instantly have two independent mature implementations. I'd hesitate to predict their popularity based off usage of their vendor-prefixed twins, as I expect most sites think twice before relying on undocumented vendor-prefixed APIs.

Anyway, usage seems like a high bar to me. It's quite unusual for new constraints PRs to even come with two independent mature implementations, let alone any actual usage at all.

ShijunS commented 7 years ago

@Jan-Ivar, agree not to predict the popularity. I personally can see value of the constraints, although it'd be nice to know if the prefixed versions are widely used already. Meanwhile, I am also looking for a real website that can show a reasonable consistency between the two implementations. So, it'd be great if there is one.

Meanwhile, I wonder if you could share your thoughts on how the proposed constraints would work with other constraints in the current spec. You mentioned "echoCancellation" earlier, another example is "volume". It might be corner cases for web app to set all constraints together, but it'd be necessary to make sure the spec coverage is reasonably complete.

jan-ivar commented 7 years ago

The httparchive hit counters in the chromium link were low (which is good or we'd have to consider standardizing the vendor prefixes!) - That said, they were the most popular of the goog constraints (56, 57), short of chromeMediaSource (68).

In other words, they're (almost) as popular as Google's screen-sharing (not having numbers for the moz equivalents).

The fiddle already has a switch for echoCancellation, so you can experiment with combinations there (careful turning it off without a headset!)

Other than the width/height(aspectRatio)/frameRate triumvirate which informed the constrainable design, I find that most constrainable features are completely orthogonal.

ShijunS commented 7 years ago

Given there is evidence that the vendor-prefixed version of the constraints are being used, I agree it'd be nice if both Chrome and FF can update the API implementation so developers can have consistent experience. @stefhak, I hope the chairs can comment whether to include new features to the spec at this point.

Meanwhile, @Jan-Ivar, please summarize the following based on the FF implementation explicitly (rather than my experiments)

  1. how will the two proposed constraints interact with the "echoCancellation" constraint?
  2. how will the two proposed constraints interact with the "volume" constraint?

Chrome folks should comment accordingly. Let's make sure it should enable a reasonable interoperability if we add them to the spec.

jan-ivar commented 7 years ago

@ShijunS Not sure what you mean with interop. Like echoCancellation, these are opaque abstractions of UA features. Sites can turn each setting on or off, and hopefully be content with the result being superior to the alternative setting.

I don't think they need to sound the same for there to be interop. Nothing in the spec mandates how UAs implement e.g. echoCancellation, nor how or when constraints might conflict, which might happen if support for e.g. mics with controllable hardware echo cancellation is added.

FWIW, Firefox implements echo cancellation, auto gain control, and noise suppression in software. Like the software features, the three boolean settings have no interdependencies; All 8 combinations are allowed. Firefox doesn't implement the volume constraint yet.

jan-ivar commented 7 years ago

@aboba Was there a resolution here I missed?

jan-ivar commented 7 years ago

I had slides last on the agenda for the May 2nd interim, but we ran out of time to discuss it.

Can we reopen this and tag it June 2017 interim topic, preferably at the top this time?

ShijunS commented 7 years ago

PR #445 for this was merged already.