Closed
Bug 987186
Opened 11 years ago
Closed 9 years ago
Implement echoCancellation, mozAutoGainControl and mozNoiseSuppression gUM constraints
Categories
(Core :: WebRTC: Audio/Video, defect, P2)
Core
WebRTC: Audio/Video
Tracking
()
RESOLVED
FIXED
mozilla46
Tracking | Status | |
---|---|---|
firefox46 | --- | fixed |
backlog | webrtc/webaudio+ |
People
(Reporter: jib, Assigned: brian.armstrong.ece+github)
References
()
Details
Attachments
(3 files, 3 obsolete files)
Since we're moving our AEC from PeerConnection to gUM, people who wont like it on by default need a way to disable it.
Reporter | ||
Comment 1•11 years ago
|
||
To clarify "cancelation" is apparently American spelling, and everywhere else is "cancellation".
Though as I type this, the Firefox spellchecker puts a red line under the former, not the latter.
Also, Google says: Did you mean: cancellation
Reporter | ||
Comment 2•11 years ago
|
||
It was a spec typo.
Summary: Implement echoCancelation (sic) gUM constraint → Implement echoCancellation gUM constraint
Comment 3•11 years ago
|
||
m-w.com says cancellation, so I think one l was simply wrong (and hopefully will be fixed soon). Thanks
Reporter | ||
Comment 4•10 years ago
|
||
Added mozAutoGainControl and mozNoiseSuppression to be done at the same time. They seem similar enough.
about:config has
media.getusermedia.aec
media.getusermedia.aec_enabled
media.getusermedia.agc
media.getusermedia.agc_enabled
media.getusermedia.noise
media.getusermedia.noise_enabled
Summary: Implement echoCancellation gUM constraint → Implement echoCancellation, mozAutoGainControl and mozNoiseSuppression gUM constraints
Updated•10 years ago
|
Assignee: jib → mreavy
Rank: 39
Flags: firefox-backlog+
Priority: -- → P3
Updated•10 years ago
|
backlog: --- → webRTC+
Flags: firefox-backlog+
Comment 6•10 years ago
|
||
Guys, any news on this? Since this is missing we are forced to tell our users to use Chrome instead of Firefox for any sound-quality oriented connections (non-altered sound quality is a must for our users).
Comment 7•9 years ago
|
||
Hi Jan, This is currently a P3, which means it's not currently a high priority, but I'd love to understand what your use case is for this to determine if this needs to be bumped in priority or if we can achieve what you need some other way.
Please needinfo me (set needinfo below to mreavy@mozilla.com) if you need a reply to a question or need action on something. Thanks.
Flags: needinfo?(jan.ambroz)
Assignee | ||
Comment 8•9 years ago
|
||
Hi, I'd also like to chime in on this bug.
I'm working on something which is probably best summarized as SDR in the browser. Because it involves encoding data into audio, I need the closest thing I can get to raw samples from the microphone with no distortion, if possible.
I have a prototype of my project which will work in Chome because I can request constraints. However, having Firefox users user my project would require me to ask them to go to about:config, which seems rather heavy-handed.
I'm sufficiently interested in getting this to work that I've started looking at the relevant Firefox source to see if I could do it myself. I'm not at all familiar with this code base but I think I see roughly which bits are involved and I'd be happy to help. I realize that my use case is somewhat unusual, as I want the mic for something other than voice.
Flags: needinfo?(mreavy)
Updated•9 years ago
|
Flags: needinfo?(jib)
Comment 9•9 years ago
|
||
Hi Marie, To be as brief as possible, our application needs to dynamically switch echoCancellation constant at run-time, so our users are able to turn AEC on and off as needed in matter of seconds.
Due to the nature of our application (voice-only is simply not good enough), this feature is crucial. From marketing point of view, I am not even able to ask our users to set anything in their internal Firefox settings, as it would not end up well.
It is sad that we do not have access to the unmodified sound from the mic without modifying the settings, that fairly limits the usability of WebRTC in Firefox from my point of view.
Flags: needinfo?(jan.ambroz)
Comment 10•9 years ago
|
||
Thanks, Jan and Brian. You've convinced me that this bug should be a higher priority. I'm going to target landing a fix for this in Firefox 46 (which is the next Nightly release cycle).
Brian -- If you want to contribute a patch for this, it's most welcome. I am committed to other things until the end of this month, but I should be able to start work on this in early January.
FYI: Firefox 46 uplifts to Aurora on Jan 25th; so one way or other, we should have a fix landed in Nightly by then.
Rank: 39 → 25
Flags: needinfo?(mreavy)
Flags: needinfo?(jib)
Priority: P3 → P2
Comment 11•9 years ago
|
||
Jib -- Can you describe what's needed for this bug? I'd appreciate a "heads up" on any possible gotchas. Either Brian or I will fix this bug, and we'd both appreciate advice on the best way to solve it. Likely you will be the reviewer for the patch when it's ready; so knowing ahead of time what you want to see would be really helpful. Thanks!
Flags: needinfo?(jib)
Reporter | ||
Comment 12•9 years ago
|
||
TL;DR: Search mxr.mozilla.org for a similar constraint and cut'n'paste stuff like webidl, MediaManager audio plumbing hopefully works, but skip the camera-backend and look instead at recently added screensharing constraints that are simpler orthogonal settings (Bug 1193075) as a model, then hunt down and replace the effects of the global settings in comment 4 where getUserMedia and applyConstraints code allocates the track's microphone source.
I wouldn't worry about concurrent access for now, since we don't solve that for cameras yet either. There are other aspects I think we can ignore as well for now, like getConstraints() and getSettings() which I'm working on in parallel (Bug 1213517).
This will be our first audio constraint, and our first ConstrainBoolean constraint, but hopefully the MediaManager plumbing works for audio as it does for video already, so hopefully you wont need to touch it.
Importantly, the constraints API is horrible overkill for the simple settings API we need here, so that's going to seem like a challenge, but I'm here to help if you have questions.
The reason for this is that the Constraints API is rooted in the needs of camera-stacks, which maintain sets of hardware "Capabilities" to pick from. I could be wrong, but I don't think the audio-stack has that.
We're probably also not looking for inherent hardware properties here, e.g. using getUserMedia({ echoCancellation: { exact: true } }) as a means to reject a microphone over not having it, or pick between different microphones, some of which have hardware aec and others that do not (however marginally useful that might be). I assume instead we simply need to set a plain orthogonal setting that wont require any "pick from capability-sets" model like the camera backend uses.
Therefore, I recommend looking instead at some of the recent tab-screensharing constraints that got added [1], like viewportOffsetX in Bug 1193075. It uses FlattenedConstraints [2], a helper class designed exclusively for orthogonal settings like e.g. volume and echo-cancellation. It lets you access constraints more directly without running the full SelectSettings algorithm, which would be way overkill here. That class also lets you largely ignore "advanced" constraints (though a few lines should be added to that class to teach it how).
Hope that helps, and let me know if you have any questions.
[1] http://mxr.mozilla.org/mozilla-central/source/dom/media/webrtc/MediaEngineTabVideoSource.cpp#154
[2] http://mxr.mozilla.org/mozilla-central/source/dom/media/webrtc/MediaTrackConstraints.h#35
Flags: needinfo?(jib)
Assignee | ||
Comment 13•9 years ago
|
||
Thank you so much for that explanation, @jib! I had some other parts of my project to work on but recently cracked open the firefox codebase to have a look at this.
I've managed to add a test for getting echoCancellation from getUserMedia and get the test passing. I'm somewhat unsure about what to do next with the plumbing, though. Looking at http://mxr.mozilla.org/mozilla-central/source/dom/media/MediaManager.cpp#1071 I see that the config I want to change is normally applied by GetUserMediaStreamRunnable calling AudioConfig, which doesn't have the user constraints.
If I understand this part correctly, it seems that what I likely want to do is remove AudioConfig and instead run the config from MediaEngineWebRTCMicrophoneSource::Restart. I think this means getting the defaults from preferences-service and then applying any user supplied constraints on top. Does that sound like a sane plan?
Thanks!
Brian
Flags: needinfo?(jib)
Reporter | ||
Comment 14•9 years ago
|
||
(In reply to Brian Armstrong from comment #13)
Yes, it seems to me you could either:
A) pass constraints in here: http://mxr.mozilla.org/mozilla-central/source/dom/media/MediaManager.cpp?rev=76ba09e7504c#1318 , or
B) Move it to MediaEngineWebRTCMicrophoneSource::Restart if possible, as you suggest.
I think the latter is going to be best, it's symmetrical with the video code, and this will make it work with applyConstraints() as well, which we'd otherwise need additional code for.
Sounds like a sane plan.
Flags: needinfo?(jib)
Assignee | ||
Comment 15•9 years ago
|
||
this doesn't actually add the plumbing for the constraint, but it allows us to support it
Review commit: https://reviewboard.mozilla.org/r/31651/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/31651/
Assignee | ||
Comment 16•9 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/31653/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/31653/
Assignee | ||
Comment 17•9 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/31655/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/31655/
Assignee | ||
Comment 18•9 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/31657/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/31657/
Assignee | ||
Comment 19•9 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/31659/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/31659/
Attachment #8710122 -
Flags: review?(jib)
Assignee | ||
Comment 20•9 years ago
|
||
Hi :jib,
I have pushed a changeset which I think allows the user to supply a ConstrainBoolean for these 3 parameters and then applies their requests on top of the defaults. I can't find any suitable test to modify to confirm that we are now actually respecting these values so I suspect a new test will need to be created.
Thanks for your guidance on this bug. I am also new to hg and mozilla's review tool so I appreciate your patience as I respond to your review.
Comment 21•9 years ago
|
||
Brian -- Thanks for taking this bug. I've officially assigned it to you since you're writing the patches. jib should be able to review all your code and provide the guidance needed to get the reviews granted. Once they are granted (i.e. once you have r+ on everything and ready to land), please needinfo me (ni?:mreavy) and I'll take care of getting it landed.
NOTE: Fx46 uplifts to DevEdition this Monday (Jan 25th). If you're trying to get this into Fx46, you'll want to have all your reviews "done" (r+ on everything) by Friday afternoon. Otherwise, this will land and ride the train in Fx47.
Thanks again for stepping up and taking on this bug! If there's anything I can do to help, please needinfo me on this bug or ping me on irc (nick: mreavy).
Assignee: mreavy → brian.armstrong.ece+github
Reporter | ||
Comment 22•9 years ago
|
||
(In reply to Brian Armstrong from comment #20)
> Thanks for your guidance on this bug. I am also new to hg and mozilla's
> review tool so I appreciate your patience as I respond to your review.
Thanks! That's a lot to take in at once, so you're doing well!
My impression of the main difference with our hg and mozReview workflow is that we seem generally expected to update (evolve) each patch (commit) in the changeset in response to review comments (This differs from git, where fixes are typically addressed with new commits).
This is probably linked to our thinking about commits as patches, where each patch ideally compiles and could land independently (though that's not always true in practice), and wont trip up bisecting tools like mozregression after they've landed.
The mozReview UI is designed with this in mind. This works well if you have the "evolve" mercurial extension enabled. Then you can use hg histedit, commit --amend, (and hg evolve or hg stabilize, if you need to).
See http://mozilla-version-control-tools.readthedocs.org/en/latest/mozreview/review-requests.html#updating-review-requests
and https://ahal.ca/blog/2014/new-mercurial-workflow/
Reporter | ||
Updated•9 years ago
|
Attachment #8710117 -
Flags: review+
Reporter | ||
Comment 23•9 years ago
|
||
Comment on attachment 8710117 [details]
MozReview Request: Bug 987186 - support echoCancellation constraint, pass test
https://reviewboard.mozilla.org/r/31651/#review28419
Note: You should add me as a reviewer on all 5 commits, not just the last one.
Whenever you touch a .webidl file, you also need a DOM reviewer, someone like smaug (please add him, as it wont let me do it).
Otherwise, this first commit looks good!
Assignee | ||
Updated•9 years ago
|
Attachment #8710118 -
Flags: review?(jib)
Assignee | ||
Updated•9 years ago
|
Attachment #8710120 -
Flags: review?(jib)
Assignee | ||
Updated•9 years ago
|
Attachment #8710121 -
Flags: review?(jib)
Assignee | ||
Comment 24•9 years ago
|
||
It sounds like I should go ahead and flatten these 5 into one, then? I made each of them so they would compile but I'm not sure any particular commit would make sense on its own.
Reporter | ||
Updated•9 years ago
|
Attachment #8710118 -
Flags: review?(jib) → review+
Reporter | ||
Comment 25•9 years ago
|
||
Comment on attachment 8710118 [details]
MozReview Request: Bug 987186 - remove AudioConfig, send agc/aec/noise from prefs
https://reviewboard.mozilla.org/r/31653/#review28423
Nice cleanup this!
::: dom/media/MediaManager.cpp:1470
(Diff revision 1)
> - LOG(("%s: default prefs: %dx%d @%dfps (min %d), %dHz test tones", __FUNCTION__,
> - mPrefs.mWidth, mPrefs.mHeight, mPrefs.mFPS, mPrefs.mMinFPS, mPrefs.mFreq));
> + LOG(("%s: default prefs: %dx%d @%dfps (min %d), %dHz test tones, aec on: %d, agc on: %d, noise on: %d, aec level: %d, agc level: %d, noise level: %d, playout delay: %d", __FUNCTION__,
> + mPrefs.mWidth, mPrefs.mHeight, mPrefs.mFPS, mPrefs.mMinFPS, mPrefs.mFreq, mPrefs.mAecOn,
> + mPrefs.mAgcOn, mPrefs.mNoiseOn, mPrefs.mAec, mPrefs.mAgc, mPrefs.mNoise, mPrefs.mPlayoutDelay));
Wordwrap to 80ish characters.
Maybe %s and ? "on":"off" for the booleans? Or overload -1 to mean off like is done elsewhere here?
::: dom/media/webrtc/MediaEngine.h:207
(Diff revision 1)
> MediaEnginePrefs()
> : mWidth(0)
> , mHeight(0)
> , mFPS(0)
> , mMinFPS(0)
> , mFreq(0)
> + , mAecOn(true)
> + , mAgcOn(false)
> + , mNoiseOn(true)
> + , mAec(0)
> + , mAgc(0)
> + , mNoise(0)
> + , mPlayoutDelay(20)
> {}
I realize we're initializing twice here. The intent seems to be for the constructor merely to zero out everything so that we can't rely on uninitialized data, and for desirable defaults to be set in MediaManager instead.
::: dom/media/webrtc/MediaEngineTabVideoSource.cpp
(Diff revision 1)
> -nsresult
> -MediaEngineTabVideoSource::Config(bool, uint32_t, bool, uint32_t, bool, uint32_t, int32_t)
> -{
> - return NS_OK;
> -}
Good riddance.
Reporter | ||
Updated•9 years ago
|
Attachment #8710120 -
Flags: review?(jib)
Reporter | ||
Comment 26•9 years ago
|
||
Comment on attachment 8710120 [details]
MozReview Request: Bug 987186 - respect echoCancellation constraint
https://reviewboard.mozilla.org/r/31655/#review28425
::: dom/webidl/MediaStreamTrack.webidl:57
(Diff revision 1)
> - ConstrainBoolean echoCancellation;
> + boolean echoCancellation;
Here's where I would recommend hg histedit to fold this commit and maybe the subsequent commit in with the last commit, since these patches go over the same code, and I see you fix this up later, and I don't really want to r+ this, even if it is interim code.
Reporter | ||
Comment 27•9 years ago
|
||
Comment on attachment 8710121 [details]
MozReview Request: Bug 987186 - also accept and respect mozNoiseSuppression and mozAutoGainControl
https://reviewboard.mozilla.org/r/31657/#review28427
Same here.
::: dom/webidl/MediaStreamTrack.webidl:58
(Diff revision 1)
> + boolean mozNoiseSuppression;
> + boolean mozAutoGainControl;
You'll need a DOM reviewer (smaug) again, and he wont want to see this version. :)
Attachment #8710121 -
Flags: review?(jib)
Reporter | ||
Comment 28•9 years ago
|
||
Comment on attachment 8710122 [details]
MozReview Request: Bug 987186 - now take params as ConstrainBooleans; r=jib
https://reviewboard.mozilla.org/r/31659/#review28429
Wfm!
::: dom/webidl/MediaStreamTrack.webidl:57
(Diff revision 1)
> - boolean echoCancellation;
> - boolean mozNoiseSuppression;
> - boolean mozAutoGainControl;
> + ConstrainBoolean echoCancellation;
> + ConstrainBoolean mozNoiseSuppression;
> + ConstrainBoolean mozAutoGainControl;
Looks good, but needs DOM review.
Attachment #8710122 -
Flags: review?(jib) → review+
Reporter | ||
Comment 29•9 years ago
|
||
(In reply to Brian Armstrong from comment #24)
> It sounds like I should go ahead and flatten these 5 into one, then?
I would only flatten the last three, because A) it's good to separate out stuff per reviewer (webidl stuff), B) you already got r+ on some of them.
Reporter | ||
Comment 30•9 years ago
|
||
(In reply to Brian Armstrong from comment #20)
> I can't find any suitable test to modify to confirm that we are now
> actually respecting these values so I suspect a new test will need to be created.
A tricky part with testing actual device properties is that test machines often don't have devices, making testing hard to automate. See bug 1088621.
Once bug 1213517 lands we can at least do simple set-get match tests.
If you could cover any manual verification you've done, and perhaps upload your test page or jsfiddle so QA can use it to verify this manually that would be great! Ideally something that covers both getUserMedia and applyConstraints. E.g. here's an applyConstraints fiddle for video: http://jsfiddle.net/Lfru4g4n/
Reporter | ||
Comment 31•9 years ago
|
||
For show, here's an audio spectrum fiddle if we want to get fancy: http://jsfiddle.net/jib1/0s769z0d/
Reporter | ||
Comment 32•9 years ago
|
||
And thanks again. Nice job on this!
Reporter | ||
Comment 33•9 years ago
|
||
Comment on attachment 8710117 [details]
MozReview Request: Bug 987186 - support echoCancellation constraint, pass test
I can't set it in mozReview, but I realized I can flag review in bugzilla at least.
Attachment #8710117 -
Flags: review?(bugs)
Reporter | ||
Comment 34•9 years ago
|
||
Comment on attachment 8710122 [details]
MozReview Request: Bug 987186 - now take params as ConstrainBooleans; r=jib
Might help with timezones.
Attachment #8710122 -
Flags: review?(bugs)
Updated•9 years ago
|
Attachment #8710117 -
Flags: review?(bugs) → review+
Updated•9 years ago
|
Attachment #8710122 -
Flags: review?(bugs) → review+
Comment 35•9 years ago
|
||
https://reviewboard.mozilla.org/r/31651/#review28515
r+ for the webidl
Assignee | ||
Updated•9 years ago
|
Attachment #8710120 -
Attachment description: MozReview Request: Bug 987186 - just use bool for echoCancellation, respect constraint → MozReview Request: Bug 987186 - respect echoCancellation constraint
Attachment #8710120 -
Flags: review?(jib)
Assignee | ||
Comment 36•9 years ago
|
||
Comment on attachment 8710120 [details]
MozReview Request: Bug 987186 - respect echoCancellation constraint
Review request updated; see interdiff: https://reviewboard.mozilla.org/r/31655/diff/1-2/
Assignee | ||
Updated•9 years ago
|
Attachment #8710121 -
Attachment is obsolete: true
Assignee | ||
Updated•9 years ago
|
Attachment #8710122 -
Attachment is obsolete: true
Assignee | ||
Comment 37•9 years ago
|
||
Comment on attachment 8710118 [details]
MozReview Request: Bug 987186 - remove AudioConfig, send agc/aec/noise from prefs
Review request updated; see interdiff: https://reviewboard.mozilla.org/r/31653/diff/1-2/
Assignee | ||
Updated•9 years ago
|
Attachment #8710120 -
Attachment is obsolete: true
Attachment #8710120 -
Flags: review?(jib)
Assignee | ||
Comment 38•9 years ago
|
||
***
Bug 987186 - also accept and respect mozNoiseSuppression and mozAutoGainControl
Review commit: https://reviewboard.mozilla.org/r/31825/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/31825/
Assignee | ||
Comment 39•9 years ago
|
||
hg evolve is properly magic, I suspect :D
Assignee | ||
Updated•9 years ago
|
Attachment #8710648 -
Flags: review?(jib)
Attachment #8710648 -
Flags: review?(bugs)
Comment 40•9 years ago
|
||
Comment on attachment 8710648 [details]
MozReview Request: Bug 987186 - respect echoCancellation constraint
https://reviewboard.mozilla.org/r/31825/#review28557
Attachment #8710648 -
Flags: review?(bugs) → review+
Reporter | ||
Comment 41•9 years ago
|
||
Reporter | ||
Comment 42•9 years ago
|
||
Comment on attachment 8710648 [details]
MozReview Request: Bug 987186 - respect echoCancellation constraint
https://reviewboard.mozilla.org/r/31825/#review28573
lgtm. If you could elaborate on verification you've done, as mentioned in comment 30 that would be great!
Attachment #8710648 -
Flags: review?(jib) → review+
Assignee | ||
Comment 43•9 years ago
|
||
(In reply to Jan-Ivar Bruaroey [:jib] from comment #31)
> For show, here's an audio spectrum fiddle if we want to get fancy:
> http://jsfiddle.net/jib1/0s769z0d/
This is a great fiddle! I've been attempting to come up with convincing tests for this patch with it.
It looks like the easiest property to confirm is mozNoiseSuppression. If we set this value to true, then the spectrum's high frequency tail cuts off pretty quickly. If we turn mozNoiseSuppression off, there is much more content in the tail. You can see this effect in http://jsfiddle.net/5taLozz6/ by toggling the value. The results will presumably vary from mic to mic but I suspect it should be pretty visible for most.
I've found a bug, though. It seems that http://jsfiddle.net/Lupob9g3/ isn't working. I've confirmed that NormalizedConstraintSet::BooleanRange::BooleanRange gets called with the right parameters and with advanced set to true, and that it correctly sets mMin and mMax. When I inspect the values of mMin and mMax on c.mEchoCancellation, though, it looks like the advanced value has not been applied. I've tried to figure out what causes this but haven't come up with a compelling explanation.
Assignee | ||
Comment 44•9 years ago
|
||
Comment on attachment 8710648 [details]
MozReview Request: Bug 987186 - respect echoCancellation constraint
Review request updated; see interdiff: https://reviewboard.mozilla.org/r/31825/diff/1-2/
Assignee | ||
Comment 45•9 years ago
|
||
Ah, I see, I had to tell FlattenedConstraints what to do with those advanced constraints, which is why it ignored them. I suspect I will personally very much want that functionality to work for the moz-prefixed constraints in particular.
With this latest changeset, I am now getting all the behavior I expect from the jsfiddle. In particular:
* If we supply nothing, we receive the values specified in about:config
* If we supply a standard boolean, it overrides the default
* If we supply a boolean in advanced, it also overrides the default
Reporter | ||
Comment 46•9 years ago
|
||
(In reply to Brian Armstrong from comment #44)
> Review request updated; see interdiff:
(Weird, another mozReview bug: the r+ in bugzilla never reverted to r? from that update)
Reporter | ||
Comment 47•9 years ago
|
||
https://reviewboard.mozilla.org/r/31825/#review28605
Thanks! I should have caught that (though some of us are secretly hoping that advanced will fade into obscurity).
Small tip for next time: when you already have r+es on a patch (especially DOM reviewers), is when it's good to add code with another fresh commit. ;)
Reporter | ||
Comment 48•9 years ago
|
||
(In reply to Brian Armstrong from comment #43)
> > For show, here's an audio spectrum fiddle if we want to get fancy:
> > http://jsfiddle.net/jib1/0s769z0d/
>
> This is a great fiddle!
I got it from :abr, not sure where he got it from, or if he made it himself.
> It looks like the easiest property to confirm is mozNoiseSuppression. If we
> set this value to true, then the spectrum's high frequency tail cuts off
> pretty quickly. If we turn mozNoiseSuppression off, there is much more
> content in the tail. You can see this effect in
> http://jsfiddle.net/5taLozz6/ by toggling the value. The results will
> presumably vary from mic to mic but I suspect it should be pretty visible
> for most.
That's great! I was hoping it could be useful for something like that.
Reporter | ||
Comment 49•9 years ago
|
||
I did a try in mozReview, and assuming it turns out green, then we can autoland from there. Maire, you wanted a ni? when this was reviewed, I think.
Flags: needinfo?(mreavy)
Comment 50•9 years ago
|
||
Thanks, Jib. I'll monitor the try and autoland to make sure this lands before uplift (assuming no surprises).
Flags: needinfo?(mreavy)
Comment 51•9 years ago
|
||
https://reviewboard.mozilla.org/r/31825/#review28605
FYI, I retriggered the Windows builds that failed due to apparent infra problems
Comment 52•9 years ago
|
||
Comment 53•9 years ago
|
||
Extra ')' in the log statement ending on dom/media/MediaManager.cpp:1480
Comment 55•9 years ago
|
||
Comment 56•9 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox46:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
Comment 57•9 years ago
|
||
Guys, first let me thank you for the work you have done! After some time I had a possibility to look into this again. I tried everything against v46.0.1 of Firefox. At the beginning, I just tried audio: { echoCancellation: false }. Then I tried specifying the same even in optional, also adding the moz* constraints mentioned. Any setting tried did not have any effect, there were still sound modifications present.
My question is: should my version of Firefox already have this fixed? If yes, should I expect the same behaviour as in case of Chrome (which I really expected), e.g. no sound modifications at all when echoCancellation is false? I assume that lack of stereo sound in this case is due to other bugs Firefox has at the moment, so I am not solving that.
Please let me know!
Assignee | ||
Comment 58•9 years ago
|
||
Hi Jan,
As someone who is _not_ a Mozilla person, and can't speak officially on their behalf, I'll share some of my observations with Firefox's web audio stack. This is just based on empirical testing on my part.
Here's a jsfiddle where you can toggle the options and watch the resulting spectrum http://jsfiddle.net/qjttcz2t/ . For testing purposes I'd recommend turning auto gain control on (it helps normalize what you see in the spectrum). I can convince myself that there's a difference in ambient room sounds with noise suppression on vs off using my laptop mic, but it's not a huge difference.
Also, as of the last time I checked, Firefox's sampling rate was still 32kHz, which means that no audio content above 16kHz will appear. Also, because it's been downsampled, Firefox's audio stack applies a low pass filter with a fairly wide cutoff, which seems to start engaging around 14kHz and has completely cut off any content at 16+kHz. It's possible this has since changed, though.
Can you describe more precisely what sort of effects you've noticed?
Flags: needinfo?(jan.ambroz)
Comment 59•9 years ago
|
||
"If yes, should I expect the same behaviour as in case of Chrome (which I really expected), e.g. no sound modifications at all when echoCancellation is false?"
Well part of the problem is that this behavior you're expecting is outside of the spec; chrome links AEC setting to their NS and AGC settings. At this point, we consider them separate settings. Also we're in the process of adding a bypass of audio processing and resampling if all of these are off; 46.0.1 does not have this code.
We've been considering doing the same (unspecified) behavior as chrome when the AEC constraint is set to false. Better yet would be to standardize *something* about what happens here.
I'll note that our defaults do have some reasoning behind them. There are many cases where you'd want NS and AGC when AEC is off - pretty much any MediaRecorder use, and in general almost all non-WebRTC uses where you're not using a "professional" setup with a remote microphone. You really don't want your headset or laptop mic picking up the laptop fans, for example. (NS is much more about that than low, uncorrelated background noise.) And any MediaRecorder/etc use where you're doing that sort of care in micing should know enough to directly control (or give the user control) over each of these parameters, and not just assume they'll be set correctly.
Comment 60•9 years ago
|
||
Thank you for all your replies! OK, I have to apologize this time, as there were too many wrong assumptions on my side.
First of all, yes, I really thought that the Chrome behaviour of echoCancellation is actually a standard way, apparently not, the docs is quite clear on this.
Next, I assumed that FF also uses that mandatory/optional media constraint groups. Again, according to the docs, this is not the standard way how to do it. I have spent too much time with Chrome.
So, to wrap it up, now it works as expected for me, apart from the stereo. Brian, I cannot test the music transmission now to be able to confirm your observations:( Anyway, like this it seems to be usable for some music, finally!
Thanks again!
Flags: needinfo?(jan.ambroz)
Comment 61•8 years ago
|
||
Why did we add new web-exposed moz-prefixed things here? This just got mentioned in the MediaStreamTrack.getSettings intent to implement thread for Blink, and they're trying to figure out what to do with them on their end now...
Flags: needinfo?(jib)
Reporter | ||
Comment 62•8 years ago
|
||
Good question. Maybe old habit, and perhaps a downside of adding the subject in 2014 and patch in 2016. :/
We prefixed them because we consider them experimental on our end and there's no spec for autoGainControl and noiseSuppression.
For context, the working group went back and forth for a while on how they wanted browsers to experiment with constraints, initially setting up a looser framework using an IANA registry that didn't require spec work. This was tied to a (perhaps false) perception that constraints could be allowed to be more vendor specific than other parts of the web platform. The elaborate support-detection, query and apply API framework around constraints contributes to this perception.
Thankfully this was dropped in late 2015 [1], a few months before this patch. At the time, the mediacapture-main spec wasn't accepting new constraints, and it wasn't really clear where new constraints would go, and the new spec language is still a bit woolly on it [2].
mozAutoGainControl and mozNoiseSuppression were our reaction to this problem. Chrome's reaction was different, choosing to trigger this audio processing (and AFAIK all audio processing) off of the single specified constraint, echoCancellation. See bug 1274392. Not sure which approach was best, but it stinks that they're different.
We could look at unprefixing them or drop them and implement bug 1274392, or both.
[1] https://github.com/w3c/mediacapture-main/pull/258
[2] https://w3c.github.io/mediacapture-main/getusermedia.html#defining-a-new-constrainable-property
Flags: needinfo?(jib)
Comment 63•8 years ago
|
||
Note that bug 1274392 doesn't assume (or shouldn't) that these are removed, but would make the defaults track the state of echoCancellation (which would be compatible with Chrome, and *usually* be what the app wants.
The problem (and it's reported to Chrome multiple times) is that tying it all to AEC ties the hands of applications. Recording apps *usually* want to turn them all off (music with remote mics), but might want noise suppression (suppresses fan noise, for example, for doing a local system-mic video recording). And Gain Control even more so - so much that we default gain control to off, Chrome defaults it to on.
Without these constraints, applications will have to ask users to modify global about:config settings for these (or just accept that they won't get what they want).
The WebRTC spec is already moving in the direction of more "direct" control and less "DWIM" interfaces; adding these constraints to the spec would be in keeping with that, though it does bring up the question of how to deal with "new" processing features. Options would be to have a "master" switch (which for Chrome the AEC setting already is), or to have a use-case/scenario/'role' setting (communication, normal recording, clean audio recording, gaming, voice recognition, etc) which sets defaults appropriate for the usecase, and perhaps allows fine-grained overrides as we mention above.
Comment 64•8 years ago
|
||
> We could look at unprefixing them or drop them and implement bug 1274392, or both.
So is the basic problem that in terms of getting these knobs standardized we've hit an impasse?
Reporter | ||
Comment 65•8 years ago
|
||
No, not an impasse, we just need to find a spec to put them in, or write one. Just work.
You need to log in
before you can comment on or make changes to this bug.
Description
•